* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Christian Couder @ 2023-09-15 10:09 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
On Wed, Sep 13, 2023 at 9:17 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> Here is a small reroll of my series to clean up some of the internals of
> 'git repack' used to track the set of existing packs.
>
> Much is unchanged from the last round, save for some additional clean-up
> on how we handle the '->util' field for each pack's string_list_item in
> response to very helpful review from those CC'd.
>
> As usual, a range-diff is available below for convenience. Thanks in
> advance for your review!
>
> Taylor Blau (8):
> builtin/repack.c: extract structure to store existing packs
> builtin/repack.c: extract marking packs for deletion
> builtin/repack.c: extract redundant pack cleanup for --geometric
> builtin/repack.c: extract redundant pack cleanup for existing packs
> builtin/repack.c: extract `has_existing_non_kept_packs()`
> builtin/repack.c: store existing cruft packs separately
> builtin/repack.c: avoid directly inspecting "util"
> builtin/repack.c: extract common cruft pack loop
I think it would be a bit nicer with s/builtin\/repack.c/repack/ in
all the above commit subjects, but I don't think it's worth a reroll.
Except for another very small nit in a commit message also not worth a
reroll, this LGTM.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v2 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`
From: Christian Couder @ 2023-09-15 10:02 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <f4f7b4c08f682b5ebca474e8c1d51d31a2da76b8.1694632644.git.me@ttaylorr.com>
On Thu, Sep 14, 2023 at 12:13 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> When there is:
>
> - at least one pre-existing packfile (which is not marked as kept),
> - repacking with the `-d` flag, and
> - not doing a cruft repack
>
> , then we pass a handful of additional options to the inner
Nit (not worth a reroll): I think the comma at the beginning of the
above line would be better after "not doing a cruft repack".
> `pack-objects` process, like `--unpack-unreachable`,
> `--keep-unreachable`, and `--pack-loose-unreachable`, in addition to
> marking any packs we just wrote for promisor remotes as kept in-core
> (with `--keep-pack`, as opposed to the presence of a ".keep" file on
> disk).
^ permalink raw reply
* [PATCH v3] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-15 8:34 UTC (permalink / raw)
To: karthik.188; +Cc: git, gitster, me
In-Reply-To: <20230912155820.136111-1-karthik.188@gmail.com>
From: Karthik Nayak <karthik.188@gmail.com>
The revision backend is used by multiple porcelain commands such as
git-rev-list(1) and git-log(1). The backend currently supports ignoring
missing links by setting the `ignore_missing_links` bit. This allows the
revision walk to skip any objects links which are missing. Expose this
bit via an `--ignore-missing-links` user option.
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, enabling this option along with the `--boundary` option
while disabling the alternate object directory allows us to find the
boundary objects between the main and alternate object directory.
Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes since v2:
- Refactored the tests thanks to Taylor!
Range diff against version 2:
1: e3f4d85732 ! 1: a08f3637a0 revision: add `--ignore-missing-links` user option
@@ Commit message
while disabling the alternate object directory allows us to find the
boundary objects between the main and alternate object directory.
+ Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## Documentation/rev-list-options.txt ##
@@ t/t6022-rev-list-alternates.sh (new)
+# We create 5 commits and move them to the alt directory and
+# create 5 more commits which will stay in the main odb.
+test_expect_success 'create repository and alternate directory' '
-+ git init main &&
-+ test_commit_bulk -C main 5 &&
-+ BOUNDARY_COMMIT=$(git -C main rev-parse HEAD) &&
-+ mkdir alt &&
-+ mv main/.git/objects/* alt &&
-+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5
++ test_commit_bulk 5 &&
++ git clone --reference=. --shared . alt &&
++ test_commit_bulk --start=6 -C alt 5
+'
+
+# when the alternate odb is provided, all commits are listed along with the boundary
+# commit.
+test_expect_success 'rev-list passes with alternate object directory' '
-+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&
-+ test_stdout_line_count = 10 cat actual &&
-+ grep $BOUNDARY_COMMIT actual
++ git -C alt rev-list --all --objects --no-object-names >actual.raw &&
++ {
++ git rev-list --all --objects --no-object-names &&
++ git -C alt rev-list --all --objects --no-object-names --not \
++ --alternate-refs
++ } >expect.raw &&
++ sort actual.raw >actual &&
++ sort expect.raw >expect &&
++ test_cmp expect actual
+'
+
++alt=alt/.git/objects/info/alternates
++
++hide_alternates () {
++ test -f "$alt.bak" || mv "$alt" "$alt.bak"
++}
++
++show_alternates () {
++ test -f "$alt" || mv "$alt.bak" "$alt"
++}
++
+# When the alternate odb is not provided, rev-list fails since the 5th commit's
+# parent is not present in the main odb.
+test_expect_success 'rev-list fails without alternate object directory' '
-+ test_must_fail git -C main rev-list HEAD
++ hide_alternates &&
++ test_must_fail git -C alt rev-list HEAD
+'
+
+# With `--ignore-missing-links`, we stop the traversal when we encounter a
+# missing link. The boundary commit is not listed as we haven't used the
+# `--boundary` options.
+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '
-+ git -C main rev-list --ignore-missing-links HEAD >actual &&
-+ test_stdout_line_count = 5 cat actual &&
-+ ! grep -$BOUNDARY_COMMIT actual
++ hide_alternates &&
++
++ git -C alt rev-list --objects --no-object-names \
++ --ignore-missing-links HEAD >actual.raw &&
++ git -C alt cat-file --batch-check="%(objectname)" \
++ --batch-all-objects >expect.raw &&
++
++ sort actual.raw >actual &&
++ sort expect.raw >expect &&
++ test_must_fail git -C alt rev-list HEAD
+'
+
+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
+# commits.
+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '
-+ git -C main rev-list --ignore-missing-links --boundary HEAD >actual &&
-+ test_stdout_line_count = 6 cat actual &&
-+ grep -$BOUNDARY_COMMIT actual
++ git -C alt rev-list --ignore-missing-links --boundary HEAD >got &&
++ grep "^-$(git rev-parse HEAD)" got
+'
+
-+# The `--ignore-missing-links` option should ensure that git-rev-list(1) doesn't
-+# fail when used alongside `--objects` when a tree is missing.
-+test_expect_success 'rev-list --ignore-missing-links works with missing tree' '
-+ echo "foo" >main/file &&
-+ git -C main add file &&
-+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m"commit 11" &&
-+ TREE_OID=$(git -C main rev-parse HEAD^{tree}) &&
-+ mkdir alt/${TREE_OID:0:2} &&
-+ mv main/.git/objects/${TREE_OID:0:2}/${TREE_OID:2} alt/${TREE_OID:0:2}/ &&
-+ git -C main rev-list --ignore-missing-links --objects HEAD >actual &&
-+ ! grep $TREE_OID actual
++test_expect_success "setup for rev-list --ignore-missing-links with missing objects" '
++ show_alternates &&
++ test_commit -C alt 11
+'
+
-+# Similar to above, it should also work when a blob is missing.
-+test_expect_success 'rev-list --ignore-missing-links works with missing blob' '
-+ echo "bar" >main/file &&
-+ git -C main add file &&
-+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main commit -m"commit 12" &&
-+ BLOB_OID=$(git -C main rev-parse HEAD:file) &&
-+ mkdir alt/${BLOB_OID:0:2} &&
-+ mv main/.git/objects/${BLOB_OID:0:2}/${BLOB_OID:2} alt/${BLOB_OID:0:2}/ &&
-+ git -C main rev-list --ignore-missing-links --objects HEAD >actual &&
-+ ! grep $BLOB_OID actual
-+'
++for obj in "HEAD^{tree}" "HEAD:11.t"
++do
++ # The `--ignore-missing-links` option should ensure that git-rev-list(1)
++ # doesn't fail when used alongside `--objects` when a tree/blob is
++ # missing.
++ test_expect_success "rev-list --ignore-missing-links with missing $type" '
++ oid="$(git -C alt rev-parse $obj)" &&
++ path="alt/.git/objects/$(test_oid_to_path $oid)" &&
++
++ mv "$path" "$path.hidden" &&
++ test_when_finished "mv $path.hidden $path" &&
++
++ git -C alt rev-list --ignore-missing-links --objects HEAD \
++ >actual &&
++ ! grep $oid actual
++ '
++done
+
+test_done
Documentation/rev-list-options.txt | 9 +++
builtin/rev-list.c | 3 +-
revision.c | 2 +
t/t6022-rev-list-alternates.sh | 93 ++++++++++++++++++++++++++++++
4 files changed, 106 insertions(+), 1 deletion(-)
create mode 100755 t/t6022-rev-list-alternates.sh
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b2..8ee713db3d 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -227,6 +227,15 @@ explicitly.
Upon seeing an invalid object name in the input, pretend as if
the bad input was not given.
+--ignore-missing-links::
+ During traversal, if an object that is referenced does not
+ exist, instead of dying of a repository corruption, pretend as
+ if the reference itself does not exist. Running the command
+ with the `--boundary` option makes these missing commits,
+ together with the commits on the edge of revision ranges
+ (i.e. true boundary objects), appear on the output, prefixed
+ with '-'.
+
ifndef::git-rev-list[]
--bisect::
Pretend as if the bad bisection ref `refs/bisect/bad`
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff715d6918..5239d83c76 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -266,7 +266,8 @@ static int finish_object(struct object *obj, const char *name UNUSED,
{
struct rev_list_info *info = cb_data;
if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
- finish_object__ma(obj);
+ if (!info->revs->ignore_missing_links)
+ finish_object__ma(obj);
return 1;
}
if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
diff --git a/revision.c b/revision.c
index 2f4c53ea20..cbfcbf6e28 100644
--- a/revision.c
+++ b/revision.c
@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
+ } else if (!strcmp(arg, "--ignore-missing-links")) {
+ revs->ignore_missing_links = 1;
} else if (opt && opt->allow_exclude_promisor_objects &&
!strcmp(arg, "--exclude-promisor-objects")) {
if (fetch_if_missing)
diff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh
new file mode 100755
index 0000000000..567dd21876
--- /dev/null
+++ b/t/t6022-rev-list-alternates.sh
@@ -0,0 +1,93 @@
+#!/bin/sh
+
+test_description='handling of alternates in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# We create 5 commits and move them to the alt directory and
+# create 5 more commits which will stay in the main odb.
+test_expect_success 'create repository and alternate directory' '
+ test_commit_bulk 5 &&
+ git clone --reference=. --shared . alt &&
+ test_commit_bulk --start=6 -C alt 5
+'
+
+# when the alternate odb is provided, all commits are listed along with the boundary
+# commit.
+test_expect_success 'rev-list passes with alternate object directory' '
+ git -C alt rev-list --all --objects --no-object-names >actual.raw &&
+ {
+ git rev-list --all --objects --no-object-names &&
+ git -C alt rev-list --all --objects --no-object-names --not \
+ --alternate-refs
+ } >expect.raw &&
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_cmp expect actual
+'
+
+alt=alt/.git/objects/info/alternates
+
+hide_alternates () {
+ test -f "$alt.bak" || mv "$alt" "$alt.bak"
+}
+
+show_alternates () {
+ test -f "$alt" || mv "$alt.bak" "$alt"
+}
+
+# When the alternate odb is not provided, rev-list fails since the 5th commit's
+# parent is not present in the main odb.
+test_expect_success 'rev-list fails without alternate object directory' '
+ hide_alternates &&
+ test_must_fail git -C alt rev-list HEAD
+'
+
+# With `--ignore-missing-links`, we stop the traversal when we encounter a
+# missing link. The boundary commit is not listed as we haven't used the
+# `--boundary` options.
+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '
+ hide_alternates &&
+
+ git -C alt rev-list --objects --no-object-names \
+ --ignore-missing-links HEAD >actual.raw &&
+ git -C alt cat-file --batch-check="%(objectname)" \
+ --batch-all-objects >expect.raw &&
+
+ sort actual.raw >actual &&
+ sort expect.raw >expect &&
+ test_must_fail git -C alt rev-list HEAD
+'
+
+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
+# commits.
+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '
+ git -C alt rev-list --ignore-missing-links --boundary HEAD >got &&
+ grep "^-$(git rev-parse HEAD)" got
+'
+
+test_expect_success "setup for rev-list --ignore-missing-links with missing objects" '
+ show_alternates &&
+ test_commit -C alt 11
+'
+
+for obj in "HEAD^{tree}" "HEAD:11.t"
+do
+ # The `--ignore-missing-links` option should ensure that git-rev-list(1)
+ # doesn't fail when used alongside `--objects` when a tree/blob is
+ # missing.
+ test_expect_success "rev-list --ignore-missing-links with missing $type" '
+ oid="$(git -C alt rev-parse $obj)" &&
+ path="alt/.git/objects/$(test_oid_to_path $oid)" &&
+
+ mv "$path" "$path.hidden" &&
+ test_when_finished "mv $path.hidden $path" &&
+
+ git -C alt rev-list --ignore-missing-links --objects HEAD \
+ >actual &&
+ ! grep $oid actual
+ '
+done
+
+test_done
--
2.41.0
^ permalink raw reply related
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Dragan Simic @ 2023-09-15 5:59 UTC (permalink / raw)
To: Linus Arver; +Cc: Junio C Hamano, git
In-Reply-To: <owlyedj0jok7.fsf@fine.c.googlers.com>
On 2023-09-15 01:57, Linus Arver wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Dragan Simic <dsimic@manjaro.org> writes:
>>
>>> Mention in the documentation that --rebase defaults to false.
>>
>> I am not quite sure if this is an improvement, though.
>>
>> It is true that, if you do not have any of your own funny
>> configuration, your "git pull" will *not* rebase.
>>
>> But that is "if you do not give the --rebase option, you do not
>> trigger the rebase action". Which is quite natural, but it is
>> different from what you wrote above, isn't it?
>>
>> When people say "the default for `--rebase` is false", they mean
>> this:
>>
>> I can say "git pull --rebase=<kind>" to specify how my current
>> branch is rebased on top of the upstream. But if I do not
>> specify the <kind>, i.e. "git pull --rebase", the command will
>> act as if I gave 'false' as the <kind>.
>>
>> At least, I would think that is what the word "default" means.
>>
>> And I would be surprised if the "default" in that sense is 'false';
>> isn't the default <kind> 'true' --- meaning "perform the plain
>> vanilla 'git rebase'", unless you explicitly asked for 'merges',
>> 'interactive' or 'false'?
>>
>> After the context of the hunk your patch touches, there is a
>> description on `pull.rebase`. Down there, if you do not set
>> `pull.rebase` or `branch.<name>.rebase` for the current branch at
>> all, the system acts as if you had `false` for these variables. In
>> other words, the default for these variables is `false`, meaning "do
>> not rebase, just merge". But the default option argument for the
>> `--rebase` option given without argument would not be `false`, I
>> would think.
>
> I think there are two reasonable interpretations of the word "default"
> here.
>
> (1) "git pull": This is equivalent to "git pull --rebase=false". So one
> could say that "git pull" defaults to "--rebase=false".
>
> (2) "git pull --rebase": This is equivalent to "git pull
> --rebase=true".
> So one could say that the "--rebase" flag (when it is used as
> "--rebase"
> without the "=..." part) defaults to "--rebase=true".
>
> I assume Dragan was thinking of only the meaning in (1). The alternate
> meaning of (2) which you brought up makes sense too.
>
> I think both flavors of "default" are reasonable interpretations
> because
> most (if not all) Git users will start out with (1) in mind as they get
> familiar with how "git pull" works without any flags (i.e., the
> "default" git-pull behavior). Eventually they'll learn about (2) and
> realize how the <kind> defaults to "true" with the "--rebase" flag.
>
> Aside: interestingly, there appears to be a "--no-rebase" option that
> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about
> --rebase,
> 2007-11-28)):
>
> --no-rebase
> This is shorthand for --rebase=false.
>
> For reference here is a more complete view of the docs for this flag:
>
> -r::
> --rebase[=false|true|merges|interactive]::
> When true, rebase the current branch on top of the upstream
> branch after fetching. If there is a remote-tracking branch
> corresponding to the upstream branch and the upstream branch
> was rebased since last fetched, the rebase uses that
> information
> to avoid rebasing non-local changes.
> +
> When set to `merges`, rebase using `git rebase --rebase-merges` so
> that
> the local merge commits are included in the rebase (see
> linkgit:git-rebase[1] for details).
> +
> When false, merge the upstream branch into the current branch.
> +
> When `interactive`, enable the interactive mode of rebase.
> +
> See `pull.rebase`, `branch.<name>.rebase` and
> `branch.autoSetupRebase` in
> linkgit:git-config[1] if you want to make `git pull` always use
> `--rebase` instead of merging.
>
> How about adding something like this instead as the very first
> paragraph
> for this flag?
>
> Supplying "--rebase" defaults to "--rebase=true". Running git-pull
> without arguments implies "--rebase=false", unless relevant
> configuration variables have been set otherwise.
>
> This way, we can discuss what "true" and "false" mean first to get
> these
> two flavors of "defaults" sorted out (perhaps also mentioning
> --no-rebase while we're at it). Then we can discuss the other <kind>
> values like "merges" and "interactive".
I'm really, really happy to see such a detailed approach to refining the
git documentation! Both Junio's and Linus's feedback are highly
appreciated.
I totally agree about adding the above-proposed text as the opening
paragraph for this option. It's much better than just saying "this is
the default", which is admittedly confusing and somewhat misleading.
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Christian Couder @ 2023-09-15 5:56 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git, Jeff King, Patrick Steinhardt
In-Reply-To: <ZQNKkn0YYLUyN5Ih@nand.local>
On Thu, Sep 14, 2023 at 8:01 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Thu, Sep 14, 2023 at 01:10:38PM +0200, Christian Couder wrote:
> > Ok, I will try to review and merge this with
> > cc/repack-sift-filtered-objects-to-separate-pack soon.
>
> I took a look at how much/little effort was going to be required, and
> luckily the changes are isolated only to a single patch. It's just your
> f1ffa71e8f (repack: add `--filter=<filter-spec>` option, 2023-09-11),
> and in particular the `write_filtered_pack()` function.
>
> I started messing around with it myself and generated the following
> fixup! which can be applied on top of your version of f1ffa71e8f. It's
> mostly straightforward, but there is a gotcha that the loop over
> non-kept packs has to change to:
>
> for_each_string_list_item(item, &existing->non_kept_packs)
> /* ... */
> for_each_string_list_item(item, &existing->cruft_packs)
> /* ... */
>
> , instead of just the first loop over non_kept_packs, since cruft packs
> are stored in a separate list.
>
> In any event, here's the fixup! I generated on top of that patch:
Thanks a lot! I will very likely use it in the new version I will send
after your series has graduated.
^ permalink raw reply
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
From: Rubén Justo @ 2023-09-15 0:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <20230912082742.GB1630538@coredump.intra.peff.net>
On 12-sep-2023 04:27:42, Jeff King wrote:
> On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:
>
> > GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
> > test return zero unintentionally:
> >
> > $ git checkout v2.40.1
> > $ make SANITIZE=leak
> > $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
> > ...
> > With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
> > # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
> >
> > Let's use invert_exit_code only if needed.
>
> Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe
> not), but then we _also_ saw entries in the leak-log. So the inversion
> cancels out, and we accidentally say everything is OK, which is wrong.
>
> I'm not quite sure of your fix, though. In the if-else chain you're
> touching, we know going in that we found a leak in the log. And then we
> cover these 5 cases:
>
> 1. if the test is marked as passing-leak
> a. if we saw no test failures, invert (and mention the leaking log)
> b. otherwise, do not invert (and mention the log)
> 2. else if we are in "check" mode
> a. if we saw no test failures, do not invert (we do have a leak,
> which is equivalent to a test failure). Mention the log.
> b. otherwise, invert (to switch back to "success", since we are
> looking for leaks), but still mention the log.
> 3. invert to trigger failure (and mention the log)
>
> And the problem is in (3). You switch it to trigger only if we have no
> failures (fixing the inversion). But should we have the same a/b split
> for this case? I.e.:
>
> 3a. if we saw no test failures, invert to cause a failure
> 3b. we saw other failures; do not invert, but _do_ mention that the
> log found extra leaks
>
> In 3b we are explaining to the user what happened. Though maybe it is
> not super important, because I think we'd have dumped the log contents
> anyway?
I think so too. At that point we've already dumped the contents of the
$TEST_RESULTS_SAN_FILE file.
IMO, when $test_failure is zero (the "if" I'm touching), the message
makes sense not so much to say that a leak has been found, but rather
because we're forcing the non-zero exit.
But when $test_failure is not zero, after we've already dumped the
log, maybe this is somewhat redundant:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 87cfea9e9a..b160ae3f7a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
then
say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
invert_exit_code=t
+ else
+ say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak"
fi
}
However, if you or anyone else thinks it adds value, I have no objection
to re-roll with it.
> Other than that, I think the patch is correct. I wondered when we ran
> this "check_test_results_san_file_" code, but it is only at the end of
> the script. So we are OK to make a definitive call on the zero/non-zero
> count of failed tests.
>
> -Peff
Thank you for taking the time to review these series.
^ permalink raw reply related
* Re: [PATCH] cache: add fake_lstat()
From: Josip Sokcevic @ 2023-09-15 0:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqr0n0h0tw.fsf@gitster.g>
On Thu, Sep 14, 2023 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> And your diff-lib change may look like this on top. This one
> unfortunately is completely untested, as I do not use macOS.
>
> Reviewing, testing, and improving it is highly appreciated.
The patches look good to me, and all tests pass on macOS.
--
Josip Sokcevic
^ permalink raw reply
* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
From: Rubén Justo @ 2023-09-15 0:10 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Ævar Arnfjörð Bjarmason
In-Reply-To: <20230912083528.GC1630538@coredump.intra.peff.net>
On 12-sep-2023 04:35:28, Jeff King wrote:
> On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo 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
> > 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.
>
> OK, so we are trying to find a case where the user is triggering
> --invert-exit-code themselves and complaining. But in the code...
>
> > @@ -1557,15 +1557,25 @@ then
> > say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
> > invert_exit_code=t
> > fi
> > - elif test -z "$passes_sanitize_leak" &&
> > - test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> > + elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> > then
> > - skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > - test_done
> > + if test -n "$invert_exit_code"
> > + then
> > + BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > + elif test -z "$passes_sanitize_leak"
> > + then
> > + skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > + test_done
> > + fi
> > fi
>
> You can see at the top of the context that we will set
> invert_exit_code=t ourselves, which will then complain here:
>
> > if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> > then
> > + if test -n "$invert_exit_code"
> > + then
> > + BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
> > + fi
> > +
> > if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
> > then
> > BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
>
> That varible-set in the earlier context is from running in "check" mode.
> So:
>
> make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true
>
> will now always fail. But this is the main way you'd want to run it
> (enabling the leak log catches more stuff, and the log-check function
> you touch in patch 2 already covers check mode).
>
> So I think you'd have to hoist your check above the if/else for setting
> up PASSING_SANITIZE_LEAK modes.
Arrg, sorry. You're right.
This was part of the series by mistake. Please, discard it.
In my tree, I have a previous commit with:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 87cfea9e9a..46b8a76e9c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1556,7 +1556,6 @@ then
if test -z "$passes_sanitize_leak"
then
say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
- invert_exit_code=t
fi
elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
then
that is part of an unsent attempt to make:
$ make SANITIZE=leak T=t7510-signed-commit.sh GIT_TEST_PASSING_SANITIZE_LEAK=check test
not to suggest, when GPG is missing, that t7510-signed-commit.sh is a
candidate to be marked as leak-free. Which is distracting to me.
However I was not satisfied with the solution and discarded it. But
unfortunately not entirely. Sorry.
>
> -Peff
^ permalink raw reply related
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Linus Arver @ 2023-09-14 23:57 UTC (permalink / raw)
To: Junio C Hamano, Dragan Simic; +Cc: git
In-Reply-To: <xmqqzg1oinq1.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Mention in the documentation that --rebase defaults to false.
>
> I am not quite sure if this is an improvement, though.
>
> It is true that, if you do not have any of your own funny
> configuration, your "git pull" will *not* rebase.
>
> But that is "if you do not give the --rebase option, you do not
> trigger the rebase action". Which is quite natural, but it is
> different from what you wrote above, isn't it?
>
> When people say "the default for `--rebase` is false", they mean
> this:
>
> I can say "git pull --rebase=<kind>" to specify how my current
> branch is rebased on top of the upstream. But if I do not
> specify the <kind>, i.e. "git pull --rebase", the command will
> act as if I gave 'false' as the <kind>.
>
> At least, I would think that is what the word "default" means.
>
> And I would be surprised if the "default" in that sense is 'false';
> isn't the default <kind> 'true' --- meaning "perform the plain
> vanilla 'git rebase'", unless you explicitly asked for 'merges',
> 'interactive' or 'false'?
>
> After the context of the hunk your patch touches, there is a
> description on `pull.rebase`. Down there, if you do not set
> `pull.rebase` or `branch.<name>.rebase` for the current branch at
> all, the system acts as if you had `false` for these variables. In
> other words, the default for these variables is `false`, meaning "do
> not rebase, just merge". But the default option argument for the
> `--rebase` option given without argument would not be `false`, I
> would think.
I think there are two reasonable interpretations of the word "default"
here.
(1) "git pull": This is equivalent to "git pull --rebase=false". So one
could say that "git pull" defaults to "--rebase=false".
(2) "git pull --rebase": This is equivalent to "git pull --rebase=true".
So one could say that the "--rebase" flag (when it is used as "--rebase"
without the "=..." part) defaults to "--rebase=true".
I assume Dragan was thinking of only the meaning in (1). The alternate
meaning of (2) which you brought up makes sense too.
I think both flavors of "default" are reasonable interpretations because
most (if not all) Git users will start out with (1) in mind as they get
familiar with how "git pull" works without any flags (i.e., the
"default" git-pull behavior). Eventually they'll learn about (2) and
realize how the <kind> defaults to "true" with the "--rebase" flag.
Aside: interestingly, there appears to be a "--no-rebase" option that
means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about --rebase,
2007-11-28)):
--no-rebase
This is shorthand for --rebase=false.
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> Documentation/git-pull.txt | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
>> index 0e14f8b5b2..d28790388e 100644
>> --- a/Documentation/git-pull.txt
>> +++ b/Documentation/git-pull.txt
>> @@ -116,7 +116,8 @@ When set to `merges`, rebase using `git rebase --rebase-merges` so that
>> the local merge commits are included in the rebase (see
>> linkgit:git-rebase[1] for details).
>> +
>> -When false, merge the upstream branch into the current branch.
>> +When false, merge the upstream branch into the current branch. This is
>> +the default.
>> +
>> When `interactive`, enable the interactive mode of rebase.
>> +
For reference here is a more complete view of the docs for this flag:
-r::
--rebase[=false|true|merges|interactive]::
When true, rebase the current branch on top of the upstream
branch after fetching. If there is a remote-tracking branch
corresponding to the upstream branch and the upstream branch
was rebased since last fetched, the rebase uses that information
to avoid rebasing non-local changes.
+
When set to `merges`, rebase using `git rebase --rebase-merges` so that
the local merge commits are included in the rebase (see
linkgit:git-rebase[1] for details).
+
When false, merge the upstream branch into the current branch.
+
When `interactive`, enable the interactive mode of rebase.
+
See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in
linkgit:git-config[1] if you want to make `git pull` always use
`--rebase` instead of merging.
How about adding something like this instead as the very first paragraph
for this flag?
Supplying "--rebase" defaults to "--rebase=true". Running git-pull
without arguments implies "--rebase=false", unless relevant
configuration variables have been set otherwise.
This way, we can discuss what "true" and "false" mean first to get these
two flavors of "defaults" sorted out (perhaps also mentioning
--no-rebase while we're at it). Then we can discuss the other <kind>
values like "merges" and "interactive".
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Sergey Organov @ 2023-09-14 23:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqled8h01w.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>>> Sounds very straight-forward.
>>>
>>> Given that "--first-parent" in "git log --first-parent -p" already
>>> defeats "-m" and shows the diff against the first parent only,
>>> people may find it confusing if "git log -d" does not act as a
>>> shorthand for that.
>>
>> It doesn't, and I believe it's a good thing, as primary function of
>> --first-parent is to change history traversal rules, and if -d did that,
>> it would be extremely confusing.
>
> I am not sure about that.
>
>> Also, --first-parent is correctly documented as implying
>> --diff-merges=first-parent, not as defeating -m.
>
> Yes, exactly. That makes me even more convinced that the intuitive
> behaviour, when we say "we have this great short-hand option that
> lets your 'git log' to do the first-parent thing with patch output",
> is to do the first-parent traversal _and_ show first-parent patches.
>
> "-d" is documented as a short-hand for "--diff-merges=first-parent
> --patch" and not for "--first-parent --patch", so the behaviour may
> correctly match documentation, but that does not make the documented
> behaviour an intuitive one. And a behaviour that is not intuitive
> is confusing.
I think both behaviors make sense, provided they are correctly
documented. I just prefer the one that is more basic, yet allows to
achieve things that another one does not.
>
>> If we read resulting documentation with a fresh eye, -d is similar to
>> --cc, and -c, just producing yet another kind of output, so I think all
>> this fits together quite nicely and shouldn't cause confusion.
>
> Another thing is that showing first-parent patch for merges while
> letting the traversal also visit the second-parent chain is not as
> useful an option as it could be, even though it is not so bad as the
> original "-m -p" that also showed second-parent patch for merges as
> well.
I don't see why desire to look at diff-to-first-parent on "side"
branches is any different from desire to look at them on "primary"
branch, sorry, so I still don't want "-d" to affect traversal or other
commit filtering rules. We do have --first-parent as well as a few
others for that.
> People would have to say "log --first-parent -p" to get the
> first-parent traversal with first-parent patch output, and they
> would not behefit from having "-d".
Well, at least they can now say "log --first-parent -d" as well ;)
Honestly, the "log --first-parent -p" (without "-m") suddenly producing
diffs for merge commits is already unnatural, needs yet another
special-casing in documentation, and then, finally, this relatively new
behavior was introduced exactly because there were no "-d" at that time,
to save typing "-m". The latter is yet another example of why "-d" in
its current form is a good idea.
That said, if you feel like there is place for a short-cut for this
particular use-case, it'd be fine with me, say:
--fpd:
short-cut for "--first-parent -d"
would fit quite nicely into the picture, I think.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v2] completion: improve doc for complex aliases
From: Linus Arver @ 2023-09-14 22:50 UTC (permalink / raw)
To: Junio C Hamano, Philippe Blain via GitGitGadget
Cc: git, Steffen Prohaska, Eric Sunshine, Philippe Blain
In-Reply-To: <xmqqo7i6khxv.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> The completion code can be told to use a particular completion for
>> aliases that shell out by using ': git <cmd> ;' as the first command of
>> the alias. This only works if <cmd> and the semicolon are separated by a
>> space, since if the space is missing __git_aliased_command returns (for
>> example) 'checkout;' instead of just 'checkout', and then
>> __git_complete_command fails to find a completion for 'checkout;'.
>>
>> The examples have that space but it's not clear if it's just for
>> style or if it's mandatory. Explicitly mention it.
>>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>
> Thanks. I scanned the case statement in the loop in the function
> and thought "hmph, everybody says ': git <cmd> ;' but is 'git'
> really needed?"
>
> I had "git l3" alias that invokes "$HOM#/bin/git-l" command, like so:
>
> [alias]
> l3 = "!sh -c ': git log ; git l \"$@\"' -"
>
> but if I did 's/: git log/: log/' it still completes just fine.
Interesting! I searched for the 'git <cmd>' and got some hits in
"t9902-completion.sh" when running "git grep -nE 'git <cmd>'":
t/t9902-completion.sh:2432:test_expect_success "completion uses <cmd> completion for alias: !sh -c 'git <cmd> ...'" '
t/t9902-completion.sh:2441:test_expect_success 'completion uses <cmd> completion for alias: !f () { VAR=val git <cmd> ... }' '
t/t9902-completion.sh:2450:test_expect_success 'completion used <cmd> completion for alias: !f() { : git <cmd> ; ... }' '
When I did 's/: git log/: log/' in the test at line 2450, the test still
passed. Perhaps we should add this "git"-less version as another test
case?
> I wonder if this hack is worth adding, instead of (or in addition
> to) requiring the user to insert $IFS to please the "parser", we can
> honor the rather obvious wish of the user in a more direct way.
>
>
>
> contrib/completion/git-completion.bash | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git c/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
> index 19139ac121..e31d71955f 100644
> --- c/contrib/completion/git-completion.bash
> +++ w/contrib/completion/git-completion.bash
> @@ -1183,7 +1183,7 @@ __git_aliased_command ()
> :) : skip null command ;;
> \'*) : skip opening quote after sh -c ;;
> *)
> - cur="$word"
> + cur="${word%;}"
> break
> esac
> done
I think this is a good defensive technique. This obviously changes the
guidance that Phillipe gave in their patch (we no longer have to worry
about adding a space or not between "word" and ";", so there's no need
to mention this explicitly any more), but to me this seems like a better
experience for our users because it's one less thing they have to worry
about.
^ permalink raw reply
* Re: [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on
From: Josip Sokcevic @ 2023-09-14 22:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: jonathantanmy, git, git
In-Reply-To: <xmqqfs3jpbg2.fsf@gitster.g>
On Tue, Sep 12, 2023 at 10:07 AM Junio C Hamano <gitster@pobox.com> wrote:
> It seems to be entirely doable, even though the stench from the
> abstraction layer violation may be horrible.
>
> ie_match_stat(), which is called by match_stat_with_submodule(),
> does pay attention to CE_FSMONITOR_VALID bit, so none of the members
> of struct stat matters when the bit is set. But the bit is not set,
> all members that fill_stat_data() and ce_match_stat_basic() care
> about do matter. Other code that follows callers of check_removed()
> do care about at least .st_mode member, and I suspect that in the
> current code .st_mode is the only member that gets looked at.
>
> So after all, I think your original "fix" was correct, but it took
> us some time to figure out why it was, which means we would want to
> explain it in the log message for developers who would want to touch
> the same area in the future.
I finished testing this - after my original fix and after running all
tests, I can confirm that `st_mode` of `struct stat` is indeed not
consumed if CE_FSMONITOR_VALID is set. But, it's fragile and likely to
cause problems in the future. Your approach of constructing `struct
stat` based on `struct cache_entry` is the way to go.
I see you created a new set of patches in a separate thread, so I'll
start those tests and report back there.
Thanks!
--
Josip Sokcevic
^ permalink raw reply
* Re: [PATCH v2] completion: improve doc for complex aliases
From: Linus Arver @ 2023-09-14 22:33 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget, git
Cc: Steffen Prohaska, Eric Sunshine, Philippe Blain
In-Reply-To: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The completion code can be told to use a particular completion for
> aliases that shell out by using ': git <cmd> ;' as the first command of
> the alias. This only works if <cmd> and the semicolon are separated by a
> space, since if the space is missing __git_aliased_command returns (for
> example) 'checkout;' instead of just 'checkout', and then
> __git_complete_command fails to find a completion for 'checkout;'.
>
> The examples have that space but it's not clear if it's just for
> style or if it's mandatory. Explicitly mention it.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> completion: improve doc for complex aliases
>
> Changes since v1:
>
> * fixed the typo pointed out by Eric
> * added an explanation of why the space is mandatory, as suggested by
> Linus
>
Thanks for the investigation. The commit message reads much better now.
This LGTM, but I think Junio's review comments [1] are worth
considering. I'll respond there also.
[1] https://lore.kernel.org/git/xmqqo7i6khxv.fsf@gitster.g/#t
^ permalink raw reply
* Re: [PATCH 2/2] diff-merges: introduce '-d' option
From: Junio C Hamano @ 2023-09-14 22:17 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <87o7i7hler.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
>> Sounds very straight-forward.
>>
>> Given that "--first-parent" in "git log --first-parent -p" already
>> defeats "-m" and shows the diff against the first parent only,
>> people may find it confusing if "git log -d" does not act as a
>> shorthand for that.
>
> It doesn't, and I believe it's a good thing, as primary function of
> --first-parent is to change history traversal rules, and if -d did that,
> it would be extremely confusing.
I am not sure about that.
> Also, --first-parent is correctly documented as implying
> --diff-merges=first-parent, not as defeating -m.
Yes, exactly. That makes me even more convinced that the intuitive
behaviour, when we say "we have this great short-hand option that
lets your 'git log' to do the first-parent thing with patch output",
is to do the first-parent traversal _and_ show first-parent patches.
"-d" is documented as a short-hand for "--diff-merges=first-parent
--patch" and not for "--first-parent --patch", so the behaviour may
correctly match documentation, but that does not make the documented
behaviour an intuitive one. And a behaviour that is not intuitive
is confusing.
> If we read resulting documentation with a fresh eye, -d is similar to
> --cc, and -c, just producing yet another kind of output, so I think all
> this fits together quite nicely and shouldn't cause confusion.
Another thing is that showing first-parent patch for merges while
letting the traversal also visit the second-parent chain is not as
useful an option as it could be, even though it is not so bad as the
original "-m -p" that also showed second-parent patch for merges as
well. People would have to say "log --first-parent -p" to get the
first-parent traversal with first-parent patch output, and they
would not behefit from having "-d".
^ permalink raw reply
* Re: [PATCH] cache: add fake_lstat()
From: Junio C Hamano @ 2023-09-14 22:00 UTC (permalink / raw)
To: git; +Cc: Josip Sokcevic
In-Reply-To: <xmqqcyykig1l.fsf@gitster.g>
And your diff-lib change may look like this on top. This one
unfortunately is completely untested, as I do not use macOS.
Reviewing, testing, and improving it is highly appreciated.
----- >8 ---------- >8 ---------- >8 -----
Subject: diff-lib: fix check_removed() when fsmonitor is active
`git diff-index` may return incorrect deleted entries when fsmonitor
is used in a repository with git submodules. This can be observed on
Mac machines, but it can affect all other supported platforms too.
If fsmonitor is used, `stat *st` is left uninitialied if cache_entry
has CE_FSMONITOR_VALID bit set. But, there are three call sites
that rely on stat afterwards, which can result in incorrect results.
We can fill members of "struct stat" that matters well enough using
the information we have in "struct cache_entry" that fsmonitor told
us is up-to-date to solve this.
Helped-by: Josip Sokcevic <sokcevic@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff-lib.c | 9 ++++++++-
t/t7527-builtin-fsmonitor.sh | 5 +++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git c/diff-lib.c w/diff-lib.c
index d8aa777a73..7799747a97 100644
--- c/diff-lib.c
+++ w/diff-lib.c
@@ -38,8 +38,15 @@
*/
static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
{
+ int stat_err;
+
assert(is_fsmonitor_refreshed(istate));
- if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
+
+ if (!(ce->ce_flags & CE_FSMONITOR_VALID))
+ stat_err = lstat(ce->name, st);
+ else
+ stat_err = fake_lstat(ce, st);
+ if (stat_err < 0) {
if (!is_missing_file_error(errno))
return -1;
return 1;
diff --git c/t/t7527-builtin-fsmonitor.sh w/t/t7527-builtin-fsmonitor.sh
index 0c241d6c14..78503158fd 100755
--- c/t/t7527-builtin-fsmonitor.sh
+++ w/t/t7527-builtin-fsmonitor.sh
@@ -809,6 +809,11 @@ my_match_and_clean () {
status --porcelain=v2 >actual.without &&
test_cmp actual.with actual.without &&
+ git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&
+ git -C super --no-optional-locks -c core.fsmonitor=false \
+ diff-index --name-status HEAD >actual.without &&
+ test_cmp actual.with actual.without &&
+
git -C super/dir_1/dir_2/sub reset --hard &&
git -C super/dir_1/dir_2/sub clean -d -f
}
^ permalink raw reply related
* Re: skip-worktree autostash on pull
From: Junio C Hamano @ 2023-09-14 21:53 UTC (permalink / raw)
To: brian m. carlson; +Cc: Blake Campbell, git
In-Reply-To: <ZQN9Azt/K28WLfqH@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Your particular case is one of many reasons we suggest this approach.
> There is in fact an --autostash argument in git pull, as well as git
> rebase, both of which work as you might expect, but in general they
> still won't work properly with --skip-worktree and given the FAQ entry
> above, we wouldn't add support for that in the option.
As you mentioned, "--autostash" will move away the local change
while your otherwise clean working tree needs to be updated from the
upstream via "git pull" (or "git pull --rebase"). So the only thing
the user may want to be careful would be "git commit -a" (and "git
add -u"), I would think. A pre-commit hook should be able to stop
you from committing with local changes to these selected paths you
want to keep following the upstream _with_ local changes (i.e. run
"diff --cached --name-only HEAD" and exit with non-zero when one of
these paths appear in its output). Upon seeing such a failure, you
can "git stash" the changes, then "git commit" again. And you do
not need to abuse assume-unchanged or skip-worktree for doing that.
^ permalink raw reply
* [PATCH] cache: add fake_lstat()
From: Junio C Hamano @ 2023-09-14 21:46 UTC (permalink / raw)
To: git; +Cc: Josip Sokcevic
At times, we may already know that a path represented by a
cache_entry ce has no changes via some out-of-line means, like
fsmonitor, and yet need the control to go through a codepath that
requires us to have "struct stat" obtained by lstat() on the path,
for various purposes (e.g. "ie_match_stat()" wants cached stat-info
is still current wrt "struct stat", "diff" wants to know st_mode).
The callers of lstat() on a tracked file, when its cache_entry knows
it is up-to-date, can instead call this helper to pretend that it
called lstat() by faking the "struct stat" information.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Just setting the .st_mode member in check_removed() relies on
the assumption that other members of "struct stat" are never
looked at a bit too much. We could use something like this to
bypass calling lstat() and instead populate from the data we have
in cache_entry.
builtin/ls-files.c | 5 ++++-
read-cache-ll.h | 8 ++++++++
read-cache.c | 27 +++++++++++++++++++++++++++
statinfo.c | 27 +++++++++++++++++++++++++++
statinfo.h | 8 ++++++++
5 files changed, 74 insertions(+), 1 deletion(-)
diff --git c/builtin/ls-files.c w/builtin/ls-files.c
index a0229c3277..7031ffcb0a 100644
--- c/builtin/ls-files.c
+++ w/builtin/ls-files.c
@@ -450,7 +450,10 @@ static void show_files(struct repository *repo, struct dir_struct *dir)
continue;
if (ce_skip_worktree(ce))
continue;
- stat_err = lstat(fullname.buf, &st);
+ if (!(ce->ce_flags & CE_FSMONITOR_VALID))
+ stat_err = lstat(fullname.buf, &st);
+ else
+ stat_err = fake_lstat(ce, &st);
if (stat_err && (errno != ENOENT && errno != ENOTDIR))
error_errno("cannot lstat '%s'", fullname.buf);
if (stat_err && show_deleted) {
diff --git c/read-cache-ll.h w/read-cache-ll.h
index 9a1a7edc5a..2a50a784f0 100644
--- c/read-cache-ll.h
+++ w/read-cache-ll.h
@@ -436,6 +436,14 @@ int match_stat_data_racy(const struct index_state *istate,
void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st);
+/*
+ * Fill members of st by members of sd enough to convince match_stat()
+ * to consider that they match. It should be usable as a replacement
+ * for lstat() for a tracked path that is known to be up-to-date via
+ * some out-of-line means (like fsmonitor).
+ */
+int fake_lstat(const struct cache_entry *ce, struct stat *st);
+
#define REFRESH_REALLY (1 << 0) /* ignore_valid */
#define REFRESH_UNMERGED (1 << 1) /* allow unmerged */
#define REFRESH_QUIET (1 << 2) /* be quiet about it */
diff --git c/read-cache.c w/read-cache.c
index 080bd39713..eb750e2e49 100644
--- c/read-cache.c
+++ w/read-cache.c
@@ -197,6 +197,33 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st
}
}
+static unsigned int st_mode_from_ce(const struct cache_entry *ce)
+{
+ extern int trust_executable_bit, has_symlinks;
+
+ switch (ce->ce_mode & S_IFMT) {
+ case S_IFLNK:
+ return has_symlinks ? S_IFLNK : (S_IFREG | 0644);
+ case S_IFREG:
+ return (ce->ce_mode & (trust_executable_bit ? 0755 : 0644)) | S_IFREG;
+ case S_IFGITLINK:
+ return S_IFDIR | 0755;
+ case S_IFDIR:
+ return ce->ce_mode;
+ default:
+ BUG("unsupported ce_mode: %o", ce->ce_mode);
+ }
+}
+
+int fake_lstat(const struct cache_entry *ce, struct stat *st)
+{
+ fake_lstat_data(&ce->ce_stat_data, st);
+ st->st_mode = st_mode_from_ce(ce);
+
+ /* always succeed as lstat() replacement */
+ return 0;
+}
+
static int ce_compare_data(struct index_state *istate,
const struct cache_entry *ce,
struct stat *st)
diff --git c/statinfo.c w/statinfo.c
index 17bb8966c3..45156109de 100644
--- c/statinfo.c
+++ w/statinfo.c
@@ -15,6 +15,33 @@ void fill_stat_data(struct stat_data *sd, struct stat *st)
sd->sd_size = st->st_size;
}
+static void set_times(struct stat *st, const struct stat_data *sd)
+{
+ st->st_ctime = sd->sd_ctime.sec;
+ st->st_mtime = sd->sd_mtime.sec;
+#ifdef NO_NSEC
+ ; /* nothing */
+#else
+#ifdef USE_ST_TIMESPEC
+ st->st_ctimespec.tv_nsec = sd->sd_ctime.nsec;
+ st->st_mtimespec.tv_nsec = sd->sd_mtime.nsec;
+#else
+ st->st_ctim.tv_nsec = sd->sd_ctime.nsec;
+ st->st_mtim.tv_nsec = sd->sd_mtime.nsec;
+#endif
+#endif
+}
+
+void fake_lstat_data(const struct stat_data *sd, struct stat *st)
+{
+ set_times(st, sd);
+ st->st_dev = sd->sd_dev;
+ st->st_ino = sd->sd_ino;
+ st->st_uid = sd->sd_uid;
+ st->st_gid = sd->sd_gid;
+ st->st_size = sd->sd_size;
+}
+
int match_stat_data(const struct stat_data *sd, struct stat *st)
{
int changed = 0;
diff --git c/statinfo.h w/statinfo.h
index 700f502ac0..5b21a30f90 100644
--- c/statinfo.h
+++ w/statinfo.h
@@ -46,6 +46,14 @@ struct stat_validity {
*/
void fill_stat_data(struct stat_data *sd, struct stat *st);
+/*
+ * The inverse of the above. When we know the cache_entry that
+ * contains sd is up-to-date, but still need to pretend we called
+ * lstat() to learn that fact, this function fills "st" enough to
+ * fool ie_match_stat().
+ */
+void fake_lstat_data(const struct stat_data *sd, struct stat *st);
+
/*
* Return 0 if st is consistent with a file not having been changed
* since sd was filled. If there are differences, return a
^ permalink raw reply related
* Re: skip-worktree autostash on pull
From: brian m. carlson @ 2023-09-14 21:37 UTC (permalink / raw)
To: Blake Campbell; +Cc: git
In-Reply-To: <AB6A85F5-3E76-4462-931E-AD76E0066C37@mpbell.io>
[-- Attachment #1: Type: text/plain, Size: 2055 bytes --]
On 2023-09-14 at 09:54:30, Blake Campbell wrote:
> Hi all - I use update-index --skip-worktree on some config files that
> I change locally and don’t want to commit, but every time I pull from
> the remote I have to go through a process of no-skip-worktree, stash,
> pull, stash pop, then skip-worktree again, which is all a bit tedious!
> Ideally some switch like --autostash for git pull would be really
> useful. Does anyone know if something like that exists?
The Git FAQ[0] outlines that, as you've noticed, skip-worktree doesn't
work for ignoring changes to tracked files:
Git doesn’t provide a way to do this. The reason is that if Git needs
to overwrite this file, such as during a checkout, it doesn’t know
whether the changes to the file are precious and should be kept, or
whether they are irrelevant and can safely be destroyed. Therefore, it
has to take the safe route and always preserve them.
It’s tempting to try to use certain features of git update-index,
namely the assume-unchanged and skip-worktree bits, but these don’t
work properly for this purpose and shouldn’t be used this way.
If you take the advice in the FAQ, then the config files won't be
tracked and you won't have this problem:
If your goal is to modify a configuration file, it can often be
helpful to have a file checked into the repository which is a template
or set of defaults which can then be copied alongside and modified as
appropriate. This second, modified file is usually ignored to prevent
accidentally committing it.
Your particular case is one of many reasons we suggest this approach.
There is in fact an --autostash argument in git pull, as well as git
rebase, both of which work as you might expect, but in general they
still won't work properly with --skip-worktree and given the FAQ entry
above, we wouldn't add support for that in the option.
[0] https://git-scm.com/docs/gitfaq#ignore-tracked-files
--
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 v3 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-14 20:25 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Denton Liu, Jeff King, Junio C Hamano
In-Reply-To: <dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de>
On Thu, Sep 14, 2023, at 10:29, Johannes Schindelin wrote:
>> [...]
>
> Right, `-G --notes` is a good argument to rethink this.
>
> A much more surgical way to address the issue at hand might be this
> (Kristoffer, what do you think? It's missing documentation for the
> newly-introduced `--show-notes-by-default` option, but you get the idea):
Looks good to me. It seems like an explicit argument is the only way to
make this work.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [RFC][PATCH 0/32] SHA256 and SHA1 interoperability
From: Eric W. Biederman @ 2023-09-14 19:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, brian m. carlson
In-Reply-To: <xmqqil8fqs6o.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
>> I am not worried about what it will take time to get the changes I
>> posted into the integration. I had only envisioned them as good enough
>> to get the technical ideas across, and had never envisioned them as
>> being accepted as is.
>
> Ah, no worries. By "integration" I did not mean "patches considered
> perfect, they are accepted, and are now part of the Git codebase".
>
> All that happens when the patches become part of the 'master'
> branch, but before that, patches that prove testable and worthy of
> getting tested will be merged to the 'next' branch and spend about a
> week there. What I meant to refer to is a step _before_ that, i.e.
> before the patches probe to be testable. New patches first appear
> on the 'seen' branch that merges "everything else" to see the
> interaction with all the topics "in flight" (i.e. not yet in
> 'master'). The 'seen' branch is reassembled from the latest
> iteration of the patches twice of thrice per day, and some patches
> are merged to 'next' and down to 'master', these "merging to prepare
> 'master', 'next' and 'seen' branches for publishing" was what I
> meant by "integration". In short, being queued on 'seen' does not
> mean all that much. It gives project participants an easy access to
> view how topics look in the larger picture, potentially interacting
> with other topics in flight, but the patches in there can be
> replaced wholesale or even dropped if they do not turn out to be
> desirable.
>
> I resolved textual conflicts and also compiler detectable semantic
> conflicts (e.g. some in-flight topics may have added callsites to a
> function your topic changes the function sigunature, or vice versa)
> to the point that the result compiles while merging this topic to
> 'seen', but tests are broken the big time, it seems, even though the
> topic by itself seems to pass the tests standalone.
That the tests are broken is very unfortunate.
I took at look at What's cooking in git.git and I did not see my topic
mentioned. So I presume I would have to perform the test merge myself
to have a sense of what the conflicts were.
Is there a time when in flight topics is low? I had a hunch that basing
my work on a brand new release would achieve that but I saw a lot of
topics in your "What's cooking" email.
I am just trying to figure out a good plan to deal with conflicts,
because the bugs need to be hunted down.
Eric
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: René Scharfe @ 2023-09-14 19:34 UTC (permalink / raw)
To: Oleg Nesterov, Junio C Hamano; +Cc: git, Alexey Gladkov
In-Reply-To: <20230913094638.GA535@redhat.com>
Am 13.09.23 um 11:46 schrieb Oleg Nesterov:
> I think I should trim CC to not spam the people who are not
> interested in this discussion...
>
> On 09/12, Junio C Hamano wrote:
>>
>> Documentation may not match the behaviour, but do we know what the
>> behaviour we want is? To me, the last example that shows the same
>> line twice (one as a real hit, the other because of "-p") looks a
>> bit counter-intuitive for the purpose of "help me locate where the
>> grep hits are". I dunno.
>
> I have another opinion. To me the 2nd "=..." marker does help to
> understand the hit location. But this doesn't matter.
You see it as another layer of information, as an annotation, an
additional line containing meta-information. I saw them as context
lines, i.e. lines from the original file shown in the original order
without duplication, like - lines, with the only place for meta-
information being the marker character itself.
> Let me repeat: scripts.
>
> I tried to explain this in 0/1 and in my other replies, but lets
> start from the very beginning once again.
>
> I've never used git-grep with -p/-n and most probably never will.
> But 3 days ago my text editor (vi clone) started to use "grep -pn".
>
> $ cat -n TEST.c
>
> 1 void func1(struct pid *);
> 2
> 3 void func2(struct pid *pid)
> 4 {
> 5 use1(pid);
> 6 }
> 7
> 8 void func3(struct pid *pid)
> 9 {
> 10 use2(pid);
> 11 }
>
>
> when I do
>
> :git-grep --untracked -pn pid TEST.c
>
> in my editor it calls the script which parses the output from git-grep
> and puts this
>
> <pre>
> <a href="TEST.c?1">TEST.c </a> 1 <b> </b> void func1(struct <i>pid</i> *);
> <a href="TEST.c?3">TEST.c </a> 3 <b> </b> void func2(struct <i>pid</i> *<i>pid</i>)
> <a href="TEST.c?5">TEST.c </a> 5 <b>func2 </b> use1(<i>pid</i>);
> <a href="TEST.c?8">TEST.c </a> 8 <b> </b> void func3(struct <i>pid</i> *<i>pid</i>)
> <a href="TEST.c?10">TEST.c </a> 10 <b>func3 </b> use2(<i>pid</i>);
> </pre>
>
> html to the text buffer which is nicely displayed as
>
> TEST.c 1 void func1(struct pid *);
> TEST.c 3 void func2(struct pid *pid)
> TEST.c 5 func2 use1(pid);
> TEST.c 8 void func3(struct pid *pid)
> TEST.c 10 func3 use2(pid);
>
> and I can use Ctrl-] to jump to the file/function/location.
>
> And this script is very simple, it parses the output line-by-line. When
> it sees the "=" marker it does some minimal post-processing, records the
> function name to display it in the 3rd column later, and goes to the next
> line.
>
> But without my patch, in this case I get
>
> TEST.c 1 void func1(struct pid *);
> TEST.c 3 void func2(struct pid *pid)
> TEST.c 5 use1(pid);
> TEST.c 8 void func3(struct pid *pid)
> TEST.c 10 use2(pid);
>
> because the output from git-grep
>
> $ git grep --untracked -pn pid TEST.c
> TEST.c:1:void func1(struct pid *);
> TEST.c:3:void func2(struct pid *pid)
> TEST.c:5: use1(pid);
> TEST.c:8:void func3(struct pid *pid)
> TEST.c:10: use2(pid);
>
> doesn't have the "=..." markers at all.
Sure, that's a problem. You could easily check whether a match is also
a function line according to the default heuristic (does the line start
with a letter, dollar sign or underscore?), e.g. with a glob like
"*:*:[a-zA-Z$_]*". If git grep uses more sophisticated rules then it
becomes impractical -- there are some impressive regexes in userdiff.c
and the script would have to figure out which language the file is
configured to be for Git in the first place.
> But TEST.c is just the trivial/artificial example. From 0/1,
>
> When I do
>
> :git-grep -pw pid kernel/sys.c
>
> in my editor without this patch, I get
>
> kernel/sys.c 224 sys_setpriority struct pid *pgrp;
> kernel/sys.c 294 sys_getpriority struct pid *pgrp;
> kernel/sys.c 952 * Note, despite the name, this returns the tgid not the pid. The tgid and
> kernel/sys.c 953 * the pid are identical unless CLONE_THREAD was specified on clone() in
> kernel/sys.c 963 /* Thread ID - the internal kernel "pid" */
> kernel/sys.c 977 sys_getppid int pid;
> kernel/sys.c 980 sys_getppid pid = task_tgid_vnr(rcu_dereference(current->real_parent));
> kernel/sys.c 983 sys_getppid return pid;
> kernel/sys.c 1073 SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
> kernel/sys.c 1077 sys_times struct pid *pgrp;
> kernel/sys.c 1080 sys_times if (!pid)
> kernel/sys.c 1081 sys_times pid = task_pid_vnr(group_leader);
> kernel/sys.c 1083 sys_times pgid = pid;
> kernel/sys.c 1094 sys_times p = find_task_by_vpid(pid);
> kernel/sys.c 1120 sys_times if (pgid != pid) {
> kernel/sys.c 1144 static int do_getpgid(pid_t pid)
> kernel/sys.c 1147 sys_times struct pid *grp;
> kernel/sys.c 1151 sys_times if (!pid)
> kernel/sys.c 1155 sys_times p = find_task_by_vpid(pid);
> kernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)
> kernel/sys.c 1174 sys_times return do_getpgid(pid);
> kernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)
> kernel/sys.c 1189 sys_getpgrp struct pid *sid;
> kernel/sys.c 1193 sys_getpgrp if (!pid)
> kernel/sys.c 1197 sys_getpgrp p = find_task_by_vpid(pid);
> kernel/sys.c 1214 static void set_special_pids(struct pid *pid)
> kernel/sys.c 1218 sys_getpgrp if (task_session(curr) != pid)
> kernel/sys.c 1219 sys_getpgrp change_pid(curr, PIDTYPE_SID, pid);
> kernel/sys.c 1221 sys_getpgrp if (task_pgrp(curr) != pid)
> kernel/sys.c 1222 sys_getpgrp change_pid(curr, PIDTYPE_PGID, pid);
> kernel/sys.c 1228 ksys_setsid struct pid *sid = task_pid(group_leader);
> kernel/sys.c 1684 SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,
> kernel/sys.c 1705 check_prlimit_permission tsk = pid ? find_task_by_vpid(pid) : current;
>
> And only the first 5 funcnames are correct.
Well, your script turns "SYSCALL_DEFINE3(setpriority, [...]" into
"sys_setpriority" etc., so it is already knows a lot about function lines.
It could be made to take a second look at match lines, I guess.
But a general solution would require git grep to somehow report both aspects
of matching function lines. That's easy if we allow ourselves to duplicate
lines. This is strange enough to warrant making it a new output format I
think.
Another possibility is to switch the precedence of : and =. With match
coloring it would still be possible to identify most positive matches in
function interactively, but not negative matches (-v) in function lines.
Probably not the best choice, since grep is primarily about finding
matching lines; function line info comes second.
Can we use two markers, i.e. both : and =? No idea what that might break.
There is a Unicode symbol named colon equals, which looks like this: ≔
We added the =, so I guess we could add that thing as well. But is the
world prepared for Unicode output? Not sure. If we need to stay in the
ASCII table the same idea could be implemented with a different character
like # or ;.
> And note that this case is very simple too (I mostly use :git-grep to scan
> the whole linux kernel tree), but even in this simple case I don't think it
> makes sense to use "git-grep -pn" directly, the output is hardly readable
> (at least to me) with or without my patch.
So with the patch below this would look like this:
kernel/sys.c=218=SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
kernel/sys.c:224: struct pid *pgrp;
kernel/sys.c=288=SYSCALL_DEFINE2(getpriority, int, which, int, who)
kernel/sys.c:294: struct pid *pgrp;
kernel/sys.c=943=SYSCALL_DEFINE1(setfsgid, gid_t, gid)
kernel/sys.c:952: * Note, despite the name, this returns the tgid not the pid. The tgid and
kernel/sys.c:953: * the pid are identical unless CLONE_THREAD was specified on clone() in
kernel/sys.c=958=SYSCALL_DEFINE0(getpid)
kernel/sys.c:963:/* Thread ID - the internal kernel "pid" */
kernel/sys.c=975=SYSCALL_DEFINE0(getppid)
kernel/sys.c:977: int pid;
kernel/sys.c:980: pid = task_tgid_vnr(rcu_dereference(current->real_parent));
kernel/sys.c:983: return pid;
kernel/sys.c#1073#SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
kernel/sys.c:1077: struct pid *pgrp;
kernel/sys.c:1080: if (!pid)
kernel/sys.c:1081: pid = task_pid_vnr(group_leader);
kernel/sys.c:1083: pgid = pid;
kernel/sys.c:1094: p = find_task_by_vpid(pid);
kernel/sys.c:1120: if (pgid != pid) {
kernel/sys.c#1144#static int do_getpgid(pid_t pid)
kernel/sys.c:1147: struct pid *grp;
kernel/sys.c:1151: if (!pid)
kernel/sys.c:1155: p = find_task_by_vpid(pid);
kernel/sys.c#1172#SYSCALL_DEFINE1(getpgid, pid_t, pid)
kernel/sys.c:1174: return do_getpgid(pid);
kernel/sys.c#1186#SYSCALL_DEFINE1(getsid, pid_t, pid)
kernel/sys.c:1189: struct pid *sid;
kernel/sys.c:1193: if (!pid)
kernel/sys.c:1197: p = find_task_by_vpid(pid);
kernel/sys.c#1214#static void set_special_pids(struct pid *pid)
kernel/sys.c:1218: if (task_session(curr) != pid)
kernel/sys.c:1219: change_pid(curr, PIDTYPE_SID, pid);
kernel/sys.c:1221: if (task_pgrp(curr) != pid)
kernel/sys.c:1222: change_pid(curr, PIDTYPE_PGID, pid);
kernel/sys.c=1225=int ksys_setsid(void)
kernel/sys.c:1228: struct pid *sid = task_pid(group_leader);
kernel/sys.c#1684#SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,
kernel/sys.c:1705: tsk = pid ? find_task_by_vpid(pid) : current;
It uses # for matches that happen to be function lines, and doesn't show
a previous function line for those anymore. Usable?
René
diff --git a/grep.c b/grep.c
index fc2d0c837a..a08da5cdcb 100644
--- a/grep.c
+++ b/grep.c
@@ -1681,6 +1681,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
goto next_line;
}
if (hit && (opt->max_count < 0 || count < opt->max_count)) {
+ char sign = ':';
count++;
if (opt->status_only)
return 1;
@@ -1697,12 +1698,14 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
opt->output(opt, " matches\n", 9);
return 1;
}
+ if (opt->funcname && match_funcname(opt, gs, bol, eol))
+ sign = '#';
/* Hit at this line. If we haven't shown the
* pre-context lines, we would need to show them.
*/
if (opt->pre_context || opt->funcbody)
show_pre_context(opt, gs, bol, eol, lno);
- else if (opt->funcname)
+ else if (opt->funcname && sign == ':')
show_funcname_line(opt, gs, bol, lno);
cno = opt->invert ? icol : col;
if (cno < 0) {
@@ -1715,7 +1718,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
*/
cno = 0;
}
- show_line(opt, bol, eol, gs->name, lno, cno + 1, ':');
+ show_line(opt, bol, eol, gs->name, lno, cno + 1, sign);
last_hit = lno;
if (opt->funcbody)
show_function = 1;
^ permalink raw reply related
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: René Scharfe @ 2023-09-14 19:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Oleg Nesterov, Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, Taylor Blau, git, Alexey Gladkov
In-Reply-To: <xmqq1qf2lxrm.fsf@gitster.g>
Am 13.09.23 um 02:31 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>>> To me, this behaviour looks as
>>>>
>>>> Show the preceding line that contains the function name of
>>>> the match, unless the _PREVIOUS_ matching line is a function
>>>> name itself.
>>
>> To me it looks like:
>>
>> Show the preceding line that contains the function name of
>> the match.
>>
>> ("Show" meaning "show once", not "show for each match again and again".)
>>
>> Or:
>>
>> Show the preceding line that contains the function name of
>> the match, unless it is already shown for a different
>> reason, e.g. as a match or as the function line of a
>> previous match.
>
> Wow, that was a mouthful, but matches my understanding. I naïvely
> thought "when showing a hit, we may add the line that matches the
> function header pattern before the hit even that header line does
> not hit the grep pattern. But if the header line does hit the grep
> pattern, we do not bother show the same thing twice." was a
> reasonable goal to have.
I agree, and that's probably why I included the "unless the matching
line is a function name itself" part. Not sure why the code doesn't
agree. A test for that aspect would have been nice. *ahem*
René
^ permalink raw reply
* Re: [PATCH] doc: pull: improve rebase=false documentation
From: Junio C Hamano @ 2023-09-14 19:00 UTC (permalink / raw)
To: Dragan Simic; +Cc: git
In-Reply-To: <226cc3ed753ee809a77ac7bfe958add7a4363390.1694661788.git.dsimic@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> Mention in the documentation that --rebase defaults to false.
I am not quite sure if this is an improvement, though.
It is true that, if you do not have any of your own funny
configuration, your "git pull" will *not* rebase.
But that is "if you do not give the --rebase option, you do not
trigger the rebase action". Which is quite natural, but it is
different from what you wrote above, isn't it?
When people say "the default for `--rebase` is false", they mean
this:
I can say "git pull --rebase=<kind>" to specify how my current
branch is rebased on top of the upstream. But if I do not
specify the <kind>, i.e. "git pull --rebase", the command will
act as if I gave 'false' as the <kind>.
At least, I would think that is what the word "default" means.
And I would be surprised if the "default" in that sense is 'false';
isn't the default <kind> 'true' --- meaning "perform the plain
vanilla 'git rebase'", unless you explicitly asked for 'merges',
'interactive' or 'false'?
After the context of the hunk your patch touches, there is a
description on `pull.rebase`. Down there, if you do not set
`pull.rebase` or `branch.<name>.rebase` for the current branch at
all, the system acts as if you had `false` for these variables. In
other words, the default for these variables is `false`, meaning "do
not rebase, just merge". But the default option argument for the
`--rebase` option given without argument would not be `false`, I
would think.
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> Documentation/git-pull.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 0e14f8b5b2..d28790388e 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -116,7 +116,8 @@ When set to `merges`, rebase using `git rebase --rebase-merges` so that
> the local merge commits are included in the rebase (see
> linkgit:git-rebase[1] for details).
> +
> -When false, merge the upstream branch into the current branch.
> +When false, merge the upstream branch into the current branch. This is
> +the default.
> +
> When `interactive`, enable the interactive mode of rebase.
> +
^ permalink raw reply
* Re: [PATCH v3 0/3] "update-index --show-index-version"
From: Junio C Hamano @ 2023-09-14 18:15 UTC (permalink / raw)
To: Linus Arver; +Cc: git
In-Reply-To: <owlypm2ljmzi.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>
>> This iteration takes suggestions by Linus Arver; the tests added by
>> [2/3] have been clarified.
>>
>
> This version LGTM, thanks!
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Taylor Blau @ 2023-09-14 18:01 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, git, Jeff King, Patrick Steinhardt
In-Reply-To: <CAP8UFD1rDb-iYf4LYb7n=K4KpQ-JR-JK4TkQpGJ-TCfTNFFbnA@mail.gmail.com>
On Thu, Sep 14, 2023 at 01:10:38PM +0200, Christian Couder wrote:
> Ok, I will try to review and merge this with
> cc/repack-sift-filtered-objects-to-separate-pack soon.
I took a look at how much/little effort was going to be required, and
luckily the changes are isolated only to a single patch. It's just your
f1ffa71e8f (repack: add `--filter=<filter-spec>` option, 2023-09-11),
and in particular the `write_filtered_pack()` function.
I started messing around with it myself and generated the following
fixup! which can be applied on top of your version of f1ffa71e8f. It's
mostly straightforward, but there is a gotcha that the loop over
non-kept packs has to change to:
for_each_string_list_item(item, &existing->non_kept_packs)
/* ... */
for_each_string_list_item(item, &existing->cruft_packs)
/* ... */
, instead of just the first loop over non_kept_packs, since cruft packs
are stored in a separate list.
In any event, here's the fixup! I generated on top of that patch:
--- 8< ---
Subject: [PATCH] fixup! repack: add `--filter=<filter-spec>` option
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 120f4241c0..0d23323d05 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -834,15 +834,13 @@ static int finish_pack_objects_cmd(struct child_process *cmd,
static int write_filtered_pack(const struct pack_objects_args *args,
const char *destination,
const char *pack_prefix,
- struct string_list *keep_pack_list,
- struct string_list *names,
- struct string_list *existing_packs,
- struct string_list *existing_kept_packs)
+ struct existing_packs *existing,
+ struct string_list *names)
{
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
FILE *in;
- int ret, i;
+ int ret;
const char *caret;
const char *scratch;
int local = skip_prefix(destination, packdir, &scratch);
@@ -853,9 +851,8 @@ static int write_filtered_pack(const struct pack_objects_args *args,
if (!pack_kept_objects)
strvec_push(&cmd.args, "--honor-pack-keep");
- for (i = 0; i < keep_pack_list->nr; i++)
- strvec_pushf(&cmd.args, "--keep-pack=%s",
- keep_pack_list->items[i].string);
+ for_each_string_list_item(item, &existing->kept_packs)
+ strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
cmd.in = -1;
@@ -872,10 +869,12 @@ static int write_filtered_pack(const struct pack_objects_args *args,
in = xfdopen(cmd.in, "w");
for_each_string_list_item(item, names)
fprintf(in, "^%s-%s.pack\n", pack_prefix, item->string);
- for_each_string_list_item(item, existing_packs)
+ for_each_string_list_item(item, &existing->non_kept_packs)
+ fprintf(in, "%s.pack\n", item->string);
+ for_each_string_list_item(item, &existing->cruft_packs)
fprintf(in, "%s.pack\n", item->string);
caret = pack_kept_objects ? "" : "^";
- for_each_string_list_item(item, existing_kept_packs)
+ for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s%s.pack\n", caret, item->string);
fclose(in);
@@ -1261,10 +1260,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
ret = write_filtered_pack(&po_args,
packtmp,
find_pack_prefix(packdir, packtmp),
- &keep_pack_list,
- &names,
- &existing_nonkept_packs,
- &existing_kept_packs);
+ &existing,
+ &names);
if (ret)
goto cleanup;
}
--
2.42.0.137.g6fe1dff026
--- >8 ---
Thanks,
Taylor
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox