* Re: [PATCH 2/2] orphan/unborn: fix use of 'orphan' in end-user facing messages
From: Rubén Justo @ 2023-12-20 21:21 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqa5q4skwg.fsf@gitster.g>
On 20-dic-2023 12:01:51, Junio C Hamano wrote:
> The two-patch series, whose second part is the message I am
> responding to, did not see much reaction, but since it came
> during an end-user puzzlement and was written to make the docs less
> puzzling, I am tempted to merge it down to 'next'.
>
> Thanks.
>
If you need some positive feedback; I've read your series and it looks
good to me and going in a good direction.
I think "unborn branch" is a more accurate term than "orphaned branch".
It's not perfect, but I don't have a better one to offer.
A nit in 1/2, which of course is not worth a re-roll, is a double blank
line near the end; which I suspect is unintentional.
Just in case anyone else is looking for the thread where the puzzlement
was reported:
https://lore.kernel.org/git/FE2AD666-88DE-4F70-8D6D-3A426689EB41@me.com/
Thank you.
^ permalink raw reply
* Re: [PATCH 1/2] Documentation/git-merge.txt: fix reference to synopsis
From: Junio C Hamano @ 2023-12-20 20:56 UTC (permalink / raw)
To: Michael Lohmann; +Cc: l.s.r, Elijah Newren, Michael Lohmann, git
In-Reply-To: <20231220195342.17590-2-mi.al.lohmann@gmail.com>
Michael Lohmann <mial.lohmann@gmail.com> writes:
> Also the previous version did not acknowledge that `--no-merge` would
> result in the precondition being fulfilled (thanks to Elijah Newren and
> Junio C Hamano for pointing that out).
This does not belong to the log message. Please write for those who
only read "git log" output after the work is merged and nothing
else. To them, errors in the previous attempt that was pointed out
by reviewers and corrected in this version do not exist.
It is perfectly fine to write something like the above after the
three-dash line. That is the place to clue reviewers about the
context of this round, reminding what happend in the previous
iteration and what the differences this round has, etc.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] Documentation/git-merge.txt: fix reference to synopsis
From: Elijah Newren @ 2023-12-20 20:45 UTC (permalink / raw)
To: Michael Lohmann; +Cc: l.s.r, gitster, Michael Lohmann, git
In-Reply-To: <20231220195342.17590-2-mi.al.lohmann@gmail.com>
Hi,
On Wed, Dec 20, 2023 at 11:54 AM Michael Lohmann <mial.lohmann@gmail.com> wrote:
>
> 437591a9d738 combined the synopsis of "The second syntax" (meaning `git
> merge --abort`) and "The third syntax" (for `git merge --continue`) into
> this single line:
>
> git merge (--continue | --abort | --quit)
>
> but it was still referred to when describing the preconditions that have
> to be fulfilled to run the respective actions. In other words:
> References by number are no longer valid after a merge of some of the
> synopses.
>
> Also the previous version did not acknowledge that `--no-merge` would
`--no-commit` rather than `--no-merge`.
> result in the precondition being fulfilled (thanks to Elijah Newren and
> Junio C Hamano for pointing that out).
>
> This change also groups `--abort` and `--continue` together when
> explaining the prerequisites in order to avoid duplication.
>
> Helped-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Documentation/git-merge.txt | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index e8ab340319..33ec5c6b19 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -46,21 +46,21 @@ 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:
> +A merge stops if there's a conflict that cannot be resolved
> +automatically or if `--no-commit` was provided when initiating the
> +merge. 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.
> -
> OPTIONS
> -------
> :git-merge: 1
> --
> 2.39.3 (Apple Git-145)
Otherwise, looks good.
^ permalink raw reply
* Re: [PATCH 02/12] treewide: remove unnecessary includes in source files
From: Elijah Newren @ 2023-12-20 20:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <xmqqedfgsm6u.fsf@gitster.g>
On Wed, Dec 20, 2023 at 11:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> diff --git a/trace2.c b/trace2.c
> >> index 6dc74dff4c7..d4220af9ae1 100644
> >> --- a/trace2.c
> >> ...
> > An in-flight topic seem to want to see git_env_bool() that is
> > declared in parse.h that is pulled in via inclusion of config.h
> > hence this hunk breaks 'seen'.
> >
> >> diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
> >> index d5ca0046c89..a0032ee3964 100644
> >> --- a/t/helper/test-trace2.c
> >> ...
> > An in-flight topic starts using "struct key_value_info" that is
> > available via the inclusion of "config.h", hence this hunk breaks
> > the build of 'seen'.
>
> It seems that we have gained another topic in flight that gets
> broken by this change. I can keep piling merge-fixes on top, but it
> does not look like a strategy that would scale well.
>
> Can we get this series thoroughly reviewed quickly to merge it down
> via 'next' to 'master' soonish, so that other topics can be rebased
> on the result, or is that too much to ask during the Winter lull?
>
> Thanks.
The Winter lull is my winter surge, so I can certainly quickly make
whatever changes are required (well, assuming I can shake this
fever...). But that doesn't help much with reviewing, since that
should be done by someone other than the author. However, these
particular type of changes are pretty innocuous; there's really not
anything clever going on, it's just a lot of gruntwork, and
does-it-compile is most of the review.
Anyway, I'll reroll, dropping or holding back any changes that
conflict with next or seen, and see if that encourages anyone to chime
in.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add builtin objectmode values support
From: Junio C Hamano @ 2023-12-20 20:07 UTC (permalink / raw)
To: git; +Cc: Joanna Wang, sunshine, tboegi
In-Reply-To: <xmqqedfrovsb.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> The attached is one possible way to plug the leak; I am not sure if
> it is the best one, though. One thing I like about the solution is
> that the approach makes sure that the mode attributes we would ever
> return are very tightly controlled and does not allow a buggy code
> to come up with "mode" to be passed to this new helper function to
> pass random and unsupported mode bits without triggering the BUG().
>
> attr.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
Anybody who want to propose a better leakfix (we cannot afford to do
with UNLEAK() as the number of leaked mode string will be unbounded)?
Otherwise, I'll squash it in to Jonanna's patch and merge it down to
'next'.
Thanks.
> diff --git c/attr.c w/attr.c
> index b03c20f768..679e42258c 100644
> --- c/attr.c
> +++ w/attr.c
> @@ -1250,10 +1250,34 @@ static struct object_id *default_attr_source(void)
> return &attr_source;
> }
>
> +static const char *interned_mode_string(unsigned int mode)
> +{
> + static struct {
> + unsigned int val;
> + char str[7];
> + } mode_string[] = {
> + { .val = 0040000 },
> + { .val = 0100644 },
> + { .val = 0100755 },
> + { .val = 0120000 },
> + { .val = 0160000 },
> + };
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(mode_string); i++) {
> + if (mode_string[i].val != mode)
> + continue;
> + if (!*mode_string[i].str)
> + snprintf(mode_string[i].str, sizeof(mode_string[i].str),
> + "%06o", mode);
> + return mode_string[i].str;
> + }
> + BUG("Unsupported mode 0%o", mode);
> +}
> +
> static const char *builtin_object_mode_attr(struct index_state *istate, const char *path)
> {
> unsigned int mode;
> - struct strbuf sb = STRBUF_INIT;
>
> if (direction == GIT_ATTR_CHECKIN) {
> struct object_id oid;
> @@ -1287,8 +1311,8 @@ static const char *builtin_object_mode_attr(struct index_state *istate, const ch
> else
> return ATTR__UNSET;
> }
> - strbuf_addf(&sb, "%06o", mode);
> - return strbuf_detach(&sb, NULL);
> +
> + return interned_mode_string(mode);
> }
>
>
^ permalink raw reply
* Re: [PATCH 2/2] orphan/unborn: fix use of 'orphan' in end-user facing messages
From: Junio C Hamano @ 2023-12-20 20:01 UTC (permalink / raw)
To: git
In-Reply-To: <xmqq4jhb977x.fsf@gitster.g>
The two-patch series, whose second part is the message I am
responding to, did not see much reaction, but since it came
during an end-user puzzlement and was written to make the docs less
puzzling, I am tempted to merge it down to 'next'.
Thanks.
^ permalink raw reply
* [PATCH 2/2] Documentation/git-merge.txt: use backticks for command wrapping
From: Michael Lohmann @ 2023-12-20 19:53 UTC (permalink / raw)
To: l.s.r, Elijah Newren, gitster; +Cc: Michael Lohmann, git
In-Reply-To: <c6814a39-b4f9-4b1e-b81b-45ffe4aa7466@web.de>
As René found in the guidance 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 all instances of single and double quotes for wraping said examples
were replaced with simple backticks.
Suggested-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Documentation/git-merge.txt | 50 ++++++++++++++++++-------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 33ec5c6b19..7332f53f2f 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -20,12 +20,12 @@ DESCRIPTION
-----------
Incorporates changes from the named commits (since the time their
histories diverged from the current branch) into the current
-branch. This command is used by 'git pull' to incorporate changes
+branch. This command is used by `git pull` to incorporate changes
from another repository and can be used by hand to merge changes
from one branch into another.
Assume the following history exists and the current branch is
-"`master`":
+`master`:
------------
A---B---C topic
@@ -33,7 +33,7 @@ Assume the following history exists and the current branch is
D---E---F---G master
------------
-Then "`git merge topic`" will replay the changes made on the
+Then `git merge topic` will replay the changes made on the
`topic` branch since it diverged from `master` (i.e., `E`) until
its current commit (`C`) on top of `master`, and record the result
in a new commit along with the names of the two parent commits and
@@ -57,7 +57,7 @@ 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
+*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.
@@ -74,8 +74,8 @@ include::merge-options.txt[]
If `--log` is specified, a shortlog of the commits being merged
will be appended to the specified message.
+
-The 'git fmt-merge-msg' command can be
-used to give a good default for automated 'git merge'
+The `git fmt-merge-msg` command can be
+used to give a good default for automated `git merge`
invocations. The automated message can include the branch description.
--into-name <branch>::
@@ -104,14 +104,14 @@ include::rerere-options.txt[]
present, apply it to the worktree.
+
If there were uncommitted worktree changes present when the merge
-started, 'git merge --abort' will in some cases be unable to
+started, `git merge --abort` will in some cases be unable to
reconstruct these changes. It is therefore recommended to always
-commit or stash your changes before running 'git merge'.
+commit or stash your changes before running `git merge`.
+
-'git merge --abort' is equivalent to 'git reset --merge' when
+`git merge --abort` is equivalent to `git reset --merge` when
`MERGE_HEAD` is present unless `MERGE_AUTOSTASH` is also present in
-which case 'git merge --abort' applies the stash entry to the worktree
-whereas 'git reset --merge' will save the stashed changes in the stash
+which case `git merge --abort` applies the stash entry to the worktree
+whereas `git reset --merge` will save the stashed changes in the stash
list.
--quit::
@@ -120,8 +120,8 @@ list.
stash entry will be saved to the stash list.
--continue::
- After a 'git merge' stops due to conflicts you can conclude the
- merge by running 'git merge --continue' (see "HOW TO RESOLVE
+ After a `git merge` stops due to conflicts you can conclude the
+ merge by running `git merge --continue` (see "HOW TO RESOLVE
CONFLICTS" section below).
<commit>...::
@@ -144,25 +144,25 @@ PRE-MERGE CHECKS
Before applying outside changes, you should get your own work in
good shape and committed locally, so it will not be clobbered if
there are conflicts. See also linkgit:git-stash[1].
-'git pull' and 'git merge' will stop without doing anything when
-local uncommitted changes overlap with files that 'git pull'/'git
-merge' may need to update.
+`git pull` and `git merge` will stop without doing anything when
+local uncommitted changes overlap with files that `git pull`/`git
+merge` may need to update.
To avoid recording unrelated changes in the merge commit,
-'git pull' and 'git merge' will also abort if there are any changes
+`git pull` and `git merge` will also abort if there are any changes
registered in the index relative to the `HEAD` commit. (Special
narrow exceptions to this rule may exist depending on which merge
strategy is in use, but generally, the index must match HEAD.)
-If all named commits are already ancestors of `HEAD`, 'git merge'
+If all named commits are already ancestors of `HEAD`, `git merge`
will exit early with the message "Already up to date."
FAST-FORWARD MERGE
------------------
Often the current branch head is an ancestor of the named commit.
-This is the most common case especially when invoked from 'git
-pull': you are tracking an upstream repository, you have committed
+This is the most common case especially when invoked from `git
+pull`: you are tracking an upstream repository, you have committed
no local changes, and now you want to update to a newer upstream
revision. In this case, a new commit is not needed to store the
combined history; instead, the `HEAD` (along with the index) is
@@ -269,7 +269,7 @@ Barbie's remark on your side. The only thing you can tell is that your
side wants to say it is hard and you'd prefer to go shopping, while the
other side wants to claim it is easy.
-An alternative style can be used by setting the "merge.conflictStyle"
+An alternative style can be used by setting the `merge.conflictStyle`
configuration variable to either "diff3" or "zdiff3". In "diff3"
style, the above conflict may look like this:
@@ -328,10 +328,10 @@ After seeing a conflict, you can do two things:
* Resolve the conflicts. Git will mark the conflicts in
the working tree. Edit the files into shape and
- 'git add' them to the index. Use 'git commit' or
- 'git merge --continue' to seal the deal. The latter command
+ `git add` them to the index. Use `git commit` or
+ `git merge --continue` to seal the deal. The latter command
checks whether there is a (interrupted) merge in progress
- before calling 'git commit'.
+ before calling `git commit`.
You can work through the conflict with a number of tools:
@@ -392,7 +392,7 @@ CONFIGURATION
branch.<name>.mergeOptions::
Sets default options for merging into branch <name>. The syntax and
- supported options are the same as those of 'git merge', but option
+ supported options are the same as those of `git merge`, but option
values containing whitespace characters are currently not supported.
include::includes/cmd-config-section-rest.txt[]
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* [PATCH 1/2] Documentation/git-merge.txt: fix reference to synopsis
From: Michael Lohmann @ 2023-12-20 19:53 UTC (permalink / raw)
To: l.s.r, Elijah Newren, gitster; +Cc: Michael Lohmann, git
In-Reply-To: <c6814a39-b4f9-4b1e-b81b-45ffe4aa7466@web.de>
437591a9d738 combined the synopsis of "The second syntax" (meaning `git
merge --abort`) and "The third syntax" (for `git merge --continue`) into
this single line:
git merge (--continue | --abort | --quit)
but it was still referred to when describing the preconditions that have
to be fulfilled to run the respective actions. In other words:
References by number are no longer valid after a merge of some of the
synopses.
Also the previous version did not acknowledge that `--no-merge` would
result in the precondition being fulfilled (thanks to Elijah Newren and
Junio C Hamano for pointing that out).
This change also groups `--abort` and `--continue` together when
explaining the prerequisites in order to avoid duplication.
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Documentation/git-merge.txt | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index e8ab340319..33ec5c6b19 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -46,21 +46,21 @@ 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:
+A merge stops if there's a conflict that cannot be resolved
+automatically or if `--no-commit` was provided when initiating the
+merge. 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.
-
OPTIONS
-------
:git-merge: 1
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* [PATCH 0/2] Documentation/git-merge.txt: fix reference to synopsis
From: Michael Lohmann @ 2023-12-20 19:53 UTC (permalink / raw)
To: l.s.r, Elijah Newren, gitster; +Cc: Michael Lohmann, git
In-Reply-To: <c6814a39-b4f9-4b1e-b81b-45ffe4aa7466@web.de>
Hey!
Thank you all for your great reviews!
> Thank you for this patch and sorry for the nitpicking below!
No need to be sorry - this is what I am here for since it is the only
way to learn ;-) (also I am not a native speaker, so I know I am
constantly making mistakes)
> "Synopsys" is a software company. A "synopsis" is a brief outline.
Oh yeah... Now I need a face-palm emoji :D This is what you get from
trusting the spell check for words you just tried to copy far too early
in the morning... And another thing to learn for me: don't submit
patches while you are not yet fully awake... Sorry for everyone who had
to read the first draft! That was indeed not very professional from
me...
> 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.
That sums it up a lot better. I wasn't happy with my first draft, but
couldn't come up with something better - now I used your explanation
with a slight variation.
> > 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.
Oh! Another thing I learned today! Thanks a lot for pointing that out!
I have to confess: I copied that sentence 1:1 from git-rebase - that is
also why it did not fit in with the next sentence... But Renés
suggestion just avoids this (and the "--continue") problem altogether,
so I went with a slight variation of it instead.
> I like this alternative wording
Same! I fumbled around with it just a bit to include your hint
> 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
We all make mistakes (and my own grammar is horrific...), but the more I
appreciate it when people suggest/ask things because that always gives
me the opportunity to learn as well. And you are totally right in that
this sentence does not "feel quite right" anyways, so I understand your
unease with it and why you wanted to discuss it ;-)
> Just being nitpicky and curious, but does the abort/continue also
> apply when you run "git merge --no-commit"?
Yes, indeed that seems to be the case (also pointed out simultaneously
by Elijah Newren). I extended Renés suggestion to mention it as well.
> > 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.
Indeed! That was very nice!
And I also added the suggested changes as a second patch. It applies
just fine to master without the first one, though that obviously would
leave the changed paragraphs from the first commit with the mixed
syntax, but that would just be a minor inconsistency until the first
patch (or a future version of it) is applied.
Cheers
Michael
Michael Lohmann (2):
Documentation/git-merge.txt: fix reference to synopsis
Documentation/git-merge.txt: use backticks for command wrapping
Documentation/git-merge.txt | 70 ++++++++++++++++++-------------------
1 file changed, 35 insertions(+), 35 deletions(-)
--
2.39.3 (Apple Git-145)
^ permalink raw reply
* Re: [PATCH 02/12] treewide: remove unnecessary includes in source files
From: Junio C Hamano @ 2023-12-20 19:34 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <xmqq1qc35sx2.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/trace2.c b/trace2.c
>> index 6dc74dff4c7..d4220af9ae1 100644
>> --- a/trace2.c
>> ...
> An in-flight topic seem to want to see git_env_bool() that is
> declared in parse.h that is pulled in via inclusion of config.h
> hence this hunk breaks 'seen'.
>
>> diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
>> index d5ca0046c89..a0032ee3964 100644
>> --- a/t/helper/test-trace2.c
>> ...
> An in-flight topic starts using "struct key_value_info" that is
> available via the inclusion of "config.h", hence this hunk breaks
> the build of 'seen'.
It seems that we have gained another topic in flight that gets
broken by this change. I can keep piling merge-fixes on top, but it
does not look like a strategy that would scale well.
Can we get this series thoroughly reviewed quickly to merge it down
via 'next' to 'master' soonish, so that other topics can be rebased
on the result, or is that too much to ask during the Winter lull?
Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/4] refs: improve handling of special refs
From: Junio C Hamano @ 2023-12-20 19:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Phillip Wood, Ramsay Jones
In-Reply-To: <cover.1702560829.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Patrick Steinhardt (4):
> wt-status: read HEAD and ORIG_HEAD via the refdb
> refs: propagate errno when reading special refs fails
> refs: complete list of special refs
> bisect: consistently write BISECT_EXPECTED_REV via the refdb
With the clear understanding that we plan to make those other than
FETCH_HEAD and MERGE_HEAD in the is_special_ref().special_refs[]
eventually not special at all, this round looked quite sensible to
me.
Let's merge it down to 'next'.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/7] reftable: fixes and optimizations (pt.2)
From: Junio C Hamano @ 2023-12-20 19:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cover.1703063544.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Patrick Steinhardt (7):
> reftable/stack: do not overwrite errors when compacting
> reftable/writer: fix index corruption when writing multiple indices
> reftable/record: constify some parts of the interface
> reftable/record: store "val1" hashes as static arrays
> reftable/record: store "val2" hashes as static arrays
> reftable/merged: really reuse buffers to compute record keys
> reftable/merged: transfer ownership of records when iterating
Something like this need to be split and sprinkled into relevant
steps in v2 in order to pass "make hdr-check", it seems.
reftable/reftable-record.h | 1 +
reftable/reftable-stack.h | 1 +
2 files changed, 2 insertions(+)
diff --git c/reftable/reftable-record.h w/reftable/reftable-record.h
index 83d252ec2c..fd1160615c 100644
--- c/reftable/reftable-record.h
+++ w/reftable/reftable-record.h
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at
#ifndef REFTABLE_RECORD_H
#define REFTABLE_RECORD_H
+#include <hash-ll.h>
#include <stdint.h>
/*
diff --git c/reftable/reftable-stack.h w/reftable/reftable-stack.h
index 1b602dda58..50b1a4f4dd 100644
--- c/reftable/reftable-stack.h
+++ w/reftable/reftable-stack.h
@@ -9,6 +9,7 @@ license that can be found in the LICENSE file or at
#ifndef REFTABLE_STACK_H
#define REFTABLE_STACK_H
+#include <hash-ll.h>
#include "reftable-writer.h"
/*
^ permalink raw reply related
* Re: reftable: How to represent deleted referees of symrefs in the reflog?
From: Han-Wen Nienhuys @ 2023-12-20 19:03 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jonathan Nieder, Terry Parker, Luca Milanesio
In-Reply-To: <ZVy0zKcmc8tjmgzs@tanuki>
On Tue, Nov 21, 2023 at 2:46 PM Patrick Steinhardt <ps@pks.im> wrote:
> To me it seems like deletions in this case only delete a particular log
> entry instead of the complete log for a particular reference. And some
> older discussion [1] seems to confirm my hunch that a complete reflog is
> deleted not with `log_type = 0x0`, but instead by writing the null
> object ID as new ID.
No, writing a null OID (more precisely a transition from $SHA1 =>
$ZERO) means that a branch was at $SHA1, and then was deleted. The
reflog continues to exist, and new entries may be added by reviving
the branch. That would add a $ZERO => $NEWSHA transition, but the
history of the branch prior to its deletion is retained.
> # This behaviour is a bit more on the weird side. We delete the
> # referee, and that causes the files backend to claim that the reflog
> # for HEAD is gone, as well. The reflog still exists though, as
> # demonstrated in the next command.
> $ git update-ref -m delete-main -d refs/heads/main
> $ git reflog show HEAD
> fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
This looks wrong to me. HEAD has a history, and that history didn't go
away because the current branch happened to be deleted.
> It kind of feels like the second step in the files backend where the
> reflog is claimed to not exist is buggy -- I'd have expected to still
right, I agree.
> see the reflog, as the HEAD reference exists quite alright and has never
> stopped to exist. And in the third step, I'd have expected to see three
> reflog entries, including the deletion that is indeed still present in
> the on-disk logfile.
>
> But with the reftable backend the problem becomes worse: we cannot even
> represent the fact that the reflog still exists, but that the deletion
> of the referee has caused the HEAD to point to the null OID, because the
> null OID indicates complete deletion of the reflog.
This doesn't match my recollection. See
https://github.com/git/git/pull/1215, or more precisely
https://github.com/git/git/blob/3b2c5ddb28fa42a8c944373bea2ca756b1723908/refs/reftable-backend.c#L1582
Removing the entire reflog means removing all the individual entries
using tombstones.
> Consequentially, if
> we wrote the null OID, we'd only be able to see the last log entry here.
>
> It may totally be that I'm missing something obvious here. But if not,
> it leaves us with a couple of options for how to fix it:
>
> 1. Disregard this edge case and accept that the reftable backend
> does not do exactly the same thing as the files backend in very
> specific situations like this.
I remember discussing with Jun that it would be acceptable to have
slight semantic differences if unavoidable for the reflogs, and there
should be a record of this in the list. I think there will always be
some differences: for example, dropping reflogs because of a dir/file
conflict seems like a bug rather than a feature.
> 2. Change the reftable format so that it can discern these cases,
> e.g. by indicating deletion via a new log type.
This will be a bit messy, because it means that every read of the
reflog has to special case the "deletion marker" to make sure it is
absent, and on every reflog write, you have to create a tombstone for
the deletion marker, to make sure the reflog exists. Also, what if you
have something that looks like:
refs/main@1 : 0000... => 345....
refs/main@0xfffffff: DELETION
the reflog doesn't exists ("DELETION"), and now it is recreated, ie.
the following is added:
refs/main@0xfffffff: TOMBSTONE DELETION (make sure reflog exists)
refs/main@2 : 0000... => abc.... (the entry we want to write)
the result is that the reflog would also surface the first entry (@1 ,
000... => 345... ) again. To prevent that from happening, you have to
write tombstones for all entries if you "delete the reflog", but then
do you really need a separate existence marker?
> None of these alternatives feel particularly great to me. In my opinion
> (2) is likely the best option, but would require us to change the format
> format that's already in use by Google in the context of multiple
> projects. So I'm not quite sure how thrilled you folks at Google and
> other users of the reftable library are with this prospect.
Google uses reftable for datacenter-local serving. The reflog is
stored in a database, because it's never involved in serving traffic.
When I implemented reftable-on-filesystem, I found a few bugs in the
reflog code because it was never exercised at Google. Terry/Jonathan
may correct me if my knowledge is outdated.
Luca manages large Gerrit installations. He might know if anyone has
been using reftable in the wild.
HTH.
--
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
^ permalink raw reply
* 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
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