* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Junio C Hamano @ 2023-09-19 21:25 UTC (permalink / raw)
To: Linus Arver; +Cc: git
In-Reply-To: <owly7comj5ll.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> [Cooking]
>>
>> [...]
>>
>> * la/trailer-cleanups (2023-09-11) 6 commits
>> (merged to 'next' on 2023-09-12 at 779c4a097a)
>> + trailer: use offsets for trailer_start/trailer_end
>> + trailer: rename *_DEFAULT enums to *_UNSPECIFIED
>> + trailer: teach find_patch_start about --no-divider
>> + trailer: split process_command_line_args into separate functions
>> + trailer: split process_input_file into separate pieces
>> + trailer: separate public from internal portion of trailer_iterator
>>
>> Code clean-up.
>>
>> Will merge to 'master'.
>> source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
>
> This isn't ready yet (still need to reroll).
Whoa, wait. I wasn't aware of any more comments that needed
addressing. Whatever improvements you have in mind, if they are
minor, letting the above graduate (they have been in 'next' for a
week without anybody complaining) and doing them as a follow-up
series would be sensible, I would think.
Thanks.
^ permalink raw reply
* [PATCH v5 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 20:26 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk,
Johannes Schindelin
In-Reply-To: <cover.1695154855.git.code@khaugsbakk.name>
Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given. In other words, this:
git log --notes=custom
is equivalent to this:
git log --no-notes --notes=custom
While:
git range-diff --notes=custom
acts like this:
git log --notes --notes-custom
This can’t be how the user expects `range-diff` to behave given that the
man page for `range-diff` under `--[no-]notes[=<ref>]` says:
> This flag is passed to the `git log` program (see git-log(1)) that
> generates the patches.
This behavior also affects `format-patch` since it uses `range-diff` for
the cover letter. Unlike `log`, though, `format-patch` is not supposed
to show the default notes if no notes-related arguments are given.[1]
But this promise is broken when the range-diff happens to have something
to say about the changes to the default notes, since that will be shown
in the cover letter.
Remedy this by introducing `--show-notes-by-default` that `range-diff` can
use to tell the `log` subprocess what to do.
§ Authors
• Fix by Johannes
• Tests by Kristoffer
† 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
showing notes, 2010-01-20).
Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/pretty-options.txt | 4 ++++
range-diff.c | 2 +-
revision.c | 7 +++++++
revision.h | 1 +
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
5 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index dc685be363a..335395b727f 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -87,6 +87,10 @@ being displayed. Examples: "--notes=foo" will show only notes from
"--notes --notes=foo --no-notes --notes=bar" will only show notes
from "refs/notes/bar".
+--show-notes-by-default::
+ Show the default notes unless options for displaying specific
+ notes are given.
+
--show-notes[=<ref>]::
--[no-]standard-notes::
These options are deprecated. Use the above --notes/--no-notes
diff --git a/range-diff.c b/range-diff.c
index ca5493984a5..c45b6d849cb 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
+ "--show-notes-by-default",
NULL);
strvec_push(&cp.args, range);
if (other_arg)
diff --git a/revision.c b/revision.c
index 2f4c53ea207..49d385257ac 100644
--- a/revision.c
+++ b/revision.c
@@ -2484,6 +2484,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->break_bar = xstrdup(optarg);
revs->track_linear = 1;
revs->track_first_time = 1;
+ } else if (!strcmp(arg, "--show-notes-by-default")) {
+ revs->show_notes_by_default = 1;
} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
skip_prefix(arg, "--notes=", &optarg)) {
if (starts_with(arg, "--show-notes=") &&
@@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+ if (!revs->show_notes_given && revs->show_notes_by_default) {
+ enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
+ revs->show_notes_given = 1;
+ }
+
return left;
}
diff --git a/revision.h b/revision.h
index 82ab400139d..50091bbd13f 100644
--- a/revision.h
+++ b/revision.h
@@ -253,6 +253,7 @@ struct rev_info {
shown_dashes:1,
show_merge:1,
show_notes_given:1,
+ show_notes_by_default:1,
show_signature:1,
pretty_given:1,
abbrev_commit:1,
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index b5f4d6a6530..b33afa1c6aa 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' '
test_cmp expect actual
'
+# `range-diff` should act like `log` with regards to notes
+test_expect_success 'range-diff with --notes=custom does not show default notes' '
+ git notes add -m "topic note" topic &&
+ git notes add -m "unmodified note" unmodified &&
+ git notes --ref=custom add -m "topic note" topic &&
+ git notes --ref=custom add -m "unmodified note" unmodified &&
+ test_when_finished git notes remove topic unmodified &&
+ test_when_finished git notes --ref=custom remove topic unmodified &&
+ git range-diff --notes=custom main..topic main..unmodified \
+ >actual &&
+ ! grep "## Notes ##" actual &&
+ grep "## Notes (custom) ##" actual
+'
+
test_expect_success 'format-patch --range-diff does not compare notes by default' '
git notes add -m "topic note" topic &&
git notes add -m "unmodified note" unmodified &&
@@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default
! grep "note" 0000-*
'
+test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
+ git notes add -m "topic note" topic &&
+ git notes --ref=custom add -m "topic note (custom)" topic &&
+ git notes add -m "unmodified note" unmodified &&
+ git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
+ test_when_finished git notes remove topic unmodified &&
+ test_when_finished git notes --ref=custom remove topic unmodified &&
+ git format-patch --notes=custom --cover-letter --range-diff=$prev \
+ main..unmodified >actual &&
+ test_when_finished "rm 000?-*" &&
+ grep "## Notes (custom) ##" 0000-* &&
+ ! grep "## Notes ##" 0000-*
+'
+
test_expect_success 'format-patch --range-diff with --no-notes' '
git notes add -m "topic note" topic &&
git notes add -m "unmodified note" unmodified &&
--
2.42.0
^ permalink raw reply related
* [PATCH v5 0/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 20:26 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1695144790.git.code@khaugsbakk.name>
Hi
See the previous cover letter for context.
§ Changes since version 4
• Remove “Root cause” section from commit message
• Doc change according to feedback
Kristoffer Haugsbakk (1):
range-diff: treat notes like `log`
Documentation/pretty-options.txt | 4 ++++
range-diff.c | 2 +-
revision.c | 7 +++++++
revision.h | 1 +
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
5 files changed, 41 insertions(+), 1 deletion(-)
Range-diff against v4:
1: 244e102cc46 ! 1: 6e114271a2e range-diff: treat notes like `log`
@@ Commit message
Remedy this by introducing `--show-notes-by-default` that `range-diff` can
use to tell the `log` subprocess what to do.
- § Root cause
-
- 8cf51561d1e (range-diff: fix a crash in parsing git-log output,
- 2020-04-15) added `--notes` in order to deal with a side-effect of
- `--pretty=medium`:
-
- > To fix this explicitly set the output format of the internally executed
- > `git log` with `--pretty=medium`. Because that cancels `--notes`, add
- > explicitly `--notes` at the end.
-
§ Authors
• Fix by Johannes
@@ Commit message
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Documentation/pretty-options.txt ##
-@@ Documentation/pretty-options.txt: message by 4 spaces (i.e. 'medium', which is the default, 'full',
- and 'fuller').
+@@ Documentation/pretty-options.txt: being displayed. Examples: "--notes=foo" will show only notes from
+ "--notes --notes=foo --no-notes --notes=bar" will only show notes
+ from "refs/notes/bar".
- ifndef::git-rev-list[]
+--show-notes-by-default::
-+ Show the default notes (see `--notes`) unless subsequent arguments
-+ are used to display specific notes.
++ Show the default notes unless options for displaying specific
++ notes are given.
+
- --notes[=<ref>]::
- Show the notes (see linkgit:git-notes[1]) that annotate the
- commit, when showing the commit log message. This is the default
+ --show-notes[=<ref>]::
+ --[no-]standard-notes::
+ These options are deprecated. Use the above --notes/--no-notes
## range-diff.c ##
@@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
--
2.42.0
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Junio C Hamano @ 2023-09-19 20:16 UTC (permalink / raw)
To: Linus Arver; +Cc: git
In-Reply-To: <owly7comj5ll.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> [Cooking]
>>
>> [...]
>>
>> * la/trailer-cleanups (2023-09-11) 6 commits
>> (merged to 'next' on 2023-09-12 at 779c4a097a)
>> + trailer: use offsets for trailer_start/trailer_end
>> + trailer: rename *_DEFAULT enums to *_UNSPECIFIED
>> + trailer: teach find_patch_start about --no-divider
>> + trailer: split process_command_line_args into separate functions
>> + trailer: split process_input_file into separate pieces
>> + trailer: separate public from internal portion of trailer_iterator
>>
>> Code clean-up.
>>
>> Will merge to 'master'.
>> source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
>
> This isn't ready yet (still need to reroll).
>
>> [...]
>>
>> * la/trailer-test-and-doc-updates (2023-09-07) 13 commits
>> - trailer doc: <token> is a <key> or <keyAlias>, not both
>> - trailer doc: separator within key suppresses default separator
>> - trailer doc: emphasize the effect of configuration variables
>> - trailer --unfold help: prefer "reformat" over "join"
>> - trailer --parse docs: add explanation for its usefulness
>> - trailer --only-input: prefer "configuration variables" over "rules"
>> - trailer --parse help: expose aliased options
>> - trailer --no-divider help: describe usual "---" meaning
>> - trailer: trailer location is a place, not an action
>> - trailer doc: narrow down scope of --where and related flags
>> - trailer: add tests to check defaulting behavior with --no-* flags
>> - trailer test description: this tests --where=after, not --where=before
>> - trailer tests: make test cases self-contained
>>
>> Test coverage for trailers has been improved.
>> source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
>
> Did you forget to add "Need more reviews"? Not sure what the status is
> for the overall series (modulo your targetd comments for some of the
> patches).
I do not have status comment for this one because I do not know what
its status is.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Linus Arver @ 2023-09-19 20:00 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqmsxmdhdw.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> [Cooking]
>
> [...]
>
> * la/trailer-cleanups (2023-09-11) 6 commits
> (merged to 'next' on 2023-09-12 at 779c4a097a)
> + trailer: use offsets for trailer_start/trailer_end
> + trailer: rename *_DEFAULT enums to *_UNSPECIFIED
> + trailer: teach find_patch_start about --no-divider
> + trailer: split process_command_line_args into separate functions
> + trailer: split process_input_file into separate pieces
> + trailer: separate public from internal portion of trailer_iterator
>
> Code clean-up.
>
> Will merge to 'master'.
> source: <pull.1563.v2.git.1694240177.gitgitgadget@gmail.com>
This isn't ready yet (still need to reroll).
> [...]
>
> * la/trailer-test-and-doc-updates (2023-09-07) 13 commits
> - trailer doc: <token> is a <key> or <keyAlias>, not both
> - trailer doc: separator within key suppresses default separator
> - trailer doc: emphasize the effect of configuration variables
> - trailer --unfold help: prefer "reformat" over "join"
> - trailer --parse docs: add explanation for its usefulness
> - trailer --only-input: prefer "configuration variables" over "rules"
> - trailer --parse help: expose aliased options
> - trailer --no-divider help: describe usual "---" meaning
> - trailer: trailer location is a place, not an action
> - trailer doc: narrow down scope of --where and related flags
> - trailer: add tests to check defaulting behavior with --no-* flags
> - trailer test description: this tests --where=after, not --where=before
> - trailer tests: make test cases self-contained
>
> Test coverage for trailers has been improved.
> source: <pull.1564.v3.git.1694125209.gitgitgadget@gmail.com>
Did you forget to add "Need more reviews"? Not sure what the status is
for the overall series (modulo your targetd comments for some of the
patches).
^ permalink raw reply
* Re: [PATCH 1/2] diff-merges: improve --diff-merges documentation
From: Sergey Organov @ 2023-09-19 19:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh6nqgltw.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> I was only trying to help you polish the text you added to explain
>>> what you called the "legacy feature" to reflect the reason behind
>>> that legacy. As you obviously were not there back then when I made
>>> "--cc" imply "-m" while keeping "-p" not to imply "-m".
>>
>> Your help is appreciated, yet unfortunately I still can't figure how to
>> improve the text based on your advice.
>
> If I were doing this patch, I would start from something like this:
>
> -m::
> By default, comparisons between parent commits and the child
> commit are not shown for merge commits, but with the `-m`
> option, `git log` can be told to show comparisons for merges
> in chosen formats (e.g. `--raw`, `-p`, `--stat`). When
> output formats (e.g. `--cc`) that are specifically designed
> to show better comparisons for merges are given, this option
> is implied; in other words, you do not have to say e.g. `git
> log -m --cc`. `git log --cc` suffices.
Well, to me this piece looks much harder to understand than current Git
documentation, and then seemingly contradicts current Git behavior and
implementation, as "log --cc -m" is not the same as "log --cc" in the
current Git (so we can't say that --cc implies -m), and "log -m --cc" is
the same as "log --cc" due to absolutely different reason: -m and --cc
are mutually exclusive options, so the last one simply takes precedence.
In the current Git, as documented, -m just produces separate diff with
respect to every parent. Simple and straightforward. Users don't need to
learn about --cc, -c, --raw, --stat... to figure what -m does and if
it's what they need. Unfortunately they still need to learn about -p,
but I'm already done trying to promote this simple change.
>
> The rest is a tangent that is not related to the above. I suspect
> that this also applies to newer `--remerge-diff`, as it also targets
> to show merges better than the original "pairwise patches" that were
> largely useless, but the right way to view what `--cc` and other
> formats do for non-merge commits is *not* to think that they "imply"
> `-p`. It is more like that the output from these formats on
> non-merge commits happen to be identical to what `-p` would produce.
> You could say that the "magic" these options know to show merge
> commits better degenerates to what `-p` gives when applied to
> non-merge commits.
>
> Another way to look at it is that `--cc` and friends, even though
> they are meant as improvements for showing merges over "-m -p" that
> gives human-unreadable pair-wise diffs, do not imply "--merges"
> (i.e. show only merge commits)---hence they have to show something
> for non-merge commits. Because output formats for all of them were
> modeled loosely [*] after "-p" output, we happened to pick it as the
> format they fall back to when they are not showing comparisons for
> merge commits.
I admit you are very creative producing these views,, but currently
these options just imply -p. Simple to understand, useful, works.
Overall, as you don't like my simple clarification, and I don't like the
direction(s) you propose, I figure I rather withdraw the part of patch
causing contention in the re-roll.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v4 1/1] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-19 19:51 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King
In-Reply-To: <6ad470bd-207f-4735-9ab5-2da5010d9ef4@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> Okay I think I understand. With that in mind I would change it to the
> patch below.
>
> I can make a new version if that looks okay.
Looking good, although I would say "unless other options are given"
("other" is optional) instead of "arguments" if I were writing this.
Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 19:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King
In-Reply-To: <xmqqled2ezfh.fsf@gitster.g>
On Tue, Sep 19, 2023, at 21:27, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>
>> [snip]
>
> Very well described. I think the rest of the proposed log message
> is redundant now we have quite a good write-up above.
Thanks!
>
>> ifndef::git-rev-list[]
>> +--show-notes-by-default::
>> + Show the default notes (see `--notes`) unless subsequent arguments
>> + are used to display specific notes.
>> +
>> --notes[=<ref>]::
>> Show the notes (see linkgit:git-notes[1]) that annotate the
>> commit, when showing the commit log message. This is the default
>
> I think the new entry should come after the description of `--notes`,
> which is the primary option around the "notes" feature.
>
> In the description, I think "subsequent" is misphrased. It makes it
> sound as if
>
> $ git log --show-notes-by-default --notes=amlog
>
> would stop showing the notes from the default notes tree (because
> the notes from the .git/refs/notes/amlog is explicitly asked for),
> while
>
> $ git log --notes=amlog --show-notes-by-default
>
> would show both the default and the custom notes, which is not what
> the code does, I think, in this hunk:
>
>> @@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>> if (revs->expand_tabs_in_log < 0)
>> revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>>
>> + if (!revs->show_notes_given && revs->show_notes_by_default) {
>> + enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
>> + revs->show_notes_given = 1;
>> + }
>> +
>> return left;
>> }
>
> Other than the above minor nits, looks very good.
>
> Thanks.
Okay I think I understand. With that in mind I would change it to the
patch below.
I can make a new version if that looks okay.
-- >8 --
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index dcd501ee505..d2df1aad647 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -59,10 +59,6 @@ message by 4 spaces (i.e. 'medium', which is the default, 'full',
and 'fuller').
ifndef::git-rev-list[]
---show-notes-by-default::
- Show the default notes (see `--notes`) unless subsequent arguments
- are used to display specific notes.
-
--notes[=<ref>]::
Show the notes (see linkgit:git-notes[1]) that annotate the
commit, when showing the commit log message. This is the default
@@ -91,6 +87,10 @@ being displayed. Examples: "--notes=foo" will show only notes from
"--notes --notes=foo --no-notes --notes=bar" will only show notes
from "refs/notes/bar".
+--show-notes-by-default::
+ Show the default notes unless arguments are given for displaying
+ specific notes.
+
--show-notes[=<ref>]::
--[no-]standard-notes::
These options are deprecated. Use the above --notes/--no-notes
^ permalink raw reply related
* Re: [PATCH v4 1/1] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-19 19:43 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King
In-Reply-To: <9f9610fa-f1cc-404a-9496-d8b77ca05d5b@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> The original CI failed.[1] Then I rebased on `master` (d4a83d07b8c (The
> tenth batch, 2023-09-18)) which also failed.[2] Problem in t5559.
>
> So: fair warning. :)
>
> [1] https://github.com/LemmingAvalanche/git/actions/runs/6238806624
> [2] https://github.com/LemmingAvalanche/git/actions/runs/6239585493
I think you can ignore these macOS ones that spew
== Info: [HTTP/2] [1] ...
in the log. The adjustments necessary for the redaction code to
deal with these new messages from cURL are known and are cooking.
^ permalink raw reply
* Re: [PATCH v4 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 19:27 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King
In-Reply-To: <244e102cc4693bb6291e03cffea6df05cdb29df3.1695144790.git.code@khaugsbakk.name>
The original CI failed.[1] Then I rebased on `master` (d4a83d07b8c (The
tenth batch, 2023-09-18)) which also failed.[2] Problem in t5559.
So: fair warning. :)
[1] https://github.com/LemmingAvalanche/git/actions/runs/6238806624
[2] https://github.com/LemmingAvalanche/git/actions/runs/6239585493
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v4 1/1] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-19 19:27 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Johannes Schindelin, Denton Liu, Jeff King
In-Reply-To: <244e102cc4693bb6291e03cffea6df05cdb29df3.1695144790.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Currently, `range-diff` shows the default notes if no notes-related
> arguments are given. This is also how `log` behaves. But unlike
> `range-diff`, `log` does *not* show the default notes if
> `--notes=<custom>` are given. In other words, this:
>
> git log --notes=custom
>
> is equivalent to this:
>
> git log --no-notes --notes=custom
>
> While:
>
> git range-diff --notes=custom
>
> acts like this:
>
> git log --notes --notes-custom
>
> This can’t be how the user expects `range-diff` to behave given that the
> man page for `range-diff` under `--[no-]notes[=<ref>]` says:
>
>> This flag is passed to the `git log` program (see git-log(1)) that
>> generates the patches.
>
> This behavior also affects `format-patch` since it uses `range-diff` for
> the cover letter. Unlike `log`, though, `format-patch` is not supposed
> to show the default notes if no notes-related arguments are given.[1]
> But this promise is broken when the range-diff happens to have something
> to say about the changes to the default notes, since that will be shown
> in the cover letter.
>
> Remedy this by introducing `--show-notes-by-default` that `range-diff` can
> use to tell the `log` subprocess what to do.
Very well described. I think the rest of the proposed log message
is redundant now we have quite a good write-up above.
> ifndef::git-rev-list[]
> +--show-notes-by-default::
> + Show the default notes (see `--notes`) unless subsequent arguments
> + are used to display specific notes.
> +
> --notes[=<ref>]::
> Show the notes (see linkgit:git-notes[1]) that annotate the
> commit, when showing the commit log message. This is the default
I think the new entry should come after the description of `--notes`,
which is the primary option around the "notes" feature.
In the description, I think "subsequent" is misphrased. It makes it
sound as if
$ git log --show-notes-by-default --notes=amlog
would stop showing the notes from the default notes tree (because
the notes from the .git/refs/notes/amlog is explicitly asked for),
while
$ git log --notes=amlog --show-notes-by-default
would show both the default and the custom notes, which is not what
the code does, I think, in this hunk:
> @@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
> if (revs->expand_tabs_in_log < 0)
> revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>
> + if (!revs->show_notes_given && revs->show_notes_by_default) {
> + enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
> + revs->show_notes_given = 1;
> + }
> +
> return left;
> }
Other than the above minor nits, looks very good.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2023, #05; Fri, 15)
From: Taylor Blau @ 2023-09-19 18:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqmsxmdhdw.fsf@gitster.g>
On Fri, Sep 15, 2023 at 06:41:15PM -0700, Junio C Hamano wrote:
> * tb/repack-existing-packs-cleanup (2023-09-13) 8 commits
> (merged to 'next' on 2023-09-14 at bb8065e89c)
> + builtin/repack.c: extract common cruft pack loop
> + builtin/repack.c: avoid directly inspecting "util"
> + builtin/repack.c: store existing cruft packs separately
> + builtin/repack.c: extract `has_existing_non_kept_packs()`
> + builtin/repack.c: extract redundant pack cleanup for existing packs
> + builtin/repack.c: extract redundant pack cleanup for --geometric
> + builtin/repack.c: extract marking packs for deletion
> + builtin/repack.c: extract structure to store existing packs
>
> The code to keep track of existing packs in the repository while
> repacking has been refactored.
>
> Will merge to 'master'.
> source: <cover.1694632644.git.me@ttaylorr.com>
Nice, I'm happy to see that this is moving along. Thanks, everybody, for
participating in the review :-). It's very nice to have a second set of
eyes when touching the repack machinery.
> * cc/repack-sift-filtered-objects-to-separate-pack (2023-09-11) 9 commits
> . gc: add `gc.repackFilterTo` config option
> . repack: implement `--filter-to` for storing filtered out objects
> . gc: add `gc.repackFilter` config option
> . repack: add `--filter=<filter-spec>` option
> . pack-bitmap-write: rebuild using new bitmap when remapping
> . repack: refactor finding pack prefix
> . repack: refactor finishing pack-objects command
> . t/helper: add 'find-pack' test-tool
> . pack-objects: allow `--filter` without `--stdout`
>
> "git repack" machinery learns to pay attention to the "--filter="
> option.
>
> May need to wait until tb/repack-existing-packs-cleanup stablizes.
> source: <20230911150618.129737-1-christian.couder@gmail.com>
I sent Christian a draft of what I think the conflict resolution should
look like when rebasing on top of tb/repack-existing-packs-cleanup [1].
I've looked over all but one of the previous rounds, and have seen the
most recent round and am happy with the result. So I think that this
could go in relatively quickly following merging the above.
I have a couple of other repacking-related topics that are in my queue,
but I've been holding off on sending them until Christian's series has
stabilized. They are:
- tb/cruft-max-size (git@github.com:ttaylorr/git.git)
- tb/cruft-preferred-inference (git@github.com:ttaylorr/git.git)
The former is on the list at [2], and needs a bit of work before
queueing again. The latter isn't on the list, but fixes a performance
bug where it is possible in rare circumstances to select the cruft pack
as preferred when generating a MIDX bitmap during repacking.
The second isn't crucial to get in any time soon, but I would love to
get it off of my queue before we start cutting pre-releases. At this
rate, I doubt it will be a problem.
[1]: https://lore.kernel.org/git/ZQNKkn0YYLUyN5Ih@nand.local/
[2]: https://lore.kernel.org/git/cover.1694123506.git.me@ttaylorr.com/
> * tb/path-filter-fix (2023-08-30) 15 commits
> - bloom: introduce `deinit_bloom_filters()`
> - commit-graph: reuse existing Bloom filters where possible
> - object.h: fix mis-aligned flag bits table
> - commit-graph: drop unnecessary `graph_read_bloom_data_context`
> - commit-graph.c: unconditionally load Bloom filters
> - t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
> - bloom: prepare to discard incompatible Bloom filters
> - bloom: annotate filters with hash version
> - commit-graph: new filter ver. that fixes murmur3
> - repo-settings: introduce commitgraph.changedPathsVersion
> - t4216: test changed path filters with high bit paths
> - t/helper/test-read-graph: implement `bloom-filters` mode
> - bloom.h: make `load_bloom_filter_from_graph()` public
> - t/helper/test-read-graph.c: extract `dump_graph_info()`
> - gitformat-commit-graph: describe version 2 of BDAT
>
> The Bloom filter used for path limited history traversal was broken
> on systems whose "char" is unsigned; update the implementation and
> bump the format version to 2.
>
> Needs more work.
> cf. <20230830200218.GA5147@szeder.dev>
> source: <cover.1693413637.git.jonathantanmy@google.com>
I think that Jonathan's most recent round of this is ready to get merged
up, cf. [3]. The outstanding issue you note in
<20230830200218.GA5147@szeder.dev> can be addressed separately, I
believe. To that end, I have a RFC-level patch proposed here [4].
[3]: https://lore.kernel.org/git/xmqqo7io8gmo.fsf@gitster.g/
[4]: https://lore.kernel.org/git/ZQnmTXUO94%2FQy8mq@nand.local/
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH] Not computing changed path filter for root commits
From: Taylor Blau @ 2023-09-19 18:21 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, szeder.dev, derrickstolee
In-Reply-To: <20230911223157.446269-1-jonathantanmy@google.com>
On Mon, Sep 11, 2023 at 03:31:56PM -0700, Jonathan Tan wrote:
> SZEDER Gábor suggested [2] that we change the revision walk to read
> changed path filters also for root commits, but I don't think that's
> possible - we have to tie reading changed path filters to when we read
> trees, and right now, we don't seem to read trees when evaluating root
> commits (rev_compare_tree() in revision.c is in the only code path that
> uses changed path filters, and it itself is only called per-parent and
> thus not called for root commits). The alternative is to not generate
> changed path filters for root commits (or what I did in this patch,
> which is to generate an all-1 filter), which seems reasonable to me.
I think between the two, the all-1's filter is the more sensible choice,
since not computing a filter is typically reserved for blowing past the
`commitGraph.maxNewFilters` setting.
But, I agree with Gábor down-thread that we could instead teach
`rev_same_tree_as_empty()` to be aware of Bloom filters, which I think
would accomplish our goal of reading Bloom filters at the root commit
while not having to tweak their generation.
Thanks,
Taylor
^ permalink raw reply
* Re: [RFC PATCH] Not computing changed path filter for root commits
From: Taylor Blau @ 2023-09-19 18:19 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Jonathan Tan, git
In-Reply-To: <20230915202912.GA8705@szeder.dev>
On Fri, Sep 15, 2023 at 10:29:12PM +0200, SZEDER Gábor wrote:
> On Mon, Sep 11, 2023 at 03:31:56PM -0700, Jonathan Tan wrote:
> > SZEDER Gábor suggested [2] that we change the revision walk to read
> > changed path filters also for root commits, but I don't think that's
> > possible - we have to tie reading changed path filters to when we read
> > trees, and right now, we don't seem to read trees when evaluating root
> > commits (rev_compare_tree() in revision.c is in the only code path that
> > uses changed path filters, and it itself is only called per-parent and
> > thus not called for root commits).
>
> When encountering a root commit during a pathspec-limited revision
> walk we call rev_same_tree_as_empty() instead of rev_compare_tree().
> All that's missing there is checking the Bloom filter and accounting
> for false positives.
I think that we'd want something like this, though I would definitely
appreciate a second set of eyes since I am not 100% confident in my
set of changes here:
--- 8< ---
diff --git a/revision.c b/revision.c
index 2f4c53ea20..1d36df49e2 100644
--- a/revision.c
+++ b/revision.c
@@ -837,14 +837,24 @@ static int rev_compare_tree(struct rev_info *revs,
static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
{
struct tree *t1 = repo_get_commit_tree(the_repository, commit);
+ int bloom_ret = 1;
if (!t1)
return 0;
+ if (revs->bloom_keys_nr) {
+ bloom_ret = check_maybe_different_in_bloom_filter(revs, commit);
+ if (!bloom_ret)
+ return 1;
+ }
+
tree_difference = REV_TREE_SAME;
revs->pruning.flags.has_changes = 0;
diff_tree_oid(NULL, &t1->object.oid, "", &revs->pruning);
+ if (bloom_ret == 1 && tree_difference == REV_TREE_SAME)
+ count_bloom_filter_false_positive++;
+
return tree_difference == REV_TREE_SAME;
}
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh
index fa9d32facf..3a45cb997b 100755
--- a/t/t4216-log-bloom.sh
+++ b/t/t4216-log-bloom.sh
@@ -162,7 +162,7 @@ test_expect_success 'setup - add commit-graph to the chain with Bloom filters' '
test_bloom_filters_used_when_some_filters_are_missing () {
log_args=$1
- bloom_trace_prefix="statistics:{\"filter_not_present\":3,\"maybe\":6,\"definitely_not\":9"
+ bloom_trace_prefix="statistics:{\"filter_not_present\":3,\"maybe\":6,\"definitely_not\":10"
setup "$log_args" &&
grep -q "$bloom_trace_prefix" "$TRASH_DIRECTORY/trace.perf" &&
test_cmp log_wo_bloom log_w_bloom
--- >8 ---
Thanks,
Taylor
^ permalink raw reply related
* Re: [PATCH 0/8] fsmonitor unused parameter cleanups
From: Junio C Hamano @ 2023-09-19 18:09 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: Jeff King, git, Jeff Hostetler, Eric DeCosta
In-Reply-To: <7356bb25-12aa-dc6a-9b32-87d13c49994c@jeffhostetler.com>
Jeff Hostetler <git@jeffhostetler.com> writes:
> On 9/18/23 6:29 PM, Jeff King wrote:
>> Here are a few cleanups of the fsmonitor code to remove or annotate
>> unused parameters (working towards my goal of making us compile clean
> ...
> LGTM
>
> Thanks,
> Jeff
Thanks, both. Let's merge it down.
^ permalink raw reply
* [PATCH v4 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 18:05 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk,
Johannes Schindelin
In-Reply-To: <cover.1695144790.git.code@khaugsbakk.name>
Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given. In other words, this:
git log --notes=custom
is equivalent to this:
git log --no-notes --notes=custom
While:
git range-diff --notes=custom
acts like this:
git log --notes --notes-custom
This can’t be how the user expects `range-diff` to behave given that the
man page for `range-diff` under `--[no-]notes[=<ref>]` says:
> This flag is passed to the `git log` program (see git-log(1)) that
> generates the patches.
This behavior also affects `format-patch` since it uses `range-diff` for
the cover letter. Unlike `log`, though, `format-patch` is not supposed
to show the default notes if no notes-related arguments are given.[1]
But this promise is broken when the range-diff happens to have something
to say about the changes to the default notes, since that will be shown
in the cover letter.
Remedy this by introducing `--show-notes-by-default` that `range-diff` can
use to tell the `log` subprocess what to do.
§ Root cause
8cf51561d1e (range-diff: fix a crash in parsing git-log output,
2020-04-15) added `--notes` in order to deal with a side-effect of
`--pretty=medium`:
> To fix this explicitly set the output format of the internally executed
> `git log` with `--pretty=medium`. Because that cancels `--notes`, add
> explicitly `--notes` at the end.
§ Authors
• Fix by Johannes
• Tests by Kristoffer
† 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
showing notes, 2010-01-20).
Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/pretty-options.txt | 4 ++++
range-diff.c | 2 +-
revision.c | 7 +++++++
revision.h | 1 +
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
5 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index dc685be363..dcd501ee50 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -59,6 +59,10 @@ message by 4 spaces (i.e. 'medium', which is the default, 'full',
and 'fuller').
ifndef::git-rev-list[]
+--show-notes-by-default::
+ Show the default notes (see `--notes`) unless subsequent arguments
+ are used to display specific notes.
+
--notes[=<ref>]::
Show the notes (see linkgit:git-notes[1]) that annotate the
commit, when showing the commit log message. This is the default
diff --git a/range-diff.c b/range-diff.c
index 2e86063491..56f6870ff9 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -60,7 +60,7 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
+ "--show-notes-by-default",
NULL);
strvec_push(&cp.args, range);
if (other_arg)
diff --git a/revision.c b/revision.c
index 2f4c53ea20..49d385257a 100644
--- a/revision.c
+++ b/revision.c
@@ -2484,6 +2484,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->break_bar = xstrdup(optarg);
revs->track_linear = 1;
revs->track_first_time = 1;
+ } else if (!strcmp(arg, "--show-notes-by-default")) {
+ revs->show_notes_by_default = 1;
} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
skip_prefix(arg, "--notes=", &optarg)) {
if (starts_with(arg, "--show-notes=") &&
@@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+ if (!revs->show_notes_given && revs->show_notes_by_default) {
+ enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
+ revs->show_notes_given = 1;
+ }
+
return left;
}
diff --git a/revision.h b/revision.h
index 82ab400139..50091bbd13 100644
--- a/revision.h
+++ b/revision.h
@@ -253,6 +253,7 @@ struct rev_info {
shown_dashes:1,
show_merge:1,
show_notes_given:1,
+ show_notes_by_default:1,
show_signature:1,
pretty_given:1,
abbrev_commit:1,
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index b5f4d6a653..b33afa1c6a 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -662,6 +662,20 @@ test_expect_success 'range-diff with multiple --notes' '
test_cmp expect actual
'
+# `range-diff` should act like `log` with regards to notes
+test_expect_success 'range-diff with --notes=custom does not show default notes' '
+ git notes add -m "topic note" topic &&
+ git notes add -m "unmodified note" unmodified &&
+ git notes --ref=custom add -m "topic note" topic &&
+ git notes --ref=custom add -m "unmodified note" unmodified &&
+ test_when_finished git notes remove topic unmodified &&
+ test_when_finished git notes --ref=custom remove topic unmodified &&
+ git range-diff --notes=custom main..topic main..unmodified \
+ >actual &&
+ ! grep "## Notes ##" actual &&
+ grep "## Notes (custom) ##" actual
+'
+
test_expect_success 'format-patch --range-diff does not compare notes by default' '
git notes add -m "topic note" topic &&
git notes add -m "unmodified note" unmodified &&
@@ -679,6 +693,20 @@ test_expect_success 'format-patch --range-diff does not compare notes by default
! grep "note" 0000-*
'
+test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
+ git notes add -m "topic note" topic &&
+ git notes --ref=custom add -m "topic note (custom)" topic &&
+ git notes add -m "unmodified note" unmodified &&
+ git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
+ test_when_finished git notes remove topic unmodified &&
+ test_when_finished git notes --ref=custom remove topic unmodified &&
+ git format-patch --notes=custom --cover-letter --range-diff=$prev \
+ main..unmodified >actual &&
+ test_when_finished "rm 000?-*" &&
+ grep "## Notes (custom) ##" 0000-* &&
+ ! grep "## Notes ##" 0000-*
+'
+
test_expect_success 'format-patch --range-diff with --no-notes' '
git notes add -m "topic note" topic &&
git notes add -m "unmodified note" unmodified &&
--
2.42.0
^ permalink raw reply related
* [PATCH v4 0/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 18:05 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1694383247.git.code@khaugsbakk.name>
Hi
Same cover letter up until but not including “Changes ...”.
Cheers
🙛 🙙
Currently, `range-diff` shows the default notes if no notes-related
arguments are given. This is also how `log` behaves. But unlike
`range-diff`, `log` does *not* show the default notes if
`--notes=<custom>` are given.
These changes are supposed to make `format-range` behave like `log` with
regards to notes.
These changes also fixes an issue with notes being shared in the cover
letter via `range-diff`, and that’s really the main motivation for
making these changes.
§ How `log` works
`log` shows the default notes if no notes arguments are given. But if
you give it specific notes to show then it forgets about the default
notes. Further, there is the convenience `--notes` option which will
show the default notes again. These options are cumulative. For example:
git log --notes --notes=custom
Will show the default notes as well as the `custom` notes.
See discussion in: https://lore.kernel.org/git/20110329143357.GA10771@sigill.intra.peff.net/
§ How `range-format` works
`range-format` passes `--notes` to `log`, which means that it does not
have the default behavior of `log` (forget the default logs if you say
e.g. `--notes=custom`). However, the man page says that (under
`--[no-]notes[=<ref>]`):
> This flag is passed to the git log program (see git-log(1)) that generates the patches.
This makes me (at least) think that `range-format` is supposed to work
just like `log` with regards to notes.
§ `format-patch` and the difference between showing and sharing
`format-patch` has a different default: it shows no notes. This makes
sense in my opinion since `format-patch` is meant to be used to share
changes with others, and you might be surprised if your notes (which
might have only been notes to yourself) are sent out in your emails
(keep in mind that notes refs are *not* pushed by default).
But the slightly faulty behavior of `range-diff` bleeds through to
`format-patch` since the latter calls the former; if you have default
notes they can be shared in the range-diff on the cover letter, even
though `format-patch` isn’t supposed to show them.
§ Changes since version 3
Dscho [rewrote] the fix: introduce a new option to pass to `git log`.
🔗 rewrote: https://lore.kernel.org/git/dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de/
§ CI (WIP)
https://github.com/LemmingAvalanche/git/actions/runs/6238806624
Kristoffer Haugsbakk (1):
range-diff: treat notes like `log`
Documentation/pretty-options.txt | 4 ++++
range-diff.c | 2 +-
revision.c | 7 +++++++
revision.h | 1 +
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
5 files changed, 41 insertions(+), 1 deletion(-)
Range-diff against v3:
1: a37dfb3748 ! 1: 244e102cc4 range-diff: treat notes like `log`
@@ Commit message
to say about the changes to the default notes, since that will be shown
in the cover letter.
- Remedy this by only conditionally passing in `--notes` to `range-diff`.
+ Remedy this by introducing `--show-notes-by-default` that `range-diff` can
+ use to tell the `log` subprocess what to do.
§ Root cause
@@ Commit message
Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
- ## range-diff.c ##
-@@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
- struct child_process cp = CHILD_PROCESS_INIT;
- struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
- struct patch_util *util = NULL;
-- int in_header = 1;
-+ int i, implicit_notes_arg = 1, in_header = 1;
- char *line, *current_filename = NULL;
- ssize_t len;
- size_t size;
- int ret = -1;
+ ## Documentation/pretty-options.txt ##
+@@ Documentation/pretty-options.txt: message by 4 spaces (i.e. 'medium', which is the default, 'full',
+ and 'fuller').
-+ for (i = 0; other_arg && i < other_arg->nr; i++)
-+ if (!strcmp(other_arg->v[i], "--notes") ||
-+ starts_with(other_arg->v[i], "--notes=") ||
-+ !strcmp(other_arg->v[i], "--no-notes")) {
-+ implicit_notes_arg = 0;
-+ break;
-+ }
+ ifndef::git-rev-list[]
++--show-notes-by-default::
++ Show the default notes (see `--notes`) unless subsequent arguments
++ are used to display specific notes.
+
- strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
- "--reverse", "--date-order", "--decorate=no",
- "--no-prefix", "--submodule=short",
+ --notes[=<ref>]::
+ Show the notes (see linkgit:git-notes[1]) that annotate the
+ commit, when showing the commit log message. This is the default
+
+ ## range-diff.c ##
@@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
++ "--show-notes-by-default",
NULL);
-+ if (implicit_notes_arg)
-+ strvec_push(&cp.args, "--notes");
strvec_push(&cp.args, range);
if (other_arg)
- strvec_pushv(&cp.args, other_arg->v);
+
+ ## revision.c ##
+@@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
+ revs->break_bar = xstrdup(optarg);
+ revs->track_linear = 1;
+ revs->track_first_time = 1;
++ } else if (!strcmp(arg, "--show-notes-by-default")) {
++ revs->show_notes_by_default = 1;
+ } else if (skip_prefix(arg, "--show-notes=", &optarg) ||
+ skip_prefix(arg, "--notes=", &optarg)) {
+ if (starts_with(arg, "--show-notes=") &&
+@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
+ if (revs->expand_tabs_in_log < 0)
+ revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+
++ if (!revs->show_notes_given && revs->show_notes_by_default) {
++ enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
++ revs->show_notes_given = 1;
++ }
++
+ return left;
+ }
+
+
+ ## revision.h ##
+@@ revision.h: struct rev_info {
+ shown_dashes:1,
+ show_merge:1,
+ show_notes_given:1,
++ show_notes_by_default:1,
+ show_signature:1,
+ pretty_given:1,
+ abbrev_commit:1,
## t/t3206-range-diff.sh ##
@@ t/t3206-range-diff.sh: test_expect_success 'range-diff with multiple --notes' '
--
2.42.0
^ permalink raw reply
* Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0
From: Taylor Blau @ 2023-09-19 17:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20230916053201.GB13092@coredump.intra.peff.net>
On Sat, Sep 16, 2023 at 01:32:01AM -0400, Jeff King wrote:
> > But I think we would happily eat a line like:
> >
> > [HTTP/2] [] [Secret: xyz]
> >
> > even lacking a stream identifier. I think that's reasonably OK in
> > practice, because we're being over-eager in redacting instead of the
> > other way around. And we're unlikely to see such a line from curl
> > anyway, so I don't think that it matters.
>
> Yes, you're correct that we'd allow an empty stream identifier. I'm
> content to leave it in the name of simplicity.
Yeah, I am definitely OK with that as well. I don't think it's worth
being overly specific in what we accept for redaction, since we're
erring on the side of being less restrictive.
> > But this may all be moot anyway, I don't feel strongly one way or the
> > other.
>
> My inclination is to leave it. I was actually tempted to just allow
> _anything_ in the brackets if only because it makes the code even
> simpler, but the "skip past digits" seemed like a reasonable middle
> ground.
Yep, same. Thanks for the sanity check :-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] fix: check parameters in json-write.c
From: Taylor Blau @ 2023-09-19 17:48 UTC (permalink / raw)
To: mark via GitGitGadget; +Cc: git, mark, wangsirun, Jeff Hostetler
In-Reply-To: <pull.1576.git.git.1695124498925.gitgitgadget@gmail.com>
[+cc Jeff Hostetler]
On Tue, Sep 19, 2023 at 11:54:58AM +0000, mark via GitGitGadget wrote:
> diff --git a/json-writer.c b/json-writer.c
> index 005c820aa42..23ba7046e5d 100644
> --- a/json-writer.c
> +++ b/json-writer.c
> @@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)
> {
> unsigned char c;
>
> + if (!in || !*in) {
> + strbuf_addstr(out, "\"\"");
> + return;
> + }
From reading the implementation of append_quoted_string(), I think that
the case where "in" is the empty string is already covered. IOW, doing
something like:
struct strbuf buf = STRBUF_INIT;
append_quoted_string(&out, "");
warning("'%s'", buf.buf);
would print out something like:
warning: '""'
as expected. Handling a NULL "in" argument is new behavior, but I am not
sure if it is appropriate to coerce a NULL input into the empty string.
I've CC'd the author of this code, whose opinion I trust more than my
own here.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper
From: Junio C Hamano @ 2023-09-19 17:18 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Ilari Liusvaara, Jiang Xin
In-Reply-To: <20230919064156.13892-1-worldhello.net@gmail.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>
> For protocol-v2, "stateless-connection" can be used to establish a
> stateless connection between the client and the server, but trying to
> establish http connection by calling "transport->vtable->connect" will
> fail. This restriction was first introduced in commit b236752a87
> (Support remote archive from all smart transports, 2009-12-09) by
> adding a limitation in the "connect_helper()" function.
The above description may not be technically wrong per-se, but I
found it confusing. The ".connect method must be defined" you are
removing was added back when there was no "stateless" variant of the
connection initiation. Many codepaths added by that patch did "if
.connect is there, call it, but otherwise die()" and I think the
code you were removing was added as a safety valve, not a limitation
or restriction. Later, process_connect_service() learned to handle
the .stateless_connect bit as a fallback for transports without
.connect method defined, and the commit added that feature, edc9caf7
(transport-helper: introduce stateless-connect, 2018-03-15), forgot
that the caller did not allow this fallback.
When b236752a (Support remote archive from all smart
transports, 2009-12-09) added "remote archive" support for
"smart transports", it was for transport that supports the
.connect method. connect_helper() function protected itself
from getting called for a transport without the method
before calling process_connect_service(), which did not work
wuth such a transport.
Later, edc9caf7 (transport-helper: introduce
stateless-connect, 2018-03-15) added a way for a transport
without the .connect method to establish a "stateless"
connection in protocol-v2, process_connect_service() was
taught to handle the "stateless" connection, making the old
safety valve in its caller that insisted that .connect
method must be defined too strict, and forgot to loosen it.
or something along that line would have been easire to follow, at
least to me.
> Remove the restriction in the "connect_helper()" function and use the
> logic in the "process_connect_service()" function to check the protocol
> version and service name. By this way, we can make a connection and do
> something useful. E.g., in a later commit, implements remote archive
> for a repository over HTTP protocol.
OK.
b236752a87 was to allow "remote archive from all smart transports",
but unfortunately HTTP was not among "smart transports". This
series is to update smart HTTP transport (aka "stateless") to also
support it? Interesting.
> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> ---
> transport-helper.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 49811ef176..2e127d24a5 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
>
> /* Get_helper so connect is inited. */
> get_helper(transport);
> - if (!data->connect)
> - die(_("operation not supported by protocol"));
>
> if (!process_connect_service(transport, name, exec))
> die(_("can't connect to subservice %s"), name);
^ permalink raw reply
* Re: [PATCH 1/2] diff-merges: improve --diff-merges documentation
From: Junio C Hamano @ 2023-09-19 16:38 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87v8c7mp1j.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I was only trying to help you polish the text you added to explain
>> what you called the "legacy feature" to reflect the reason behind
>> that legacy. As you obviously were not there back then when I made
>> "--cc" imply "-m" while keeping "-p" not to imply "-m".
>
> Your help is appreciated, yet unfortunately I still can't figure how to
> improve the text based on your advice.
If I were doing this patch, I would start from something like this:
-m::
By default, comparisons between parent commits and the child
commit are not shown for merge commits, but with the `-m`
option, `git log` can be told to show comparisons for merges
in chosen formats (e.g. `--raw`, `-p`, `--stat`). When
output formats (e.g. `--cc`) that are specifically designed
to show better comparisons for merges are given, this option
is implied; in other words, you do not have to say e.g. `git
log -m --cc`. `git log --cc` suffices.
The rest is a tangent that is not related to the above. I suspect
that this also applies to newer `--remerge-diff`, as it also targets
to show merges better than the original "pairwise patches" that were
largely useless, but the right way to view what `--cc` and other
formats do for non-merge commits is *not* to think that they "imply"
`-p`. It is more like that the output from these formats on
non-merge commits happen to be identical to what `-p` would produce.
You could say that the "magic" these options know to show merge
commits better degenerates to what `-p` gives when applied to
non-merge commits.
Another way to look at it is that `--cc` and friends, even though
they are meant as improvements for showing merges over "-m -p" that
gives human-unreadable pair-wise diffs, do not imply "--merges"
(i.e. show only merge commits)---hence they have to show something
for non-merge commits. Because output formats for all of them were
modeled loosely [*] after "-p" output, we happened to pick it as the
format they fall back to when they are not showing comparisons for
merge commits.
[Footnote]
* Here, `-p` roughly means "what GNU patch and `git apply` take".
Output from `-c` and `--cc` on merge commits do not qualify, but
they are loosely modeled after it.
^ permalink raw reply
* Re: [PATCH 1/3] refs: push lock management into packed backend
From: Han-Wen Nienhuys @ 2023-09-19 15:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys
In-Reply-To: <xmqqa5tjje0y.fsf@gitster.g>
On Tue, Sep 19, 2023 at 12:46 AM Junio C Hamano <gitster@pobox.com> wrote:
> This looks a bit more convoluted than necessary. Is it the same as
>
> if (refs->be->transaction_begin &&
> refs->be->transaction_begin(refs, tr, err))
> FREE_AND_NULL(tr);
Changed this in the next version.
> > + /* TODO: leaks on error path. */
> > + ref_transaction_free(packed_transaction);
> > + packed_transaction = NULL;
> > + backend_data->packed_transaction = NULL;
> > + } else {
>
> If it were just a matter of flipping the early return and freeing of
> the transaction before going to clean-up, then that would have been
> less effort than leaving the TODO: comment. What other things are
> needed to plug this leak?
you're not missing something. I didn't have enough time yesterday to
look into all the details.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Liana Sebastian
^ permalink raw reply
* Re: [PATCH v3] revision: add `--ignore-missing-links` user option
From: Junio C Hamano @ 2023-09-19 15:13 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, me
In-Reply-To: <CAOLa=ZQdtdpu3KMMpvgr16A19xtjtOXG=HAtNrLKv97-D=Cd+g@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> If I remove the hardcoding, it would mean that
> `--ignore-missing-links` would skip missing commits but for
> non-commits objects, the user would have to pass
> `--missing=allow-any` else rev-list would still error out with a
> missing object error.
>
> Don't you think this would be confusing for the user? I'm happy
> to send a revised version removing this hardcoding if you still
> think otherwise :)
Yes. This is an example of flexibility and ergonomics at odds, and
for a low-level plumbing like rev-list, I would prefer not to limit
the flexibility unnecessarily.
I do not care about the ability to pass allow-any here. But when
you traverse a range A..B with the --ignore-missing-links option,
the reporting mechanism based on the --boundary cannot tell which
ones are at the usual "traversal boundaries" and which ones are ones
beyond the broken links, can it? If you allowed the users to pass
'print', then those reported with '?' prefix would be the missing
ones. The ones that are reported with '-' prefix may still be
mixture of the two kinds, but you can now subtract one set from the
other set to see which ones are true boundaries and which ones are
missing. The hardcoded "we do not let __ma() logic to kick in"
makes it impossible, which is what I find disturbing.
Thanks.
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Michael Strawbridge @ 2023-09-19 14:37 UTC (permalink / raw)
To: Bagas Sanjaya, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
In-Reply-To: <4eb0d21b-be43-529a-bfdd-3f1d2b4d3e84@amd.com>
On 2023-09-19 10:04, Michael Strawbridge wrote:
> On 2023-09-19 00:44, Bagas Sanjaya wrote:
>> On Mon, Sep 18, 2023 at 04:26:44PM -0400, Michael Strawbridge wrote:
>>> Hi,
>>>
>>> Author of a8022c5f7b67 (send-email: expose header information to
>>> git-send-email's sendemail-validate hook, 2023-04-19) here.
>>>
>>> On 2023-09-18 08:56, Bagas Sanjaya wrote:
>>>> I triggered this issue on patch series with cover letter. To reproduce:
>>>>
>>>> 1. Clone git.git repo, then branch off:
>>>>
>>>> ```
>>>> $ git clone https://github.com/git/git.git && cd git
>>>> $ git checkout -b test
>>>> ```
>>>>
>>>> 2. Make two dummy signed-off commits:
>>>>
>>>> ```
>>>> $ echo test > test && git add test && git commit -s -m "test"
>>>> $ echo "test test" >> test && git commit -a -s -m "test test"
>>>> ```
>>>>
>>>> 3. Generate patch series:
>>>>
>>>> ```
>>>> $ mkdir /tmp/test
>>>> $ git format-patch -o /tmp/test --cover-letter main
>>>> ```
>>>>
>>>> 4. Send the series to dummy address:
>>>>
>>>> ```
>>>> $ git send-email --to="pi <pi@pi>" /tmp/test/*.patch
>>>> ```
>>> I tried to repro this today on my side. I can repro the error when
>>> using the address "pi <pi@pi>" but that's not a valid email address and
>>> so one would expect it to fail in the extract_valid_address_or_die
>>> function with the error that you mention. As soon as I make the address
>>> valid like "pi <pi@pi.com>", git send-email no longer complains.
>>>
>>> In your original case, are you trying to send email to an invalid email
>>> address? Is it an alias by chance?
>> I triggered this regression when I passed multiple addresses separated by comma
>> (like `--to="foo <foo@acme.com>,bar <bar@acme.com>"`, but somehow I managed to
>> reduce the trigger to one address only (in this case, "pi <pi@pi.com>"). As for
>> multiple addresses part, let me know if I should post another regression
>> report.
>>
> Hm. I'm not sure what to say. I have used the below docker container
> as a test environment and don't seem to find issues with 'git send-email
> --to="pi <pi@pi.com>" /email/test/*.patch' nor with 'git send-email
> --to="foo <foo@acme.com>,bar <bar@acme.com>" /email/test/*.patch'.
>
> Maybe if you could try the following test environment too and see if you
> can reproduce it inside the docker container:
>
> NOTE: I assume you install docker on your system
>
> Step 1) Create folder with the below files inside
>
> Dockerfile:
>
> ...
>
> FROM debian:trixie
>
> RUN apt-get update && \
> apt-get install -y git git-email vim
>
> WORKDIR /
>
> RUN git clone https://github.com/git/git.git && \
> cd git && \
> git checkout -b test
>
> #COPY git-send-email /usr/lib/git-core/git-send-email
>
> RUN git config --global user.email "you@example.com"
> RUN git config --global user.name "Your Name"
>
> #specific error case
> RUN cd git && echo '#!/bin/sh \n\
> patatt sign --hook "${1}"' > .git/hooks/sendemail-validate
>
> RUN cd git && echo test > test && git add test && git commit -s -m "test"
> RUN cd git && echo "test test" >> test && git commit -a -s -m "test test"
> RUN mkdir -p /email/test
> RUN cd git && git format-patch -o /email/test --cover-letter master
> RUN sed -i 's/\*\*\* SUBJECT HERE \*\*\*/test/'
> /email/test/0000-cover-letter.patch
>
> ...
>
>
> run.sh:
>
> ...
>
> #!/bin/sh
>
> sudo docker stop git-send-email-debug
> sudo docker rm git-send-email-debug
>
> sudo docker build -t git-send-email-debug:latest .
>
> sudo docker run -it --name git-send-email-debug git-send-email-debug:latest
>
> ...
>
>
> Step 2) Make run.sh executable and start run.sh to create docker
> container shell. Inside the container's shell (will pop up
> automatically) please try this:
>
> git send-email --to="foo <foo@acme.com>,bar <bar@acme.com>"
> /email/test/*.patch
>
>
> Please let me know the results of the test case above and any other
> things you try that have interesting results.
>
>
> Thank you!
>
Whoops, somehow I missed the other responses on this thread until I
looked on the web archive version of this mailing list. I see that a
solution to "Use of uninitialized value $address" has already been proposed.
I suppose I may have mistook what issue was being reported. I had
originally understood the problem to be that hook related logic was
failing with correct email addresses, but it seems rather that we are
trying to fix an error that occurs when an email address that fails
extract_valid_address_or_die() is given. Feel free to ignore my last
email if that is all we are trying to solve.
^ permalink raw reply
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Michael Strawbridge @ 2023-09-19 14:04 UTC (permalink / raw)
To: Bagas Sanjaya, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
In-Reply-To: <ZQknHjKdGZV3vJpV@debian.me>
On 2023-09-19 00:44, Bagas Sanjaya wrote:
> On Mon, Sep 18, 2023 at 04:26:44PM -0400, Michael Strawbridge wrote:
>> Hi,
>>
>> Author of a8022c5f7b67 (send-email: expose header information to
>> git-send-email's sendemail-validate hook, 2023-04-19) here.
>>
>> On 2023-09-18 08:56, Bagas Sanjaya wrote:
>>> I triggered this issue on patch series with cover letter. To reproduce:
>>>
>>> 1. Clone git.git repo, then branch off:
>>>
>>> ```
>>> $ git clone https://github.com/git/git.git && cd git
>>> $ git checkout -b test
>>> ```
>>>
>>> 2. Make two dummy signed-off commits:
>>>
>>> ```
>>> $ echo test > test && git add test && git commit -s -m "test"
>>> $ echo "test test" >> test && git commit -a -s -m "test test"
>>> ```
>>>
>>> 3. Generate patch series:
>>>
>>> ```
>>> $ mkdir /tmp/test
>>> $ git format-patch -o /tmp/test --cover-letter main
>>> ```
>>>
>>> 4. Send the series to dummy address:
>>>
>>> ```
>>> $ git send-email --to="pi <pi@pi>" /tmp/test/*.patch
>>> ```
>> I tried to repro this today on my side. I can repro the error when
>> using the address "pi <pi@pi>" but that's not a valid email address and
>> so one would expect it to fail in the extract_valid_address_or_die
>> function with the error that you mention. As soon as I make the address
>> valid like "pi <pi@pi.com>", git send-email no longer complains.
>>
>> In your original case, are you trying to send email to an invalid email
>> address? Is it an alias by chance?
> I triggered this regression when I passed multiple addresses separated by comma
> (like `--to="foo <foo@acme.com>,bar <bar@acme.com>"`, but somehow I managed to
> reduce the trigger to one address only (in this case, "pi <pi@pi.com>"). As for
> multiple addresses part, let me know if I should post another regression
> report.
>
Hm. I'm not sure what to say. I have used the below docker container
as a test environment and don't seem to find issues with 'git send-email
--to="pi <pi@pi.com>" /email/test/*.patch' nor with 'git send-email
--to="foo <foo@acme.com>,bar <bar@acme.com>" /email/test/*.patch'.
Maybe if you could try the following test environment too and see if you
can reproduce it inside the docker container:
NOTE: I assume you install docker on your system
Step 1) Create folder with the below files inside
Dockerfile:
...
FROM debian:trixie
RUN apt-get update && \
apt-get install -y git git-email vim
WORKDIR /
RUN git clone https://github.com/git/git.git && \
cd git && \
git checkout -b test
#COPY git-send-email /usr/lib/git-core/git-send-email
RUN git config --global user.email "you@example.com"
RUN git config --global user.name "Your Name"
#specific error case
RUN cd git && echo '#!/bin/sh \n\
patatt sign --hook "${1}"' > .git/hooks/sendemail-validate
RUN cd git && echo test > test && git add test && git commit -s -m "test"
RUN cd git && echo "test test" >> test && git commit -a -s -m "test test"
RUN mkdir -p /email/test
RUN cd git && git format-patch -o /email/test --cover-letter master
RUN sed -i 's/\*\*\* SUBJECT HERE \*\*\*/test/'
/email/test/0000-cover-letter.patch
...
run.sh:
...
#!/bin/sh
sudo docker stop git-send-email-debug
sudo docker rm git-send-email-debug
sudo docker build -t git-send-email-debug:latest .
sudo docker run -it --name git-send-email-debug git-send-email-debug:latest
...
Step 2) Make run.sh executable and start run.sh to create docker
container shell. Inside the container's shell (will pop up
automatically) please try this:
git send-email --to="foo <foo@acme.com>,bar <bar@acme.com>"
/email/test/*.patch
Please let me know the results of the test case above and any other
things you try that have interesting results.
Thank you!
^ 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;
as well as URLs for NNTP newsgroup(s).