* Re: [RFC][PATCH 0/32] SHA256 and SHA1 interoperability
From: brian m. carlson @ 2023-09-10 15:38 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, Junio C Hamano
In-Reply-To: <87sf7ol0z3.fsf@email.froward.int.ebiederm.org>
[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]
On 2023-09-08 at 23:05:52, Eric W. Biederman wrote:
>
> I would like to see the SHA256 transition happen so I started playing
> with the k2204-transition-interop branch of brian m. carlson's tree.
>
> Before I go farther I need to some other folks to look at this and see
> if this is a general direction that the git project can stand.
I'm really excited to see this and I think it's a great way forward.
I've taken a brief look at each patch, and I don't see anything that
should be a dealbreaker. I left a few comments, although I think your
mailserver is blocking mine at the moment, so you may not have received
them (hopefully you can read them on the list in the interim).
You may also feel free to simply adjust the commit message for the
patches of mine you've modified without needing to document that you've
changed them. I expect that you will have changed them when you submit
them, if only to resolve conflicts. After all, Junio does so all the
time.
> This patchset is not complete it does not implement converting a
> received pack of the compatibility hash into the hash function of the
> repository, nor have I written any automated tests. Both need to happen
> before this is finalized.
Speaking of tests, one set of tests I had intended to write and think
should be written, but had not yet implemented, is tests for
round-tripping objects. That is, the SHA-1 value we get for a revision
in a pure SHA-1 repository should obviously be the same as the SHA-1
value we get in a SHA-256 repository in interop mode, and we should be
able to use the `test_oid_cache` functionality to hard-code the desired
objects. I think it would be also helpful to do this for fixed objects
that are doubly-signed (with both algorithms) as well, since that's a
tricky edge case that we'll want to avoid breaking. Other edge cases
will include things like merge commits, including octopus merges.
But overall, I think this is a great improvement, and I'm very excited
to see someone picking up some of this work and moving it forward.
Thanks for doing so.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH 02/32] doc hash-function-transition: Replace compatObjectFormat with compatMap
From: Eric W. Biederman @ 2023-09-10 18:00 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano
In-Reply-To: <ZP3UCQf+9D/J3wqT@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On 2023-09-08 at 23:10:19, Eric W. Biederman wrote:
>> Ir makes a lot of sense for the hash algorithm that determines how all
>
> Minor nit: "It".
>
>> diff --git a/Documentation/technical/hash-function-transition.txt b/Documentation/technical/hash-function-transition.txt
>> index 4b937480848a..10572c5794f9 100644
>> --- a/Documentation/technical/hash-function-transition.txt
>> +++ b/Documentation/technical/hash-function-transition.txt
>> @@ -148,14 +148,14 @@ Detailed Design
>> Repository format extension
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> A SHA-256 repository uses repository format version `1` (see
>> -Documentation/technical/repository-version.txt) with extensions
>> -`objectFormat` and `compatObjectFormat`:
>> +Documentation/technical/repository-version.txt) with the extension
>> +`objectFormat`, and an optional core.compatMap configuration.
>>
>> [core]
>> repositoryFormatVersion = 1
>> + compatMap = on
>> [extensions]
>> objectFormat = sha256
>> - compatObjectFormat = sha1
>
> While I'm in favour of an approach that uses the compat map, the
> situation we've implemented here doesn't specify the extra hash
> algorithm. We want this approach to work just as well for moving from
> SHA-1 to SHA-256 as it might for a future transition from SHA-256 to,
> say, SHA-3-512, if that becomes necessary.
>
> Making a future transition easier has been a goal of my SHA-256 work
> (because who wants to write several hundred patches in such a case?), so
> my hope is we can keep that here as well by explicitly naming the
> algorithm we're using.
>
> I also wonder if an approach that doesn't use an extension is going to
> be helpful. Say, that I have a repository that is using Git 3.x, which
> supports interop, but I also need to use Git 2.x, which does not. While
> it's true that Git 2.x can read my SHA-256 repository, it won't write
> the appropriate objects into the map, and thus it will be practically
> very difficult to actually use Git 3.x to push data to a repository of a
> different hash function. We might well prefer to have Git 2.x not work
> with the repository at all rather than have incomplete data preventing
> us from, well, interoperating.
First it is my hope that we can get a command such as "git gc" to scan
the repository and fill in all of the missing compatibility hashes.
Not so much for day to day work, but for people able to enable
compatibility hashes on an existing repository. Enabling compatibility
hashes on a sha1 repository is going to be necessary to create a sha256
repository from it. A depth first walk, or a topological sort of the
objects pretty much has to happen as a separate pass. So it makes sense
just to require all of the objects have their compatibility hash
computed before attempting to generate a pack in the compatibility
format.
I say all of that and I feel silly.
The core and optimized path is what whatever receive pack does to deal
with a pack in the repositories compatibility format. Once that is
built we can create a sha256 repository from a sha1 repository just
by cloning it, and letting receive-pack figure out the details.
Before we can generate a sha256 pack from a sha1 pack we still need
to compute the sha256 hash of every object, but that can be very
optimized and local to the case of receiving a non-native pack. So a
repository that generates a compatibility hash for all of it's objects
is not necessary to transition to another hash algorithm. All we need
is another repository in the other format.
That said there is value in being able to add compatibility hashes
to an existing repository. The upstream repository can just convert
to the new hash function and all of the downstream repositories
can compute their compatibility hashes and convert when they are ready.
Basically once a git with transition support exists any repository can
convert at any time without creating a problem for other repositories.
In my head it seems cheaper/safer to compute the compatibility hash of
every object in an existing repository than it does to convert a
repository. Is it?
I think that if the first pull from a repository in another format can
trigger the initial computation of the compatibility hash (like the
first use of a reverse index triggers the creation of the reverse
index), then it will definitely be easier to just enable compatibility
hashes in an existing repository.
The additional hash computation step every pull from upstream (even when
well optimized) should be an incentive for people to fully convert their
repositories after the upstream has converted.
That is when things get tricky and the transition plan has not talked
about. There are references to existing oid's in email, bug trackers,
and commit comments. Digging through the history and dealing with those
references is something that developers are going to need to do for the
rest of the life of a project.
Which means eventually we will need to support a mode where we have some
packs with a ``.compat'' index but we no longer compute or generate the
old hash for new objects.
In summary. I agree that compatMap is likely insufficient. So far I
think it is too cheap/easy to generate the missing mappings to make it a
mandatory requirement that all operations always generate them.
I also agree that making the configuration resilient foreseeable future
demands is a good idea.
So I will push this change farther out in the patch series.
Eric
^ permalink raw reply
* Re: [PATCH 01/32] doc hash-file-transition: A map file for mapping between sha1 and sha256
From: Eric W. Biederman @ 2023-09-10 18:07 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano
In-Reply-To: <ZP3Rr8Ei0sG0lg0R@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On 2023-09-08 at 23:10:18, Eric W. Biederman wrote:
>> The v3 pack index file as documented has a lot of complexity making it
>> difficult to implement correctly. I worked with bryan's preliminary
>> implementation and it took several passes to get the bugs out.
>>
>> The complexity also requires multiple table look-ups to find all of
>> the information that is needed to translate from one kind of oid to
>> another. Which can't be good for cache locality.
>>
>> Even worse coming up with a new index file version requires making
>> changes that have the potentialy to break anything that uses the index
>> of a pack file.
>>
>> Instead of continuing to deal with the chance of braking things
>> besides the oid mapping functionality, the additional complexity in
>> the file format, and worry if the performance would be reasonable I
>> stripped down the problem to it's fundamental complexity and came up
>> with a file format that is exactly about mapping one kind of oid to
>> another, and only supports two kinds of oids.
>
> I think this is a fine approach, and as I'm sure you noticed from my
> series, it's a lot more robust than trying to implement pack v3. I'd be
> fine with going with this approach instead of pack v3.
I think I got your pack v3 working but it was at a minimum a serious
distraction.
I worry a little bit that this might leave some performance on the
table, with something like a 256 way jump table like we have in the
index file.
Still I figure we can start simple and when we start optimizing and
profiling we can revisit the format if it shows up as a performance
issue.
Eric
^ permalink raw reply
* Re: [RFC][PATCH 0/32] SHA256 and SHA1 interoperability
From: Eric W. Biederman @ 2023-09-10 18:20 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Junio C Hamano
In-Reply-To: <ZP3i9WdpDKlsWuNP@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> On 2023-09-08 at 23:05:52, Eric W. Biederman wrote:
>>
>> I would like to see the SHA256 transition happen so I started playing
>> with the k2204-transition-interop branch of brian m. carlson's tree.
>>
>> Before I go farther I need to some other folks to look at this and see
>> if this is a general direction that the git project can stand.
>
> I'm really excited to see this and I think it's a great way forward.
> I've taken a brief look at each patch, and I don't see anything that
> should be a dealbreaker. I left a few comments, although I think your
> mailserver is blocking mine at the moment, so you may not have received
> them (hopefully you can read them on the list in the interim).
I can. I will see if I can figure out what is happening with direct
reception tomorrow.
> You may also feel free to simply adjust the commit message for the
> patches of mine you've modified without needing to document that you've
> changed them. I expect that you will have changed them when you submit
> them, if only to resolve conflicts. After all, Junio does so all the
> time.
Thanks. I was doing my best at striking a balance between giving credit
where is credit is due, and pointing out the bugs are probably mine.
>> This patchset is not complete it does not implement converting a
>> received pack of the compatibility hash into the hash function of the
>> repository, nor have I written any automated tests. Both need to happen
>> before this is finalized.
>
> Speaking of tests, one set of tests I had intended to write and think
> should be written, but had not yet implemented, is tests for
> round-tripping objects. That is, the SHA-1 value we get for a revision
> in a pure SHA-1 repository should obviously be the same as the SHA-1
> value we get in a SHA-256 repository in interop mode, and we should be
> able to use the `test_oid_cache` functionality to hard-code the desired
> objects. I think it would be also helpful to do this for fixed objects
> that are doubly-signed (with both algorithms) as well, since that's a
> tricky edge case that we'll want to avoid breaking. Other edge cases
> will include things like merge commits, including octopus merges.
Yes. I think we can use cat-file to do that. Have two repositories one
in each format. Verify that when cat-file prints out an object given
the native oid cat-file prints out what was put in. Similarly verify
that when cat-file prints out an object given the compatibility oid
cat-file prints out the expected conversion. That logic performed in
both repositories should work.
> But overall, I think this is a great improvement, and I'm very excited
> to see someone picking up some of this work and moving it forward.
> Thanks for doing so.
Thanks.
Then next goal is to get enough merged that I can test the round-trip
conversions. More than anything else we need to know the conversion
functionality is solid.
Plus I expect that while 32 patches were important to show the scope of
the work, but a bit much to fully review and merge all at once.
Eric
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: Taylor Blau @ 2023-09-10 18:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <2d6f3d74-687a-2d40-5c0c-abc396aef80f@web.de>
On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
> Add an int pointer, value_int, to struct option to provide a typed value
> pointer for the various integer options. It allows type checks at
> compile time, which is not possible with the void pointer, value. Its
> use is optional for now.
This is an interesting direction. I wonder about whether or not you'd
consider changing the option structure to contain a tagged union type
that represents some common cases we'd want from a parse-options
callback, something like:
struct option {
/* ... */
union {
void *value;
int *value_int;
/* etc ... */
} u;
enum option_type t;
};
where option_type has some value corresponding to "void *", another for
"int *", and so on.
Alternatively, perhaps you are thinking that we'd use both the value
pointer and the value_int pointer to point at potentially different
values in the same callback. I don't have strong feelings about it, but
I'd just as soon encourage us to shy away from that approach, since
assigning a single callback parameter to each function seems more
organized.
> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
> const char *s, *arg;
> const int unset = flags & OPT_UNSET;
> int err;
> + int *value_int = opt->value_int ? opt->value_int : opt->value;
>
> if (unset && p->opt)
> return error(_("%s takes no value"), optname(opt, flags));
Reading this hunk, I wonder whether we even need a type tag (the
option_type enum above) if each callback knows a priori what type it
expects. But I think storing them together in a union makes sense to do.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] diff --no-index: fix -R with stdin
From: Taylor Blau @ 2023-09-10 18:41 UTC (permalink / raw)
To: René Scharfe; +Cc: Martin Storsjö, git, Phillip Wood, Junio C Hamano
In-Reply-To: <22fdfa3b-f90e-afcc-667c-705fb7670245@web.de>
On Sun, Sep 10, 2023 at 12:12:52AM +0200, René Scharfe wrote:
> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff. 1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R. Swap them like the other metadata variables.
>
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!
Great patch ;-). LGTM!
Thanks,
Taylor
^ permalink raw reply
* Re: [bug] git clone command leaves orphaned ssh process
From: Taylor Blau @ 2023-09-10 18:47 UTC (permalink / raw)
To: Max Amelchenko; +Cc: Bagas Sanjaya, git, Hideaki Yoshifuji, Junio C Hamano
In-Reply-To: <CAN47KsUe=qicr4wZWd33EV+cciUr8ztP2veoOkcw0JBtvsBGjw@mail.gmail.com>
On Sun, Sep 10, 2023 at 12:47:14PM +0300, Max Amelchenko wrote:
> Output of second ps aux command (after running git clone):
>
> bash-4.2# ps aux
>
> USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
>
> root 1 0.0 0.0 715708 5144 pts/0 Ssl+ 09:43 0:00
> /usr/local/bin/aws-lambda-rie /var/runtime/bootstrap
>
> root 14 0.0 0.0 114096 3088 pts/1 Ss 09:43 0:00 bash
>
> root 167 0.5 0.0 0 0 pts/1 Z 09:46 0:00 [ssh] <defunct>
>
> root 168 0.0 0.0 118296 3408 pts/1 R+ 09:46 0:00 ps aux
>
> See the added ssh defunct process.
Hmm... I wasn't quite able to reproduce this locally. Below
`git.compile` points to a Git executable built from the v2.40.1 tag
corresponding to your bug report:
$ host='ssh://*****@*****lab-prod.server.sim.cloud/terraform/modules/aws-eks'
$ git.compile clone "$host" /tmp/x
Cloning into '/tmp/x'...
ssh: Could not resolve hostname *****lab-prod.server.sim.cloud: Name or service not known
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
and then:
$ ps aux | grep defunct
ttaylorr 3688844 0.0 0.0 6340 2180 pts/1 S+ 14:45 0:00 grep --color defunct
Thanks,
Taylor
^ permalink raw reply
* [PATCH v3 0/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-10 22:06 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk
In-Reply-To: <cover.1693584310.git.code@khaugsbakk.name>
Hi
The cover letter up until the “Changes” section is mostly the same except
I deleted the justification for my approach to the problem since we don't
use that approach any more.
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 2
Dscho provided an [alternative] solution. My three patches and his patch
have been squashed into one.
🔗 alternative: https://lore.kernel.org/git/94b9535b-8c2a-eb8f-90fb-cd0f998ec57e@gmx.de/
§ CI
https://github.com/LemmingAvalanche/git/actions/runs/6139150031
Kristoffer Haugsbakk (1):
range-diff: treat notes like `log`
range-diff.c | 13 +++++++++++--
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 2 deletions(-)
Range-diff against v2:
1: e9a5910831 ! 1: a37dfb3748 range-diff: treat notes like `log`
@@ Commit message
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:
+ man page for `range-diff` under `--[no-]notes[=<ref>]` says:
- > This flag is passed to the git log program (see git-log(1)) that
+ > 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
@@ Commit message
to say about the changes to the default notes, since that will be shown
in the cover letter.
- Remedy this by co-opting the `--standard-notes` option which has been
- deprecated since ab18b2c0df[2] and which is currently only documented in
- `pretty-options`.
+ Remedy this by only conditionally passing in `--notes` to `range-diff`.
+
+ § 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).
- † 2: log/pretty-options: Document --[no-]notes and deprecate old notes
- options, 2011-03-30
+ 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;
+
++ 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;
++ }
++
+ strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
+ "--reverse", "--date-order", "--decorate=no",
+ "--no-prefix", "--submodule=short",
@@ range-diff.c: static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
-+ "--standard-notes",
NULL);
++ if (implicit_notes_arg)
++ strvec_push(&cp.args, "--notes");
strvec_push(&cp.args, range);
if (other_arg)
-
- ## revision.c ##
-@@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
- disable_display_notes(&revs->notes_opt, &revs->show_notes);
- revs->show_notes_given = 1;
- } else if (!strcmp(arg, "--standard-notes")) {
-- revs->show_notes_given = 1;
-- revs->notes_opt.use_default_notes = 1;
-+ disable_display_notes(&revs->notes_opt, &revs->show_notes);
-+ revs->show_notes_given = 0;
-+ enable_default_display_notes(&revs->notes_opt,
-+ &revs->show_notes);
-+ revs->notes_opt.use_default_notes = -1;
- } else if (!strcmp(arg, "--no-standard-notes")) {
- revs->notes_opt.use_default_notes = 0;
- } else if (!strcmp(arg, "--oneline")) {
+ strvec_pushv(&cp.args, other_arg->v);
## t/t3206-range-diff.sh ##
@@ t/t3206-range-diff.sh: test_expect_success 'range-diff with multiple --notes' '
2: f7308b7abf < -: ---------- doc: pretty-options: remove documentation for deprecated options
3: 80245bbb7e < -: ---------- revision: comment `--no-standard-notes` as deprecated
--
2.42.0
^ permalink raw reply
* [PATCH v3 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-10 22:06 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Denton Liu, Jeff King, Kristoffer Haugsbakk,
Johannes Schindelin
In-Reply-To: <cover.1694383247.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 only conditionally passing in `--notes` to `range-diff`.
§ 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>
---
range-diff.c | 13 +++++++++++--
t/t3206-range-diff.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/range-diff.c b/range-diff.c
index 2e86063491..fbb81a92cc 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -41,12 +41,20 @@ 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;
+ 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;
+ }
+
strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
"--reverse", "--date-order", "--decorate=no",
"--no-prefix", "--submodule=short",
@@ -60,8 +68,9 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
- "--notes",
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);
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
* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
From: Rubén Justo @ 2023-09-10 22:58 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <CAPig+cR+rZ4+Qyfz5YQVMixW0oC65iOzxymwOaUnPqSTVScW7g@mail.gmail.com>
On 10/9/23 3:59, Eric Sunshine wrote:
> On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:
>> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
>> use internnlly the --invert-exit-code machinery. Therefore if the user
>
> s/internnlly/internally/
Thanks!
>
>> wants to use --invert-exit-code in combination with them, the result
>> will be confusing.
>>
>> For the same reason, we are already using BAIL_OUT if the user tries to
>> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
>>
>> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
>> GIT_TEST_SANITIZE_LEAK_LOG=true.
>>
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
^ permalink raw reply
* Idea: indirect authorship info
From: Yawar Amin @ 2023-09-11 4:17 UTC (permalink / raw)
To: git
Hi,
I have an idea about enabling management of authorship info in a git repo, to
make it easier to manage and potentially remove author/committer/tagger PII (in
the context of GDPR), without having to change any commit history/SHAs.
Apologies if this has been brought up before, but I failed to find anything
relevant from some quick searches:
- https://lore.kernel.org/git/?q=indirect+authors
- https://lore.kernel.org/git/?q=authors+file
- https://lore.kernel.org/git/?q=people+file
Potential use cases:
- Someone requests that their names and email addresses be removed from a public
repo's history under GDPR Right to be Forgotten (although, based on [1], it's
not clear if projects could be forced to do honour RTBF or not)
- Someone requests that their legal name change be reflected in a public repo
I am interested in hearing your thoughts on this basic idea:
- On committing/tagging/creating a note, the identity of the author is not saved
in the commit etc. object itself but in a separate file e.g. `.git/people`:
`d0efaf97-e18a-4197-b2c0-61c05efec75e <-> Yawar Amin <yawar.amin@gmail.com>`
- Instead of the real identity of the author, a pointer to the `people` file
entry is stored e.g. `Author: d0efaf97-e18a-4197-b2c0-61c05efec75e`
- If an entry for the person already exists in the `people` file, it is reused
- When syncing with a remote repo, new entries in the `people` file are synced
along with other objects (in an append-only manner, not editing existing
entries)
- Git uses the `people` file to cross-reference and fill in authorship
info when it renders commit etc. objects like in `git log`, `git show` etc.
- If git can't find the authorship info in the `people` file it renders some
appropriate default e.g. `(Unknown)`
Project owners (with write access to the hosted repo) are able to edit and push
changes to the `people` file. In this way they can fulfill change requests like
the ones I mentioned above.
Regards,
Yawar
[1] https://www.dataprotection.ie/en/individuals/know-your-rights/right-erasure-articles-17-19-gdpr
^ permalink raw reply
* Re: Idea: indirect authorship info
From: Junio C Hamano @ 2023-09-11 5:23 UTC (permalink / raw)
To: Yawar Amin; +Cc: git
In-Reply-To: <CAJbYVJJUVUPNPpGAMGZKVWxYu2PiOf_16kzts009qpeLwoim+g@mail.gmail.com>
Yawar Amin <yawar.amin@gmail.com> writes:
> Apologies if this has been brought up before, but I failed to find anything
> relevant from some quick searches:
>
> - https://lore.kernel.org/git/?q=indirect+authors
> - https://lore.kernel.org/git/?q=authors+file
> - https://lore.kernel.org/git/?q=people+file
Try "mailmap+deadname". You essentially reinvented these earlier
proposals, except that they reused the existing ".mailmap"
mechanism, and how the key is chosen.
While I do not think of any reason why the desire to achieve the end
goal of these efforts is bad, some parts of your design (and other
proposals) need rethinking.
Projects often need to know and show who did what for legal reasons.
Imagine an old commit needs to be shown to document who made the
contribution to the project. An in-tree ".mailmap" file can give
adequate guarantee that the anatomized commit author name the commit
carries documents who it the really is in the ".mailmap" file
recorded in the tree of that partcular commit, even if some
contributors who stopped contributing and asked to go back to
anonymity have disappeared from that file. But they may still
appear if you did "git log -p .mailmap", which would meet the needs
of these projects, but it means that you cannot be forgotten and
have to live with the consequences of what you did in the past. On
the other hand, if there is no in-tree ".mailmap" (or "people")
file, it brings up many questions. It becomes unclear who will keep
track of the latest version. There needs a way to guarantee that the
entries still in the mapping file can be used to verify the claim
that some person did a particular commit. Of course, as long as it
is distributed to project participants for communication ("hey you
worked on this feature 6 months ago; can you answer a few
questions?") and verification ("yes, this commit was done by this
person who was not affiliated with company X in any way. how do you
substantiate your claim that this project stole it from the
company?") purposes, somebody will bound to make and keep copies,
which means that you cannot become truly anonymous, after making
yourself known.
As to the choice of the anonymous key used as a stand-in value for
the author and the committer identity, using something that is not
deterministic (like uuid) is not a good idea. If the name/address
are hashed with some algorithm that is cryptographically secure and
is one-way, it would probably suffice both for anonymity purposes
(as you need to "reverse" such a hash to get to the real author) and
allows easy verification (if you need to "prove" that an anonymised
author 9f5d8e44edfb7e1aa4dcf34acc3b4d643f83e1b6 recorded in the
commit object is an author with a known name/address, you can feed
that name/address to the hash function and if it hashes to the same
value, that claim is as good as having the name/address directly
recorded in the commit object.
^ permalink raw reply
* Re: [PATCH] completion(switch/checkout): treat --track and -t the same
From: Junio C Hamano @ 2023-09-11 6:09 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Todd Zullinger, Johannes Schindelin via GitGitGadget, git
In-Reply-To: <4f7d166c-040e-eb7c-ef27-81f9748d44a0@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Thanks for catching this. It is a debugging left-over, when I wanted to
> make sure that the `-t` validation I added would run immediately.
>
> I see that Junio helpfully dropped it before merging down to `next`, so I
> will refrain from sending a v2.
Yup. Thanks, all.
^ permalink raw reply
* Re: [PATCH 02/32] doc hash-function-transition: Replace compatObjectFormat with compatMap
From: Junio C Hamano @ 2023-09-11 6:11 UTC (permalink / raw)
To: brian m. carlson; +Cc: Eric W. Biederman, git
In-Reply-To: <ZP3UCQf+9D/J3wqT@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
>> +Documentation/technical/repository-version.txt) with the extension
>> +`objectFormat`, and an optional core.compatMap configuration.
>>
>> [core]
>> repositoryFormatVersion = 1
>> + compatMap = on
>> [extensions]
>> objectFormat = sha256
>> - compatObjectFormat = sha1
>
> While I'm in favour of an approach that uses the compat map, the
> situation we've implemented here doesn't specify the extra hash
> algorithm. We want this approach to work just as well for moving from
> SHA-1 to SHA-256 as it might for a future transition from SHA-256 to,
> say, SHA-3-512, if that becomes necessary.
>
> Making a future transition easier has been a goal of my SHA-256 work
> (because who wants to write several hundred patches in such a case?), so
> my hope is we can keep that here as well by explicitly naming the
> algorithm we're using.
>
> I also wonder if an approach that doesn't use an extension is going to
> be helpful. Say, that I have a repository that is using Git 3.x, which
> supports interop, but I also need to use Git 2.x, which does not. While
> it's true that Git 2.x can read my SHA-256 repository, it won't write
> the appropriate objects into the map, and thus it will be practically
> very difficult to actually use Git 3.x to push data to a repository of a
> different hash function. We might well prefer to have Git 2.x not work
> with the repository at all rather than have incomplete data preventing
> us from, well, interoperating.
Very sensible line of thought and suggestion to move the topic
forward. Very much appreciated.
^ permalink raw reply
* Re: [PATCH 12/32] bulk-checkin: hash object with compatibility algorithm
From: Junio C Hamano @ 2023-09-11 6:17 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <20230908231049.2035003-12-ebiederm@xmission.com>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> struct hashfile_checkpoint checkpoint = {0};
> struct pack_idx_entry *idx = NULL;
> + const struct git_hash_algo *compat = the_repository->compat_hash_algo;
> + struct object_id compat_oid = {};
bulk-checkin.c:267:39: error: ISO C forbids empty initializer braces [-Werror=pedantic]
^ permalink raw reply
* Re: [PATCH 14/32] commit: write commits for both hashes
From: Junio C Hamano @ 2023-09-11 6:25 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <20230908231049.2035003-14-ebiederm@xmission.com>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> + struct strbuf sig = STRBUF_INIT, compat_sig = STRBUF_INIT;
> + struct object_id *parent_buf = NULL;
> + struct object_id compat_oid = {};
Ditto.
struct object_id compat_oid = { 0 };
would be our zero-initialization convention.
Thanks.
^ permalink raw reply
* Re: [PATCH 26/32] object-file-convert: Implement convert_object_file_{begin,step,end}
From: Junio C Hamano @ 2023-09-11 6:28 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <20230908231049.2035003-26-ebiederm@xmission.com>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> + const struct git_hash_algo *from = state->from;
> + const struct git_hash_algo *to = state->to;
> + struct strbuf *out = state->outbuf;
> + const char *buffer = state->buf;
> + size_t payload_size, size = state->buf_len;;
The excess ';' at the end is an empty statment, hence ...
> + struct object_id oid;
> const char *p;
> + int ret = 0;
... these three violate our "no declaration after statement" house rule.
^ permalink raw reply
* Re: [PATCH 25/32] pack-compat-map: Add support for .compat files of a packfile
From: Junio C Hamano @ 2023-09-11 6:30 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <20230908231049.2035003-25-ebiederm@xmission.com>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> diff --git a/pack-write.c b/pack-write.c
> index b19ddf15b284..f22eea964f77 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -12,6 +12,7 @@
> #include "pack-revindex.h"
> #include "path.h"
> #include "strbuf.h"
> +#include "object-file-convert.h"
> ...
> +/*
> + * The *hash contains the pack content hash.
> + * The objects array is passed in sorted.
> + */
> +const char *write_compat_map_file(const char *compat_map_name,
> + struct pack_idx_entry **objects,
> + int nr_objects, const unsigned char *hash)
Include "pack-compat-map.h"; otherwise the compiler would complain
for missing prototypes.
^ permalink raw reply
* Re: [RFC][PATCH 0/32] SHA256 and SHA1 interoperability
From: Junio C Hamano @ 2023-09-11 6:37 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, brian m. carlson
In-Reply-To: <87sf7ol0z3.fsf@email.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> I would like to see the SHA256 transition happen so I started playing
> with the k2204-transition-interop branch of brian m. carlson's tree.
I needed these tweaks to build the series standalone on 'master' (or
2.42). There are semantic merge conflicts with some topics in flight
when this is merged to 'seen', so it may take me a bit more time to
push the integration result.
Thanks.
builtin/fast-import.c | 2 +-
bulk-checkin.c | 2 +-
commit.c | 2 +-
object-file-convert.c | 4 ++--
pack-write.c | 1 +
5 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 66c471bc73..93cc4a491c 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -784,7 +784,7 @@ struct pack_index_names {
static struct pack_index_names create_index(void)
{
- struct pack_index_names tmp = {};
+ struct pack_index_names tmp = { 0 };
struct pack_idx_entry **idx, **c, **last;
struct object_entry *e;
struct object_entry_pool *o;
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 3206412a19..d63b3ffa01 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -264,7 +264,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
const struct git_hash_algo *compat = the_repository->compat_hash_algo;
- struct object_id compat_oid = {};
+ struct object_id compat_oid = { 0 };
seekback = lseek(fd, 0, SEEK_CUR);
if (seekback == (off_t) -1)
diff --git a/commit.c b/commit.c
index 54f19ed032..2e2b805d5e 100644
--- a/commit.c
+++ b/commit.c
@@ -1654,7 +1654,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
struct strbuf buffer, compat_buffer;
struct strbuf sig = STRBUF_INIT, compat_sig = STRBUF_INIT;
struct object_id *parent_buf = NULL;
- struct object_id compat_oid = {};
+ struct object_id compat_oid = { 0 };
size_t i, nparents;
/* Not having i18n.commitencoding is the same as having utf-8 */
diff --git a/object-file-convert.c b/object-file-convert.c
index 2306e17dd5..148e61d24f 100644
--- a/object-file-convert.c
+++ b/object-file-convert.c
@@ -26,7 +26,7 @@ int repo_submodule_oid_to_algop(struct repository *repo,
for (i = 0; i < repo->index->cache_nr; i++) {
const struct cache_entry *ce = repo->index->cache[i];
- struct repository subrepo = {};
+ struct repository subrepo = { 0 };
int ret;
if (!S_ISGITLINK(ce->ce_mode))
@@ -205,7 +205,7 @@ static int convert_tag_object_step(struct object_file_convert_state *state)
const struct git_hash_algo *to = state->to;
struct strbuf *out = state->outbuf;
const char *buffer = state->buf;
- size_t payload_size, size = state->buf_len;;
+ size_t payload_size, size = state->buf_len;
struct object_id oid;
const char *p;
int ret = 0;
diff --git a/pack-write.c b/pack-write.c
index f22eea964f..b2ec09737e 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -7,6 +7,7 @@
#include "remote.h"
#include "chunk-format.h"
#include "pack-mtimes.h"
+#include "pack-compat-map.h"
#include "oidmap.h"
#include "pack-objects.h"
#include "pack-revindex.h"
--
2.42.0-158-g94e83dcf5b
^ permalink raw reply related
* Re: [PATCH v5 0/8] Repack objects into separate packfiles based on a filter
From: Christian Couder @ 2023-09-11 15:20 UTC (permalink / raw)
To: Taylor Blau
Cc: Junio C Hamano, git, John Cai, Jonathan Tan, Jonathan Nieder,
Derrick Stolee, Patrick Steinhardt
In-Reply-To: <ZNwFlcS3SOS9h77N@nand.local>
On Wed, Aug 16, 2023 at 1:09 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Tue, Aug 15, 2023 at 03:32:23PM -0700, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:
> > > but I wonder if a more complete fix would be something like:
> > > ...
> > > @@ -966,6 +972,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> > > if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
> > > die(_(incremental_bitmap_conflict_error));
> > >
> > > + if (write_bitmaps && po_args.filter_options.choice)
> > > + die(_(filtered_bitmap_conflict_error));
> > > +
> >
> > It sounds like the most direct fix.
I would be Ok with such a fix, if we think that we don't want to fix
the underlying issue, or if we think that fixing the underlying issue
is not enough...
> I agree.
>
> I think that we would be OK to not change the implementation of
> rebuild_bitmap(), or its caller in fill_bitmap_commit(), since this only
> bites us when bitmapping a filtered pack, and we should catch that case
> well before getting this deep into the bitmap code.
>
> But it does seem suspect that we rebuild right into ent->bitmap, so we
> may want to consider doing something like:
>
> --- >8 ---
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index f6757c3cbf..f4ecdf8b0e 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -413,15 +413,19 @@ static int fill_bitmap_commit(struct bb_commit *ent,
>
> if (old_bitmap && mapping) {
> struct ewah_bitmap *old = bitmap_for_commit(old_bitmap, c);
> + struct bitmap *remapped = bitmap_new();
> /*
> * If this commit has an old bitmap, then translate that
> * bitmap and add its bits to this one. No need to walk
> * parents or the tree for this commit.
> */
> - if (old && !rebuild_bitmap(mapping, old, ent->bitmap)) {
> + if (old && !rebuild_bitmap(mapping, old, remapped)) {
> + bitmap_or(ent->bitmap, remapped);
> + bitmap_free(remapped);
> reused_bitmaps_nr++;
> continue;
> }
> + bitmap_free(remapped);
> }
>
> /*
> --- 8< ---
>
> on top.
>
> Applying that patch and then rerunning the tests with the appropriate
> TEST variables causes the 'git repack' to fail as expected, ensuring
> that the containing test passes.
...however I think that fixing this underlying issue is important, as
it might cause other tricky issues in the future, for example if other
bitmap code is copying or reusing this code.
So I just sent a version 6 of this series with this change in a new
patch. I hope my explanations in the commit message are good enough.
Thanks for finding the cause of the CI test failures and suggesting this fix,
Christian.
^ permalink raw reply
* [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-11 12:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, René Scharfe, Taylor Blau,
git
In-Reply-To: <20230911121126.GA17383@redhat.com>
show_funcname_line() returns when "lno <= opt->last_shown" and this
is not right in that the ->last_shown line (which matched the pattern)
can also have the actual function name we need to report.
Change this code to check "lno < opt->last_shown". While at it, move
this check up to avoid the unnecessary "find the previous bol" loop.
Note that --lno can't underflow, lno==0 is not possible in this loop.
Simple test-case:
$ cat TEST.c
void func(void);
void func1(xxx)
{
use1(xxx);
}
void func2(xxx)
{
use2(xxx);
}
$ git grep --untracked -pn xxx TEST.c
before the patch:
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
TEST.c:5: use1(xxx);
TEST.c:8:void func2(xxx)
TEST.c:10: use2(xxx);
after the patch:
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
TEST.c=3=void func1(xxx)
TEST.c:5: use1(xxx);
TEST.c:8:void func2(xxx)
TEST.c=8=void func2(xxx)
TEST.c:10: use2(xxx);
which looks much better to me.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
grep.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/grep.c b/grep.c
index 0904d55b24..7cad8352f4 100644
--- a/grep.c
+++ b/grep.c
@@ -1350,12 +1350,11 @@ static void show_funcname_line(struct grep_opt *opt, struct grep_source *gs,
while (bol > gs->buf) {
const char *eol = --bol;
+ if (--lno < opt->last_shown)
+ break;
+
while (bol > gs->buf && bol[-1] != '\n')
bol--;
- lno--;
-
- if (lno <= opt->last_shown)
- break;
if (match_funcname(opt, gs, bol, eol)) {
show_line(opt, bol, eol, gs->name, lno, 0, '=');
--
2.25.1.362.g51ebf55
^ permalink raw reply related
* Re: [PATCH] diff --no-index: fix -R with stdin
From: Martin Storsjö @ 2023-09-11 19:20 UTC (permalink / raw)
To: René Scharfe; +Cc: git, Phillip Wood, Junio C Hamano
In-Reply-To: <22fdfa3b-f90e-afcc-667c-705fb7670245@web.de>
[-- Attachment #1: Type: text/plain, Size: 665 bytes --]
On Sun, 10 Sep 2023, René Scharfe wrote:
> When -R is given, queue_diff() swaps the mode and name variables of the
> two files to produce a reverse diff. 1e3f26542a (diff --no-index:
> support reading from named pipes, 2023-07-05) added variables that
> indicate whether files are special, i.e named pipes or - for stdin.
> These new variables were not swapped, though, which broke the handling
> of stdin with with -R. Swap them like the other metadata variables.
>
> Reported-by: Martin Storsjö <martin@martin.st>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> Great bug report, thank you!
Thanks for the extremely swift response and fix!
// Martin
^ permalink raw reply
* [PATCH v6 4/9] repack: refactor finding pack prefix
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
Create a new find_pack_prefix() to refactor code that handles finding
the pack prefix from the packtmp and packdir global variables, as we are
going to need this feature again in following commit.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
builtin/repack.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 4f53b24958..8de3009b9f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -781,6 +781,17 @@ static int write_cruft_pack(const struct pack_objects_args *args,
return finish_pack_objects_cmd(&cmd, names, local);
}
+static const char *find_pack_prefix(const char *packdir, const char *packtmp)
+{
+ const char *pack_prefix;
+ if (!skip_prefix(packtmp, packdir, &pack_prefix))
+ die(_("pack prefix %s does not begin with objdir %s"),
+ packtmp, packdir);
+ if (*pack_prefix == '/')
+ pack_prefix++;
+ return pack_prefix;
+}
+
int cmd_repack(int argc, const char **argv, const char *prefix)
{
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -1028,12 +1039,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
printf_ln(_("Nothing new to pack."));
if (pack_everything & PACK_CRUFT) {
- const char *pack_prefix;
- if (!skip_prefix(packtmp, packdir, &pack_prefix))
- die(_("pack prefix %s does not begin with objdir %s"),
- packtmp, packdir);
- if (*pack_prefix == '/')
- pack_prefix++;
+ const char *pack_prefix = find_pack_prefix(packdir, packtmp);
if (!cruft_po_args.window)
cruft_po_args.window = po_args.window;
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH v3 0/1] range-diff: treat notes like `log`
From: Johannes Schindelin @ 2023-09-11 13:23 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git, Denton Liu, Jeff King
In-Reply-To: <cover.1694383247.git.code@khaugsbakk.name>
[-- Attachment #1: Type: text/plain, Size: 374 bytes --]
Hi Kristoffer,
On Mon, 11 Sep 2023, Kristoffer Haugsbakk wrote:
> § Changes since version 2
>
> Dscho provided an [alternative] solution. My three patches and his patch
> have been squashed into one.
>
> 🔗 alternative: https://lore.kernel.org/git/94b9535b-8c2a-eb8f-90fb-cd0f998ec57e@gmx.de/
Thank you, this iteration looks good to me!
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: René Scharfe @ 2023-09-11 20:11 UTC (permalink / raw)
To: Oleg Nesterov, Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, Taylor Blau, git
In-Reply-To: <20230911121211.GA17401@redhat.com>
Am 11.09.23 um 14:12 schrieb Oleg Nesterov:
> show_funcname_line() returns when "lno <= opt->last_shown" and this
> is not right in that the ->last_shown line (which matched the pattern)
> can also have the actual function name we need to report.
>
> Change this code to check "lno < opt->last_shown". While at it, move
> this check up to avoid the unnecessary "find the previous bol" loop.
>
> Note that --lno can't underflow, lno==0 is not possible in this loop.
>
> Simple test-case:
>
> $ cat TEST.c
> void func(void);
>
> void func1(xxx)
> {
> use1(xxx);
> }
>
> void func2(xxx)
> {
> use2(xxx);
> }
>
> $ git grep --untracked -pn xxx TEST.c
>
> before the patch:
>
> TEST.c=1=void func(void);
> TEST.c:3:void func1(xxx)
> TEST.c:5: use1(xxx);
> TEST.c:8:void func2(xxx)
> TEST.c:10: use2(xxx);
>
> after the patch:
>
> TEST.c=1=void func(void);
> TEST.c:3:void func1(xxx)
> TEST.c=3=void func1(xxx)
> TEST.c:5: use1(xxx);
> TEST.c:8:void func2(xxx)
> TEST.c=8=void func2(xxx)
> TEST.c:10: use2(xxx);
>
> which looks much better to me.
Interesting idea to treat function lines as annotations of matches
instead of as special context. Showing lines twice feels wasteful, but
at least for -p it might be justifiable from that angle. Wouldn't you
have to repeat function line 3 before the match in line 8, though?
The reason for not repeating a matched function line was that it
doesn't add much information under the assumption that it's easy to
identify function lines visually. I assume this still holds, but
perhaps it doesn't for more complicated languages?
I have to admit that I almost never use --show-function, but instead
use the related --function-context a lot. So my practical experience
with the former option is very limited.
> grep.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
The patch would need to update Documentation/git-grep.txt as well to
reflect the changed output.
René
^ 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