* Re: [PATCH 0/3] some send-email --compose fixes
From: Junio C Hamano @ 2023-10-20 21:42 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231020100343.GA2194322@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> [culling the rather large cc, as we moving off the original topic]
>
> On Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:
>
>> and there's your perl array ref (from the square brackets, which are
>> necessary because we're sticking it in a hash value). But even before
>> your patch, this seems to end up as garbage. The code which reads
>> $parsed_line does not dereference the array.
>>
>> The patch to fix it is only a few lines (well, more than that with some
>> light editorializing in the comments):
>
> So here's the fix in a cleaned up form, guided by my own comments from
> earlier. ;) I think this is actually all orthogonal to the patch you are
> working on, so yours could either go on top or just be applied
> separately.
>
> [1/3]: doc/send-email: mention handling of "reply-to" with --compose
> [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
> [3/3]: send-email: handle to/cc/bcc from --compose message
Nice.
With the approach suggested to move the validation down to where the
necessary addresses are already all defined, Michael observed "whoa,
why am I getting stringified array ref?". If that is the only issue
in the approach, queuing these three patches first and then have
Michael's fix on top of them sounds like the cleanest thing to do.
Will queue on top of v2.42.0 to help those who may want to backport
these to the maintenance track.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Junio C Hamano @ 2023-10-20 21:45 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231020101310.GB2673716@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> - the handling for to/cc/bcc is totally broken.
It is good to see another evidence that "--compose" is probably not
as often as used as we thought. With enough bugs discovered,
perhaps someday we can declare "it cannot be that the feature is
used in the wild, without anybody getting hit by these bugs---let's
deprecate and eventually remove it" ;-)
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Jacob Stopak @ 2023-10-20 21:48 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <fd26df85661d554ced9d8e0445f75952@manjaro.org>
On Fri, Oct 20, 2023 at 08:48:12PM +0200, Dragan Simic wrote:
> On 2023-10-20 20:39, Jacob Stopak wrote:
> > This is a proposal / proof-of-concept for a new table-based output
> > format for the git status command, and for dry runs (-n) of the git add
> > command. This could be extended to create visual dry runs for other
> > other commands like rm, mv, restore, stash, commit, and clean.
>
> Huh, please don't get me wrong, but based on the examples provided below, I
> really think that's only wasted screen estate, providing little or no help
> in understanding the performed operations.
>
> I appreciate your effort, but IMHO it makes little sense from the usability
> standpoint.
>
Thanks for the quick (and honest ;) reply - I appreciate it and no offense
taken! But let me try to expand on my reasoning a bit.
I agree with you that Git users who are already comfortable with Git,
the command-line, and their workflows would be unlikely to use this in
their day to day work.
The main benefits of this format are for beginners and folks who
are still learning Git to use it as needed:
* To beginners, the concepts of working directory and "staging area"
can be very abstract. By representing these concepts as table columns
on the screen, (a format that 99% of humans are used to interpreting),
they become more tangible and intuitive to new users.
* In Git, changes fly around all over the place, in all sorts of
directions. Even small hints at this movement can be very helpful to
understand what the heck is going on. The table format (esp with
arrows used in the 'git add' version) highlights the "flow" of
changes through the workflow in a way that the current default format
doesn't. The current dry runs just show the filenames being added
without context of _where_ they come from and where they are going.
Not to mention many commands don't even have dry runs. This might
sound like a small thing, but to a newbie having that extra level of
confirmation and understanding can make a big difference.
* Git doesn't exactly have a reputation as a user-friendly tool, and
much of that stems from the difficulty of learning Git. So we should
try to make it more approachable to normal humans. This format
(esp if applied to a wide variety of commands as dry runs) would
provide a rudimentary visual output that is more intuitive to users.
* This flag doesn't change any default behavior, it can easily be
tossed on for newbie use (either when teaching a newbie or when the
newbie is practicing on their own). Given this usage, the screen
realestate is not really a concern. I.e. this would be used
specifically when needed for the extra info/clarity it provides,
not to be efficient with the terminal space.
That's my perspective anyway, but of course the point of this is to
propose it to the community and hear the response, so even if it's
not included it's still a good experience :D.
^ permalink raw reply
* Re: [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well
From: Junio C Hamano @ 2023-10-20 21:51 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Phillip Wood
In-Reply-To: <20231020093654.922890-3-oswald.buddenhagen@gmx.de>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> At least after the successive trimming of enum rebase_type mentioned in
> the previous commit, this code did exactly what imply_merge() does, so
> just call it instead.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Hmph, I do not recall suggesting it, but the resulting code does
make sense. ;-)
>
> ---
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
> builtin/rebase.c | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 44cc1eed12..4a093bb125 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
> if (options.strategy) {
> options.strategy = xstrdup(options.strategy);
> - switch (options.type) {
> - case REBASE_APPLY:
> - die(_("--strategy requires --merge or --interactive"));
> - case REBASE_MERGE:
> - /* compatible */
> - break;
> - case REBASE_UNSPECIFIED:
> - options.type = REBASE_MERGE;
> - break;
> - default:
> - BUG("unhandled rebase type (%d)", options.type);
> - }
> + imply_merge(&options, "--strategy");
> }
>
> if (options.root && !options.onto_name)
^ permalink raw reply
* Re: [PATCH v3 0/3] rebase refactoring
From: Junio C Hamano @ 2023-10-20 22:07 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
In-Reply-To: <20231020093654.922890-1-oswald.buddenhagen@gmx.de>
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> broken out of the bigger series, as the aggregation just unnecessarily holds it
> up.
>
> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>
> Oswald Buddenhagen (3):
> rebase: simplify code related to imply_merge()
> rebase: handle --strategy via imply_merge() as well
> rebase: move parse_opt_keep_empty() down
>
> builtin/rebase.c | 44 ++++++++++++++------------------------------
> 1 file changed, 14 insertions(+), 30 deletions(-)
Looking quite straight-forward and I didn't see anythihng
potentially controversial.
Will queue. Thanks.
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Dragan Simic @ 2023-10-20 23:02 UTC (permalink / raw)
To: Jacob Stopak; +Cc: git
In-Reply-To: <ZTL1wJIIK/5YWQK5.jacob@initialcommit.io>
On 2023-10-20 23:48, Jacob Stopak wrote:
> On Fri, Oct 20, 2023 at 08:48:12PM +0200, Dragan Simic wrote:
>> On 2023-10-20 20:39, Jacob Stopak wrote:
>> > This is a proposal / proof-of-concept for a new table-based output
>> > format for the git status command, and for dry runs (-n) of the git add
>> > command. This could be extended to create visual dry runs for other
>> > other commands like rm, mv, restore, stash, commit, and clean.
>>
>> Huh, please don't get me wrong, but based on the examples provided
>> below, I
>> really think that's only wasted screen estate, providing little or no
>> help
>> in understanding the performed operations.
>>
>> I appreciate your effort, but IMHO it makes little sense from the
>> usability
>> standpoint.
>>
> Thanks for the quick (and honest ;) reply - I appreciate it and no
> offense
> taken! But let me try to expand on my reasoning a bit.
Thank you!
> I agree with you that Git users who are already comfortable with Git,
> the command-line, and their workflows would be unlikely to use this in
> their day to day work.
>
> The main benefits of this format are for beginners and folks who
> are still learning Git to use it as needed:
Oh, I always do my best to put myself in the shoes of the targeted
audience. Maybe I sometimes fail at that, I don't know, but that's why
we're here to discuss it further.
> * To beginners, the concepts of working directory and "staging area"
> can be very abstract. By representing these concepts as table
> columns
> on the screen, (a format that 99% of humans are used to
> interpreting),
> they become more tangible and intuitive to new users.
Frankly, based on my rather broad experience, there are two primary
categories of the beginners in the world of version control software
(VCS), be it git or any other product:
1) People who are forced to use some VCS at work, and they actually
don't give a damn about it.
2) True enthusiasts who love what they do, and who love expanding their
knowledge.
For the first category, nothing helps. For the second category, a
nicely written tutorial is all they needed to start with, aided later
with the man pages, Stack Exchange, and perhaps some textbook.
> * In Git, changes fly around all over the place, in all sorts of
> directions. Even small hints at this movement can be very helpful
> to
> understand what the heck is going on. The table format (esp with
> arrows used in the 'git add' version) highlights the "flow" of
> changes through the workflow in a way that the current default
> format
> doesn't. The current dry runs just show the filenames being added
> without context of _where_ they come from and where they are going.
> Not to mention many commands don't even have dry runs. This might
> sound like a small thing, but to a newbie having that extra level
> of
> confirmation and understanding can make a big difference.
Please don't get me wrong, I understand your reasoning, but again, it
all comes down to the two categories described above. IMHO, the second
category will likely start turning off the default hints sooner than
turning the table formatting on. The first category will choose some
GUI anyway.
> * Git doesn't exactly have a reputation as a user-friendly tool, and
> much of that stems from the difficulty of learning Git. So we
> should
> try to make it more approachable to normal humans. This format
> (esp if applied to a wide variety of commands as dry runs) would
> provide a rudimentary visual output that is more intuitive to
> users.
No pain, no gain. That's the ancient mantra, but IMHO it still applies
very well to many things, and of course not to the first category
mentioned above. Nothing applies to that category.
> * This flag doesn't change any default behavior, it can easily be
> tossed on for newbie use (either when teaching a newbie or when the
> newbie is practicing on their own). Given this usage, the screen
> realestate is not really a concern. I.e. this would be used
> specifically when needed for the extra info/clarity it provides,
> not to be efficient with the terminal space.
As I already assumed above, the targeted audience will likely start
turning the default hints off, rather than turning the table formatting
on. Maybe I'm wrong there, who knows.
> That's my perspective anyway, but of course the point of this is to
> propose it to the community and hear the response, so even if it's
> not included it's still a good experience :D.
Let's hear more thoughts from other people, of course.
^ permalink raw reply
* Re: [RESEND] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Junio C Hamano @ 2023-10-20 23:19 UTC (permalink / raw)
To: Marc Branchaud
Cc: Oswald Buddenhagen, git, Phillip Wood, Christian Couder,
Charvi Mendiratta
In-Reply-To: <841c3b59-9e7c-4492-9d66-8af42c3222ea@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> [1] Makes me wonder if rebase should also support "squash -c"...
How would it be different from "fixup -c", though?
A "pick" followed by either "fixup -c" or "squash -c" will have the
same effect on the contents (i.e., apply the changes both commits
want to make to their respective parents) and present the end user
an editor buffer filled with the commit specified by the "-c"
option, right?
Or am I missing something?
^ permalink raw reply
* Re: [RFC PATCH 0/5] Introduce -t, --table for status/add commands
From: Junio C Hamano @ 2023-10-20 23:28 UTC (permalink / raw)
To: Dragan Simic; +Cc: Jacob Stopak, git
In-Reply-To: <d3bbe53c3b910f891c80465ea0c3f53f@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Please don't get me wrong, I understand your reasoning, but again, it
> all comes down to the two categories described above. IMHO, the
> second category will likely start turning off the default hints sooner
> than turning the table formatting on. The first category will choose
> some GUI anyway.
You are not alone in feeling the impedance mismatch between the
intended audience the patch(es) try to help (pointy-clicky GUI
users) and the codebase the patch(es) modify (perhaps spartan
command line interface). I did wonder why this is not made as a
part of sugarcoating the command line interface with some GUI that
shows what could be added, what has been added, and the stuff in its
"git status" equivalent.
^ permalink raw reply
* Re: [RESEND] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Marc Branchaud @ 2023-10-20 23:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Oswald Buddenhagen, git, Phillip Wood, Christian Couder,
Charvi Mendiratta
In-Reply-To: <xmqq1qdoq3tt.fsf@gitster.g>
On 2023-10-20 19:19, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
>
>> [1] Makes me wonder if rebase should also support "squash -c"...
>
> How would it be different from "fixup -c", though?
>
> A "pick" followed by either "fixup -c" or "squash -c" will have the
> same effect on the contents (i.e., apply the changes both commits
> want to make to their respective parents) and present the end user
> an editor buffer filled with the commit specified by the "-c"
> option, right?
>
> Or am I missing something?
No, you're not.
I should have added "as a convenience". Squash and fixup are similar
enough that it seems reasonable for them to both support -c. Saves
people from having to remember that only fixup allows -c.
M.
^ permalink raw reply
* Re: [RESEND] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Junio C Hamano @ 2023-10-21 0:22 UTC (permalink / raw)
To: Marc Branchaud
Cc: Oswald Buddenhagen, git, Phillip Wood, Christian Couder,
Charvi Mendiratta
In-Reply-To: <b5bc179d-46b6-4c48-bfe5-769dac38489b@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> I should have added "as a convenience". Squash and fixup are similar
> enough that it seems reasonable for them to both support -c. Saves
> people from having to remember that only fixup allows -c.
Yeah, "fixup" could have been a plain "squash" with some option. It
could have been two options, "-i" ("ignore message of this one") and
"-o" ("use message of this one alone"), and then today's "fixup"
would have been "squash -i", and today's "fixup -c" would have been
"squash -o".
But I agree that "squash -c" is something one may find tempting to
type, after learning "fixup -c".
I forgot to comment on the real contents of your review, by the way.
Everything you said was reasonable.
^ permalink raw reply
* Re: [PATCH v3 1/1] bugreport: include +i in outfile suffix as needed
From: Junio C Hamano @ 2023-10-21 0:39 UTC (permalink / raw)
To: Jacob Stopak, Emily Shaffer; +Cc: git
In-Reply-To: <20231016214045.146862-2-jacob@initialcommit.io>
Jacob Stopak <jacob@initialcommit.io> writes:
> int cmd_bugreport(int argc, const char **argv, const char *prefix)
> {
> struct strbuf buffer = STRBUF_INIT;
> struct strbuf report_path = STRBUF_INIT;
> int report = -1;
> time_t now = time(NULL);
> - struct tm tm;
> enum diagnose_mode diagnose = DIAGNOSE_NONE;
> char *option_output = NULL;
> - char *option_suffix = "%Y-%m-%d-%H%M";
> + char *option_suffix = "";
> + int option_suffix_is_from_user = 0;
> const char *user_relative_path = NULL;
> char *prefixed_filename;
> - size_t output_path_len;
> int ret;
> + int i = 0;
OK, I think between me and you, we stared at this piece of code long
enough to make ourselves numb. The original "at most one report per
a minute" default came from the very original in 238b439d
(bugreport: add tool to generate debugging info, 2020-04-16) and
that is what we are changing, so let me summon its author as an area
expert for a pair of fresh eyes to see if they can offer any new
insights.
Thanks.
https://lore.kernel.org/git/20231016214045.146862-1-jacob@initialcommit.io/
^ permalink raw reply
* Re: [PATCH] typo: fix the typo 'neeed' into 'needed' in the comment under merge-ort.c
From: 王常新 @ 2023-10-21 1:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqv8b1s0pf.fsf@gitster.g>
> 2023年10月21日 00:44，Junio C Hamano <gitster@pobox.com> 写道：
>
> 王常新 <wchangxin824@gmail.com> writes:
>
>> It is my official name. But the email address is not a valid one. Should I rewrite the commit message?
>
> We try to keep the name and address on Signed-off-by: the official
> one that we can give court if/when some copyright troll sues us (see
> Documentation/SubmittingPatches:sign-off), and one of them (if more
> than one developers signed off the patch) must match the primary
> author's name and address.
>
> Thanks.
Sorry about that, I am not quite familiar with the process. I mean I can receive emails at both @qq.com and @gmail.com, but <foril@foril.space> in the signed-off-by trailer in the commit message doesn’t actually exist.
Given this situation, I am unsure of the next steps to correct this issue. Would it be appropriate for me to use 'rebase -i' to amend the erroneous commit message, followed by a force push to update the PR on GitHub? After this, is the correct following step to add another comment with "/submit" to finalize the changes?
Sincerely apologize for any inconvenience my mistake may have caused and appreciate your guidance on resolving this matter. Your patience and support in this learning process mean a lot to me.
^ permalink raw reply
* Re: [PATCH v4 1/4] hex-ll: separate out non-hash-algo functions
From: Linus Arver @ 2023-10-21 4:14 UTC (permalink / raw)
To: Jonathan Tan, git
Cc: Calvin Wan, phillip.wood123, Junio C Hamano, Jonathan Tan
In-Reply-To: <02ecc00e9c7226c9eeb960cc49c8c03dcb182a38.1696021277.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> From: Calvin Wan <calvinwan@google.com>
>
> In order to further reduce all-in-one headers, separate out functions in
> hex.h that do not operate on object hashes into its own file, hex-ll.h,
Nit: I was wondering what the "-ll" in "hex-ll.h" meant, then found
d1cbe1e6d8 (hash-ll.h: split out of hash.h to remove dependency on
repository.h, 2023-04-22) which seems to have set the precedent for this
naming style. Might be worth including here.
^ permalink raw reply
* Re: [RESEND] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-21 7:26 UTC (permalink / raw)
To: Marc Branchaud
Cc: git, Junio C Hamano, Phillip Wood, Christian Couder,
Charvi Mendiratta
In-Reply-To: <841c3b59-9e7c-4492-9d66-8af42c3222ea@xiplink.com>
On Fri, Oct 20, 2023 at 05:40:01PM -0400, Marc Branchaud wrote:
>I think the original text's "those identified by" is a bit vague: Does
>"those" mean "messages" or "commits"? The sentence reads like "those"
>stands for "messages", but then of course you don't identify *messages*
>with "squash" commands.
>
fair enough, though the repetition makes it linguistically inferior.
>Maybe emphasize the word "only" in the sentence (i.e. spell it as
>'only').
>
that seems excessive to me. i'm not assuming that my readers are dumb.
> To really drive the point home it could say something like
> obtained 'only' from the "fixup -c" commit, dropping the
> messages of all the other involved commits
>
as above.
also, i'm actually uneasy about including the exact behavior in the
first place, as it codifies something questionable - a better response
from git would be complaining about it. i will drop it.
>> (having more than one "fixup -c" commit
>> +makes no sense, and only the message from the last one is used).
>
>"Makes no sense" seems a bit opinionated (although I agree with the
>sentiment).
>
i'm not terribly worried about readers who have an aversion towards
being told facts ...
anyway, i will use "is incorrect" instead, as it seems more to the
point.
>Also, you can legitimately have more than one "fixup -c" in the overall
>instruction set, as long as there's at least one "pick" command in
>between, e.g.
>
yes, but the context is a single fixup sequence. the above comments
about readers and repetition apply here, too.
>[1] Makes me wonder if rebase should also support "squash -c"...
>
the distinction is that "squash" combines the messages, while "fixup"
discards them, and the -c merely changes what is discarded. softening
that up seems counter-productive to me.
thanks
^ permalink raw reply
* Re: [PATCH] typo: fix the typo 'neeed' into 'needed' in the comment under merge-ort.c
From: Bagas Sanjaya @ 2023-10-21 8:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: 王常新, git
In-Reply-To: <xmqqfs25rzo1.fsf@gitster.g>
On 21/10/2023 00:06, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
>
>> On 20/10/2023 09:14, 王常新 wrote:
>>> It is my official name. But the email address is not a valid one. Should I rewrite the commit message?
>>>
>>
>> Please don't top-post, reply inline with appropriate context instead.
>>
>> Did you mean that you can't receive ML traffic on your @qq.com address?
>> If so, resend with your @gmail.com address as patch author (you need
>> to set user.name and user.email accordingly).
>
> Isn't that opposite from what we would normally recommend, though?
>
> If the true authorship e-mail is in an environment where sending
> patches are inconvenient, you would still want to do your commits
> under the identity you want to appear in the final history of the
> project, so you do not futz with user.name and user.email; you'd
> send a message with in-body header that shows an extra From: line
> (followed by a blank line) that records the true authorship from an
> environment whose sender e-mail address may differ.
>
> E.g. You would see these fields in the e-mail heeader:
>
> From: 王常新 <wchangxin824@gmail.com>
> Subject: [PATCH] merge-ort.c: comment typofix
>
> and your message would begin like so (indented only for illustration
> purposes---the real one should be flushed to the left edge of the
> page):
>
> From: 王常新 <real-email-address-of-mr-wang@do.ma.in>
>
> There is 'needed' misspelt as 'neeed' in the source file;
> fix it.
>
> Signed-off-by: 王常新 <real-email-address-of-mr-wang@do.ma.in>
>
> This feature is designed so that other people, different from the
> author of the patch, can relay it to the recipient(s) while
> preserving the authorship information.
>
> Although it is not needed in this case, you can override "Subject:"
> the same way with an in-body header, like so:
>
> From: 王常新 <real-email-address-of-mr-wang@do.ma.in>
> Subject: real title of the patch to be used
>
> There is 'needed' misspelt as 'neeed' in the source file;
> fix it.
>
> Signed-off-by: 王常新 <real-email-address-of-mr-wang@do.ma.in>
>
> and it would replace what we read from the Subject: e-mail header.
Thanks for the explanation! I was confused then...
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* Re: Is there any interest in localizing term delimiters in git messages?
From: Peter Krefting @ 2023-10-21 9:30 UTC (permalink / raw)
To: Git List
In-Reply-To: <CAP6f5Mmi=f4DPcFwfvEiJMdKMa0BUyZ019mc8uFXyOufgD4NjA@mail.gmail.com>
Alexander Shopov:
> Hello all,
>
> Is there any interest in being able to change the delimiters of the
> changeable terms in git messages?
>
> Typical example:
> ORIGINAL
> msgid " (use \"git rm --cached <file>...\" to unstage)"
I think there should be something indicating the variables, and with
Unicode there are better choices than the ASCII
less-than-greater-than, for instance U+2039/U+203A. In the same way,
we could also fix the quotation marks:
" (use “git rm --cached ‹file›...” to unstage)"
The source should perhaps still be ASCII-only to be compatible with
older systems, but we could create a en_US.UTF-8 localization file
that does the above, and apply similar changes to other localizations
(I have been thinking about doing it to the Swedish translation for a
while, but so far have not come around to; of course quoting differs
from language to language, with different styles for ‘English’,
“American”, „German”, ”Swedish” and «Norwegian», for instance; it is
all very confusing and difficult to get right).
--
\\// Peter - http://www.softwolves.pp.se/
^ permalink raw reply
* Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-21 10:27 UTC (permalink / raw)
To: Isoken June Ibizugbe, git; +Cc: christian.couder, gitster
In-Reply-To: <331e1ab3-2e8c-497d-a05d-ef197d664188@gmail.com>
On 19-oct-2023 21:20:24, Rubén Justo wrote:
> So, aside from the confusing message, this iteration looks good to me.
Reviewing again the messages in builtin/branch.c, if you finally
re-roll, maybe you want to add:
diff --git a/builtin/branch.c b/builtin/branch.c
index e7ee9bd0f1..31da889e95 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -777,7 +777,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
- die(_("HEAD not found below refs/heads!"));
+ die(_("HEAD not found below refs/heads"));
But if you don't include this nit, that's fine with me. I still think
the changes you've already made are correct.
Thank you.
^ permalink raw reply related
* Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines
From: Rubén Justo @ 2023-10-21 10:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Isoken June Ibizugbe, git, christian.couder
In-Reply-To: <xmqqwmvhqjyx.fsf@gitster.g>
On 20-oct-2023 10:31:18, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> > On 19-oct-2023 09:40:51, Isoken June Ibizugbe wrote:
> >
> >> As per the CodingGuidelines document, it is recommended that a single-line
> >> message provided to error messages such as die(), error() and warning(),
> >
> > This is confusing; some multi-line messages are fixed in this series.
What I expected to see is a re-roll with no mention about
single/multi-line.
> >> should start with a lowercase letter and should not end with a period.
> >> Also this patch fixes the tests broken by the changes.
>
> "Also this patch fixes the tests broken by the changes" -> "Adjust
> tests to match updated messages".
That is a more palatable description of why the tests are being touched
in this series. Thanks.
^ permalink raw reply
* Re: [PATCH] typo: fix the typo 'neeed' into 'needed' in the comment under merge-ort.c
From: Junio C Hamano @ 2023-10-21 17:26 UTC (permalink / raw)
To: 王常新; +Cc: git
In-Reply-To: <DE904895-230C-436E-B7DE-499E6E503DB9@gmail.com>
王常新 <wchangxin824@gmail.com> writes:
> Sorry about that, I am not quite familiar with the process. I mean
> I can receive emails at both @qq.com and @gmail.com, but
> <foril@foril.space> in the signed-off-by trailer in the commit
> message doesn’t actually exist.
I cannot tell you which between these two to use, as I do not know
your situation. When a contributor works on Git and send a patch as
an employee of a company, sometimes the employer wants to see their
name prominently shown in the commit, and that is why we see commits
by folks working on Git for GitHub for example with their
@GitHub.com addresses, even though they may have personal addresses
at @gmail.com. When a contribution is made as a hobbist (which I
was back when I started contributing to this project), people seem
to prefer using their personal address over using their work
address, so that the name and address recorded in the commit will
stay with them even when they move on.
Whatever name and address you choose, if you are using GGG, you'd
need to update your commits locally, perhaps like so:
$ git commit --amend --author="Name <a@dd.re.ss>"
(and in the editor you have a chance to make sure your sign-off
matches the authorship).
After that I would suppose that you force push the result to update
your pull-request and /submit again (I am not a user of GGG, so I
may have got the details wrong).
Thanks.
^ permalink raw reply
* Re: [PATCH 06/11] t: convert tests to not access reflog via the filesystem
From: Junio C Hamano @ 2023-10-21 23:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <eaac658bbfd8259ed9a3cce6ca3c8486d6682e8f.1697607222.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 5f505e2f353..b1d2c014132 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -90,7 +90,8 @@ test_expect_success "deleting current branch adds message to HEAD's log" '
> git symbolic-ref HEAD $m &&
> git update-ref -m delete-$m -d $m &&
> test_must_fail git show-ref --verify -q $m &&
> - grep "delete-$m$" .git/logs/HEAD
> + test-tool ref-store main for-each-reflog-ent HEAD >actual &&
> + grep "delete-$m$" actual
> '
>
> test_expect_success "deleting by HEAD adds message to HEAD's log" '
> @@ -99,7 +100,8 @@ test_expect_success "deleting by HEAD adds message to HEAD's log" '
> git symbolic-ref HEAD $m &&
> git update-ref -m delete-by-head -d HEAD &&
> test_must_fail git show-ref --verify -q $m &&
> - grep "delete-by-head$" .git/logs/HEAD
> + test-tool ref-store main for-each-reflog-ent HEAD >actual &&
> + grep "delete-by-head$" actual
> '
These are quite straight-forward.
> test_expect_success "verifying $m's log (logged by config)" '
> - test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
> + test_when_finished "git update-ref -d $m && git reflog expire --expire=all --all && rm -rf actual expect" &&
> test-tool ref-store main for-each-reflog-ent $m >actual &&
> test_cmp actual expect
> '
The approach forces us to assume that "git reflog expire" performs
correctly in order to test reflog, but it probably is OK---we'll
notice breakages in "reflog expire" in other tests, hopefully.
^ permalink raw reply
* Re: [PATCH 0/7] log: decorate pseudorefs and other refs
From: Junio C Hamano @ 2023-10-22 0:13 UTC (permalink / raw)
To: Andy Koppe; +Cc: git
In-Reply-To: <20231019193911.1669705-1-andy.koppe@gmail.com>
Andy Koppe <andy.koppe@gmail.com> writes:
> This patch series adds three slots to the color.decorate.<slot> config
> option:
> - 'symbol' for coloring the punctuation symbols used around the refs in
> decorations, which currently use the same color as the commit hash.
> - 'ref' for coloring refs other than branches, remote-tracking branches,
> tags and the stash, which currently are not colored when included in
> decorations through custom decoration filter options.
> - 'pseudoref' for coloring pseudorefs such as ORIG_HEAD or MERGE_HEAD.
> Include them in decorations by default.
>
> This series is to replace the 'decorate: add color.decorate.symbols
> config option' patch proposed at:
> https://lore.kernel.org/git/20231003205442.22963-1-andy.koppe@gmail.com
If that is the case, it probably would have been nicer to mark the
series as [PATCH v2].
Also, can you make messages [1/7]..[7/7] replies to [0/7] when you
send them out? It seems that all 8 of them (including the cover
letter) are replies to the previous round, which looked a bit
unusual.
As to the contents of the series:
[1/7] nicely lays out the color documentation; I do not think the
extra verbosity was absolutely needed for existing ones
(e.g., when a reader sees 'tag', the reader knows the color
will be applied to tags), but the more exotic ones the series
will be adding may deserve extra explanation on what they
are, so I guess it is OK.
[2/7] is a trivial readability improvement. It obviously should be
left outside the scope of this series, but we should notice
the same pattern in similar color tables (e.g., wt-status.c
has one, diff.c has another) and perform the same clean-up as
a #leftoverbits item.
[3/7] They way _NIL color is used to control the defaulting looked
a bit unusual, but clever way to use a non-constant color
defined elsewhere as its default. A similar trick is used in
wt-status.c:color() for STATUS_ONBRANCH, so this is nothing
new.
[4/7] The name of new member .include added to ref_namespace_info
will not be understood by anybody unless they are too deeply
obsessed by decoration mechansim. As the namespace_info
covers far wider interest, so a name that *shouts* that it is
about decoration filter must be used to be understood by
readers of the code.
To be quite honest, "decoration filter" is probably a name
that will not be understood by anybody, but coming up with a
better name for it is probably outside the scope of this
series.
[5/7] I am not sure if "other refs" should be an item in the
namespace_info array. If it is truly "catch-all", then
shouldn't the refs in other namespaces without their own
decoration (e.g. ones in refs/notes/ and refs/prefetch/) be
colored in the same way as this new class? And if so, having
it as an independent element that sits next to these other
classes smells like a strange design.
Another more worrying thing is that existing .ref members are
designed to never overlap with each other, but this one
obviously does. When a caller with a ref (or a pseudoref)
asks "which namespace does this one belong to", does the
existing code still do the right thing with this new element?
Without it, because there was no overlap, an implementation
can randomly search in the namespace_info table and stop at
the first hit, but now with the overlapping and widely open
.ref = "refs/", the implementation of the search must know
that it is a fallback position (i.e. if it found a match with
the fallback .ref = "refs/" , unless it looked at all other
entries that could begin with "refs/" and are more specific,
it needs to keep going).
[6/7] This is pretty straight-forward, assuming that the existing
is_pseudoref_syntax() function does the right thing. I am
not sure about that, though. A refname with '-' is allowed
to be called a pseudoref???
Also, not a fault of this patch, but the "_syntax" in its
name is totally unnecessary, I would think. At first glance,
I suspected that the excuse to append _syntax may have been
to signal the fact that the helper function does not check if
there actually is such a ref, but examining a few helpers
defined nearby tells us that such an excuse does not make
sense:
int is_per_worktree_ref(const char *) {
return starts_with(refname, "refs/worktree/") ||
starts_with(refname, "refs/bisect/") ||
starts_with(refname, "refs/rewritten/");
}
int is_pseudoref_syntax(const char *);
int is_current_worktree_ref(const char *ref) {
return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
}
All these three work on the refname and based on what is in
that refname string, decides what kind of ref it is. There
is nothing especially "syntax" about the second one, and we
should rename it as part of #leftoverbits clean-up effort.
Another unrelated tangent is that is_per_worktree_ref() shown
above and the namespace_info array we saw earlier are not
even aware of each other, which is maintenance nightmare
waiting to happen.
[7/7] Allowing pseudorefs to optionally used when decorating might
be a good idea, but I do not think it is particularly a good
design decision to enable it by default.
Each of them forming a separate "namespace" also looks like a
poor design, as being able to group multiple things into one
family and treat them the same way is the primary point of
"namespace", I would think. You do not want to say "I want
to decorate off of ORIG_HEAD and FETCH_HEAD"; instead you
would want to say "I want to decorate off of any pseudoref".
^ permalink raw reply
* [RFC PATCH 0/2] pretty: add %I formatting for patch-id
From: Michael McClimon @ 2023-10-22 2:27 UTC (permalink / raw)
To: git; +Cc: Michael McClimon
I'll say up front that this patch doesn't actually work, but I am stuck
on it and interested in continuing, and am hopeful someone can point me
in something closer to the right direction. (Also, the proposed log
messages are not good, and I would not actually submit the patch for
merge consideration with these messages.)
I would like to have a single-command way to get the patch id for a
commit: the thing you'd see in a pipeline like
git diff-tree --patch-with-raw HEAD | git patch-id
My initial thought was to add a --patch-id flag to git diff-tree, but
then I thought that maybe better would be to add a pretty specifier to
do so, so that (for instance) you could generate patch-ids for
everything in a branch by saying something like
git log --pretty='%I %H' start..
I have taken a pass at doing so here, but it doesn't _work_, and I'm not
sure why. It seemed like the thing to do was to use commit_patch_id from
patch-ids.c, but I think that either I'm not holding it correctly, or
that it's not fit for purpose. It was added (or rather, made public) in
ded2c097 (patch-ids: make commit_patch_id() a public helper function,
2016-04-26), which was in service of fa2ab86d (format-patch: add
'--base' option to record base tree info, 2016-04-26). It seems like at
the very least, it probably won't work as expected on merge commits.
The thing that is perplexing to me is that it _does_ appear to work on
some commits, for example (where 8b3aa36f is a recent-ish commit from
master, chosen at random):
$ ./git --no-pager show -s --format='%I %H' 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08
ec66952bdef82a1fc6d31c1057195af31c86da48 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08
$ git diff-tree --patch-with-raw 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08 | git patch-id
ec66952bdef82a1fc6d31c1057195af31c86da48 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08
But for other commits, like the one in the test here, it does not. I
have done a bit of investigation, but I would not really call myself a C
programmer and I'm not super familiar with the codebase, so I'm a bit
stuck. I thought maybe at first I wasn't initializing the diff_options
correctly, but I suspect the problem is actually more fundamental than
that.
Anyway: I would be happy to hear any response at all, whether that's
a) a pointer to some other way of implementing a %I pretty format,
b) an easier-to-implement possible solution (maybe diff-tree --patch-id
would actually be better), or c) saying the thing I want isn't feasible
and I should stick with the pipeline. Thanks!
Michael McClimon (2):
patch-ids: add const modifier to commit
pretty: add 'I' placeholder for patch-id
patch-ids.c | 4 ++--
patch-ids.h | 2 +-
pretty.c | 11 +++++++++++
t/t4205-log-pretty-formats.sh | 7 +++++++
4 files changed, 21 insertions(+), 3 deletions(-)
--
2.42.0.424.gceadf0f3
^ permalink raw reply
* [RFC PATCH 1/2] patch-ids: add const modifier to commit
From: Michael McClimon @ 2023-10-22 2:27 UTC (permalink / raw)
To: git; +Cc: Michael McClimon
In-Reply-To: <20231022022800.69219-1-michael@mcclimon.org>
These aren't modified by these functions, and I want to use them
somewhere where the commit is in fact a const.
Signed-off-by: Michael McClimon <michael@mcclimon.org>
---
patch-ids.c | 4 ++--
patch-ids.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/patch-ids.c b/patch-ids.c
index c3e1a0dd..ecfd7ba0 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -6,13 +6,13 @@
#include "hex.h"
#include "patch-ids.h"
-static int patch_id_defined(struct commit *commit)
+static int patch_id_defined(const struct commit *commit)
{
/* must be 0 or 1 parents */
return !commit->parents || !commit->parents->next;
}
-int commit_patch_id(struct commit *commit, struct diff_options *options,
+int commit_patch_id(const struct commit *commit, struct diff_options *options,
struct object_id *oid, int diff_header_only)
{
if (!patch_id_defined(commit))
diff --git a/patch-ids.h b/patch-ids.h
index 490d7393..3f61d88a 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -19,7 +19,7 @@ struct patch_ids {
struct diff_options diffopts;
};
-int commit_patch_id(struct commit *commit, struct diff_options *options,
+int commit_patch_id(const struct commit *commit, struct diff_options *options,
struct object_id *oid, int);
int init_patch_ids(struct repository *, struct patch_ids *);
int free_patch_ids(struct patch_ids *);
--
2.42.0.424.gceadf0f3
^ permalink raw reply related
* [RFC PATCH 2/2] pretty: add 'I' placeholder for patch-id
From: Michael McClimon @ 2023-10-22 2:28 UTC (permalink / raw)
To: git; +Cc: Michael McClimon
In-Reply-To: <20231022022800.69219-1-michael@mcclimon.org>
This doesn't actually work yet, and the test is probably in the wrong
place, but I think it's sort of close enough to send an RFC patch to ask
some questions.
Signed-off-by: Michael McClimon <michael@mcclimon.org>
---
pretty.c | 11 +++++++++++
t/t4205-log-pretty-formats.sh | 7 +++++++
2 files changed, 18 insertions(+)
diff --git a/pretty.c b/pretty.c
index cf964b06..47e2e6e9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -19,6 +19,7 @@
#include "trailer.h"
#include "run-command.h"
#include "object-name.h"
+#include "patch-ids.h"
/*
* The limit for formatting directives, which enable the caller to append
@@ -1571,6 +1572,16 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
c->pretty_ctx->abbrev);
}
return 1;
+ case 'I':
+ {
+ struct diff_options diffopt;
+ struct object_id patch_id;
+ repo_diff_setup(the_repository, &diffopt);
+ if (commit_patch_id(commit, &diffopt, &patch_id, 0))
+ die(_("cannot get patch id"));
+ strbuf_addstr(sb, oid_to_hex(&patch_id));
+ return 1;
+ }
case 'm': /* left/right/bottom */
strbuf_addstr(sb, get_revision_mark(NULL, commit));
return 1;
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e3d655e6..1e9fdcfe 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -38,6 +38,13 @@ test_expect_success 'set up basic repos' '
git config --unset i18n.commitEncoding
'
+# %I placeholder
+test_expect_success '%I placeholder is a patch-id' '
+ git diff-tree --patch-with-raw HEAD | git patch-id >expected &&
+ git show -s --pretty="%I %H" >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'alias builtin format' '
git log --pretty=oneline >expected &&
git config pretty.test-alias oneline &&
--
2.42.0.424.gceadf0f3
^ permalink raw reply related
* [PATCH v3] merge-ort.c: fix typo 'neeed' to 'needed'
From: Wangchangxin via GitGitGadget @ 2023-10-22 2:46 UTC (permalink / raw)
To: git; +Cc: Wangchangxin, 王常新
In-Reply-To: <pull.1592.v2.git.git.1697719216137.gitgitgadget@gmail.com>
From: =?UTF-8?q?=E7=8E=8B=E5=B8=B8=E6=96=B0?= <wchangxin824@gmail.com>
Signed-off-by: 王常新 (Wang Changxin) <wchangxin824@gmail.com>
---
typo: fix the typo 'neeed' into 'needed' in the comment under merge-o…
the comments on line 2039 under merge-ort.c should be :
this is needed if we have content merges of content merges rather than
this is neeed if we have content merges of content merges
fix the typo
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1592%2FforiLLL%2Fcomment_patch-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1592/foriLLL/comment_patch-v3
Pull-Request: https://github.com/git/git/pull/1592
Range-diff vs v2:
1: 3934ee6b684 ! 1: 9320154b91a merge-ort.c: fix typo 'neeed' to 'needed'
@@
## Metadata ##
-Author: foril <1571825323@qq.com>
+Author: 王常新 <wchangxin824@gmail.com>
## Commit message ##
merge-ort.c: fix typo 'neeed' to 'needed'
- Signed-off-by: 王常新 (Wang Changxin) <foril@foril.space>
+ Signed-off-by: 王常新 (Wang Changxin) <wchangxin824@gmail.com>
## merge-ort.c ##
@@ merge-ort.c: static int handle_content_merge(struct merge_options *opt,
merge-ort.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..aee6f7d8173 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2036,7 +2036,7 @@ static int handle_content_merge(struct merge_options *opt,
* the three blobs to merge on various sides of history.
*
* extra_marker_size is the amount to extend conflict markers in
- * ll_merge; this is neeed if we have content merges of content
+ * ll_merge; this is needed if we have content merges of content
* merges, which happens for example with rename/rename(2to1) and
* rename/add conflicts.
*/
base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
--
gitgitgadget
^ 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