* Re: [PATCH v3 1/3] diff-merges: improve --diff-merges documentation
From: Elijah Newren @ 2023-10-09 17:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sergey Organov, git
In-Reply-To: <xmqqmswv3p11.fsf@gitster.g>
On Fri, Oct 6, 2023 at 11:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Elijah Newren <newren@gmail.com> writes:
> >
> >> In my opinion, --remerge-diff does this better; wouldn't we want a
> >> ...
> > Between -c and --cc, I do not think there is anything that makes us
> > favor -c over --cc. While the algorithm to decide which hunks out
> > of -c's output to omit was being polished, comparison with -c served
> > a good way to give baseline, but once --cc has become solid, I do
> > not think I've used -c myself.
Perhaps, then, the user manual should either omit -c, or recommend
users use --cc instead?
> > I personally find that a very trivial merge resolution is far easier
> > to read with --cc than --remerge-diff, the latter being way too
> > verbose.
Ah, indeed, for those that know the --cc output format well (it takes
a bit to figure out for newcomers), your example demonstrates this
nicely. Thanks.
> > Also, --cc and -c should work inside a read-only repository where
> > you only have read access to. If remerge needs to write some
> > objects to the repository, then you'd need some hack to give a
> > writable object store overlay via the alternate odb mechanism, or
> > something, right?
Well, it does use a temporary object store with the alternate odb
mechanism already, but I don't think there's any code to allow the
user to input the location for the temporary store, and thus we'd
probably attempt to write it underneath the same read-only directory.
So, yes, read-only repositories would likely be problematic for
--remerge-diff.
However, are read-only repositories worth mentioning in the documentation here?
> Well, the above did not come out as well as I intended, as I forgot
> to prefix it with something I thought was obvious from what I said
> in the recent discussion in the earlier iteration of this topic,
> where I said that it would be "--remerge-diff", if I were to pick an
> option that is so useful that it deserves short and sweet single
> letter. Narutally, it came after we gained experience with "--cc",
> so it would be surprising if it did worse. Just like it is natural
> to expect that "--cc" would give more useful output than "-m -p"
> that predates everybody else.
>
> In short, I would say "--remerge-diff" would give output that is the
> easiest to grok among the three modern variants to show the changes
> a merge introduces.
>
> The above two cases, where I said cc does better than remerge-diff,
> were meant as _exceptions_ for that general sentiment.
Thanks, this is useful. This does make me wonder, though: Should we
perhaps guide users as to what we recommend (and recommend against) in
this documentation?
If we have lots of options and they all shine on different usecases,
it makes sense to just provide a long list of possibilities for users.
But if we generally feel that one is entirely supplanted by another
(e.g. -c by --cc) it seems beneficial to mention that, and if we
generally feel that one will often be clearer or more useful than the
others (e.g. --remerge-diff), it seems beneficial to recommend it.
Thoughts?
Also, perhaps this would be best to include in a follow-up series (as
it appears from Sergey's latest iteration that we are leaving other
tweaks for a later series anyway), if we do decide we want to do it...
^ permalink raw reply
* Re: [PATCH 00/25] Documentation fixes
From: Elijah Newren @ 2023-10-09 16:46 UTC (permalink / raw)
To: Taylor Blau; +Cc: Elijah Newren via GitGitGadget, git
In-Reply-To: <ZSNbALj63zjzOURN@nand.local>
On Sun, Oct 8, 2023 at 6:44 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Sun, Oct 08, 2023 at 06:45:02AM +0000, Elijah Newren via GitGitGadget wrote:
> > It turns out that AI is pretty good at making small fixes to documentation;
> > certainly not perfect, but it provides quite good signal. Unfortunately,
> > there is a lot to sift through. Some points about my strategy:
>
> Quite interesting ;-).
>
> I'm curious to learn a little bit more about your
> strategy beyond what you wrote:
>
> - What tool did you use? ChatGPT? Something home-grown?
A mixture of gpt-4 and gpt-4-32k (I would have just used gpt-4, but
trying to give it a full file blows the token limit on several of
Git's documentation files).
Also, it was sent to an internally hosted instance. On this internal
instance, it seemed to require passing the
api-version=2023-03-15-preview parameter. I don't really know what
that parameter means, but I suspect it might have been some
6-months-ish old version of gpt-4?
> - (Assuming this was generated by some sort of LLM): what did you
> prompt it with?
Note that it was exactly one file per prompt, which was as follows:
"""
For the asciidoc file below, are there any typos, grammatical errors,
or wording problems? If so, please highlight them along with proposed
corrections:
--------------------
${FILE_CONTENTS}
"""
If I had to do it over, I'd be much more explicit about the output
format. Probably, "Please respond by outputting the full file, with
any corrections included. If there are no corrections, simply output
the original file as-is." which would allow me to simply diff the
output and look at the changes.
Also, I would probably specify that "The ascii doc file starts three
lines below, just after the line of dashes", hoping that would help it
avoid sometimes presuming that the dashes were part of the file.
> - What was the output format: the edited text in its entirety, or a
> patch that can be applied on top?
My wording was unfortunately vague, so I sometimes got human prose
instructing me with a change to make, sometimes I got a bulleted list
in the form "${old_text} -> ${new_text}", but most of the time it
printed the file (or a subset thereof) with corrections. I also had
all the output concatenated into one large file, which made it "fun"
to work through all the changes. Even when diffing files, I manually
applied any changes I saw to the actual file (which did risk
introducing new typos, and missing some of the corrections, but did
ensure I reviewed everything).
Also, not only did I get different output formats, but there were many
times the file was cut off at some point. I sometimes assumed that
just meant there were no changes outside that region, but there were
times where there was only one change and it had given me hundreds of
lines of context around it before it cut off, so it did leave me with
the feeling it might have only processed or responded to part of the
file.
There were also several times where the changes it suggested were a
no-op, making me wonder if it just failed or something -- I looked at
it really closely (including sometimes piping the output through xxd,
and thus once noticed a change of tab-after-period to
space-after-period), but when it was responding with human prose and
said something like "Change the sentence that reads '${old_version}'
-> '${old_version}', it made me wonder if something just went haywire
with the LLM and I should retry.
However, despite the above issues making me think there are more
documentation issues to be found with an LLM, I didn't re-check any
files unless I got an error with no output (e.g. excessive number of
tokens, or I've hit rate limits on using the API). I didn't bother,
because the firehose of changes it provided me even without those
caveats was far more than enough to deal with.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: Junio C Hamano @ 2023-10-09 16:35 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <4014e490-c6c1-453d-b0ed-645220e3e614@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 07.10.23 um 10:20 schrieb Junio C Hamano:
>> * rs/parse-options-value-int (2023-09-18) 2 commits
>> - parse-options: use and require int pointer for OPT_CMDMODE
>> - parse-options: add int value pointer to struct option
>> ...
> I don't mind removing this topic from seen for now; it's not ready, yet.
After seeing the discussion moved to giving a more type safe enum
support and then somehow convesation seemed to have petered out, I
was wondering if we figured out the problem space enough to see an
updated version with well defined scope and good problem
description. I do not mind keeping it on 'seen', especially if
these two early steps are not expected to change. It is not like
they are causing any maintenance burden.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/4] ref-cache.c: fix prefix matching in ref iteration
From: Victoria Dye @ 2023-10-09 16:21 UTC (permalink / raw)
To: Patrick Steinhardt, Junio C Hamano; +Cc: Victoria Dye via GitGitGadget, git
In-Reply-To: <ZSPQLjJwq-7SjsDT@tanuki>
Patrick Steinhardt wrote:
>> Allowing prefix="refs/heads/v1.0" to yield entry="refs/heads/v1"
>> (case #2 above that this patch fixes the behaviour for) would cause
>> ref_iterator_advance() to return a ref outside the hierarhcy,
>> wouldn't it? So it appears to me that either one of the two would
>> be true:
>>
>> * the code is structured in such a way that such a condition does
>> not actually happen (in which case this patch would be a no-op),
>> or
>>
>> * there is a bug in the current code that is fixed by this patch,
>> whose externally observable behaviour can be verified with a
>> test.
>>
>> It is not quite clear to me which is the case here. The code with
>> the patch looks more logical than the original, but I am not sure
>> how to demonstrate the existing breakage (if any).
>
> Agreed, I also had a bit of a hard time to figure out whether this is an
> actual bug fix, a performance improvement or merely a refactoring.
>
I originally operated on the assumption that it was the first case, which is
why I didn't include a test in this patch. Commands like 'for-each-ref',
'show-ref', etc. either use an empty prefix or a directory prefix with a
trailing slash, which won't trigger this issue. I encountered the problem
while working on a builtin that filtered refs by a user-specified prefix -
the results included refs that should not have been matched, which led me to
this fix.
Scanning through the codebase again, though, I do see a way to replicate the
issue:
$ git update-ref refs/bisect/b HEAD
$ git rev-parse --abbrev-ref --bisect
refs/bisect/b
Because 'rev-parse --bisect' uses the "refs/bisect/bad" prefix (no trailing
slash) and does no additional filtering in its 'for_each_fullref_in'
callback, refs like "refs/bisect/b" and "refs/bisect/ba" are (incorrectly)
matched. I'll re-roll with the added test.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: Taylor Blau @ 2023-10-09 16:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh6n24zf1.fsf@gitster.g>
On Sat, Oct 07, 2023 at 01:20:02AM -0700, Junio C Hamano wrote:
> * tb/repack-max-cruft-size (2023-10-05) 4 commits
> (merged to 'next' on 2023-10-06 at b3ca6df3b9)
> + builtin/repack.c: avoid making cruft packs preferred
> + builtin/repack.c: implement support for `--max-cruft-size`
> + builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE
> + t7700: split cruft-related tests to t7704
>
> "git repack" learned "--max-cruft-size" to prevent cruft packs from
> growing without bounds.
>
> Will merge to 'master'.
> source: <cover.1696293862.git.me@ttaylorr.com>
> source: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>
Thanks. On a semi-related note, did you want to pick up my patch in
https://lore.kernel.org/git/035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com/
? That should address a performance bug that occurs when a repository
(incorrectly) chooses a cruft pack as its preferred pack source when
writing a MIDX bitmap, significantly impeding the pack-reuse mechanism.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-09 16:08 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Junio C Hamano, git, Elijah Newren, Eric W. Biederman, Jeff King
In-Reply-To: <ZSPb1OYRrQSUugtg@tanuki>
On Mon, Oct 09, 2023 at 12:54:12PM +0200, Patrick Steinhardt wrote:
> In Gitaly, we usually set up quarantine directories for all operations
> that create objects. This allows us to discard any newly written objects
> in case either the RPC call gets cancelled or in case our access checks
> determine that the change should not be allowed. The logic is rather
> simple:
>
> 1. Create a new temporary directory.
>
> 2. Set up the new temporary directory as main object database via
> the `GIT_OBJECT_DIRECTORY` environment variable.
>
> 3. Set up the main repository's object database via the
> `GIT_ALTERNATE_OBJECT_DIRECTORIES` environment variable.
Is there a reason not to run Git in the quarantine environment and list
the main repository as an alternate via $GIT_DIR/objects/info/alternates
instead of the GIT_ALTERNATE_OBJECT_DIRECTORIES environment variable?
> 4. Execute Git commands that write objects with these environment
> variables set up. The new objects will end up neatly contained in
> the temporary directory.
>
> 5. Once done, either discard the temporary object database or
> migrate objects into the main object daatabase.
Interesting. I'm curious why you don't use the builtin tmp_objdir
mechanism in Git itself. Do you need to run more than one command in the
quarantine environment? If so, that makes sense that you'd want to have
a scratch repository that lasts beyond the lifetime of a single process.
> I wonder whether this would be a viable approach for you, as well.
I think that the main problem that we are trying to solve with this
series is creating a potentially large number of loose objects. I think
that you could do something like what you propose above, with a 'git
repacks -adk' before moving its objects over back to the main repository.
But since we're working in a single process only when doing a merge-tree
operation, I think it is probably more expedient to write the pack's
bytes directly.
> Patrick
Thanks,
Taylor
^ permalink raw reply
* [PATCH v4 3/3] completion: complete '--dd'
From: Sergey Organov @ 2023-10-09 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Sergey Organov
In-Reply-To: <20231009160535.236523-1-sorganov@gmail.com>
'--dd' only makes sense for 'git log' and 'git show', so add it to
__git_log_show_options which is referenced in the completion for these
two commands.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 133ec92bfae7..ca4fa39f3ff8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2042,7 +2042,7 @@ __git_log_shortlog_options="
"
# Options accepted by log and show
__git_log_show_options="
- --diff-merges --diff-merges= --no-diff-merges --remerge-diff
+ --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
"
__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
--
2.25.1
^ permalink raw reply related
* [PATCH v4 1/3] diff-merges: improve --diff-merges documentation
From: Sergey Organov @ 2023-10-09 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Sergey Organov
In-Reply-To: <20231009160535.236523-1-sorganov@gmail.com>
* Put descriptions of convenience shortcuts first, so they are the
first things reader observes rather than lengthy detailed stuff.
* Get rid of very long line containing all the --diff-merges formats
by replacing them with <format>, and putting each supported format
on its own line.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
Documentation/diff-options.txt | 100 ++++++++++++++++++---------------
Documentation/git-log.txt | 2 +-
2 files changed, 55 insertions(+), 47 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9f33f887711d..69065c0e90a8 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -43,66 +43,74 @@ endif::git-diff[]
endif::git-format-patch[]
ifdef::git-log[]
---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
+-m::
+ Show diffs for merge commits in the default format. This is
+ similar to '--diff-merges=on', except `-m` will
+ produce no output unless `-p` is given as well.
+
+-c::
+ Produce combined diff output for merge commits.
+ Shortcut for '--diff-merges=combined -p'.
+
+--cc::
+ Produce dense combined diff output for merge commits.
+ Shortcut for '--diff-merges=dense-combined -p'.
+
+--remerge-diff::
+ Produce remerge-diff output for merge commits.
+ Shortcut for '--diff-merges=remerge -p'.
+
--no-diff-merges::
+ Synonym for '--diff-merges=off'.
+
+--diff-merges=<format>::
Specify diff format to be used for merge commits. Default is
- {diff-merges-default} unless `--first-parent` is in use, in which case
- `first-parent` is the default.
+ {diff-merges-default} unless `--first-parent` is in use, in
+ which case `first-parent` is the default.
+
---diff-merges=(off|none):::
---no-diff-merges:::
+The following formats are supported:
++
+--
+off, none::
Disable output of diffs for merge commits. Useful to override
implied value.
+
---diff-merges=on:::
---diff-merges=m:::
--m:::
- This option makes diff output for merge commits to be shown in
- the default format. `-m` will produce the output only if `-p`
- is given as well. The default format could be changed using
- `log.diffMerges` configuration parameter, which default value
+on, m::
+ Make diff output for merge commits to be shown in the default
+ format. The default format could be changed using
+ `log.diffMerges` configuration variable, whose default value
is `separate`.
+
---diff-merges=first-parent:::
---diff-merges=1:::
- This option makes merge commits show the full diff with
- respect to the first parent only.
+first-parent, 1::
+ Show full diff with respect to first parent. This is the same
+ format as `--patch` produces for non-merge commits.
+
---diff-merges=separate:::
- This makes merge commits show the full diff with respect to
- each of the parents. Separate log entry and diff is generated
- for each parent.
+separate::
+ Show full diff with respect to each of parents.
+ Separate log entry and diff is generated for each parent.
+
---diff-merges=remerge:::
---diff-merges=r:::
---remerge-diff:::
- With this option, two-parent merge commits are remerged to
- create a temporary tree object -- potentially containing files
- with conflict markers and such. A diff is then shown between
- that temporary tree and the actual merge commit.
+combined, c::
+ Show differences from each of the parents to the merge
+ result simultaneously instead of showing pairwise diff between
+ a parent and the result one at a time. Furthermore, it lists
+ only files which were modified from all parents.
++
+dense-combined, cc::
+ Further compress output produced by `--diff-merges=combined`
+ by omitting uninteresting hunks whose contents in the parents
+ have only two variants and the merge result picks one of them
+ without modification.
++
+remerge, r::
+ Remerge two-parent merge commits to create a temporary tree
+ object--potentially containing files with conflict markers
+ and such. A diff is then shown between that temporary tree
+ and the actual merge commit.
+
The output emitted when this option is used is subject to change, and
so is its interaction with other options (unless explicitly
documented).
-+
---diff-merges=combined:::
---diff-merges=c:::
--c:::
- With this option, diff output for a merge commit shows the
- differences from each of the parents to the merge result
- simultaneously instead of showing pairwise diff between a
- parent and the result one at a time. Furthermore, it lists
- only files which were modified from all parents. `-c` implies
- `-p`.
-+
---diff-merges=dense-combined:::
---diff-merges=cc:::
---cc:::
- With this option the output produced by
- `--diff-merges=combined` is further compressed by omitting
- uninteresting hunks whose contents in the parents have only
- two variants and the merge result picks one of them without
- modification. `--cc` implies `-p`.
+--
--combined-all-paths::
This flag causes combined diffs (used for merge commits) to
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2a66cf888074..9b7ec96e767a 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -124,7 +124,7 @@ Note that unless one of `--diff-merges` variants (including short
will not show a diff, even if a diff format like `--patch` is
selected, nor will they match search options like `-S`. The exception
is when `--first-parent` is in use, in which case `first-parent` is
-the default format.
+the default format for merge commits.
:git-log: 1
:diff-merges-default: `off`
--
2.25.1
^ permalink raw reply related
* [PATCH v4 2/3] diff-merges: introduce '--dd' option
From: Sergey Organov @ 2023-10-09 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Sergey Organov
In-Reply-To: <20231009160535.236523-1-sorganov@gmail.com>
This option provides a shortcut to request diff with respect to first
parent for any kind of commit, universally. It's implemented as pure
synonym for "--diff-merges=first-parent --patch".
Gives user quick and universal way to see what changes, exactly, were
brought to a branch by merges as well as by regular commits.
Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
Documentation/diff-options.txt | 5 +++++
Documentation/git-log.txt | 2 +-
diff-merges.c | 3 +++
t/t4013-diff-various.sh | 8 ++++++++
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 69065c0e90a8..23f95e6172b9 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -56,6 +56,11 @@ ifdef::git-log[]
Produce dense combined diff output for merge commits.
Shortcut for '--diff-merges=dense-combined -p'.
+--dd::
+ Produce diff with respect to first parent for both merge and
+ regular commits.
+ Shortcut for '--diff-merges=first-parent -p'.
+
--remerge-diff::
Produce remerge-diff output for merge commits.
Shortcut for '--diff-merges=remerge -p'.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 9b7ec96e767a..579682172fe4 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -120,7 +120,7 @@ By default, `git log` does not generate any diff output. The options
below can be used to show the changes made by each commit.
Note that unless one of `--diff-merges` variants (including short
-`-m`, `-c`, and `--cc` options) is explicitly given, merge commits
+`-m`, `-c`, `--cc`, and `--dd` options) is explicitly given, merge commits
will not show a diff, even if a diff format like `--patch` is
selected, nor will they match search options like `-S`. The exception
is when `--first-parent` is in use, in which case `first-parent` is
diff --git a/diff-merges.c b/diff-merges.c
index ec97616db1df..45507588a279 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -131,6 +131,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
} else if (!strcmp(arg, "--cc")) {
set_dense_combined(revs);
revs->merges_imply_patch = 1;
+ } else if (!strcmp(arg, "--dd")) {
+ set_first_parent(revs);
+ revs->merges_imply_patch = 1;
} else if (!strcmp(arg, "--remerge-diff")) {
set_remerge_diff(revs);
revs->merges_imply_patch = 1;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 5de1d190759f..4b474808311e 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -473,6 +473,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
test_cmp expected actual
'
+test_expect_success 'log --dd matches --diff-merges=1 -p' '
+ git log --diff-merges=1 -p master >result &&
+ process_diffs result >expected &&
+ git log --dd master >result &&
+ process_diffs result >actual &&
+ test_cmp expected actual
+'
+
test_expect_success 'deny wrong log.diffMerges config' '
test_config log.diffMerges wrong-value &&
test_expect_code 128 git log
--
2.25.1
^ permalink raw reply related
* [PATCH v4 0/3] diff-merges: introduce '--dd' option
From: Sergey Organov @ 2023-10-09 16:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Sergey Organov
In-Reply-To: <20230909125446.142715-1-sorganov@gmail.com>
This new convenience option requests full diff with respect to first
parent, so that
git log --dd
will output diff with respect to first parent for every commit,
universally, no matter how many parents the commit turns out to have.
Gives user quick and universal way to see what changes, exactly, were
brought to a branch by merges as well as by regular commits.
'--dd' is implemented as pure synonym for "--diff-merges=first-parent
--patch".
The first commit in the series tweaks diff-merges documentation a bit,
and is valuable by itself. It's put here as '--dd' implementation
commit depends on it in its documentation part.
Note: the need for this new convenience option mostly emerged from
denial by the community of patches that modify '-m' behavior to imply
'-p' as the rest of similar options (such as --cc) do. So, basically,
'--dd' is what '-m' should have been to be more useful.
Updates in v4:
* Removed "(which see)" reference from documentation that caused
confusion.
* Removed explanation why it's --dd and not simply -d from commit
message.
* Refined --remerge-diff short description according to Junio and
Elijah comments.
* Added explanation of --dd purpose.
* Fixed style and syntax of "on,m::" description.
Updates in v3:
* Option renamed from '-d' to '--dd' due to Junio overpowering
request to keep short-and-sweet '-d' reserved for another (yet
unspecified) use.
* Added completion of '--dd' to git-completion.bash.
Updates in v2:
* Reordered documentation for diff-merges formats in accordance with
Junio recommendation.
* Removed clarification of surprising -m behavior due to controversy
with Junio on how exactly it should look like.
Sergey Organov (3):
diff-merges: improve --diff-merges documentation
diff-merges: introduce '--dd' option
completion: complete '--dd'
Documentation/diff-options.txt | 105 ++++++++++++++-----------
Documentation/git-log.txt | 4 +-
contrib/completion/git-completion.bash | 2 +-
diff-merges.c | 3 +
t/t4013-diff-various.sh | 8 ++
5 files changed, 73 insertions(+), 49 deletions(-)
Interdiff against v3:
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index f80d493dd4c8..23f95e6172b9 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -45,7 +45,7 @@ endif::git-format-patch[]
ifdef::git-log[]
-m::
Show diffs for merge commits in the default format. This is
- similar to '--diff-merges=on' (which see) except `-m` will
+ similar to '--diff-merges=on', except `-m` will
produce no output unless `-p` is given as well.
-c::
@@ -62,7 +62,7 @@ ifdef::git-log[]
Shortcut for '--diff-merges=first-parent -p'.
--remerge-diff::
- Produce diff against re-merge.
+ Produce remerge-diff output for merge commits.
Shortcut for '--diff-merges=remerge -p'.
--no-diff-merges::
@@ -83,7 +83,7 @@ off, none::
on, m::
Make diff output for merge commits to be shown in the default
format. The default format could be changed using
- `log.diffMerges` configuration parameter, which default value
+ `log.diffMerges` configuration variable, whose default value
is `separate`.
+
first-parent, 1::
--
2.25.1
^ permalink raw reply related
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Jeff King @ 2023-10-09 15:56 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git, avarab
In-Reply-To: <20231009103651+0200.580831-stepnem@smrk.net>
On Mon, Oct 09, 2023 at 10:36:51AM +0200, Štěpán Němec wrote:
> > Ah, true, I was thinking that the DESCRIPTION section would be the first
> > thing users would read, but I didn't notice the headline. I agree that
> > what it says is probably sufficient (though arguably "type and size" is
> > slightly inaccurate there; I said "details" in my proposed text but
> > maybe that is too vague).
>
> We could also leave the NAME vague(r) and put an additional sentence at
> the beginning of DESCRIPTION:
Yup, that is a good suggestion. Do you want to wrap all of this
discussion up as a patch?
-Peff
^ permalink raw reply
* Re: [PATCH v6] merge-tree: add -X strategy option
From: Jeff King @ 2023-10-09 15:53 UTC (permalink / raw)
To: phillip.wood; +Cc: Izzy via GitGitGadget, git, Elijah Newren, Izzy
In-Reply-To: <a482d047-dd40-436d-8daa-0c74780af11f@gmail.com>
On Mon, Oct 09, 2023 at 10:58:07AM +0100, Phillip Wood wrote:
> > @@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
> > {
> > struct commit *parent1, *parent2;
> > struct commit_list *merge_bases = NULL;
> > - struct merge_options opt;
> > + struct merge_options opt = o->merge_options;
>
> Copying struct merge_options by value here is unusual in our code base. I
> don't think it introduces a bug (there is no function to free the resources
> allocated in struct merge_options so we do not end up freeing them twice for
> example) but it would be clearer that it was safe if you did
>
> struct merge_options *opt = &o->merge_options;
>
> and updated the code to reflect that opt is now a pointer or just replaced
> all uses of "opt" with "o->merge_options"
I agree that struct-copying is an unusual pattern, and we'd potentially
run into problems with duplication. But I think it is even trickier than
that here. We also go on to actually _modify_ opt in this function,
assigning to various members (both directly, and I think the merge code
itself will write to opt->priv).
So if we use a pointer (rather than struct assignment), those changes
will persist in the merge_options struct that was passed in. Which is
also weird.
Between the two, I think using a pointer is probably the least-weird.
This real_merge() function is only called once, and is a static-local
helper for cmd_merge_tree(). So the two functions work as a single unit,
and munging "opt" is not a big deal.
-Peff
^ permalink raw reply
* Re: [OUTREACHY] Permission To Work On Tasks
From: Christian Couder @ 2023-10-09 13:45 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git, Junio C Hamano
In-Reply-To: <CAP8UFD0PKAGchx5iqyZqCdua-KYJcrmO2FfNf_vt7xs=+7YL4Q@mail.gmail.com>
On Sat, Oct 7, 2023 at 9:20 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sat, Oct 7, 2023 at 1:29 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Naomi Ibe <naomi.ibeh69@gmail.com> writes:
> > > Second issue is this https://github.com/gitgitgadget/git/issues/302 .
> > > Is it still available to be worked on? I notice it was opened in 2019
> >
> > Stepping back a bit, do you agree with what the issue says?
> > Remember, these "issue"s are merely one person's opinion and not
> > endorsed by the community.
> >
> > Before you ask "is it still available", do you know the current
> > status (not the status of the "issue")? Have you looked at "git
> > commit --help" to find it out yourself to see if "now" is singled
> > out? Here is what we say in our documentation:
> >
> > In addition to recognizing all date formats above, the --date
> > option will also try to make sense of other, more human-centric
> > date formats, such as relative dates like "yesterday" or "last
> > Friday at noon".
> >
> > So apparently it is still "available". It is a different matter how
> > well a patch that adds "now" to the examples listed there will be
> > accepted, though. During a microproject, one of the things new
> > contributors are expected to learn is to convince others the cause
> > of their patches with the proposed commit log message well.
>
> Yeah, I think this issue, if it is indeed an issue, is not something
> easy to "fix" for a newcomer as it requires to be familiar with our
> documentation and perhaps our code too, or to research them enough to
> understand what a good improvement would be. So you could perhaps do
> it, but it would likely require more work.
Also if it's only a documentation improvement, it might not actually
be a proper microproject as we want them to be "code related".
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Christian Couder @ 2023-10-09 13:38 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git, Johannes Schindelin
In-Reply-To: <CAJHH8bEa=xE_xNdbW4rDJQQ9dacAuFQseajdtBmGnZ1bDxZsxQ@mail.gmail.com>
On Mon, Oct 9, 2023 at 3:33 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> Thank you Christian for the help, I have another idea for a micro
> project "modernizing a test script". I have found test scripts that
> needs modernizing , which is to rename the setup test 'prepare
> reference tree' to 'setup'. Is it appropriate for a micro project and
> should I go ahead with it.
I am not sure these kinds of renames are worth doing, but if you find
some clear doc or Junio saying that setup tests should be named
"setup", then that might be worth a try.
^ permalink raw reply
* Re: [Outreachy] Introduction and Interest in Contributing to the Git Community
From: Isoken Ibizugbe @ 2023-10-09 13:31 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Johannes Schindelin
In-Reply-To: <CAP8UFD3WzgADiy07uLGpj23r3jrUnYh_Wdsc1N8ZoaAHQPDZag@mail.gmail.com>
Thank you Christian for the help, I have another idea for a micro
project "modernizing a test script". I have found test scripts that
needs modernizing , which is to rename the setup test 'prepare
reference tree' to 'setup'. Is it appropriate for a micro project and
should I go ahead with it.
On Sun, Oct 8, 2023 at 11:50 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sat, Oct 7, 2023 at 12:26 PM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
> >
> > Good day,
> > I am interested in working on this issue
> > https://github.com/gitgitgadget/git/issues/1555 as a micro project. Is
> > it worth doing and appropriate for a micro project?
>
> (I have added Dscho, alias Johannes Schindelin, in Cc as he might know
> more about this.)
>
> I think it's worth doing, but I am not sure, as I think it might
> generate regressions for a low number of users who might rely on
> core.fileMode for some special use cases. (Not sure we would want to
> support such hypothetical use cases though.)
>
> Also it's not so simple to implement, you will have to dig a bit in
> the code, and not just implement it, but likely also write tests for
> it. So it's definitely significantly more complex than a simple "test
> cleanup" micro-project. I might overlook things too.
>
> But if you feel like it suits your skills well and are not afraid with
> something a bit complex, I would think that you can go for it.
>
> Best,
> Christian.
^ permalink raw reply
* [PATCH v3] merge-ort: initialize repo in index state
From: John Cai via GitGitGadget @ 2023-10-09 13:21 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, John Cai, John Cai
In-Reply-To: <pull.1583.v2.git.git.1696781998420.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
initialize_attr_index() does not initialize the repo member of
attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=<tree>"
global option to "git", 2023-05-06), this became a problem because
istate->repo gets passed down the call chain starting in
git_check_attr(). This gets passed all the way down to
replace_refs_enabled(), which segfaults when accessing r->gitdir.
Fix this by initializing the repository in the index state.
Signed-off-by: John Cai <johncai86@gmail.com>
Helped-by: Christian Couder <christian.couder@gmail.com>
---
merge-ort: initialize repo in index state
initialize_attr_index() does not initialize the repo member of
attr_index. Starting in 44451a2e5e (attr: teach "--attr-source=" global
option to "git", 2023-05-06), this became a problem because istate->repo
gets passed down the call chain starting in git_check_attr(). This gets
passed all the way down to replace_refs_enabled(), which segfaults when
accessing r->gitdir.
Fix this by initializing the repository in the index state.
Changes since V2:
* fixed test by using printf instead of echo
Changes since v1:
* using opt->repo to avoid hardcoding another the_repository
* clarified test
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1583%2Fjohn-cai%2Fjc%2Fpopulate-repo-when-init-attr-index-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1583/john-cai/jc/populate-repo-when-init-attr-index-v3
Pull-Request: https://github.com/git/git/pull/1583
Range-diff vs v2:
1: e178236064a ! 1: 792b01fa616 merge-ort: initialize repo in index state
@@ t/t4300-merge-tree.sh: EXPECTED
+ git checkout @{-1} &&
+ tree=$(git --attr-source=gitattributes merge-tree --write-tree \
+ --merge-base "$base" --end-of-options "$source" "$merge") &&
-+ echo "foo\nbar\nbaz" >expect &&
++ printf "foo\nbar\nbaz\n" >expect &&
+ git cat-file -p "$tree:file1" >actual &&
+ test_cmp expect actual
+ )
merge-ort.c | 1 +
t/t4300-merge-tree.sh | 27 +++++++++++++++++++++++++++
2 files changed, 28 insertions(+)
diff --git a/merge-ort.c b/merge-ort.c
index 7857ce9fbd1..36537256613 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1902,6 +1902,7 @@ static void initialize_attr_index(struct merge_options *opt)
struct index_state *attr_index = &opt->priv->attr_index;
struct cache_entry *ce;
+ attr_index->repo = opt->repo;
attr_index->initialized = 1;
if (!opt->renormalize)
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 57c4f26e461..c3a03e54187 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -86,6 +86,33 @@ EXPECTED
test_cmp expected actual
'
+test_expect_success '3-way merge with --attr-source' '
+ test_when_finished rm -rf 3-way &&
+ git init 3-way &&
+ (
+ cd 3-way &&
+ test_commit initial file1 foo &&
+ base=$(git rev-parse HEAD) &&
+ git checkout -b brancha &&
+ echo bar >>file1 &&
+ git commit -am "adding bar" &&
+ source=$(git rev-parse HEAD) &&
+ git checkout @{-1} &&
+ git checkout -b branchb &&
+ echo baz >>file1 &&
+ git commit -am "adding baz" &&
+ merge=$(git rev-parse HEAD) &&
+ git checkout -b gitattributes &&
+ test_commit "gitattributes" .gitattributes "file1 merge=union" &&
+ git checkout @{-1} &&
+ tree=$(git --attr-source=gitattributes merge-tree --write-tree \
+ --merge-base "$base" --end-of-options "$source" "$merge") &&
+ printf "foo\nbar\nbaz\n" >expect &&
+ git cat-file -p "$tree:file1" >actual &&
+ test_cmp expect actual
+ )
+'
+
test_expect_success 'file change A, B (same)' '
git reset --hard initial &&
test_commit "change-a-b-same-A" "initial-file" "AAA" &&
base-commit: 493f4622739e9b64f24b465b21aa85870dd9dc09
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Patrick Steinhardt @ 2023-10-09 10:54 UTC (permalink / raw)
To: Taylor Blau
Cc: Junio C Hamano, git, Elijah Newren, Eric W. Biederman, Jeff King
In-Reply-To: <ZSCR7e6KKqFv8mZk@nand.local>
[-- Attachment #1: Type: text/plain, Size: 3831 bytes --]
On Fri, Oct 06, 2023 at 07:02:05PM -0400, Taylor Blau wrote:
> On Fri, Oct 06, 2023 at 03:35:25PM -0700, Junio C Hamano wrote:
> > Taylor Blau <me@ttaylorr.com> writes:
> >
> > > When using merge-tree often within a repository[^1], it is possible to
> > > generate a relatively large number of loose objects, which can result in
> > > degraded performance, and inode exhaustion in extreme cases.
> >
> > Well, be it "git merge-tree" or "git merge", new loose objects tend
> > to accumulate until "gc" kicks in, so it is not a new problem for
> > mere mortals, is it?
>
> Yeah, I would definitely suspect that this is more of an issue for
> forges than individual Git users.
>
> > As one "interesting" use case of "merge-tree" is for a Git hosting
> > site with bare repositories to offer trial merges, without which
> > majority of the object their repositories acquire would have been in
> > packs pushed by their users, "Gee, loose objects consume many inodes
> > in exchange for easier selective pruning" becomes an issue, right?
>
> Right.
>
> > Just like it hurts performance to have too many loose object files,
> > presumably it would also hurt performance to keep too many packs,
> > each came from such a trial merge. Do we have a "gc" story offered
> > for these packs created by the new feature? E.g., "once merge-tree
> > is done creating a trial merge, we can discard the objects created
> > in the pack, because we never expose new objects in the pack to the
> > outside, processes running simultaneously, so instead closing the
> > new packfile by calling flush_bulk_checkin_packfile(), we can safely
> > unlink the temporary pack. We do not even need to spend cycles to
> > run a gc that requires cycles to enumerate what is still reachable",
> > or something like that?
>
> I know Johannes worked on something like this recently. IIRC, it
> effectively does something like:
>
> struct tmp_objdir *tmp_objdir = tmp_objdir_create(...);
> tmp_objdir_replace_primary_odb(tmp_objdir, 1);
>
> at the beginning of a merge operation, and:
>
> tmp_objdir_discard_objects(tmp_objdir);
>
> at the end. I haven't followed that work off-list very closely, but it
> is only possible for GitHub to discard certain niche kinds of
> merges/rebases, since in general we make the objects created during test
> merges available via refs/pull/N/{merge,rebase}.
>
> I think that like anything, this is a trade-off. Having lots of packs
> can be a performance hindrance just like having lots of loose objects.
> But since we can represent more objects with fewer inodes when packed,
> storing those objects together in a pack is preferable when (a) you're
> doing lots of test-merges, and (b) you want to keep those objects
> around, e.g., because they are reachable.
In Gitaly, we usually set up quarantine directories for all operations
that create objects. This allows us to discard any newly written objects
in case either the RPC call gets cancelled or in case our access checks
determine that the change should not be allowed. The logic is rather
simple:
1. Create a new temporary directory.
2. Set up the new temporary directory as main object database via
the `GIT_OBJECT_DIRECTORY` environment variable.
3. Set up the main repository's object database via the
`GIT_ALTERNATE_OBJECT_DIRECTORIES` environment variable.
4. Execute Git commands that write objects with these environment
variables set up. The new objects will end up neatly contained in
the temporary directory.
5. Once done, either discard the temporary object database or
migrate objects into the main object daatabase.
I wonder whether this would be a viable approach for you, as well.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 2/3] rev-list: move `show_commit()` to the bottom
From: Karthik Nayak @ 2023-10-09 10:55 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20231009105528.17777-1-karthik.188@gmail.com>
The `show_commit()` function already depends on `finish_commit()`, and
in the upcoming commit, we'll also add a dependency on
`finish_object__ma()`. Since in C symbols must be declared before
they're used, let's move `show_commit()` below both `finish_commit()`
and `finish_object__ma()`, so the code is cleaner as a whole without the
need for declarations.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 85 +++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 43 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ea77489c38..98542e8b3c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -100,7 +100,48 @@ static off_t get_object_disk_usage(struct object *obj)
return size;
}
-static void finish_commit(struct commit *commit);
+static inline void finish_object__ma(struct object *obj)
+{
+ /*
+ * Whether or not we try to dynamically fetch missing objects
+ * from the server, we currently DO NOT have the object. We
+ * can either print, allow (ignore), or conditionally allow
+ * (ignore) them.
+ */
+ switch (arg_missing_action) {
+ case MA_ERROR:
+ die("missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ case MA_ALLOW_ANY:
+ return;
+
+ case MA_PRINT:
+ oidset_insert(&missing_objects, &obj->oid);
+ return;
+
+ case MA_ALLOW_PROMISOR:
+ if (is_promisor_object(&obj->oid))
+ return;
+ die("unexpected missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(&obj->oid));
+ return;
+
+ default:
+ BUG("unhandled missing_action");
+ return;
+ }
+}
+
+static void finish_commit(struct commit *commit)
+{
+ free_commit_list(commit->parents);
+ commit->parents = NULL;
+ free_commit_buffer(the_repository->parsed_objects,
+ commit);
+}
+
static void show_commit(struct commit *commit, void *data)
{
struct rev_list_info *info = data;
@@ -219,48 +260,6 @@ static void show_commit(struct commit *commit, void *data)
finish_commit(commit);
}
-static void finish_commit(struct commit *commit)
-{
- free_commit_list(commit->parents);
- commit->parents = NULL;
- free_commit_buffer(the_repository->parsed_objects,
- commit);
-}
-
-static inline void finish_object__ma(struct object *obj)
-{
- /*
- * Whether or not we try to dynamically fetch missing objects
- * from the server, we currently DO NOT have the object. We
- * can either print, allow (ignore), or conditionally allow
- * (ignore) them.
- */
- switch (arg_missing_action) {
- case MA_ERROR:
- die("missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- case MA_ALLOW_ANY:
- return;
-
- case MA_PRINT:
- oidset_insert(&missing_objects, &obj->oid);
- return;
-
- case MA_ALLOW_PROMISOR:
- if (is_promisor_object(&obj->oid))
- return;
- die("unexpected missing %s object '%s'",
- type_name(obj->type), oid_to_hex(&obj->oid));
- return;
-
- default:
- BUG("unhandled missing_action");
- return;
- }
-}
-
static int finish_object(struct object *obj, const char *name UNUSED,
void *cb_data)
{
--
2.41.0
^ permalink raw reply related
* [PATCH 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-09 10:55 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20231009105528.17777-1-karthik.188@gmail.com>
The `--missing` object option in rev-list currently works only with
missing blobs/trees. For missing commits the revision walker fails with
a fatal error.
Let's extend the functionality of `--missing` option to also support
commit objects. This is done by adding a new `MISSING` flag that the
revision walker sets whenever it encounters a missing tree/commit. The
revision walker will now continue the traversal and call `show_commit()`
even for missing commits. In rev-list we can then check for this flag
and call the existing code for parsing `--missing` objects.
A scenario where this option would be used is to find the boundary
objects between different object directories. Consider a repository with
a main object directory (GIT_OBJECT_DIRECTORY) and one or more alternate
object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES). In such a
repository, using the `--missing=print` option while disabling the
alternate object directory allows us to find the boundary objects
between the main and alternate object directory.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/rev-list.c | 6 +++
object.h | 2 +-
revision.c | 9 ++++-
revision.h | 3 ++
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++++++++++
5 files changed, 91 insertions(+), 3 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 98542e8b3c..604ae01d0c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -149,6 +149,12 @@ static void show_commit(struct commit *commit, void *data)
display_progress(progress, ++progress_counter);
+ if (revs->do_not_die_on_missing_objects &&
+ commit->object.flags & MISSING) {
+ finish_object__ma(&commit->object);
+ return;
+ }
+
if (show_disk_usage)
total_disk_usage += get_object_disk_usage(&commit->object);
diff --git a/object.h b/object.h
index 114d45954d..12913e6116 100644
--- a/object.h
+++ b/object.h
@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);
/*
* object flag allocation:
- * revision.h: 0---------10 15 23------27
+ * revision.h: 0---------10 15 22------27
* fetch-pack.c: 01 67
* negotiator/default.c: 2--5
* walker.c: 0-2
diff --git a/revision.c b/revision.c
index e789834dd1..615645d3d1 100644
--- a/revision.c
+++ b/revision.c
@@ -1168,7 +1168,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
for (parent = commit->parents; parent; parent = parent->next) {
struct commit *p = parent->item;
int gently = revs->ignore_missing_links ||
- revs->exclude_promisor_objects;
+ revs->exclude_promisor_objects ||
+ revs->do_not_die_on_missing_objects;
if (repo_parse_commit_gently(revs->repo, p, gently) < 0) {
if (revs->exclude_promisor_objects &&
is_promisor_object(&p->object.oid)) {
@@ -1176,7 +1177,11 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
break;
continue;
}
- return -1;
+
+ if (!revs->do_not_die_on_missing_objects)
+ return -1;
+ else
+ p->object.flags |= MISSING;
}
if (revs->sources) {
char **slot = revision_sources_at(revs->sources, p);
diff --git a/revision.h b/revision.h
index c73c92ef40..07906bc3bc 100644
--- a/revision.h
+++ b/revision.h
@@ -41,6 +41,9 @@
/* WARNING: This is also used as REACHABLE in commit-graph.c. */
#define PULL_MERGE (1u<<15)
+/* WARNING: This is also used as NEEDS_BITMAP in pack-bitmap.c. */
+#define MISSING (1u<<22)
+
#define TOPO_WALK_EXPLORED (1u<<23)
#define TOPO_WALK_INDEGREE (1u<<24)
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
new file mode 100755
index 0000000000..bbff66e4fc
--- /dev/null
+++ b/t/t6022-rev-list-missing.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='handling of missing objects in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# We setup the repository with two commits, this way HEAD is always
+# available and we can hide commit 1.
+test_expect_success 'create repository and alternate directory' '
+ test_commit 1 &&
+ test_commit 2
+'
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ test_expect_success "rev-list --missing=error fails with missing object $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ test_must_fail git rev-list --missing=error --objects \
+ --no-object-names HEAD
+ '
+done
+
+for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
+do
+ for action in "allow-any" "print"
+ do
+ test_expect_success "rev-list --missing=$action with missing $obj" '
+ oid="$(git rev-parse $obj)" &&
+ path=".git/objects/$(test_oid_to_path $oid)" &&
+
+ # Before the object is made missing, we use rev-list to
+ # get the expected oids.
+ git rev-list --objects --no-object-names \
+ HEAD ^$obj >expect.raw &&
+
+ # Since both the commits have the `1.t` blob, rev-list
+ # will print it since its reachable from either commit. Unless
+ # the blob itself is missing.
+ if [ $obj != "HEAD:1.t" ]; then
+ echo $(git rev-parse HEAD:1.t) >>expect.raw
+ fi &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git rev-list --missing=$action --objects --no-object-names \
+ HEAD >actual.raw &&
+
+ # When the action is to print, we should also add the missing
+ # oid to the expect list.
+ case $action in
+ allow-any)
+ ;;
+ print)
+ grep ?$oid actual.raw &&
+ echo ?$oid >>expect.raw
+ ;;
+ esac &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+ '
+ done
+done
+
+
+test_done
--
2.41.0
^ permalink raw reply related
* [PATCH 1/3] revision: rename bit to `do_not_die_on_missing_objects`
From: Karthik Nayak @ 2023-10-09 10:55 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
In-Reply-To: <20231009105528.17777-1-karthik.188@gmail.com>
The bit `do_not_die_on_missing_tree` is used in revision.h to ensure the
revision walker does not die when encountering a missing tree. This is
currently exclusively set within `builtin/rev-list.c` to ensure the
`--missing` option works with missing trees.
In the upcoming commits, we will extend `--missing` to also support
missing commits. So let's rename the bit to
`do_not_die_on_missing_objects`, which is object type agnostic and can
be used for both trees/commits.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/reflog.c | 2 +-
builtin/rev-list.c | 2 +-
list-objects.c | 2 +-
revision.h | 17 +++++++++--------
4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index df63a5892e..9e369a5977 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -298,7 +298,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
struct rev_info revs;
repo_init_revisions(the_repository, &revs, prefix);
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
revs.ignore_missing = 1;
revs.ignore_missing_links = 1;
if (verbose)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff715d6918..ea77489c38 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -561,7 +561,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
}
if (arg_missing_action)
- revs.do_not_die_on_missing_tree = 1;
+ revs.do_not_die_on_missing_objects = 1;
argc = setup_revisions(argc, argv, &revs, &s_r_opt);
diff --git a/list-objects.c b/list-objects.c
index c25c72b32c..47296dff2f 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -177,7 +177,7 @@ static void process_tree(struct traversal_context *ctx,
is_promisor_object(&obj->oid))
return;
- if (!revs->do_not_die_on_missing_tree)
+ if (!revs->do_not_die_on_missing_objects)
die("bad tree object %s", oid_to_hex(&obj->oid));
}
diff --git a/revision.h b/revision.h
index 50091bbd13..c73c92ef40 100644
--- a/revision.h
+++ b/revision.h
@@ -212,18 +212,19 @@ struct rev_info {
/*
* Blobs are shown without regard for their existence.
- * But not so for trees: unless exclude_promisor_objects
+ * But not so for trees/commits: unless exclude_promisor_objects
* is set and the tree in question is a promisor object;
* OR ignore_missing_links is set, the revision walker
- * dies with a "bad tree object HASH" message when
- * encountering a missing tree. For callers that can
- * handle missing trees and want them to be filterable
+ * dies with a "bad <type> object HASH" message when
+ * encountering a missing object. For callers that can
+ * handle missing trees/commits and want them to be filterable
* and showable, set this to true. The revision walker
- * will filter and show such a missing tree as usual,
- * but will not attempt to recurse into this tree
- * object.
+ * will filter and show such a missing object as usual,
+ * but will not attempt to recurse into this tree/commit
+ * object. The revision walker will also set the MISSING
+ * flag for such objects.
*/
- do_not_die_on_missing_tree:1,
+ do_not_die_on_missing_objects:1,
/* for internal use only */
exclude_promisor_objects:1;
--
2.41.0
^ permalink raw reply related
* [PATCH 0/3] rev-list: add support for commits in `--missing`
From: Karthik Nayak @ 2023-10-09 10:55 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The `--missing` option in git-rev-list(1) was introduced intitally
to deal with missing blobs in the context of promissory notes.
Eventually the option was extended to also support tree objects in
7c0fe330d5 (rev-list: handle missing tree objects properly,2018-10-05).
This patch series extends the `--missing` option to also add support for
commit objects. We do this by introducing a new flag `MISSING` which is
added whenever we encounter a missing commit during traversal. Then in
`builtin/rev-list` we check for this flag and take the appropriate
action based on the `--missing=*` option used.
This series is an alternate to the patch series I had posted earlier:
https://lore.kernel.org/git/20230908174208.249184-1-karthik.188@gmail.com/.
In that patch, we introduced an option `--ignore-missing-links` which
was added to expose the `ignore_missing_links` bit to the user. The
issue in that patch was that, the option `--ignore-missing-links` didn't
play well the pre-existing `--missing` option. This series avoids that
route and just extends the `--missing` option for commits to solve the
same problem.
Karthik Nayak (3):
revision: rename bit to `do_not_die_on_missing_objects`
rev-list: move `show_commit()` to the bottom
rev-list: add commit object support in `--missing` option
builtin/reflog.c | 2 +-
builtin/rev-list.c | 93 +++++++++++++++++++------------------
list-objects.c | 2 +-
object.h | 2 +-
revision.c | 9 +++-
revision.h | 20 ++++----
t/t6022-rev-list-missing.sh | 74 +++++++++++++++++++++++++++++
7 files changed, 145 insertions(+), 57 deletions(-)
create mode 100755 t/t6022-rev-list-missing.sh
--
2.41.0
^ permalink raw reply
* Re: [PATCH 1/4] ref-cache.c: fix prefix matching in ref iteration
From: Patrick Steinhardt @ 2023-10-09 10:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Victoria Dye via GitGitGadget, git, Victoria Dye
In-Reply-To: <xmqqfs2n8lnn.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]
On Fri, Oct 06, 2023 at 02:51:24PM -0700, Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Victoria Dye <vdye@github.com>
> >
> > Update 'cache_ref_iterator_advance' to skip over refs that are not matched
> > by the given prefix.
> >
> > Currently, a ref entry is considered "matched" if the entry name is fully
> > contained within the prefix:
> >
> > * prefix: "refs/heads/v1"
> > * entry: "refs/heads/v1.0"
> >
> > OR if the prefix is fully contained in the entry name:
> >
> > * prefix: "refs/heads/v1.0"
> > * entry: "refs/heads/v1"
> >
> > The first case is always correct, but the second is only correct if the ref
> > cache entry is a directory, for example:
> >
> > * prefix: "refs/heads/example"
> > * entry: "refs/heads/"
> >
> > Modify the logic in 'cache_ref_iterator_advance' to reflect these
> > expectations:
> >
> > 1. If 'overlaps_prefix' returns 'PREFIX_EXCLUDES_DIR', then the prefix and
> > ref cache entry do not overlap at all. Skip this entry.
> > 2. If 'overlaps_prefix' returns 'PREFIX_WITHIN_DIR', then the prefix matches
> > inside this entry if it is a directory. Skip if the entry is not a
> > directory, otherwise iterate over it.
> > 3. Otherwise, 'overlaps_prefix' returned 'PREFIX_CONTAINS_DIR', indicating
> > that the cache entry (directory or not) is fully contained by or equal to
> > the prefix. Iterate over this entry.
> >
> > Note that condition 2 relies on the names of directory entries having the
> > appropriate trailing slash. The existing function documentation of
> > 'create_dir_entry' explicitly calls out the trailing slash requirement, so
> > this is a safe assumption to make.
>
> Thanks for explaining it very well and clearly.
>
> Allowing prefix="refs/heads/v1.0" to yield entry="refs/heads/v1"
> (case #2 above that this patch fixes the behaviour for) would cause
> ref_iterator_advance() to return a ref outside the hierarhcy,
> wouldn't it? So it appears to me that either one of the two would
> be true:
>
> * the code is structured in such a way that such a condition does
> not actually happen (in which case this patch would be a no-op),
> or
>
> * there is a bug in the current code that is fixed by this patch,
> whose externally observable behaviour can be verified with a
> test.
>
> It is not quite clear to me which is the case here. The code with
> the patch looks more logical than the original, but I am not sure
> how to demonstrate the existing breakage (if any).
Agreed, I also had a bit of a hard time to figure out whether this is an
actual bug fix, a performance improvement or merely a refactoring.
Patrick
> > Signed-off-by: Victoria Dye <vdye@github.com>
> > ---
> > refs/ref-cache.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> > index 2294c4564fb..6e3b725245c 100644
> > --- a/refs/ref-cache.c
> > +++ b/refs/ref-cache.c
> > @@ -412,7 +412,8 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
> >
> > if (level->prefix_state == PREFIX_WITHIN_DIR) {
> > entry_prefix_state = overlaps_prefix(entry->name, iter->prefix);
> > - if (entry_prefix_state == PREFIX_EXCLUDES_DIR)
> > + if (entry_prefix_state == PREFIX_EXCLUDES_DIR ||
> > + (entry_prefix_state == PREFIX_WITHIN_DIR && !(entry->flag & REF_DIR)))
> > continue;
> > } else {
> > entry_prefix_state = level->prefix_state;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/4] Performance improvement & cleanup in loose ref iteration
From: Patrick Steinhardt @ 2023-10-09 10:04 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <pull.1594.git.1696615769.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5313 bytes --]
On Fri, Oct 06, 2023 at 06:09:25PM +0000, Victoria Dye via GitGitGadget wrote:
> While investigating ref iteration performance in builtins like
> 'for-each-ref' and 'show-ref', I found two small improvement opportunities.
>
> The first patch tweaks the logic around prefix matching in
> 'cache_ref_iterator_advance' so that we correctly skip refs that do not
> actually match a given prefix. The unnecessary iteration doesn't seem to be
> causing any bugs in the ref iteration commands that I've tested, but it
> doesn't hurt to be more precise (and it helps with some other patches I'm
> working on ;) ).
>
> The next three patches update how 'loose_fill_ref_dir' determines the type
> of ref cache entry to create (directory or regular). On platforms that
> include d_type information in 'struct dirent' (as far as I can tell, all
> except NonStop & certain versions of Cygwin), this allows us to skip calling
> 'stat'. In ad-hoc testing, this improved performance of 'git for-each-ref'
> by about 20%.
I've done a small set of benchmarks with my usual test repositories,
which is linux.git with a bunch of references added. The repository
comes in four sizes:
- small: 50k references
- medium: 500k references
- high: 1.1m references
- huge: 12m references
Unfortunately, I couldn't really reproduce the performance improvements.
In fact, the new version runs consistently a tiny bit slower than the
old version:
# Old version, which is 3a06386e31 (The fifteenth batch, 2023-10-04).
Benchmark 1: git for-each-ref (revision=old,refcount=small)
Time (mean ± σ): 135.5 ms ± 1.2 ms [User: 76.4 ms, System: 59.0 ms]
Range (min … max): 134.8 ms … 136.9 ms 3 runs
Benchmark 2: git for-each-ref (revision=old,refcount=medium)
Time (mean ± σ): 822.7 ms ± 2.2 ms [User: 697.4 ms, System: 125.1 ms]
Range (min … max): 821.1 ms … 825.2 ms 3 runs
Benchmark 3: git for-each-ref (revision=old,refcount=high)
Time (mean ± σ): 1.960 s ± 0.015 s [User: 1.702 s, System: 0.257 s]
Range (min … max): 1.944 s … 1.973 s 3 runs
# New version, which is your tip.
Benchmark 4: git for-each-ref (revision=old,refcount=huge)
Time (mean ± σ): 16.815 s ± 0.054 s [User: 15.091 s, System: 1.722 s]
Range (min … max): 16.760 s … 16.869 s 3 runs
Benchmark 5: git for-each-ref (revision=new,refcount=small)
Time (mean ± σ): 136.0 ms ± 0.2 ms [User: 78.8 ms, System: 57.1 ms]
Range (min … max): 135.8 ms … 136.2 ms 3 runs
Benchmark 6: git for-each-ref (revision=new,refcount=medium)
Time (mean ± σ): 830.4 ms ± 21.2 ms [User: 691.3 ms, System: 138.7 ms]
Range (min … max): 814.2 ms … 854.5 ms 3 runs
Benchmark 7: git for-each-ref (revision=new,refcount=high)
Time (mean ± σ): 1.966 s ± 0.013 s [User: 1.717 s, System: 0.249 s]
Range (min … max): 1.952 s … 1.978 s 3 runs
Benchmark 8: git for-each-ref (revision=new,refcount=huge)
Time (mean ± σ): 16.945 s ± 0.037 s [User: 15.182 s, System: 1.760 s]
Range (min … max): 16.910 s … 16.983 s 3 runs
Summary
git for-each-ref (revision=old,refcount=small) ran
1.00 ± 0.01 times faster than git for-each-ref (revision=new,refcount=small)
6.07 ± 0.06 times faster than git for-each-ref (revision=old,refcount=medium)
6.13 ± 0.17 times faster than git for-each-ref (revision=new,refcount=medium)
14.46 ± 0.17 times faster than git for-each-ref (revision=old,refcount=high)
14.51 ± 0.16 times faster than git for-each-ref (revision=new,refcount=high)
124.09 ± 1.15 times faster than git for-each-ref (revision=old,refcount=huge)
125.05 ± 1.12 times faster than git for-each-ref (revision=new,refcount=huge)
The performance regression isn't all that concerning, but it makes me
wonder why I see things becoming slower rather than faster. My guess is
that this is because all my test repositories are well-packed and don't
have a lot of loose references. But I just wanted to confirm how you
benchmarked your change and what the underlying shape of your test repo
was.
Patrick
> Thanks!
>
> * Victoria
>
> Victoria Dye (4):
> ref-cache.c: fix prefix matching in ref iteration
> dir.[ch]: expose 'get_dtype'
> dir.[ch]: add 'follow_symlink' arg to 'get_dtype'
> files-backend.c: avoid stat in 'loose_fill_ref_dir'
>
> diagnose.c | 42 +++---------------------------------------
> dir.c | 33 +++++++++++++++++++++++++++++++++
> dir.h | 16 ++++++++++++++++
> refs/files-backend.c | 14 +++++---------
> refs/ref-cache.c | 3 ++-
> 5 files changed, 59 insertions(+), 49 deletions(-)
>
>
> base-commit: 3a06386e314565108ad56a9bdb8f7b80ac52fb69
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1594%2Fvdye%2Fvdye%2Fref-iteration-cleanup-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1594/vdye/vdye/ref-iteration-cleanup-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1594
> --
> gitgitgadget
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: Phillip Wood @ 2023-10-09 9:59 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Elijah Newren, Izzy
In-Reply-To: <xmqqh6n24zf1.fsf@gitster.g>
On 07/10/2023 09:20, Junio C Hamano wrote:
> * ty/merge-tree-strategy-options (2023-09-25) 1 commit
> (merged to 'next' on 2023-09-29 at aa65b54416)
> + merge-tree: add -X strategy option
>
> "git merge-tree" learned to take strategy backend specific options
> via the "-X" option, like "git merge" does.
>
> Will merge to 'master'.
> source: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>
We may want to hold off on this as there is a wierd copy-by-value of
struct merge-options cf <a482d047-dd40-436d-8daa-0c74780af11f@gmail.com>
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v6] merge-tree: add -X strategy option
From: Phillip Wood @ 2023-10-09 9:58 UTC (permalink / raw)
To: Izzy via GitGitGadget, git; +Cc: Elijah Newren, Jeff King, Izzy
In-Reply-To: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>
On 24/09/2023 03:23, Izzy via GitGitGadget wrote:
Thanks for updating the patch, sorry it has taken me a while to look at
this.
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> static int real_merge(struct merge_tree_options *o,
> @@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
> {
> struct commit *parent1, *parent2;
> struct commit_list *merge_bases = NULL;
> - struct merge_options opt;
> + struct merge_options opt = o->merge_options;
Copying struct merge_options by value here is unusual in our code base.
I don't think it introduces a bug (there is no function to free the
resources allocated in struct merge_options so we do not end up freeing
them twice for example) but it would be clearer that it was safe if you did
struct merge_options *opt = &o->merge_options;
and updated the code to reflect that opt is now a pointer or just
replaced all uses of "opt" with "o->merge_options"
> + if (xopts.nr && o.mode == MODE_TRIVIAL)
> + die(_("--trivial-merge is incompatible with all other options"));
This is good, thanks for adding it.
> + for (int x = 0; x < xopts.nr; x++)
This is not worth a re-roll on its own but as xopts.nr is a size_t we
should declare x to be the same type rather than an int.
The tests are pretty minimal, ideally we would check that "-X" works
with "--stdin", that it is rejected by a trivial merge and that one of
the other strategy options works.
Best Wishes
Phillip
^ 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