* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Kyle Lippincott @ 2024-03-09 0:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqy1ase1vo.fsf@gitster.g>
On Fri, Mar 8, 2024 at 3:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Kyle Lippincott <spectral@google.com> writes:
>
> >> $ cat ../seconary/.git
> >
> > Nit: typo, should be `secondary` (missing the `d`)
>
> Good eyes. Thanks.
>
> >> + /*
> >> + * We should be a subdirectory of /.git/worktrees inside
> >> + * the $GIT_DIR of the primary worktree.
> >> + *
> >> + * NEEDSWORK: some folks create secondary worktrees out of a
> >> + * bare repository; they don't count ;-), at least not yet.
> >> + */
> >> + if (!strstr(path, "/.git/worktrees/"))
> >
> > Do we need to be concerned about path separators being different on
> > Windows? Or have they already been normalized here?
>
> I am not certain offhand, but if they need to tolerate different
> separators, they can send in patches.
>
> >> + goto out;
> >> +
> >> + /*
> >> + * Does gitdir that points at the ".git" file at the root of
> >> + * the secondary worktree roundtrip here?
> >> + */
> >
> > What loss of security do we have if we don't have as stringent of a
> > check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?
>
> No loss of security.
Then should we just do that?
+ /* Assumption: `path` points to the root of a $GIT_DIR. */
static int is_repo_with_working_tree(const char *path)
{
- return ends_with_path_components(path, ".git");
+ /* $GIT_DIR immediately below the primary working tree */
+ if (ends_with_path_components(path, ".git"))
+ return 1;
+
+ /*
+ * Also allow $GIT_DIRs in secondary worktrees.
+ * These do not end in .git, but are still considered safe because
+ * of the .git component in the path.
+ */
+ if (strstr(path, "/.git/worktrees/"))
+ return 1;
+
+ return 0;
}
>
> These "keep result the status we want to return if we want to return
> immediately here, and always jump to the out label instead of
> returning" patterns is mere a disciplined way to make it easier to
> write code that does not leak. The very first one may not have to
> do the "goto out" and instead immediately return, but by writing
> this way, I do not need to be always looking out to shoot down
> patches that adds new check and/or allocations before this
> condition and early "out".
>
> > Or maybe we even combine the existing ends_with(.git) check with this
>
> At the mechanical level, perhaps, but I'd want logically separate
> things treated as distinct cases. One is about being inside
> $GIT_DIR of the primary worktrees (where more than majority of users
> will encounter) and the new one is about being inside $GIT_DIR of
> secondaries.
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Junio C Hamano @ 2024-03-09 1:14 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
In-Reply-To: <CAO_smVjD8DFcvveAg2iiWGhtNJGCT1ieAUzJbX3TNNJjm-5rMw@mail.gmail.com>
Kyle Lippincott <spectral@google.com> writes:
>> > What loss of security do we have if we don't have as stringent of a
>> > check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?
>>
>> No loss of security.
>
> Then should we just do that?
I do not see what you mean.
> + /* Assumption: `path` points to the root of a $GIT_DIR. */
> static int is_repo_with_working_tree(const char *path)
> {
> - return ends_with_path_components(path, ".git");
> + /* $GIT_DIR immediately below the primary working tree */
> + if (ends_with_path_components(path, ".git"))
> + return 1;
> +
> + /*
> + * Also allow $GIT_DIRs in secondary worktrees.
> + * These do not end in .git, but are still considered safe because
> + * of the .git component in the path.
> + */
> + if (strstr(path, "/.git/worktrees/"))
> + return 1;
> +
> + return 0;
> }
Ah, no. I thought you were asking "goto out" vs "return", and my
answer was about these two. Whether you leave with "goto out" or
"return", it does not change the security posture. Direct return
will raise the risk of leaking resources after careless future
updates to the code.
I didn't get that you do not want to see the other two "sanity
check".
Losing these sanity checks may not lose "security" per-se, but it
may raise the risk of misidentification. A healthy GIT_DIR of a
secondary worktree should satisfy these two extra conditions.
^ permalink raw reply
* Re: [PATCH v4 2/2] index-pack: --fsck-objects to take an optional argument for fsck msgs
From: John Cai @ 2024-03-09 1:55 UTC (permalink / raw)
To: SZEDER Gábor
Cc: John Cai via GitGitGadget, git, Jonathan Tan, Patrick Steinhardt
In-Reply-To: <20240308222439.GB1908@szeder.dev>
Hi Szeder,
On 8 Mar 2024, at 17:24, SZEDER Gábor wrote:
> On Thu, Feb 01, 2024 at 01:38:02AM +0000, John Cai via GitGitGadget wrote:
>> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
>> index 496fffa0f8a..a58f91035d1 100755
>> --- a/t/t5300-pack-object.sh
>> +++ b/t/t5300-pack-object.sh
>> @@ -441,8 +441,7 @@ test_expect_success 'index-pack with --strict' '
>> )
>> '
>>
>> -test_expect_success 'index-pack with --strict downgrading fsck msgs' '
>> - test_when_finished rm -rf strict &&
>> +test_expect_success 'setup for --strict and --fsck-objects downgrading fsck msgs' '
>> git init strict &&
>> (
>> cd strict &&
>> @@ -457,12 +456,32 @@ test_expect_success 'index-pack with --strict downgrading fsck msgs' '
>>
>> EOF
>> git hash-object --literally -t commit -w --stdin <commit >commit_list &&
>> - PACK=$(git pack-objects test <commit_list) &&
>> - test_must_fail git index-pack --strict "test-$PACK.pack" &&
>> - git index-pack --strict="missingEmail=ignore" "test-$PACK.pack"
>> + git pack-objects test <commit_list >pack-name
>> )
>> '
>>
>> +test_with_bad_commit () {
>> + must_fail_arg="$1" &&
>> + must_pass_arg="$2" &&
>> + (
>> + cd strict &&
>> + test_expect_fail git index-pack "$must_fail_arg" "test-$(cat pack-name).pack"
>
> There is no such command as 'test_expect_fail', resulting in:
Indeed, thanks for catching this.
>
> expecting success of 5300.34 'index-pack with --strict downgrading fsck msgs':
> test_with_bad_commit --strict --strict="missingEmail=ignore"
>
> + test_with_bad_commit --strict --strict=missingEmail=ignore
> + must_fail_arg=--strict
> + must_pass_arg=--strict=missingEmail=ignore
> + cd strict
> + cat pack-name
> + test_expect_fail git index-pack --strict test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
> ./t5300-pack-object.sh: 468: eval: test_expect_fail: not found
> + cat pack-name
> + git index-pack --strict=missingEmail=ignore test-e4e1649155bf444fbd9cd85e376628d6eaf3d3bd.pack
> e4e1649155bf444fbd9cd85e376628d6eaf3d3bd
>
> ok 34 - index-pack with --strict downgrading fsck msgs
>
> The missing command should fail the test, but it doesn't, because the
> &&-chain is broken on this line as well.
yes will need to fix this as well
thanks
John
^ permalink raw reply
* Re: [PATCH] git: --no-lazy-fetch option
From: Linus Arver @ 2024-03-09 1:57 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <20240217052932.GC539459@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 16, 2024 at 09:22:20AM -0800, Junio C Hamano wrote:
>
> [...]
>> Add documentation and note that for this variable, unlike many
>> boolean-looking environment variables, only the presence matters,
>> not what value it is set to.
>
> Yuck. IMHO depending on GIT_NO_REPLACE_OBJECTS=0 is somewhat crazy, and
> we could consider the current behavior a bug. It's probably not _that_
> big a deal either way (because I would not expect anybody to set it that
> way in the first place). But I wonder if being consistent across
> variables trumps retaining weird historical compatibility for such a
> far-fetched case. I dunno. I guess this is https://xkcd.com/1172/. :)
I totally agree with your take on this. Would such cleanup patches
(e.g., changing the behavior of GIT_NO_REPLACE_OBJECTS=0 to be "false"
instead of "true") be acceptable as #leftoverbits?
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Kyle Meyer @ 2024-03-09 3:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kyle Lippincott
In-Reply-To: <20240308211957.3758770-3-gitster@pobox.com>
Junio C Hamano writes:
> Now, for the same reason, let's allow command execution from within
> the $GIT_DIR directory of a secondary worktree. This is useful for
> tools working with secondary worktrees when the 'bareRepository'
> setting is set to 'explicit'.
Does the same reason also apply to .git/modules/$name ?
> In the previous commit, we created a helper function to house the
> logic that checks if a directory that looks like a bare repository
> is actually a part of a non-bare repository. Extend the helper
> function to also check if the apparent bare-repository is a $GIT_DIR
> of a secondary worktree, by checking three things:
>
> * The path to the $GIT_DIR must be a subdirectory of
> ".git/worktrees/", which is the primary worktree [*].
>
> * Such $GIT_DIR must have file "gitdir", that records the path of
> the ".git" file that is at the root level of the secondary
> worktree.
>
> * That ".git" file in turn points back at the $GIT_DIR we are
> inspecting.
>
> The latter two points are merely for checking sanity. The security
> lies in the first requirement.
In the case of .git/modules/, the second point doesn't apply because
there's no gitdir file. But perhaps the core.worktree setting could be
used for the same purpose.
$ pwd
/path/to/super/.git/modules/sub
$ git config core.worktree
../../../sub
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Elijah Newren @ 2024-03-09 3:28 UTC (permalink / raw)
To: Stefan Haller
Cc: Junio C Hamano, git, Derrick Stolee, Phillip Wood,
Christian Couder
In-Reply-To: <42426c93-84fe-47d2-a41c-16284a86f003@haller-berlin.de>
On Thu, Mar 7, 2024 at 12:16 PM Stefan Haller <lists@haller-berlin.de> wrote:
>
> Elijah, thanks for your patience with this. I appreciate the time and
> energy you put into understanding what I want to achieve. The questions
> you are asking help me understand my proposal better myself.
>
> It seems that I didn't do a very good job at getting my point across so
> far, so I'll try again in a more structured way.
I think I fell short on communication on a few points as well; sorry about that.
> Let's begin by describing two very different user scenarios:
>
> 1) Stacked branches. Git supports these reasonably well for simple cases
> through the "rebase --update-refs" command (and the "rebase.updateRefs"
> config), but since they are not a first-class concept, git needs to rely
> on heuristics to determine which branches are part of a stack. For
> simple cases this works very well, but more esoteric cases can have
> problems (e.g. a non-linear topology of multiple stacks that may share
> common base branches and then diverge, in which case rebasing one of
> them destroys the others; or degenerate stacks involving "empty"
> branches either in the middle or at the top, in which case there's no
> way to tell what the order of the branches is supposed to be).
>
> 2) Copying a branch, and rebasing it away from the original one (for
> non-stacked branches, see below). The use case is that you have a branch
> called topic-1 (branched off main), which is pushed and in review
> already, with CI running on it, and you want to test whether it works on
> devel, so you make a new branch called topic-1-on-devel off of topic-1,
> and rebase it onto devel. You want to make a draft PR of that new branch
> to have CI run on it, too, and of course you want to keep the original
> branch untouched. For me and most of my co-worker that I have observed
> in pairing sessions, the natural way to achieve this is as described
> above: checkout a new branch, and rebase it where you want it to go.
>
> Next I'll describe my goals, and my non-goals. I know I can easily
> achieve 2) by simply not using --update-refs, but I like to have
> "rebase.updateRefs" set to true by default because it is so useful, and
> having to remember to use --no-update-refs whenever I do 2) is annoying.
>
> So my goal is to make 2) work well (in the simple, non-stacked case)
> even when "rebase.updateRefs" is true, while not making 1) work any
> worse in the "normal", non-degenerate case.
Thanks, this is helpful background.
> I'm _not_ trying to fix the problems that --update-refs has today (I
> briefly mentioned some of them above, but there are more), and I'm not
> trying to make 2) work well with stacked branches. It would certainly be
> nice if that would work too, but I don't think it can without
> introducing branch stacks as a first-class feature in git, so I'll have
> to live with not supporting that case well. It would still be a big
> improvement for me without that.
> >> When I create a copy of a
> >> branch, I do that only to rebase the copy somewhere else _immediately_,
> >> leaving the original branch where it was.
> >
> > If it is inherently tied like this, why not create the new branch
> > immediately after the rebase (with active_branch@{1} as the start
> > point), instead of creating it immediately before?
>
> That would be the wrong way round. I want to leave the original branch
> untouched, make a new branch and rebase that away from the original.
Ah, sorry for misunderstanding. Still, though, what's wrong with running
git branch -f original_branch original_branch@{1}
after the operation? That'll make the original branch point to where
it was before the rebase operation. Since there's no separation in
time between when you create the new copy branch and do this rebase
operation, it's not a matter of forgetting that there was this
original branch that you wanted to reflect its own pre-rebase state,
right?
Also, since you're not using the git cli directly but going through
lazygit, isn't this something you can just include in lazygit as part
of whatever overall operation is creating the new copy branch and
rebasing it?
> >> Which means that I encounter
> >> copied branches only at the top of the stack, not in the middle. Which
> >> means that I'm fine with keeping the current behavior of "rebase
> >> --update-ref" to update both copies of that middle-of-the-stack branch,
> >> because it never happens in practice for me.
> >
> > You've really lost me here; are you saying you're fine changing the
> > design to add inherent edgecase bugs to the code because those edge
> > cases "never happen in practice for me"?
>
> Wait, now you are really turning things around. You make it sound like
> my proposal is responsible for what you call a "bug" here. It's not, git
> already behaves like this (and you may or may not consider that a
> problem), and my proposal doesn't change anything about it. It doesn't
> "fix" it, that's right (and this is what I referred to when I said "I'm
> fine with it"), but it doesn't make it any worse either.
Ah, I see where I was unclear as well, and my lack of clarity stemmed
from not understanding your proposal. To try to close the loop, allow
me to re-translate your "This is a good point, but..it never happens
in practice for me." paragraph, the way I _erroneously_ read it at the
time:
"""
For my new proposal, the case you bring up is a good point. But it
doesn't happen for me, so I propose to leave it as undefined behavior.
[As undefined behavior, anyone that triggers it is likely to get
behavior they deem buggy and not like it, but that won't affect me.]
"""
Now, obviously, that doesn't sound quite right. I knew it at the
time, but reading and re-reading your paragraph, it kept coming out
that way for me. Thus I tried to ask if that's what you really meant,
and apologizing in advance if I was mis-reading.
Anyway, with the extra explanation in your latest email, I now see
that you weren't leaving it undefined, but your proposal wasn't clear
to me either in that paragraph or in combination with the rest of your
previous email. Sorry for my misunderstanding.
> > By "leaf branches", do you mean (a) those commits explicitly mentioned
> > on the command line for being replayed, (b) only the subset of the
> > branches mentioned on the command line which aren't an ancestor of
> > another commit being replayed, or (c) something else?
>
> If I understand you right (and if I understand the user interface of
> git-replay right), then what I mean is the combination of all single
> commits that are mentioned on the command line, plus the right side of
> all A..B ranges that are mentioned on the command line. In my mental
> model those are "the things that are being rebased" (please let me know
> if that mental model is wrong), and I am proposing to exclude all
> branches from updating that point to any of those and are not mentioned
> on the command line, because they can be considered copies.
>
> > Let me re-ask my question another way. If someone runs
> > git replay --onto A --contained ^B ^C D E F
> > when branches G, H, & I are in the revision range of "^B ^C D E F",
> > with G in particular pointing where D does and H pointing where E
> > does, and E contains D in its history, and F contains commits that are
> > in neither D nor E, how do I figure out which of D-I should be
> > updated?
>
> D, E, F, and I are updated, G and H are not; this seems very obvious to
> me. D, E, and F because they are all mentioned explicitly; G and H are
> not updated because they point to one of the "things-to-be-rebased", so
> they are copies; I is updated because it is contained in E but does not
> point at one of the "things-to-be-rebased", so it's part of a "stack"
> (or whatever you want to call this topology).
>
> It's a heuristic; we need a way to distinguish things that are part of a
> stack from things that are copies. My heuristic for this relies on the
> assumption that the stack is not degenerate in the sense that it doesn't
> contain any "empty" branches in the middle or at the top of the stack,
> otherwise it wouldn't be possible to distinguish the two.
Ah, okay, now I understand the concrete proposal; thanks.
> However, _if_ we decide to change
> "rebase --update-ref", then I think it would make sense to change
> "replay --contains" in the same way, so that they behave more consistently.
Yep, makes sense.
> >> One last remark: whenever I describe my use case involving copies of
> >> branches, people tell me not to do that, use detached heads instead, or
> >> other ways to achieve what I want. But then I don't understand why my
> >> proposal would make a difference for them. If you don't use copied
> >> branches, then why do you care whether "rebase --update-refs" or "replay
> >> --contained" moves those copies or not? I still haven't heard a good
> >> argument for why the current behavior is desirable, except for the one
> >> example of a degenerate stack that Phillip Wood described in [1].
> >
> > The current behavior is easy to describe and explain to users, and
> > generalizes nicely to cases of replaying multiple diverging and
> > converging branches.
>
> It sounds like you value the property of being easy to describe higher
> than doing the expected thing in as many cases as possible.
There's certainly some truth to that. For example, "three-way merges"
are unsophisticated, and folks have suggested various ways of "making
them smarter" over the years (though there was more of this years
ago). To quote someone else on that:
"""
Me _personally_, I want to have something that is very repeatable and
non-clever. Something I understand _or_ tells me that it can't do it.
"""
and
"""
I just don't like your notion that you should support the 5% problem with
ugly hacks, and then you dismiss the 95% problem with "nothing else does
it either".
In other words, we're already merging manually for the 95%. Why do you
think the 5% is so important?
"""
Three-way merging and rebase --update-refs are obviously quite
different, so this might not be a good analogy. I'm just saying you
are right that I do sometimes tend to have the same biases as the
author of the above quotes.
But, to be more direct about this particular issue, I've actually had
the usecase Phillip described, and never run into yours. Yes, it's
rare that I've run into Phillip's described case, but rare is still
more often than never. That said, I totally accept that I might be an
oddball. So, I think it's important to look at the alternatives:
* If we make no modifications to --update-refs:
* the --update-refs behavior is very simple to describe
* folks with your usecase immediately understand why the copied
branch was updated, even though you didn't want it to be
* you have a trivial workaround you can run, as mentioned above (git
branch -f original_branch original_branch@{1})
* If we modify --update-refs as you suggest:
* the --update-refs behavior is more complicated to describe to users
* folks with Phillip's usecase probably assume a bug and report it
since it isn't going to make any sense to them (and, my guess is, many
would report a bug even if the behavior is documented)
The downsides for the latter option seem worse to me, so unless the
first usecase is predominant, I'd rather not make a change. Granted,
you did claim the first usecase would be far more common, and you may
be right, but it's not so clear cut to me; I don't know how to
validate that. I'd at least first like to hear why the workaround for
your usecase that looks trivial to me is too onerous for you.
^ permalink raw reply
* Re: [PATCH 3/4] merge options: add a conflict style member
From: Elijah Newren @ 2024-03-09 4:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: phillip.wood123, Phillip Wood via GitGitGadget, git, Phillip Wood
In-Reply-To: <xmqqwmqcfz4y.fsf@gitster.g>
On Fri, Mar 8, 2024 at 8:48 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> phillip.wood123@gmail.com writes:
>
> > I agree it is confusing, Elijah renamed it from ll-merge.c relatively
> > recently 6723899932e (merge-ll: rename from ll-merge, 2023-05-16). It
> > looks like the idea was to group it with the other merge* files:
>
> That reasoning cuts both ways. When you are only interested in the
> upper level API functions, being able to skip ll-anything as "low
> level details" is a powerful thing. merge-ll & hash-ll separated
> far apart will make it impossible.
merge-ll is wildly different than every other *-ll.h file we have in
the tree; the latter set of files may be misnamed, for reasons other
than what you are suggesting here. hash-ll, hex-ll, etc. exist due to
the main include file having some rarely used API that require more
#include statements, and most users of e.g. hex functions can get away
with just including hex-ll.h rather than the full hex.h. Thus,
hex-ll.h is _not_ "low level details that you can skip", but "the
_primary_ data structures and functions". It doesn't get the name
hex.h, though, because if we did that then the folks that need both
primary parts of the API and the occasional additional function would
need to have two hex-related includes. Also, every function declared
within hex-ll.h is still defined within hex.c; there is no separate
hex-ll.c file, and the same is true of all the other *-ll.h files
other than merge-ll.h. As such, there is absolutely no relation
between hex-ll.h, hash-ll.h, fsmonitor-ll.h, etc. Using an ll- prefix
on those filenames thus makes no sense to me. (It's not clear that
-ll even makes sense as a suffix for these files either, but it's not
clear what else to use instead. If I recall correctly I originally
put forward "-basics" as a possible name suffix for these files, but
someone else suggested "-ll", and not having any better ideas or
strong opinions I just went with it.)
merge-ll is different in that it is actually a separate level of the
API, and there are both a merge-ll.h file and a merge-ll.c file.
I originally had proposed only adding the hex-ll.h, hash-ll.h,
fsmonitor-ll.h, etc. files, but you suggested that ll-merge should
either be renamed or that these new files should be
(https://lore.kernel.org/git/CABPp-BErrVUnuDjL73edDpmkKUvs6Ny6cYwvueXw1toB4JcF-Q@mail.gmail.com/).
Now, all that said, and assuming we were to go back to a world where
the other *-ll.h files don't exist (or are renamed independently with
a different suffix), I'm still not understanding why ll-merge makes
more sense to you and Phillip than merge-ll. Could you explain more?
If you're only interested in the upper-level API functions, that
suggests you are already at the function level and looking within a
given file. The low-level functions are already split out into a
separate file, so you just don't go looking at that separate file.
However, if you're interested in "where in this codebase do I find the
stuff related to merging", then you aren't in a file but in a
directory listing. The first usecase gains no benefit from renaming
these files back to ll-merge.[ch], while the other usecase would have
been much improved in my experience from being named merge-ll.[ch].
Granted, it's a really minor point, which was why I never brought it
up until you suggested making all the other *-ll.h files and
ll-merge.[ch] consistent; since renaming the other *-ll.h files made
no sense at all to me, I went with renaming ll-merge.[ch] to
merge-ll.[ch].
There's probably some other angle to this that you two are viewing
this from that just isn't apparent to me. Sorry for not seeing it
(yet). Hopefully the above context is at least helpful, though.
^ permalink raw reply
* Re: [PATCH 3/4] merge options: add a conflict style member
From: Junio C Hamano @ 2024-03-09 5:46 UTC (permalink / raw)
To: Elijah Newren
Cc: phillip.wood123, Phillip Wood via GitGitGadget, git, Phillip Wood
In-Reply-To: <CABPp-BFBNDkdL5Z-VOrv2Z3aWaA3OpWFqd6ZGmU9YC8jBRsAog@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> a different suffix), I'm still not understanding why ll-merge makes
> more sense to you and Phillip than merge-ll.
> Could you explain more?
Language and grammar? In other words, "low-level merge routines" is
an understandable phrase, while "merge lo-level routines" is not.
^ permalink raw reply
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
From: Junio C Hamano @ 2024-03-09 5:53 UTC (permalink / raw)
To: Kyle Meyer; +Cc: git, Kyle Lippincott
In-Reply-To: <87msr8qef9.fsf@kyleam.com>
Kyle Meyer <kyle@kyleam.com> writes:
>> Now, for the same reason, let's allow command execution from within
>> the $GIT_DIR directory of a secondary worktree. This is useful for
>> tools working with secondary worktrees when the 'bareRepository'
>> setting is set to 'explicit'.
>
> Does the same reason also apply to .git/modules/$name ?
Perhaps. I do not actively work on submodules so unlike those who
are always thinking about improving the user experience around them,
I did not think of those ".git/modules/$name" things as something
similar to the ".git/worktrees/$name" things.
Often hooks (and probably third-party tools) run after chdir to be
in $GIT_DIR, so the problems they face when their /etc/gitconfig
forces them to use safe.bareRepository=explicit are probably very
similar either way.
^ permalink raw reply
* Feature request - summarise changes by folder
From: Alex Kubiesa @ 2024-03-09 10:05 UTC (permalink / raw)
To: Git Mailing List (git@vger.kernel.org)
Hi,
I'm new here so please let me know if there is a better place/format to post something like this.
I am using Git for a personal ML project which has a large number of image files (about 32,000 as of today). Every time I add or remove image data, the number or added/deleted files is so large that it overwhelms Git Bash and other Git clients and IDE extensions. If I have other unstaged changes at the same time, for example changing a few lines in a data generating script, they get lost in the noise.
To solve this, my suggestion would be to add an option to `git diff` or `git status` which counts added/deleted/changed files by folder. From that information, one could drill down into specific folders to get a handle on all the changes. I tried some existing options like `git diff --dirstat=files` and `git diff --shortstat` but neither of these gave me enough confidence to stage and commit my changes.
For now, I am piping `git status` to a file and scanning through the file manually, but it would be great if there was a better option.
Thanks,
Alex
^ permalink raw reply
* Re: [RFC PATCH 1/1] config: learn the "hostname:" includeIf condition
From: Ignacio Encinas Rubio @ 2024-03-09 10:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqil1xk9fv.fsf@gitster.g>
On 7/3/24 22:40, Junio C Hamano wrote:
> Ignacio Encinas <ignacio@iencinas.com> writes:
>
>> Currently, customizing the configuration depending on the machine running
>> git has to be done manually.
>>
>> Add support for a new includeIf keyword "hostname:" to conditionally
>> include configuration files depending on the hostname.
>>
>> Signed-off-by: Ignacio Encinas <ignacio@iencinas.com>
>> ---
>> Documentation/config.txt | 9 +++++++++
>> config.c | 16 ++++++++++++++++
>> t/t1305-config-include.sh | 22 ++++++++++++++++++++++
>> 3 files changed, 47 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index e3a74dd1c1..9a22fd2609 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -186,6 +186,11 @@ As for the naming of this keyword, it is for forwards compatibility with
>> a naming scheme that supports more variable-based include conditions,
>> but currently Git only supports the exact keyword described above.
>
>> +`hostname`::
>> + The data that follows the keyword `hostname:` is taken to be a
>> + pattern with standard globbing wildcards. If the current
>> + hostname matches the pattern, the include condition is met.
>> +
>
> OK. This seems to copy its phrasing from the existing text for
> "gitdir" and "onbranch", which greatly helps the description for
> these features consistent.
>
>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..e0611fc342 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -317,6 +317,20 @@ static int include_by_branch(const char *cond, size_t cond_len)
>> return ret;
>> }
>>
>> +static int include_by_hostname(const char *cond, size_t cond_len)
>> +{
>> + int ret;
>> + char my_host[HOST_NAME_MAX + 1];
>> + struct strbuf pattern = STRBUF_INIT;
>> + if (xgethostname(my_host, sizeof(my_host)))
>> + return 0;
>> +
>> + strbuf_add(&pattern, cond, cond_len);
>> + ret = !wildmatch(pattern.buf, my_host, 0);
>> + strbuf_release(&pattern);
>> + return ret;
>> +}
>
> Have a blank line between the end of the decl block (our codebase
> frowns upon decl-after-statement) and the first statement,
> i.e. before "if (xgethostname...".
>
> Otherwise this looks reasonable to me.
Got it.
>> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
>> index 5cde79ef8c..ee78d9cade 100755
>> --- a/t/t1305-config-include.sh
>> +++ b/t/t1305-config-include.sh
>> @@ -357,4 +357,26 @@ test_expect_success 'include cycles are detected' '
>> grep "exceeded maximum include depth" stderr
>> '
>>
>> +test_expect_success 'conditional include, hostname' '
>> + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
>> + echo "[test]twelve=12" >.git/bar12 &&
>> + test_must_fail git config test.twelve &&
>
> Emulating other tests in this file that uses here document may make
> it a bit easier to read? E.g.,
>
> cat >>.gitconfig <<-EOF &&
> [includeIf "hostname:$(hostname)a"]
> path = bar12
> EOF
Thanks for pointing that out. I just read the last tests from that file
where they used the echo "..." >> approach. Do you think it is
worthwhile rewriting those tests to use the approach you suggested?
By the way, before contributing, I saw there is some work on moving to
unit tests. I wasn't sure how to test this particular feature there, so
I went with the "old" approach as it seemed more natural. Is this ok?
>> + echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
>
> Ditto for the remainder of the patch.
>
> Thanks.
Thank you for the review.
^ permalink raw reply
* Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names
From: Phillip Wood @ 2024-03-09 11:28 UTC (permalink / raw)
To: René Scharfe, phillip.wood, git
Cc: Josh Steadmon, Achu Luma, Christian Couder
In-Reply-To: <3ef0927f-4d7b-4061-925e-c113d1c8730d@web.de>
Hi René
On 06/03/2024 18:16, René Scharfe wrote:
> Hello Phillip,
>
> Am 04.03.24 um 10:51 schrieb Phillip Wood:
>> On 03/03/2024 10:13, René Scharfe wrote:
>>> TEST_CTYPE_FUNC defines a function for testing a character classifier,
>>> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.
>>>
>>> Avoid the need to define a class-specific function by letting
>>> TEST_CHAR_CLASS do all the work. This is done by using the internal
>>> functions test__run_begin() and test__run_end(), but they do exist to be
>>> used in test macros after all.
>>
>> Those internal functions exist to implement the TEST() macro, they
>> are not really intended for use outside that (which is why they are
>> marked as private in the header file). If we ever want to update the
>> implementation of TEST() it will be a lot harder if we're using the
>> internal implementation directly in test files. Unit tests should be
>> wrapping TEST() if it is appropriate but not the internal
>> implementation directly.
>
> forcing tests to be expressions and not allow them to use statements is
> an unusual requirement. I don't see how the added friction would make
> tests any better. It just requires more boilerplate code and annoying
> repetition. What kind of changes do you envision that would be
> hindered by allowing statements?
On reflection I don't think I'm objecting to allowing statements, only
the use of the private functions to do so. If we tweak test__run_begin()
and test__run_end() so that the description is passed to
test__run_begin() and we invert the return value of that function to
match what test__run_end() is expecting then we can have
#define TEST_BEGIN(...) \
do { \
int run__ = test__run_begin(__VA_ARGS__); \
if (run__)
#define TEST_END \
test_run_end(run__); \
} while (0)
Which allow test authors to write
TEST_BEGIN("my test") {
/* test body here */
} TEST_END;
The macros insulate the test code from having to worry about
test_skip_all() and the "do { ... } while (0)" means that the compiler
will complain if the author forgets TEST_END. I'm slightly on the fence
about including the braces in the macros instead as that would make them
harder to misuse but it would be less obvious that the test body is run
in its own block. The compiler will allow the test author to
accidentally nest two calls to TEST_BEGIN() but there is an assertion in
test__run_begin() which will catch that at run time.
The slight downside compared to TEST() is that it is harder to return
early if a check fails - we'd need something like
TEST_BEGIN("my test") {
if (!check(0))
goto fail
/* more checks */
fail:
;
} TEST_END;
Also unlike TEST(), TEST_END does not indicate to the caller whether the
test failed or not but I'm not sure that matters in practice.
What do you think?
Best Wishes
Phillip
>> Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a
>> single function that was passed a ctype predicate, an input array and
>> an array of expected results. Unfortunately I don't think that is
>> possible due the the way the ctype predicates are implemented. Having
>> separate macros to define the test function and to run the test is
>> annoying but I don't think it is really worth exposing the internal
>> implementation just to avoid it.
>
> The classifiers are currently implemented as macros. We could turn them
> into inline functions and would then be able to pass them to a test
> function. Improving testability is a good idea, but also somehow feels
> like the tail wagging the dog. It would be easy, though, I think. And
> less gross than:
>
>>> Alternatively we could unroll the loop to provide a very long expression
>>> that tests all 256 characters and EOF and hand that to TEST, but that
>>> seems awkward and hard to read.
>
> ... which would yield unsightly test macros and huge test binaries. But
> it would certainly be possible, and keep the definitions of the actual
> tests clean.
>
> René
>
^ permalink raw reply
* Re: What's cooking in git.git (Mar 2024, #02; Thu, 7)
From: Phillip Wood @ 2024-03-09 11:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe
In-Reply-To: <xmqqttlghgqs.fsf@gitster.g>
On 08/03/2024 15:42, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> On 08/03/2024 02:26, Junio C Hamano wrote:
>>> * rs/t-ctype-simplify (2024-03-03) 4 commits
>>> (merged to 'next' on 2024-03-04 at 9bd84a8877)
>>> + t-ctype: avoid duplicating class names
>>> + t-ctype: align output of i
>>> + t-ctype: simplify EOF check
>>> + t-ctype: allow NUL anywhere in the specification string
>>> Code simplification to one unit-test program.
>>> Will merge to 'master'.
>>> source: <20240303101330.20187-1-l.s.r@web.de>
>>
>> I have some concerns about the last patch of this series
>> c.f. <0947cb09-8b07-4fcd-bbe2-ae37c2cd5ec7@gmail.com> It might be too
>> late for this series but using the internal implementation functions
>> rather than TEST() is not a pattern that I would want us to encourage.
>
> I actually think it is merely showing the lack of necessary features
> in TEST() and other public macros/functions of the relatively new
> low level test framework. If a user of the framework needs to use
> the "internal implementation functions", that gives an incentive to
> those who are pushing for the test framework to polish and update it,
> so that such a framework client does not have to go deep into the
> implementation details. When they come up with an improved framework,
> they naturally have the first target to adjust to the framework to
> demonstrate that they made the world a better place ;-)
I think that's fair - I'll flesh out the details of a couple of helper
macros TEST_BEGIN and TEST_END for tests like this in consultation with
René in the other thread.
I do hope though that unit test authors will feel free to improve the
framework themselves if they find it lacking as a contributor writing an
integration test would do with the integration test framework. As we get
more experience with unit tests were bound to want more helper functions.
> And I think such an update can come after the dust settles in this
> case. It is just a single simple test that is isolated and nobody
> other than the unit-test folks should care about, as the subject of
> test has not seen any change for a long time.
Agreed
Best Wishes
Phillip
^ permalink raw reply
* [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)
From: Johannes Schindelin via GitGitGadget @ 2024-03-09 14:09 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Dirk Gouders, Jeff King, Johannes Schindelin
Jeff King reported that Coverity pointed out a problem in the patch series
"The merge-base logic vs missing commit objects" (which made it into the
next branch already): The return value of merge_submodules() is assigned to
an unsigned, single-bit variable, which as a consequence is not able to hold
a negative value indicating a non-recoverable error.
I looked into this issue and am happy to report that there are no other
instances of the same issue in that patch series. The first patch in this
here patch series addresses that issue.
While looking into this issue I also noticed that the merge_submodule()
function did not even return negative values! This was an oversight on my
part (which I attribute with a large amount of self-compassion to my utter
lack of enthusiasm for submodules as a Git feature), and the second patch in
this here patch series addresses that.
This is a follow-up for
https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
based on the js/merge-base-with-missing-commit branch.
Johannes Schindelin (2):
merge-recursive: prepare for `merge_submodule()` to report errors
merge-ort/merge-recursive: do report errors in `merge_submodule()`
merge-ort.c | 5 +++++
merge-recursive.c | 21 +++++++++++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
base-commit: caaf1a2942c25c1f1a15818b718c9f641e52beef
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1686%2Fdscho%2Fmerge-base-and-missing-objects-followup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1686/dscho/merge-base-and-missing-objects-followup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1686
--
gitgitgadget
^ permalink raw reply
* [PATCH 1/2] merge-recursive: prepare for `merge_submodule()` to report errors
From: Johannes Schindelin via GitGitGadget @ 2024-03-09 14:09 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Dirk Gouders, Jeff King, Johannes Schindelin,
Johannes Schindelin
In-Reply-To: <pull.1686.git.1709993397.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The `merge_submodule()` function returns an integer that indicates
whether the merge was clean (returning 1) or unclean (returning 0).
Like the version in `merge-ort.c`, the version in `merge-recursive.c`
does not report any errors (such as repository corruption) by returning
-1 as of time of writing, even if the callers in `merge-ort.c` are
prepared for exactly such errors.
However, we want to teach (both variants of) the `merge_submodule()`
function that trick: to report errors by returning -1. Therefore,
prepare the caller in `merge-recursive.c` to handle that scenario.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-recursive.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 32e9d6665de..f3132a9ecae 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1426,13 +1426,14 @@ static int merge_mode_and_contents(struct merge_options *opt,
/* FIXME: bug, what if modes didn't match? */
result->clean = (merge_status == 0);
} else if (S_ISGITLINK(a->mode)) {
- result->clean = merge_submodule(opt, &result->blob.oid,
- o->path,
- &o->oid,
- &a->oid,
- &b->oid);
- if (result->clean < 0)
+ int clean = merge_submodule(opt, &result->blob.oid,
+ o->path,
+ &o->oid,
+ &a->oid,
+ &b->oid);
+ if (clean < 0)
return -1;
+ result->clean = clean;
} else if (S_ISLNK(a->mode)) {
switch (opt->recursive_variant) {
case MERGE_VARIANT_NORMAL:
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/2] merge-ort/merge-recursive: do report errors in `merge_submodule()`
From: Johannes Schindelin via GitGitGadget @ 2024-03-09 14:09 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Dirk Gouders, Jeff King, Johannes Schindelin,
Johannes Schindelin
In-Reply-To: <pull.1686.git.1709993397.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing
commits, 2024-02-28), I taught `merge_submodule()` to handle errors
reported by `repo_in_merge_bases_many()`.
However, those errors were not passed through to the callers. That was
unintentional, and this commit remedies that.
Note that `find_first_merges()` can now also return -1 (because it
passes through that return value from `repo_in_merge_bases()`), and this
commit also adds the forgotten handling for that scenario.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-ort.c | 5 +++++
merge-recursive.c | 8 ++++++++
2 files changed, 13 insertions(+)
diff --git a/merge-ort.c b/merge-ort.c
index 033c4348e2d..5d36c04f509 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt,
_("Failed to merge submodule %s "
"(repository corrupt)"),
path);
+ ret = -1;
goto cleanup;
}
if (ret2 > 0)
@@ -1829,6 +1830,7 @@ static int merge_submodule(struct merge_options *opt,
_("Failed to merge submodule %s "
"(repository corrupt)"),
path);
+ ret = -1;
goto cleanup;
}
if (!ret2) {
@@ -1848,6 +1850,7 @@ static int merge_submodule(struct merge_options *opt,
_("Failed to merge submodule %s "
"(repository corrupt)"),
path);
+ ret = -1;
goto cleanup;
}
if (ret2 > 0) {
@@ -1866,6 +1869,7 @@ static int merge_submodule(struct merge_options *opt,
_("Failed to merge submodule %s "
"(repository corrupt)"),
path);
+ ret = -1;
goto cleanup;
}
if (ret2 > 0) {
@@ -1899,6 +1903,7 @@ static int merge_submodule(struct merge_options *opt,
_("Failed to merge submodule %s "
"(repository corrupt)"),
path);
+ ret = -1;
break;
case 0:
path_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,
diff --git a/merge-recursive.c b/merge-recursive.c
index f3132a9ecae..fc772c2b113 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1246,12 +1246,14 @@ static int merge_submodule(struct merge_options *opt,
ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);
if (ret2 < 0) {
output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ ret = -1;
goto cleanup;
}
if (ret2 > 0)
ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);
if (ret2 < 0) {
output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ ret = -1;
goto cleanup;
}
if (!ret2) {
@@ -1263,6 +1265,7 @@ static int merge_submodule(struct merge_options *opt,
ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
if (ret2 < 0) {
output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ ret = -1;
goto cleanup;
}
if (ret2) {
@@ -1281,6 +1284,7 @@ static int merge_submodule(struct merge_options *opt,
ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
if (ret2 < 0) {
output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+ ret = -1;
goto cleanup;
}
if (ret2) {
@@ -1312,6 +1316,10 @@ static int merge_submodule(struct merge_options *opt,
parent_count = find_first_merges(&subrepo, &merges, path,
commit_a, commit_b);
switch (parent_count) {
+ case -1:
+ output(opt, 1,_("Failed to merge submodule %s (repository corrupt)"), path);
+ ret = -1;
+ break;
case 0:
output(opt, 1, _("Failed to merge submodule %s (merge following commits not found)"), path);
break;
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)
From: Elijah Newren @ 2024-03-09 16:51 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Dirk Gouders, Jeff King,
Johannes Schindelin
In-Reply-To: <pull.1686.git.1709993397.gitgitgadget@gmail.com>
On Sat, Mar 9, 2024 at 6:10 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Jeff King reported that Coverity pointed out a problem in the patch series
> "The merge-base logic vs missing commit objects" (which made it into the
> next branch already): The return value of merge_submodules() is assigned to
> an unsigned, single-bit variable, which as a consequence is not able to hold
> a negative value indicating a non-recoverable error.
>
> I looked into this issue and am happy to report that there are no other
> instances of the same issue in that patch series. The first patch in this
> here patch series addresses that issue.
>
> While looking into this issue I also noticed that the merge_submodule()
> function did not even return negative values! This was an oversight on my
> part (which I attribute with a large amount of self-compassion to my utter
> lack of enthusiasm for submodules as a Git feature), and the second patch in
> this here patch series addresses that.
>
> This is a follow-up for
> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
> based on the js/merge-base-with-missing-commit branch.
This series looks good to me; thanks.
^ permalink raw reply
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Philippe Blain @ 2024-03-09 17:22 UTC (permalink / raw)
To: Junio C Hamano, Philippe Blain via GitGitGadget; +Cc: git
In-Reply-To: <xmqqwmqiudna.fsf@gitster.g>
Hi Junio,
Le 2024-03-03 à 17:57, Junio C Hamano a écrit :
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> if (msg) {
>> - advise("%s\n", msg);
>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT, "%s\n", msg);
>> /*
>> * A conflict has occurred but the porcelain
>> * (typically rebase --interactive) wants to take care
>
> This hunk is good. The block removes the CHERRY_PICK_HEAD after
> giving this advice and then returns.
>
>> @@ -480,22 +480,25 @@ static void print_advice(struct repository *r, int show_hint,
>>
>> if (show_hint) {
>> if (opts->no_commit)
>> - advise(_("after resolving the conflicts, mark the corrected paths\n"
>> - "with 'git add <paths>' or 'git rm <paths>'"));
>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
>> + _("after resolving the conflicts, mark the corrected paths\n"
>> + "with 'git add <paths>' or 'git rm <paths>'"));
>> else if (opts->action == REPLAY_PICK)
>> - advise(_("After resolving the conflicts, mark them with\n"
>> - "\"git add/rm <pathspec>\", then run\n"
>> - "\"git cherry-pick --continue\".\n"
>> - "You can instead skip this commit with \"git cherry-pick --skip\".\n"
>> - "To abort and get back to the state before \"git cherry-pick\",\n"
>> - "run \"git cherry-pick --abort\"."));
>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
>> + _("After resolving the conflicts, mark them with\n"
>> + "\"git add/rm <pathspec>\", then run\n"
>> + "\"git cherry-pick --continue\".\n"
>> + "You can instead skip this commit with \"git cherry-pick --skip\".\n"
>> + "To abort and get back to the state before \"git cherry-pick\",\n"
>> + "run \"git cherry-pick --abort\"."));
>> else if (opts->action == REPLAY_REVERT)
>> - advise(_("After resolving the conflicts, mark them with\n"
>> - "\"git add/rm <pathspec>\", then run\n"
>> - "\"git revert --continue\".\n"
>> - "You can instead skip this commit with \"git revert --skip\".\n"
>> - "To abort and get back to the state before \"git revert\",\n"
>> - "run \"git revert --abort\"."));
>> + advise_if_enabled(ADVICE_SEQUENCER_CONFLICT,
>> + _("After resolving the conflicts, mark them with\n"
>> + "\"git add/rm <pathspec>\", then run\n"
>> + "\"git revert --continue\".\n"
>> + "You can instead skip this commit with \"git revert --skip\".\n"
>> + "To abort and get back to the state before \"git revert\",\n"
>> + "run \"git revert --abort\"."));
>> else
>> BUG("unexpected pick action in print_advice()");
>> }
>
> This hunk can be improved. If I were doing this patch, I probably
> would have just done
>
> - if (show_hint) {
> + if (show_hint && advice_enabled(ADVICE_SEQUENCER_CONFLICT)) {
>
> and nothing else, and doing so would keep the block easier to extend
> and maintain in the future.
>
> Because the block is all about "show_hint", we have code to print
> advice messages and nothing else in it currently, and more
> importantly, we will not add anything other than code to print
> advice messages in it. Because of that, skipping everything when
> ADVICE_SEQUENCER_CONFLICT is not enabled will not cause problems
> (unlike the earlier hunk---which will break if we added "&&
> advice_enabled()" to "if (msg)"). That way, when somebody teaches
> this code a new kind of opts->action, they do not have to say
> "advice_if_enabled(ADVICE_SEQUENCER_CONFLICT()"; they can just use
> "advise()".
That's true and makes the changes simpler, thank you for the suggestion.
I'll do that in v2.
Philippe.
^ permalink raw reply
* Re: [RFC PATCH 1/1] config: learn the "hostname:" includeIf condition
From: Junio C Hamano @ 2024-03-09 17:38 UTC (permalink / raw)
To: Ignacio Encinas Rubio; +Cc: git
In-Reply-To: <01a9baa2-b36b-4b4e-8e54-7645e35d1a47@iencinas.com>
Ignacio Encinas Rubio <ignacio@iencinas.com> writes:
>>> +test_expect_success 'conditional include, hostname' '
>>> + echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
>>> + echo "[test]twelve=12" >.git/bar12 &&
>>> + test_must_fail git config test.twelve &&
>>
>> Emulating other tests in this file that uses here document may make
>> it a bit easier to read? E.g.,
>>
>> cat >>.gitconfig <<-EOF &&
>> [includeIf "hostname:$(hostname)a"]
>> path = bar12
>> EOF
>
> Thanks for pointing that out. I just read the last tests from that file
> where they used the echo "..." >> approach. Do you think it is
> worthwhile rewriting those tests to use the approach you suggested?
I had an impression that existing ones do not have ugliness of
backslash-quoting and do not benefit from such a rewrite to use
here-document as much as the one you added does.
If that is not the case, and existing ones would concretely improve
the readability with such a rewrite to use here-document, surely.
If we want to do that route, we should either do one of the two.
- The patch [1/2] does such a "style update" only on existing tests
to improve their readability, and then patch [2/2] then does your
addition to the tests, together with the code change.
- Or you do this patch alone, without touching existing tests, but
with your tests added in an easier-to-read style. And then after
the dust settles, a separate "style udpate" patch clean the
existing ones up.
> By the way, before contributing, I saw there is some work on moving to
> unit tests. I wasn't sure how to test this particular feature there, so
> I went with the "old" approach as it seemed more natural. Is this ok?
We are not really "moving to". We did not have a test framework for
effective unit tests, so some tests that would have been easier to
manage as unit tests were instead written as an end-to-end
integration tests, which was what we had a framework for. These
tests are moving to, but for a test like "the user uses the
'[includeIf X:Y] path = P' construct---does the git command really
shows the effect of include from P when condition X:Y holds?", the
unit testing framework would not be a better fit than the end-to-end
behaviour test, I would say.
^ permalink raw reply
* Re: [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)
From: Junio C Hamano @ 2024-03-09 17:45 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Dirk Gouders, Jeff King,
Johannes Schindelin
In-Reply-To: <pull.1686.git.1709993397.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Jeff King reported that Coverity pointed out a problem in the patch series
> "The merge-base logic vs missing commit objects" (which made it into the
> next branch already): The return value of merge_submodules() is assigned to
> an unsigned, single-bit variable, which as a consequence is not able to hold
> a negative value indicating a non-recoverable error.
>
> I looked into this issue and am happy to report that there are no other
> instances of the same issue in that patch series. The first patch in this
> here patch series addresses that issue.
>
> While looking into this issue I also noticed that the merge_submodule()
> function did not even return negative values! This was an oversight on my
> part (which I attribute with a large amount of self-compassion to my utter
> lack of enthusiasm for submodules as a Git feature), and the second patch in
> this here patch series addresses that.
>
> This is a follow-up for
> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
> based on the js/merge-base-with-missing-commit branch.
Thanks.
>
> Johannes Schindelin (2):
> merge-recursive: prepare for `merge_submodule()` to report errors
> merge-ort/merge-recursive: do report errors in `merge_submodule()`
>
> merge-ort.c | 5 +++++
> merge-recursive.c | 21 +++++++++++++++------
> 2 files changed, 20 insertions(+), 6 deletions(-)
>
>
> base-commit: caaf1a2942c25c1f1a15818b718c9f641e52beef
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1686%2Fdscho%2Fmerge-base-and-missing-objects-followup-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1686/dscho/merge-base-and-missing-objects-followup-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1686
^ permalink raw reply
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Philippe Blain @ 2024-03-09 17:53 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood; +Cc: Philippe Blain via GitGitGadget, git
In-Reply-To: <xmqqy1axq3t1.fsf@gitster.g>
Hi Phillip and Junio,
Le 2024-03-04 à 12:56, Junio C Hamano a écrit :
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> ... So we probably do need a new config variable but
>> I think it should have a generic name - not be sequencer specific so
>> we can extend its scope in the future to "git merge", "git am -3",
>> "git stash" etc.
>
> A very good point. Thanks for your careful thinking.
OK, I agree we can make the new advice more generic, but I'm lacking
inspiration for the name. Maybe 'advice.mergeConflicted' ?
Or 'advice.resolveConflictedMerge' ? though this is close to the existing
'resolveConflict'...
Maybe just 'advice.mergeConflict' ?
Thanks,
Philippe.
^ permalink raw reply
* Re: [PATCH 2/2] merge-ort/merge-recursive: do report errors in `merge_submodule()`
From: Junio C Hamano @ 2024-03-09 17:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Dirk Gouders, Jeff King,
Johannes Schindelin
In-Reply-To: <50fe1a26515c06afec5ac7fb723727e1365a14fc.1709993397.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing
> commits, 2024-02-28), I taught `merge_submodule()` to handle errors
> reported by `repo_in_merge_bases_many()`.
>
> However, those errors were not passed through to the callers. That was
> unintentional, and this commit remedies that.
>
> Note that `find_first_merges()` can now also return -1 (because it
> passes through that return value from `repo_in_merge_bases()`), and this
> commit also adds the forgotten handling for that scenario.
Good clean-up. But this "oops, we did not check for errors" makes
me wonder if we are better off adopting "by default we assume an
error, until we are sure we are good" pattern, i.e.
func()
{
int ret = -1; /* assume worst */
do stuff;
if (...) {
error(_("this is bad"));
goto cleanup;
}
do stuff;
if (...) {
error(_("this is bad, too"));
goto cleanup;
}
/* ok we are happy */
ret = 0;
cleanup:
release resources;
return ret;
}
The patch to both functions do make it appear that they are good
candidates for application of the pattern to me.
Thanks.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> merge-ort.c | 5 +++++
> merge-recursive.c | 8 ++++++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 033c4348e2d..5d36c04f509 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0)
> @@ -1829,6 +1830,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (!ret2) {
> @@ -1848,6 +1850,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0) {
> @@ -1866,6 +1869,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0) {
> @@ -1899,6 +1903,7 @@ static int merge_submodule(struct merge_options *opt,
> _("Failed to merge submodule %s "
> "(repository corrupt)"),
> path);
> + ret = -1;
> break;
> case 0:
> path_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,
> diff --git a/merge-recursive.c b/merge-recursive.c
> index f3132a9ecae..fc772c2b113 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1246,12 +1246,14 @@ static int merge_submodule(struct merge_options *opt,
> ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2 > 0)
> ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (!ret2) {
> @@ -1263,6 +1265,7 @@ static int merge_submodule(struct merge_options *opt,
> ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2) {
> @@ -1281,6 +1284,7 @@ static int merge_submodule(struct merge_options *opt,
> ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
> if (ret2 < 0) {
> output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> goto cleanup;
> }
> if (ret2) {
> @@ -1312,6 +1316,10 @@ static int merge_submodule(struct merge_options *opt,
> parent_count = find_first_merges(&subrepo, &merges, path,
> commit_a, commit_b);
> switch (parent_count) {
> + case -1:
> + output(opt, 1,_("Failed to merge submodule %s (repository corrupt)"), path);
> + ret = -1;
> + break;
> case 0:
> output(opt, 1, _("Failed to merge submodule %s (merge following commits not found)"), path);
> break;
^ permalink raw reply
* Re: [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)
From: Junio C Hamano @ 2024-03-09 17:56 UTC (permalink / raw)
To: Elijah Newren
Cc: Johannes Schindelin via GitGitGadget, git, Patrick Steinhardt,
Dirk Gouders, Jeff King, Johannes Schindelin
In-Reply-To: <CABPp-BFKciBPN0WAaGaK4tb8hXit22Up4LMoJNxo-+DqEspD+A@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
>> This is a follow-up for
>> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,
>> based on the js/merge-base-with-missing-commit branch.
>
> This series looks good to me; thanks.
Thanks, both.
^ permalink raw reply
* Re: [PATCH] sequencer: allow disabling conflict advice
From: Philippe Blain @ 2024-03-09 18:01 UTC (permalink / raw)
To: phillip.wood, Philippe Blain via GitGitGadget, git
In-Reply-To: <3df4790a-7ee1-4c72-a3da-ba8a48d546b8@gmail.com>
Hi Phillip,
Le 2024-03-04 à 05:12, Phillip Wood a écrit :
> Hi Philippe
>
> On 02/03/2024 16:18, Philippe Blain via GitGitGadget wrote:
>
> It would also be good to update the "rebase --apply" implementation to respect this advice config to be consistent with "rebase --merge".
Yes, this is a good idea. This would take care of 'git am' at the same time
since both are implemented in builtin/am.c::die_user_resolve.
Thanks,
Philippe.
^ permalink raw reply
* [PATCH v2 0/1] Add hostname condition to includeIf
From: Ignacio Encinas @ 2024-03-09 18:18 UTC (permalink / raw)
To: git; +Cc: Ignacio Encinas
In-Reply-To: <20240307205006.467443-1-ignacio@iencinas.com>
Extend includeIf to take hostname into account. Motivating request can
be found here [1].
[1] https://github.com/gitgitgadget/git/issues/1665
Changes since v1:
* Add blank line between declarations and code in `include_by_branch`.
* Rewrite "echo"s used in tests to make them more readable.
config: learn the "hostname:" includeIf condition
Documentation/config.txt | 9 +++++++++
config.c | 17 ++++++++++++++++
t/t1305-config-include.sh | 42 +++++++++++++++++++++++++++++++++++++++
3 files changed, 68 insertions(+)
Range-diff against v1:
1: 10a9bca68753 ! 1: cf175154109e config: learn the "hostname:" includeIf condition
@@ config.c: static int include_by_branch(const char *cond, size_t cond_len)
+ int ret;
+ char my_host[HOST_NAME_MAX + 1];
+ struct strbuf pattern = STRBUF_INIT;
++
+ if (xgethostname(my_host, sizeof(my_host)))
+ return 0;
+
@@ t/t1305-config-include.sh: test_expect_success 'include cycles are detected' '
'
+test_expect_success 'conditional include, hostname' '
-+ echo "[includeIf \"hostname:$(hostname)a\"]path=bar12" >>.git/config &&
-+ echo "[test]twelve=12" >.git/bar12 &&
++ cat >>.git/config <<-EOF &&
++ [includeIf "hostname:$(hostname)a"]
++ path = bar12
++ EOF
++ cat >>.git/bar12 <<-EOF &&
++ [test]
++ twelve=12
++ EOF
++
+ test_must_fail git config test.twelve &&
+
-+ echo "[includeIf \"hostname:$(hostname)\"]path=bar12" >>.git/config &&
++ cat >>.git/config <<-EOF &&
++ [includeIf "hostname:$(hostname)"]
++ path = bar12
++ EOF
+ echo 12 >expect &&
+ git config test.twelve >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'conditional include, hostname, wildcard' '
-+ echo "[includeIf \"hostname:$(hostname)a*\"]path=bar13" >>.git/config &&
-+ echo "[test]thirteen=13" >.git/bar13 &&
++ cat >>.git/config <<-EOF &&
++ [includeIf "hostname:$(hostname)a*"]
++ path = bar13
++ EOF
++ cat >>.git/bar13 <<-EOF &&
++ [test]
++ thirteen = 13
++ EOF
++
+ test_must_fail git config test.thirteen &&
+
-+ echo "[includeIf \"hostname:$(hostname)*\"]path=bar13" >>.git/config &&
++ cat >>.git/config <<-EOF &&
++ [includeIf "hostname:$(hostname)*"]
++ path = bar13
++ EOF
+ echo 13 >expect &&
+ git config test.thirteen >actual &&
+ test_cmp expect actual
base-commit: e09f1254c54329773904fe25d7c545a1fb4fa920
--
2.44.0
^ 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