* Re: [PATCH 04/12] setup: start tracking ref storage format when
From: Junio C Hamano @ 2023-12-20 18:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <6564659d403de098799ddb8101b74c2803a655d4.1703067989.git.ps@pks.im>
There is a topic in-flight that introduces the .compat_hash_algo
member to the repo_fmt structure. Seeing a conflict resolution like
the attached (there are many others that are similar in spirit), I
have to wonder if we want to add repo_set_ref_storage_format()
helper function. There are many assignments to .ref_storage_format
member after this series is applied.
Note that I haven't read the series in full, so take the above with
a grain of salt---it might turn out to be that direct assignment is
more desirable, I dunno.
Thanks.
diff --cc setup.c
index 2f4571c12a,5038e78cdd..0000000000
--- i/setup.c
+++ w/setup.c
@@@ ...
@@@ -1584,8 -1577,8 +1595,10 @@@ const char *setup_git_directory_gently(
}
if (startup_info->have_repository) {
repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
+ repo_set_compat_hash_algo(the_repository,
+ repo_fmt.compat_hash_algo);
+ the_repository->ref_storage_format =
+ repo_fmt.ref_storage_format;
the_repository->repository_format_worktree_config =
repo_fmt.worktree_config;
/* take ownership of repo_fmt.partial_clone */
^ permalink raw reply
* Re: [PATCH 4/8] SubmittingPatches: update extra tags list
From: Josh Soref @ 2023-12-20 17:42 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, phillip.wood
In-Reply-To: <CABPp-BFjotLN4sCe+6uHAU7qhr1COM0B4EdW0f0-X-xf5qXinA@mail.gmail.com>
Elijah Newren wrote:
> Yeah, well it appears that those of us who have been here in Rome for
> a while aren't a fan of it anymore either; see also Junio's response
> in this thread about that.
Right.
> [Josh Soref <jsoref@gmail.com> wrote:]
> > Given that Noticed-by is more common than Co-authored-by, I have a
> > hard time arguing that it shouldn't be included.
>
> Not if you look at the last three years; there Co-authored-by is more
> than 17 times more common than Noticed-by.
> Given the large drop-off in usage of the Noticed-by trailer, I'd
> suggest just leaving it out.
> Perhaps replace
>
> You can also create your own tag or use one that's in common usage
> such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
>
> with
>
> While you can also create your own trailer if the situation warrants it, we
> encourage you to instead use one of the common trailers in this project
> highlighted above.
>
> ?
Let's have some fun (inception):
commit eac2211332f754c5f4127a58aafb9882bfe939e8
Author: Josh Soref <jsoref@gmail.com>
Date: Wed Dec 20 12:34:54 2023 -0500
SubmittingPatches: discourage new trailers
There seems to be consensus amongst the core Git community on a working
set of common trailers, and there are non-trivial costs to people
inventing new trailers (research to discover what they mean/how they
differ from existing trailers) such that inventing new ones is generally
unwarranted and not something to be recommended to new contributors.
Suggested-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Josh Soref <jsoref@gmail.com>
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 32e9023877..58dfe40504 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -358,4 +358,5 @@ If you like, you can put extra tags at the end:
-You can also create your own tag or use one that's in common usage
-such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
+While you can also create your own trailer if the situation warrants it, we
+encourage you to instead use one of the common trailers in this project
+highlighted above.
^ permalink raw reply related
* Re: [PATCH] Documentation/git-merge.txt: fix reference to synopsys
From: René Scharfe @ 2023-12-20 17:18 UTC (permalink / raw)
To: Elijah Newren; +Cc: Michael Lohmann, git, Michael Lohmann
In-Reply-To: <CABPp-BHBncDqCSvGm6Ow2D2+JQLf_3htwnxZ-RWV+tsxiH1rhg@mail.gmail.com>
Am 20.12.23 um 17:29 schrieb Elijah Newren:
>
> On Wed, Dec 20, 2023 at 7:46 AM René Scharfe <l.s.r@web.de> wrote:
>>
>> Am 20.12.23 um 08:05 schrieb Michael Lohmann:
>>
>>> +It is possible that a merge failure will prevent this process from being
>>> +completely automatic. "`git merge --continue`" and "`git merge --abort`"
>> ^^^^^^^^^
>> automatically
>
> Do you perhaps mean "completed automatically" (i.e. change both of the
> last two words in that sentence, and not just the last one)?
Possibly. This looks like a case of me making a mistake while criticizing
someone else's grammar, though. Which happens almost every time. o_O
René
^ permalink raw reply
* Re: [PATCH 7/8] SubmittingPatches: clarify GitHub artifact format
From: Elijah Newren @ 2023-12-20 17:00 UTC (permalink / raw)
To: Josh Soref; +Cc: git
In-Reply-To: <CACZqfqDVBHyBCgUwAJDv838cvF4xTo_UobA-_s6hqTzTG1c9qg@mail.gmail.com>
On Wed, Dec 20, 2023 at 8:16 AM Josh Soref <jsoref@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> wrote:
> > > From: Josh Soref <jsoref@gmail.com>
> > >
> > > GitHub wraps artifacts generated by workflows in a .zip file.
> > >
> > > Internally workflows can package anything they like in them.
> >
> > s/Internally/Internal/?
>
> No, it's actually:
>
> s/Internally/Internally,/
>
> In that, what a workflow does to structure the contents of a .zip
> artifact is an implementation decision of the workflow.
Ah, that makes sense with the comma; thanks.
^ permalink raw reply
* Re: [PATCH] Documentation/git-merge.txt: fix reference to synopsys
From: Junio C Hamano @ 2023-12-20 16:51 UTC (permalink / raw)
To: René Scharfe; +Cc: Michael Lohmann, git, Michael Lohmann
In-Reply-To: <c6814a39-b4f9-4b1e-b81b-45ffe4aa7466@web.de>
René Scharfe <l.s.r@web.de> writes:
>> +It is possible that a merge failure will prevent this process from being
>> +completely automatic. "`git merge --continue`" and "`git merge --abort`"
> ^^^^^^^^^
> automatically
>
>> +can only be run after the merge has resulted in conflicts.
>
> The connection between these two sentences feels weak to me. Are "merge
> failure" and "conflicts" the same? Perhaps something like this:
>
> A merge stops if there's a conflict that cannot be resolved
> automatically. At that point you can run `git merge --abort` or
> `git merge --continue`.
Just being nitpicky and curious, but does the abort/continue also
apply when you run "git merge --no-commit"?
I do not do documentation all that much these days, but when I was
involved heavily in discussions on documentation patches, I often
said "'git merge' gives back control to the user" to refer to both
cases, either because it couldn't complete the work it was asked to
do, or because it was asked to stop before completing the work.
> The only guidance I found is this paragraph from CodingGuidelines:
>
> Literal examples (e.g. use of command-line options, command names,
> branch names, URLs, pathnames (files and directories), configuration and
> environment variables) must be typeset in monospace (i.e. wrapped with
> backticks)
>
> So shouldn't we wrap all commands in backticks and nothing more?
> Probably worth a separate patch.
Thanks for a good review.
^ permalink raw reply
* Re: [PATCH 4/8] SubmittingPatches: update extra tags list
From: Elijah Newren @ 2023-12-20 16:49 UTC (permalink / raw)
To: Josh Soref; +Cc: git, phillip.wood
In-Reply-To: <CACZqfqAiSpGP5ABN7MZ50Za=-vAEKnqE0ravzEMbLJCByfRd8w@mail.gmail.com>
On Wed, Dec 20, 2023 at 8:09 AM Josh Soref <jsoref@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 10:30 AM Elijah Newren <newren@gmail.com> wrote:
> > On Wed, Dec 20, 2023 at 7:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > > Thanks for adding these, they are widely used and should be documented.
> > > The patch also adds a mention for "Noticed-by:" - I'm less convinced by
> > > the need for that as I explain below.
> > >
> > > > Updating the create suggestion to something less commonly used.
> > >
> > > I'm not quite sure I understand what you mean by this sentence.
>
> Tentatively rewritten as:
> Updating the "create your own tag" suggestion as 'Mentored-by' has been
> promoted.
>
> This commit is adding bulleted items including promoting 'Mentored-by',
> which means that the suggestion of "invent your own" would really need
> a new suggestion.
>
> Personally I'm not a fan of "invent your own", but I'm trying to
> follow "when in Rome" (which is a big thing in the Git process
> documentation covered by the two files subject to this series). More
> on this later.
Yeah, well it appears that those of us who have been here in Rome for
a while aren't a fan of it anymore either; see also Junio's response
in this thread about that.
> > > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > > > index 32e90238777..694a7bafb68 100644
> > > > @@ -348,6 +348,8 @@ If you like, you can put extra tags at the end:
> > > >
> > > > . `Reported-by:` is used to credit someone who found the bug that
> > > > the patch attempts to fix.
> > > > +. `Noticed-by:` liked `Reported-by:` indicates someone who noticed
> > > > + the item being fixed.
> > >
> > > I wonder if we really need a separate "Noticed-by" footer in addition to
> > > "Reported-by". Personally I use the latter to acknowledge the person who
> > > reported the issue being fix regards of weather I'm fixing a bug or some
> > > other issue.
> >
> > I'm not sure I'd mention both either; feels like we're making it hard
> > for users to choose. Also, I think there's a minor distinction
> > between them, but it's hard to convey simply: To me, Reported-by
> > suggests someone sent a mail to the list specifically about the bug or
> > issue in question. Also, to me, Noticed-by suggests that during a
> > back-and-forth discussion of some sort on another topic, a fellow Git
> > contributor noticed an issue and mentioned it as an aside. But,
> > that's how _I_ would have used them, I didn't do any digging to find
> > out if that's really how they are used.
>
> Given that Noticed-by is more common than Co-authored-by, I have a
> hard time arguing that it shouldn't be included.
Not if you look at the last three years; there Co-authored-by is more
than 17 times more common than Noticed-by.
> You could see that I struggled with it based on my lousy first drafts [1].
>
> Anyway, tentatively:
>
> . `Noticed-by:` like `Reported-by:` indicates a Git contributor who
> called the item (being fixed) out as an aside.
>
> Here, I'm struggling with the tension between "noticed-by probably
> hints that something is being fixed" and "noticed-by is addressing who
> suggested it and why we're attributing it to them"
>
> "as an aside" is itself an ellipsis for something like "as an aside to
> some unrelated discussion and didn't really circle back to it as a top
> level discussion point, but here we're closing the loop" -- but this
> is obviously way too long-winded to be the written form
Given the large drop-off in usage of the Noticed-by trailer, I'd
suggest just leaving it out.
> > Either way, if we're going to define them as synonyms, I'd rather we
> > just left the less common one out. If there's a distinction, and it's
> > not a pain to describe, then maybe it'd be worth adding both.
> >
> > If we do add both, though, we at least need to fix "liked" to "like"
> > in your description.
>
> Right, it's a "first draft" [1]...
:-)
[...]
> I personally agree. I think encouraging non-core contributors to
> invent their own is not a good idea. It leads to various things
> (including inconsistently cased items because users fail to review the
> current set / understand them/their-construction).
>
> Saying that it's ok for core contributors to suggest a new tag and
> that if a core contributor suggests a new tag that the person writing
> the current series should just accept the tag and trust that it'll be
> ok.
>
> Note: I'm not going to draft wording to this effect on my own, and if
> I were to provide such a change, it'd be its own commit prior to the
> one here, because it's a significant process change as opposed to
> clarifying the list of recommended tags.
Perhaps replace
You can also create your own tag or use one that's in common usage
such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
with
While you can also create your own trailer if the situation warrants it, we
encourage you to instead use one of the common trailers in this project
highlighted above.
?
^ permalink raw reply
* Re: [PATCH 6/8] SubmittingPatches: clarify GitHub visual
From: Junio C Hamano @ 2023-12-20 16:39 UTC (permalink / raw)
To: Elijah Newren
Cc: René Scharfe, Josh Soref via GitGitGadget, git, Josh Soref
In-Reply-To: <CABPp-BFU+4Xy8tVrU5qV3GX7Mr3-nOEtSgix3MSne5VVW2hz2Q@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> ... I'm tempted to just
> oversimplify ("...marked with red."), but am slightly concerned about
> red/green color-blind folks.
(sheepishly raises hand). Thanks for being considerate.
> I suspect they'd figure it out anyway by
> comparing the checkmarks (within green) to the x's (within red),
I 100% agree with this.
> but
> if we want to be more detailed, perhaps we drop the "cross" altogether
> and just describe it literally: "...marked with a red circle
> containing a white x."?
Thanks.
^ permalink raw reply
* Re: [PATCH 4/8] SubmittingPatches: update extra tags list
From: Junio C Hamano @ 2023-12-20 16:31 UTC (permalink / raw)
To: Phillip Wood; +Cc: Josh Soref via GitGitGadget, git, Elijah Newren, Josh Soref
In-Reply-To: <35fc350d-018a-49cf-a28e-e5ce21fe7dec@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> What's the difference between "Improved-by:" and "Helped-by:"? In
> general I'd prefer for us not to encourage new trailers where we
> already have a suitable one in use.
I agree with the direction to limit the trailer tokens to common
ones, as the repertoire we have sufficiently covers the needs
already, and strongly discourage folks to waste their time trying to
be "original".
It might be useful to run stats of trailers that were used in the
past, say, 3 years and that may give us a good guide to decide which
ones to limit ourselves to.
Thanks for reviewing.
^ permalink raw reply
* Re: [PATCH] Documentation/git-merge.txt: fix reference to synopsys
From: Elijah Newren @ 2023-12-20 16:29 UTC (permalink / raw)
To: René Scharfe; +Cc: Michael Lohmann, git, Michael Lohmann
In-Reply-To: <c6814a39-b4f9-4b1e-b81b-45ffe4aa7466@web.de>
Hi,
I'm getting in on the fun by adding a little nit-picking of my own. :-)
On Wed, Dec 20, 2023 at 7:46 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 20.12.23 um 08:05 schrieb Michael Lohmann:
>
> Thank you for this patch and sorry for the nitpicking below!
>
> > 437591a9d738 changed the synopsys from two separate lines for `--abort`
>
> "Synopsys" is a software company. A "synopsis" is a brief outline.
>
> > and `--continue` to a single line (and it also simultaneously added
> > `--quit`). That way the "enumeration" of the syntax for `--continue` is
> > no longer valid. Since `--quit` is now also part of the same syntax
> > line, a general statement cannot be made any more. Instead of trying to
> > enumerate the synopsys, be explicit in the limitations of when
> > respective actions are valid.
>
> Had to think a moment before I understood that "enumeration" refers to
> "The second syntax" and "The third syntax", which have been combined
> into this line:
>
> git merge (--continue | --abort | --quit)
>
> And it does make sense that we can no longer say "second syntax" and
> only refer to "git merge --abort", or "third syntax" and mean "git
> merge --continue". In other words: References by number are no longer
> valid after a merge of some of the synopses.
Thanks for explaining; I also missed that in reading over the original
patch. It'd be great if Michael could update the commit message to
make this a bit more clear.
> > This change also groups `--abort` and `--continue` together when
> > explaining the circumstances under which they can be run in order to
> > avoid duplication.
> >
> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> > ---
> > Documentation/git-merge.txt | 19 +++++++++----------
> > 1 file changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> > index e8ab340319..d8863cc943 100644
> > --- a/Documentation/git-merge.txt
> > +++ b/Documentation/git-merge.txt
> > @@ -46,21 +46,20 @@ a log message from the user describing the changes. Before the operation,
> > D---E---F---G---H master
> > ------------
> >
> > -The second syntax ("`git merge --abort`") can only be run after the
> > -merge has resulted in conflicts. 'git merge --abort' will abort the
> > -merge process and try to reconstruct the pre-merge state. However,
> > -if there were uncommitted changes when the merge started (and
> > -especially if those changes were further modified after the merge
> > -was started), 'git merge --abort' will in some cases be unable to
> > -reconstruct the original (pre-merge) changes. Therefore:
> > +It is possible that a merge failure will prevent this process from being
> > +completely automatic. "`git merge --continue`" and "`git merge --abort`"
> ^^^^^^^^^
> automatically
Do you perhaps mean "completed automatically" (i.e. change both of the
last two words in that sentence, and not just the last one)? That
would make sense to me, and I like that wording a little better. But
I think either you need to change both of the last two words of that
sentence (my preference), or neither of them.
> > +can only be run after the merge has resulted in conflicts.
>
> The connection between these two sentences feels weak to me.
This sentence is a bit more problematic than that: Even when there are
no conflicts, "git merge --no-commit" will also stop a merge, and one
can then use either --abort or --continue. So the assertion made by
this sentence that you're reviewing is not accurate.
> Perhaps something like this:
>
> A merge stops if there's a conflict that cannot be resolved
> automatically. At that point you can run `git merge --abort` or
> `git merge --continue`.
I like this alternative wording; it avoids the incorrect assertion and
uses something equivalent to the "completed automatically" suggested
above.
^ permalink raw reply
* Re: [PATCH 6/8] SubmittingPatches: clarify GitHub visual
From: Josh Soref @ 2023-12-20 16:25 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, René Scharfe
In-Reply-To: <CABPp-BFU+4Xy8tVrU5qV3GX7Mr3-nOEtSgix3MSne5VVW2hz2Q@mail.gmail.com>
Elijah Newren <newren@gmail.com> wrote:
> On Tue, Dec 19, 2023 at 6:44 AM René Scharfe <l.s.r@web.de> wrote:
> > Am 19.12.23 um 09:41 schrieb Josh Soref via GitGitGadget:
> > > From: Josh Soref <jsoref@gmail.com>
> > > Some people would expect a cross to be upright, and potentially have
> > > unequal lengths...
> > There are lots of types of crosses. And while looking them up on
> > Wikipedia I learned today that an x-cross is called "saltire" in
> > English. I only knew it as St. Andrew's cross before.
> >
> > > GitHub uses a white x overlaying a solid red circle to indicate failure.
> >
> > They call it "x-circle-fill"
> > (https://primer.github.io/octicons/x-circle-fill-16).
> > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > > @@ -604,7 +604,7 @@ to your fork of Git on GitHub. You can monitor the test state of all your
> > > branches here: `https://github.com/<Your GitHub handle>/git/actions/workflows/main.yml`
> > >
> > > If a branch did not pass all test cases then it is marked with a red
> > > -cross. In that case you can click on the failing job and navigate to
> > > ++x+. In that case you can click on the failing job and navigate to
> >
> > In the commit message you say the x is white, here it's red, so what is
> > it? IIUC the circle is red and the x-cross inside is the same color as
> > the background, i.e. white in light mode and black in dark mode. No
> > idea how to express that in one word. Perhaps "red circle containing
> > and x-cross"?
This was an oversimplification, which I deeply regret.
It uses a simple red x (❌) in some views, e.g.:
https://github.com/gitgitgadget/git/pull/1620
And in other views it uses a red circle with what's actually a
transparent x (white at the top if using light mode, gray fill if you
select linux-leaks on the left, and dark gray fill in the log view):
https://github.com/gitgitgadget/git/actions/runs/7265353611/job/19794849212?pr=1620
> There's an "and" vs "an" typo there, I think. I'm tempted to just
> oversimplify ("...marked with red."), but am slightly concerned about
> red/green color-blind folks. I suspect they'd figure it out anyway by
> comparing the checkmarks (within green) to the x's (within red), but
> if we want to be more detailed, perhaps we drop the "cross" altogether
> and just describe it literally: "...marked with a red circle
> containing a white x."?
Your text aligns with what I drafted as a response but didn't send:
I think it's simplest to say an `x`, or maybe "red color and an x".
^ permalink raw reply
* Re: [PATCH 7/8] SubmittingPatches: clarify GitHub artifact format
From: Josh Soref @ 2023-12-20 16:16 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <CABPp-BFbLN7dsKvm+___y6s8OWARhXB5BHq5N3sKWaf66RPzDg@mail.gmail.com>
Elijah Newren <newren@gmail.com> wrote:
> > From: Josh Soref <jsoref@gmail.com>
> >
> > GitHub wraps artifacts generated by workflows in a .zip file.
> >
> > Internally workflows can package anything they like in them.
>
> s/Internally/Internal/?
No, it's actually:
s/Internally/Internally,/
In that, what a workflow does to structure the contents of a .zip
artifact is an implementation decision of the workflow.
^ permalink raw reply
* Re: [PATCH 4/8] SubmittingPatches: update extra tags list
From: Josh Soref @ 2023-12-20 16:09 UTC (permalink / raw)
To: git; +Cc: phillip.wood, Elijah Newren
In-Reply-To: <CABPp-BH_iP2KjPi-5kW8ROQWfy8XoUmbGhyT-Y1dZGtCtZXQGQ@mail.gmail.com>
On Wed, Dec 20, 2023 at 10:30 AM Elijah Newren <newren@gmail.com> wrote:
> On Wed, Dec 20, 2023 at 7:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > Thanks for adding these, they are widely used and should be documented.
> > The patch also adds a mention for "Noticed-by:" - I'm less convinced by
> > the need for that as I explain below.
> >
> > > Updating the create suggestion to something less commonly used.
> >
> > I'm not quite sure I understand what you mean by this sentence.
Tentatively rewritten as:
Updating the "create your own tag" suggestion as 'Mentored-by' has been
promoted.
This commit is adding bulleted items including promoting 'Mentored-by',
which means that the suggestion of "invent your own" would really need
a new suggestion.
Personally I'm not a fan of "invent your own", but I'm trying to
follow "when in Rome" (which is a big thing in the Git process
documentation covered by the two files subject to this series). More
on this later.
> > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > > index 32e90238777..694a7bafb68 100644
> > > @@ -348,6 +348,8 @@ If you like, you can put extra tags at the end:
> > >
> > > . `Reported-by:` is used to credit someone who found the bug that
> > > the patch attempts to fix.
> > > +. `Noticed-by:` liked `Reported-by:` indicates someone who noticed
> > > + the item being fixed.
> >
> > I wonder if we really need a separate "Noticed-by" footer in addition to
> > "Reported-by". Personally I use the latter to acknowledge the person who
> > reported the issue being fix regards of weather I'm fixing a bug or some
> > other issue.
>
> I'm not sure I'd mention both either; feels like we're making it hard
> for users to choose. Also, I think there's a minor distinction
> between them, but it's hard to convey simply: To me, Reported-by
> suggests someone sent a mail to the list specifically about the bug or
> issue in question. Also, to me, Noticed-by suggests that during a
> back-and-forth discussion of some sort on another topic, a fellow Git
> contributor noticed an issue and mentioned it as an aside. But,
> that's how _I_ would have used them, I didn't do any digging to find
> out if that's really how they are used.
Given that Noticed-by is more common than Co-authored-by, I have a
hard time arguing that it shouldn't be included.
You could see that I struggled with it based on my lousy first drafts [1].
Anyway, tentatively:
. `Noticed-by:` like `Reported-by:` indicates a Git contributor who
called the item (being fixed) out as an aside.
Here, I'm struggling with the tension between "noticed-by probably
hints that something is being fixed" and "noticed-by is addressing who
suggested it and why we're attributing it to them"
"as an aside" is itself an ellipsis for something like "as an aside to
some unrelated discussion and didn't really circle back to it as a top
level discussion point, but here we're closing the loop" -- but this
is obviously way too long-winded to be the written form.
> Either way, if we're going to define them as synonyms, I'd rather we
> just left the less common one out. If there's a distinction, and it's
> not a pain to describe, then maybe it'd be worth adding both.
>
> If we do add both, though, we at least need to fix "liked" to "like"
> in your description.
Right, it's a "first draft" [1]...
> > > +. `Co-authored-by:` is used to indicate that multiple people
> > > + contributed to the work of a patch.
> >
> > Junio's comments in [1] may be helpful here
> >
> > If co-authors closely worked together (possibly but not necessarily
> > outside the public view), exchanging drafts and agreeing on the
> > final version before sending it to the list, by one approving the
> > other's final draft, Co-authored-by may be appropriate.
> >
> > I would prefer to see more use of Helped-by when suggestions for
> > improvements were made, possibly but not necessarily in a concrete
> > "squashable patch" form, the original author accepted before
> > sending the new version out, and the party who made suggestions saw
> > the updated version at the same time as the general public.
> >
> > So I think we should be clear that "Co-authored-by:" means that multiple
> > authors worked closely together on the patch, rather than just
> > contributed to it.
Tentatively:
. `Co-authored-by:` is used to indicate that people exchanged drafts
of a patch before submitting it.
Note that I'm dropping `multiple` -- people implies that.
. `Helped-by:` is used to credit someone who suggested ideas for
changes without providing the precise changes in patch form.
> > I think we use "Montored-by:" specifically to credit the mentor on
> > patches written by GSoC or Outreachy interns and "Helped-by:" for the
> > general case of someone helping the patch author.
>
> Yes; I'd like for these to be distinguished in this way or something similar.
. `Mentored-by:` is used to credit someone with helping develop a
patch as part of a mentorship program (e.g., GSoC or Outreachy).
> > > You can also create your own tag or use one that's in common usage
> > > -such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
> > > +such as "Thanks-to:", "Based-on-patch-by:", or "Improved-by:".
> >
> > What's the difference between "Improved-by:" and "Helped-by:"?
I dunno. This entire section (as I called out at the beginning) is
frustrating...
Anyway, see all of us struggle with it below:
> > In
> > general I'd prefer for us not to encourage new trailers where we already
> > have a suitable one in use.
If this text needs to survive without being drastically changed, it
needs a warning saying "don't create a new tag if there's an already
used tag that seems like it'll cover what you're trying to say --
here's how to review them...". Note that I'd argue the cost being
asked of someone to do this research far exceeds what is reasonable to
ask of a new contributor, which is why I'd rather say that new
contributors shouldn't be inventing tags.
> Agreed.
I personally agree. I think encouraging non-core contributors to
invent their own is not a good idea. It leads to various things
(including inconsistently cased items because users fail to review the
current set / understand them/their-construction).
Saying that it's ok for core contributors to suggest a new tag and
that if a core contributor suggests a new tag that the person writing
the current series should just accept the tag and trust that it'll be
ok.
Note: I'm not going to draft wording to this effect on my own, and if
I were to provide such a change, it'd be its own commit prior to the
one here, because it's a significant process change as opposed to
clarifying the list of recommended tags.
> > Thanks for working on this, it will be good to have better descriptions
> > of our common trailers.
>
> Agreed here as well; thanks for the work, Josh.
[1] The Late Show With Stephen Colbert has a running segment called
First Drafts which showcases lousy first drafts of greeting cards,
they've https://www.cbsstore.com/products/the-late-show-with-stephen-colbert-first-drafts-greeting-card-pack-sc1193
^ permalink raw reply
* Re: [PATCH] rebase: use strvec_pushf() for format-patch revisions
From: Junio C Hamano @ 2023-12-20 15:57 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <47089803-c45b-4f33-b542-146c313f0902@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 19.12.23 um 18:12 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>> In run_am(), a strbuf is used to create a revision argument that is then
>>> added to the argument list for git format-patch using strvec_push().
>>> Use strvec_pushf() to add it directly instead, simplifying the code.
>>>
>>> Signed-off-by: René Scharfe <l.s.r@web.de>
>>> ---
>>
>> Makes sense. Between the location of the original strbuf_addf()
>> call and the new strvec_pushf() call, nobody mucks with *opts so
>> this change won't affect the correctness. We no longer use the
>> extra strbuf, and upon failing to open the rebased-patches file,
>> we no longer leak the contents of it. Good.
>
> Ha! I didn't even notice that leak on error. Perhaps the two release
> calls at the end gave me the illusion of cleanliness? Or more likely
> the opportunity to use strvec_pushf() grabbed my full attention (tunnel
> vision).
Perhaps I'll amend the end of the log message, like so, before
merging it down to 'next', then.
..., simplifying the code and plugging a small leak on the error
codepath.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/8] Minor improvements to CodingGuidelines and SubmittingPatches
From: Elijah Newren @ 2023-12-20 15:43 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <pull.1623.git.1702975319.gitgitgadget@gmail.com>
On Tue, Dec 19, 2023 at 12:42 AM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> These are a bunch of things I've run into over my past couple of attempts to
> contribute to Git.
>
> * Incremental punctuation/grammatical improvements
> * Update extra tags suggestions based on common usage
> * drop reference to an article that was discontinued over a decade ago
> * update GitHub references
> * harmonize non-ASCII while I'm here
I commented on patches 4, 6, and 7 suggesting some changes. Patch 1
is "meh" to me, but it doesn't hurt anything. The other four all look
like good cleanups to me.
^ permalink raw reply
* Re: [PATCH] Documentation/git-merge.txt: fix reference to synopsys
From: René Scharfe @ 2023-12-20 15:43 UTC (permalink / raw)
To: Michael Lohmann, git; +Cc: Michael Lohmann
In-Reply-To: <20231220070528.8049-1-mi.al.lohmann@gmail.com>
Am 20.12.23 um 08:05 schrieb Michael Lohmann:
Thank you for this patch and sorry for the nitpicking below!
> 437591a9d738 changed the synopsys from two separate lines for `--abort`
"Synopsys" is a software company. A "synopsis" is a brief outline.
> and `--continue` to a single line (and it also simultaneously added
> `--quit`). That way the "enumeration" of the syntax for `--continue` is
> no longer valid. Since `--quit` is now also part of the same syntax
> line, a general statement cannot be made any more. Instead of trying to
> enumerate the synopsys, be explicit in the limitations of when
> respective actions are valid.
Had to think a moment before I understood that "enumeration" refers to
"The second syntax" and "The third syntax", which have been combined
into this line:
git merge (--continue | --abort | --quit)
And it does make sense that we can no longer say "second syntax" and
only refer to "git merge --abort", or "third syntax" and mean "git
merge --continue". In other words: References by number are no longer
valid after a merge of some of the synopses.
> This change also groups `--abort` and `--continue` together when
> explaining the circumstances under which they can be run in order to
> avoid duplication.
>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Documentation/git-merge.txt | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index e8ab340319..d8863cc943 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -46,21 +46,20 @@ a log message from the user describing the changes. Before the operation,
> D---E---F---G---H master
> ------------
>
> -The second syntax ("`git merge --abort`") can only be run after the
> -merge has resulted in conflicts. 'git merge --abort' will abort the
> -merge process and try to reconstruct the pre-merge state. However,
> -if there were uncommitted changes when the merge started (and
> -especially if those changes were further modified after the merge
> -was started), 'git merge --abort' will in some cases be unable to
> -reconstruct the original (pre-merge) changes. Therefore:
> +It is possible that a merge failure will prevent this process from being
> +completely automatic. "`git merge --continue`" and "`git merge --abort`"
^^^^^^^^^
automatically
> +can only be run after the merge has resulted in conflicts.
The connection between these two sentences feels weak to me. Are "merge
failure" and "conflicts" the same? Perhaps something like this:
A merge stops if there's a conflict that cannot be resolved
automatically. At that point you can run `git merge --abort` or
`git merge --continue`.
> +
> +'git merge --abort' will abort the merge process and try to reconstruct
> +the pre-merge state. However, if there were uncommitted changes when the
> +merge started (and especially if those changes were further modified
> +after the merge was started), 'git merge --abort' will in some cases be
> +unable to reconstruct the original (pre-merge) changes. Therefore:
>
> *Warning*: Running 'git merge' with non-trivial uncommitted changes is
> discouraged: while possible, it may leave you in a state that is hard to
> back out of in the case of a conflict.
>
> -The third syntax ("`git merge --continue`") can only be run after the
> -merge has resulted in conflicts.
What's with the quoting? It was inconsistent before, but I wonder what
would be correct here. Switching between straight single quotes ('')
and curved double quotes ("``") seems a bit arbitrary.
And I'm not even sure if these quotes really are what I think they are
based on https://docs.asciidoctor.org/asciidoc/latest/subs/quotes/. On
https://git-scm.com/docs/git-merge single quotes get rendered as <em>,
backticks as <code> (which makes sense) and curved double quotes as
<code> surrounded by straight double quotes (which looks weird).
The only guidance I found is this paragraph from CodingGuidelines:
Literal examples (e.g. use of command-line options, command names,
branch names, URLs, pathnames (files and directories), configuration and
environment variables) must be typeset in monospace (i.e. wrapped with
backticks)
So shouldn't we wrap all commands in backticks and nothing more?
Probably worth a separate patch.
> -
> OPTIONS
> -------
> :git-merge: 1
^ permalink raw reply
* Re: [PATCH 6/8] SubmittingPatches: clarify GitHub visual
From: Elijah Newren @ 2023-12-20 15:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Josh Soref via GitGitGadget, git, Josh Soref
In-Reply-To: <e788cf7b-56c7-48c2-ad4f-65d9c9e73ad5@web.de>
On Tue, Dec 19, 2023 at 6:44 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 19.12.23 um 09:41 schrieb Josh Soref via GitGitGadget:
> > From: Josh Soref <jsoref@gmail.com>
> >
> > Some people would expect a cross to be upright, and potentially have
> > unequal lengths...
>
> There are lots of types of crosses. And while looking them up on
> Wikipedia I learned today that an x-cross is called "saltire" in
> English. I only knew it as St. Andrew's cross before.
>
> > GitHub uses a white x overlaying a solid red circle to indicate failure.
>
> They call it "x-circle-fill"
> (https://primer.github.io/octicons/x-circle-fill-16).
>
> >
> > Signed-off-by: Josh Soref <jsoref@gmail.com>
> > ---
> > Documentation/SubmittingPatches | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > index d7a84f59478..8e19c7f82e4 100644
> > --- a/Documentation/SubmittingPatches
> > +++ b/Documentation/SubmittingPatches
> > @@ -604,7 +604,7 @@ to your fork of Git on GitHub. You can monitor the test state of all your
> > branches here: `https://github.com/<Your GitHub handle>/git/actions/workflows/main.yml`
> >
> > If a branch did not pass all test cases then it is marked with a red
> > -cross. In that case you can click on the failing job and navigate to
> > ++x+. In that case you can click on the failing job and navigate to
>
> In the commit message you say the x is white, here it's red, so what is
> it? IIUC the circle is red and the x-cross inside is the same color as
> the background, i.e. white in light mode and black in dark mode. No
> idea how to express that in one word. Perhaps "red circle containing
> and x-cross"?
There's an "and" vs "an" typo there, I think. I'm tempted to just
oversimplify ("...marked with red."), but am slightly concerned about
red/green color-blind folks. I suspect they'd figure it out anyway by
comparing the checkmarks (within green) to the x's (within red), but
if we want to be more detailed, perhaps we drop the "cross" altogether
and just describe it literally: "...marked with a red circle
containing a white x."?
^ permalink raw reply
* Re: [PATCH 4/8] SubmittingPatches: update extra tags list
From: Elijah Newren @ 2023-12-20 15:30 UTC (permalink / raw)
To: phillip.wood; +Cc: Josh Soref via GitGitGadget, git, Josh Soref
In-Reply-To: <35fc350d-018a-49cf-a28e-e5ce21fe7dec@gmail.com>
To add to what Phillip said...
On Wed, Dec 20, 2023 at 7:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Josh
>
> On 19/12/2023 08:41, Josh Soref via GitGitGadget wrote:
> > From: Josh Soref <jsoref@gmail.com>
> >
> > Add items with at least 100 uses:
> > - Co-authored-by
> > - Helped-by
> > - Mentored-by
> > - Suggested-by
>
> Thanks for adding these, they are widely used and should be documented.
> The patch also adds a mention for "Noticed-by:" - I'm less convinced by
> the need for that as I explain below.
>
> > Updating the create suggestion to something less commonly used.
>
> I'm not quite sure I understand what you mean by this sentence.
Same.
> > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > index 32e90238777..694a7bafb68 100644
> > --- a/Documentation/SubmittingPatches
> > +++ b/Documentation/SubmittingPatches
> > @@ -348,6 +348,8 @@ If you like, you can put extra tags at the end:
> >
> > . `Reported-by:` is used to credit someone who found the bug that
> > the patch attempts to fix.
> > +. `Noticed-by:` liked `Reported-by:` indicates someone who noticed
> > + the item being fixed.
>
> I wonder if we really need a separate "Noticed-by" footer in addition to
> "Reported-by". Personally I use the latter to acknowledge the person who
> reported the issue being fix regards of weather I'm fixing a bug or some
> other issue.
I'm not sure I'd mention both either; feels like we're making it hard
for users to choose. Also, I think there's a minor distinction
between them, but it's hard to convey simply: To me, Reported-by
suggests someone sent a mail to the list specifically about the bug or
issue in question. Also, to me, Noticed-by suggests that during a
back-and-forth discussion of some sort on another topic, a fellow Git
contributor noticed an issue and mentioned it as an aside. But,
that's how _I_ would have used them, I didn't do any digging to find
out if that's really how they are used.
Either way, if we're going to define them as synonyms, I'd rather we
just left the less common one out. If there's a distinction, and it's
not a pain to describe, then maybe it'd be worth adding both.
If we do add both, though, we at least need to fix "liked" to "like"
in your description.
> > . `Acked-by:` says that the person who is more familiar with the area
> > the patch attempts to modify liked the patch.
> > . `Reviewed-by:`, unlike the other tags, can only be offered by the
> > @@ -355,9 +357,17 @@ If you like, you can put extra tags at the end:
> > patch after a detailed analysis.
> > . `Tested-by:` is used to indicate that the person applied the patch
> > and found it to have the desired effect.
> > +. `Co-authored-by:` is used to indicate that multiple people
> > + contributed to the work of a patch.
>
> Junio's comments in [1] may be helpful here
>
> If co-authors closely worked together (possibly but not necessarily
> outside the public view), exchanging drafts and agreeing on the
> final version before sending it to the list, by one approving the
> other's final draft, Co-authored-by may be appropriate.
>
> I would prefer to see more use of Helped-by when suggestions for
> improvements were made, possibly but not necessarily in a concrete
> "squashable patch" form, the original author accepted before
> sending the new version out, and the party who made suggestions saw
> the updated version at the same time as the general public.
>
> So I think we should be clear that "Co-authored-by:" means that multiple
> authors worked closely together on the patch, rather than just
> contributed to it.
>
> [1] https://lore.kernel.org/git/xmqqmth8wwcf.fsf@gitster.g/
>
> > +. `Helped-by:` is used to credit someone with helping develop a
> > + patch.
> > +. `Mentored-by:` is used to credit someone with helping develop a
> > + patch.
>
> I think we use "Montored-by:" specifically to credit the mentor on
> patches written by GSoC or Outreachy interns and "Helped-by:" for the
> general case of someone helping the patch author.
Yes; I'd like for these to be distinguished in this way or something similar.
> > +. `Suggested-by:` is used to credit someone with suggesting the idea
> > + for a patch.
> >
> > You can also create your own tag or use one that's in common usage
> > -such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
> > +such as "Thanks-to:", "Based-on-patch-by:", or "Improved-by:".
>
> What's the difference between "Improved-by:" and "Helped-by:"? In
> general I'd prefer for us not to encourage new trailers where we already
> have a suitable one in use.
Agreed.
> Thanks for working on this, it will be good to have better descriptions
> of our common trailers.
Agreed here as well; thanks for the work, Josh.
^ permalink raw reply
* Re: [PATCH v6] subtree: fix split processing with multiple subtrees present
From: Christian Couder @ 2023-12-20 15:25 UTC (permalink / raw)
To: Zach FettersMoore; +Cc: Zach FettersMoore via GitGitGadget, git
In-Reply-To: <CAP8UFD19phFz54d8fDM=MBRMSD9Rz4R0_463KgptN8eeFs7MnQ@mail.gmail.com>
On Tue, Dec 12, 2023 at 5:06 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Dec 11, 2023 at 4:39 PM Zach FettersMoore
> <zach.fetters@apollographql.com> wrote:
> >
> > >>
> > >> From: Zach FettersMoore <zach.fetters@apollographql.com>
>
> > >> To see this in practice you can use the open source GitHub repo
> > >> 'apollo-ios-dev' and do the following in order:
> > >>
> > >> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'
> > >> directories
> > >> -Create a commit containing these changes
> > >> -Do a split on apollo-ios-codegen
> > >> - Do a fetch on the subtree repo
> > >> - git fetch git@github.com:apollographql/apollo-ios-codegen.git
> > >> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> >
> > > Now I get the following without your patch at this step:
> > >
> > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > > [...]/libexec/git-core/git-subtree: 318: Maximum function recursion
> > > depth (1000) reached
> > >
> > > Line 318 in git-subtree.sh contains the following:
> > >
> > > missed=$(cache_miss "$@") || exit $?
> > >
> > > With your patch it seems to work:
> > >
> > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > > Merge made by the 'ort' strategy.
> > > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4
> >
> > Looking into this some it looks like it could be a bash config
> > difference? My machine always runs it all the way through vs
> > failing for recursion depth. Although that would also be an issue
> > which is solved by this fix.
>
> I use Ubuntu where /bin/sh is dash so my current guess is that dash
> might have a smaller recursion limit than bash.
>
> I just found https://stackoverflow.com/questions/69493528/git-subtree-maximum-function-recursion-depth
> which seems to agree.
>
> I will try to test using bash soon.
Sorry, to not have tried earlier before with bash.
Now I have tried it and yeah it works fine with you patch, while
without it the last step of the reproduction recipe takes a lot of
time and results in a core dump:
/home/christian/libexec/git-core/git-subtree: line 924: 857920 Done
eval "$grl"
857921 Segmentation fault (core dumped) | while read rev parents; do
process_split_commit "$rev" "$parents";
done
So overall I think your patch is great! Thanks!
^ permalink raw reply
* Re: [PATCH 7/8] SubmittingPatches: clarify GitHub artifact format
From: Elijah Newren @ 2023-12-20 15:21 UTC (permalink / raw)
To: Josh Soref via GitGitGadget; +Cc: git, Josh Soref
In-Reply-To: <cdab65a425944e9d10f6aa6be378162cd5450c81.1702975320.git.gitgitgadget@gmail.com>
On Tue, Dec 19, 2023 at 12:42 AM Josh Soref via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Josh Soref <jsoref@gmail.com>
>
> GitHub wraps artifacts generated by workflows in a .zip file.
>
> Internally workflows can package anything they like in them.
s/Internally/Internal/?
>
> A recently generated failure artifact had the form:
>
> windows-artifacts.zip
> Length Date Time Name
> --------- ---------- ----- ----
> 76001695 12-19-2023 01:35 artifacts.tar.gz
> 11005650 12-19-2023 01:35 tracked.tar.gz
> --------- -------
> 87007345 2 files
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/SubmittingPatches | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 8e19c7f82e4..b4fa52ae348 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -606,7 +606,8 @@ branches here: `https://github.com/<Your GitHub handle>/git/actions/workflows/ma
> If a branch did not pass all test cases then it is marked with a red
> +x+. In that case you can click on the failing job and navigate to
> "ci/run-build-and-tests.sh" and/or "ci/print-test-failures.sh". You
> -can also download "Artifacts" which are tarred (or zipped) archives
> +can also download "Artifacts" which are zip archives containing
> +tarred (or zipped) archives
> with test data relevant for debugging.
>
> Then fix the problem and push your fix to your GitHub fork. This will
> --
> gitgitgadget
>
^ permalink raw reply
* Re: [PATCH 4/8] SubmittingPatches: update extra tags list
From: Phillip Wood @ 2023-12-20 15:18 UTC (permalink / raw)
To: Josh Soref via GitGitGadget, git; +Cc: Elijah Newren, Josh Soref
In-Reply-To: <e5c7f29af439c48f59b2f35af93a7972e66a5b6b.1702975320.git.gitgitgadget@gmail.com>
Hi Josh
On 19/12/2023 08:41, Josh Soref via GitGitGadget wrote:
> From: Josh Soref <jsoref@gmail.com>
>
> Add items with at least 100 uses:
> - Co-authored-by
> - Helped-by
> - Mentored-by
> - Suggested-by
Thanks for adding these, they are widely used and should be documented.
The patch also adds a mention for "Noticed-by:" - I'm less convinced by
the need for that as I explain below.
> Updating the create suggestion to something less commonly used.
I'm not quite sure I understand what you mean by this sentence.
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 32e90238777..694a7bafb68 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -348,6 +348,8 @@ If you like, you can put extra tags at the end:
>
> . `Reported-by:` is used to credit someone who found the bug that
> the patch attempts to fix.
> +. `Noticed-by:` liked `Reported-by:` indicates someone who noticed
> + the item being fixed.
I wonder if we really need a separate "Noticed-by" footer in addition to
"Reported-by". Personally I use the latter to acknowledge the person who
reported the issue being fix regards of weather I'm fixing a bug or some
other issue.
> . `Acked-by:` says that the person who is more familiar with the area
> the patch attempts to modify liked the patch.
> . `Reviewed-by:`, unlike the other tags, can only be offered by the
> @@ -355,9 +357,17 @@ If you like, you can put extra tags at the end:
> patch after a detailed analysis.
> . `Tested-by:` is used to indicate that the person applied the patch
> and found it to have the desired effect.
> +. `Co-authored-by:` is used to indicate that multiple people
> + contributed to the work of a patch.
Junio's comments in [1] may be helpful here
If co-authors closely worked together (possibly but not necessarily
outside the public view), exchanging drafts and agreeing on the
final version before sending it to the list, by one approving the
other's final draft, Co-authored-by may be appropriate.
I would prefer to see more use of Helped-by when suggestions for
improvements were made, possibly but not necessarily in a concrete
"squashable patch" form, the original author accepted before
sending the new version out, and the party who made suggestions saw
the updated version at the same time as the general public.
So I think we should be clear that "Co-authored-by:" means that multiple
authors worked closely together on the patch, rather than just
contributed to it.
[1] https://lore.kernel.org/git/xmqqmth8wwcf.fsf@gitster.g/
> +. `Helped-by:` is used to credit someone with helping develop a
> + patch.
> +. `Mentored-by:` is used to credit someone with helping develop a
> + patch.
I think we use "Montored-by:" specifically to credit the mentor on
patches written by GSoC or Outreachy interns and "Helped-by:" for the
general case of someone helping the patch author.
> +. `Suggested-by:` is used to credit someone with suggesting the idea
> + for a patch.
>
> You can also create your own tag or use one that's in common usage
> -such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
> +such as "Thanks-to:", "Based-on-patch-by:", or "Improved-by:".
What's the difference between "Improved-by:" and "Helped-by:"? In
general I'd prefer for us not to encourage new trailers where we already
have a suitable one in use.
Thanks for working on this, it will be good to have better descriptions
of our common trailers.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH 12/12] t9500: write "extensions.refstorage" into config
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]
In t9500 we're writing a custom configuration that sets up gitweb. This
requires us manually ensure that the repository format is configured as
required, including both the repository format version and extensions.
With the introduction of the "extensions.refStorage" extension we need
to update the test to also write this new one.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/t9500-gitweb-standalone-no-errors.sh | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 0333065d4d..7679780fb8 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -627,6 +627,7 @@ test_expect_success \
test_expect_success 'setup' '
version=$(git config core.repositoryformatversion) &&
algo=$(test_might_fail git config extensions.objectformat) &&
+ refstorage=$(test_might_fail git config extensions.refstorage) &&
cat >.git/config <<-\EOF &&
# testing noval and alternate separator
[gitweb]
@@ -637,6 +638,10 @@ test_expect_success 'setup' '
if test -n "$algo"
then
git config extensions.objectformat "$algo"
+ fi &&
+ if test -n "$refstorage"
+ then
+ git config extensions.refstorage "$refstorage"
fi
'
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 11/12] builtin/clone: introduce `--ref-format=` value flag
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]
Introduce a new `--ref-format` value flag for git-clone(1) that allows
the user to specify the ref format that is to be used for a newly
initialized repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-clone.txt | 6 ++++++
builtin/clone.c | 12 +++++++++++-
t/t5601-clone.sh | 17 +++++++++++++++++
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index c37c4a37f7..6e43eb9c20 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -311,6 +311,12 @@ or `--mirror` is given)
The result is Git repository can be separated from working
tree.
+--ref-format=<ref-format::
+
+Specify the given ref storage format for the repository. The valid values are:
++
+include::ref-storage-format.txt[]
+
-j <n>::
--jobs <n>::
The number of submodules fetched at the same time.
diff --git a/builtin/clone.c b/builtin/clone.c
index 20c161e94a..b635bbcb43 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -72,6 +72,7 @@ static char *remote_name = NULL;
static char *option_branch = NULL;
static struct string_list option_not = STRING_LIST_INIT_NODUP;
static const char *real_git_dir;
+static const char *ref_format;
static char *option_upload_pack = "git-upload-pack";
static int option_verbosity;
static int option_progress = -1;
@@ -157,6 +158,8 @@ static struct option builtin_clone_options[] = {
N_("any cloned submodules will be shallow")),
OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
N_("separate git dir from working tree")),
+ OPT_STRING(0, "ref-format", &ref_format, N_("format"),
+ N_("specify the reference format to use")),
OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
N_("set config inside the new repository")),
OPT_STRING_LIST(0, "server-option", &server_options,
@@ -930,6 +933,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
int submodule_progress;
int filter_submodules = 0;
int hash_algo;
+ int ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;
const int do_not_override_repo_unix_permissions = -1;
struct transport_ls_refs_options transport_ls_refs_options =
@@ -955,6 +959,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (option_single_branch == -1)
option_single_branch = deepen ? 1 : 0;
+ if (ref_format) {
+ ref_storage_format = ref_storage_format_by_name(ref_format);
+ if (ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
+ die(_("unknown ref storage format '%s'"), ref_format);
+ }
+
if (option_mirror)
option_bare = 1;
@@ -1106,7 +1116,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
* their on-disk data structures.
*/
init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN,
- REF_STORAGE_FORMAT_UNKNOWN, NULL,
+ ref_storage_format, NULL,
do_not_override_repo_unix_permissions, INIT_DB_QUIET | INIT_DB_SKIP_REFDB);
if (real_git_dir) {
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 47eae641f0..fb1b9c686d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -157,6 +157,23 @@ test_expect_success 'clone --mirror does not repeat tags' '
'
+test_expect_success 'clone with files ref format' '
+ test_when_finished "rm -rf ref-storage" &&
+ git clone --ref-format=files --mirror src ref-storage &&
+ echo files >expect &&
+ git -C ref-storage rev-parse --show-ref-format >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'clone with garbage ref format' '
+ cat >expect <<-EOF &&
+ fatal: unknown ref storage format ${SQ}garbage${SQ}
+ EOF
+ test_must_fail git clone --ref-format=garbage --mirror src ref-storage 2>err &&
+ test_cmp expect err &&
+ test_path_is_missing ref-storage
+'
+
test_expect_success 'clone to destination with trailing /' '
git clone src target-1/ &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 10/12] builtin/init: introduce `--ref-format=` value flag
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4852 bytes --]
Introduce a new `--ref-format` value flag for git-init(1) that allows
the user to specify the ref format that is to be used for a newly
initialized repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-init.txt | 7 +++++++
builtin/init-db.c | 13 ++++++++++++-
t/t0001-init.sh | 26 ++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 6f0d2973bf..e8dc645bb5 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -11,6 +11,7 @@ SYNOPSIS
[verse]
'git init' [-q | --quiet] [--bare] [--template=<template-directory>]
[--separate-git-dir <git-dir>] [--object-format=<format>]
+ [--ref-format=<format>]
[-b <branch-name> | --initial-branch=<branch-name>]
[--shared[=<permissions>]] [<directory>]
@@ -57,6 +58,12 @@ values are 'sha1' and (if enabled) 'sha256'. 'sha1' is the default.
+
include::object-format-disclaimer.txt[]
+--ref-format=<format>::
+
+Specify the given ref storage format for the repository. The valid values are:
++
+include::ref-storage-format.txt[]
+
--template=<template-directory>::
Specify the directory from which templates will be used. (See the "TEMPLATE
diff --git a/builtin/init-db.c b/builtin/init-db.c
index b6e80feab6..770b2822fe 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -58,6 +58,7 @@ static int shared_callback(const struct option *opt, const char *arg, int unset)
static const char *const init_db_usage[] = {
N_("git init [-q | --quiet] [--bare] [--template=<template-directory>]\n"
" [--separate-git-dir <git-dir>] [--object-format=<format>]\n"
+ " [--ref-format=<format>]\n"
" [-b <branch-name> | --initial-branch=<branch-name>]\n"
" [--shared[=<permissions>]] [<directory>]"),
NULL
@@ -77,8 +78,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
const char *template_dir = NULL;
unsigned int flags = 0;
const char *object_format = NULL;
+ const char *ref_format = NULL;
const char *initial_branch = NULL;
int hash_algo = GIT_HASH_UNKNOWN;
+ int ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;
int init_shared_repository = -1;
const struct option init_db_options[] = {
OPT_STRING(0, "template", &template_dir, N_("template-directory"),
@@ -96,6 +99,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
N_("override the name of the initial branch")),
OPT_STRING(0, "object-format", &object_format, N_("hash"),
N_("specify the hash algorithm to use")),
+ OPT_STRING(0, "ref-format", &ref_format, N_("format"),
+ N_("specify the reference format to use")),
OPT_END()
};
@@ -159,6 +164,12 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
die(_("unknown hash algorithm '%s'"), object_format);
}
+ if (ref_format) {
+ ref_storage_format = ref_storage_format_by_name(ref_format);
+ if (ref_storage_format == REF_STORAGE_FORMAT_UNKNOWN)
+ die(_("unknown ref storage format '%s'"), ref_format);
+ }
+
if (init_shared_repository != -1)
set_shared_repository(init_shared_repository);
@@ -237,6 +248,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
flags |= INIT_DB_EXIST_OK;
return init_db(git_dir, real_git_dir, template_dir, hash_algo,
- REF_STORAGE_FORMAT_UNKNOWN, initial_branch,
+ ref_storage_format, initial_branch,
init_shared_repository, flags);
}
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 30ce752cc1..b131d665db 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -576,6 +576,32 @@ test_expect_success 'init with GIT_DEFAULT_REF_FORMAT=garbage' '
test_cmp expect err
'
+test_expect_success 'init with --ref-format=files' '
+ test_when_finished "rm -rf refformat" &&
+ git init --ref-format=files refformat &&
+ echo files >expect &&
+ git -C refformat rev-parse --show-ref-format >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 're-init with same format' '
+ test_when_finished "rm -rf refformat" &&
+ git init --ref-format=files refformat &&
+ git init --ref-format=files refformat &&
+ echo files >expect &&
+ git -C refformat rev-parse --show-ref-format >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'init with --ref-format=garbage' '
+ test_when_finished "rm -rf refformat" &&
+ cat >expect <<-EOF &&
+ fatal: unknown ref storage format ${SQ}garbage${SQ}
+ EOF
+ test_must_fail git init --ref-format=garbage refformat 2>err &&
+ test_cmp expect err
+'
+
test_expect_success MINGW 'core.hidedotfiles = false' '
git config --global core.hidedotfiles false &&
rm -rf newdir &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 09/12] builtin/rev-parse: introduce `--show-ref-format` flag
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]
Introduce a new `--show-ref-format` to git-rev-parse(1) that causes it
to print the ref format used by a repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Documentation/git-rev-parse.txt | 3 +++
builtin/rev-parse.c | 4 ++++
t/t1500-rev-parse.sh | 17 +++++++++++++++++
3 files changed, 24 insertions(+)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 912fab9f5e..546faf9017 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -307,6 +307,9 @@ The following options are unaffected by `--path-format`:
input, multiple algorithms may be printed, space-separated.
If not specified, the default is "storage".
+--show-ref-format::
+ Show the reference storage format used for the repository.
+
Other Options
~~~~~~~~~~~~~
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index fde8861ca4..99725a6ff1 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -1059,6 +1059,10 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
puts(the_hash_algo->name);
continue;
}
+ if (!strcmp(arg, "--show-ref-format")) {
+ puts(ref_storage_format_to_name(the_repository->ref_storage_format));
+ continue;
+ }
if (!strcmp(arg, "--end-of-options")) {
seen_end_of_options = 1;
if (filter & (DO_FLAGS | DO_REVS))
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 3f9e7f62e4..a669e592f1 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -208,6 +208,23 @@ test_expect_success 'rev-parse --show-object-format in repo' '
grep "unknown mode for --show-object-format: squeamish-ossifrage" err
'
+test_expect_success 'rev-parse --show-ref-format' '
+ test_detect_ref_format >expect &&
+ git rev-parse --show-ref-format >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'rev-parse --show-ref-format with invalid storage' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ (
+ cd repo &&
+ git config extensions.refstorage broken &&
+ test_must_fail git rev-parse --show-ref-format 2>err &&
+ grep "error: invalid value for ${SQ}extensions.refstorage${SQ}: ${SQ}broken${SQ}" err
+ )
+'
+
test_expect_success '--show-toplevel from subdir of working tree' '
pwd >expect &&
git -C sub/dir rev-parse --show-toplevel >actual &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 08/12] t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar
From: Patrick Steinhardt @ 2023-12-20 10:55 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]
Introduce a new GIT_TEST_DEFAULT_REF_FORMAT environment variable that
lets developers run the test suite with a different default ref format
without impacting the ref format used by non-test Git invocations. This
is modeled after GIT_TEST_DEFAULT_OBJECT_FORMAT, which does the same
thing for the repository's object format.
Adapt the setup of the `REFFILES` test prerequisite to be conditionally
set based on the default ref format.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/README | 3 +++
t/test-lib-functions.sh | 5 +++++
t/test-lib.sh | 11 ++++++++++-
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/t/README b/t/README
index 36463d0742..621d3b8c09 100644
--- a/t/README
+++ b/t/README
@@ -479,6 +479,9 @@ GIT_TEST_DEFAULT_HASH=<hash-algo> specifies which hash algorithm to
use in the test scripts. Recognized values for <hash-algo> are "sha1"
and "sha256".
+GIT_TEST_DEFAULT_REF_FORMAT=<format> specifies which ref storage format
+to use in the test scripts. Recognized values for <format> are "files".
+
GIT_TEST_NO_WRITE_REV_INDEX=<boolean>, when true disables the
'pack.writeReverseIndex' setting.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 03292602fb..61871b2a3b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1659,6 +1659,11 @@ test_detect_hash () {
test_hash_algo="${GIT_TEST_DEFAULT_HASH:-sha1}"
}
+# Detect the hash algorithm in use.
+test_detect_ref_format () {
+ echo "${GIT_TEST_DEFAULT_REF_FORMAT:-files}"
+}
+
# Load common hash metadata and common placeholder object IDs for use with
# test_oid.
test_oid_init () {
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4685cc3d48..fc93aa57e6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -542,6 +542,8 @@ export EDITOR
GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}"
export GIT_DEFAULT_HASH
+GIT_DEFAULT_REF_FORMAT="${GIT_TEST_DEFAULT_REF_FORMAT:-files}"
+export GIT_DEFAULT_REF_FORMAT
GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"
export GIT_TEST_MERGE_ALGORITHM
@@ -1745,7 +1747,14 @@ parisc* | hppa*)
;;
esac
-test_set_prereq REFFILES
+case "$GIT_DEFAULT_REF_FORMAT" in
+files)
+ test_set_prereq REFFILES;;
+*)
+ echo 2>&1 "error: unknown ref format $GIT_DEFAULT_REF_FORMAT"
+ exit 1
+ ;;
+esac
( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
test -z "$NO_CURL" && test_set_prereq LIBCURL
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox