{"thread":{"id":"52730","subject":"[PATCH] doc: add documentation for git log --no-follow","startedAt":"2020-01-31T23:32:11Z","lastAt":"2020-02-03T20:27:52Z","messageCount":5,"participants":["Étienne Servais","Jeff King","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"390976","messageId":"85e71c97-9e0a-863e-179f-a6e1f14365ce@voucoux.fr","threadId":"52730","inReplyTo":null,"subject":"[PATCH] doc: add documentation for git log --no-follow","fromName":"Étienne Servais","fromEmail":"etienne.servais@voucoux.fr","sentAt":"2020-01-31T23:24:31Z","receivedAt":"2020-01-31T23:32:11Z","isPatch":true,"sender":{"key":"etienne.servais@voucoux.fr","avatar":null},"body":"This feature was added by commit\n076c98372e (log: add \"log.follow\" configuration variable, 2015-07-07)\nbut remained undocumented.\n\nSigned-off-by: Étienne Servais \n---\nThis is my first patch to git \\o/\nSent with thunderbird with help of format-patch'doc (So, it works!).\nI've tested the patch and git am works well on maint and next.\nI couldn't figure if I shall merge the --no-follow doc with the follow\nas is done for --no-decorate and --decorate just after. \nI couldn't think of a proper phrasing so I'll be glad to get suggestion on this.\n\n Documentation/git-log.txt | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-log.txt b/Documentation/git-log.txt\nindex bed09bb09e..cc2ad98167 100644\n--- a/Documentation/git-log.txt\n+++ b/Documentation/git-log.txt\n@@ -28,6 +28,9 @@ OPTIONS\n \tContinue listing the history of a file beyond renames\n \t(works only for a single file).\n \n+--no-follow::\n+\tOverride the log.follow configuration.\n+\n --no-decorate::\n --decorate[=short|full|auto|no]::\n \tPrint out the ref names of any commits that are shown. If 'short' is\n@@ -205,7 +208,8 @@ log.follow::\n \tIf `true`, `git log` will act as if the `--follow` option was used when\n \ta single is given. This has the same limitations as `--follow`,\n \ti.e. it cannot be used to follow multiple files and does not work well\n-\ton non-linear history.\n+\ton non-linear history. This setting can be disabled by the `--no-follow`\n+\toption.\n \n log.showRoot::\n \tIf `false`, `git log` and related commands will not treat the\n-- \n2.17.1\n\n"},{"id":"390987","messageId":"20200201111636.GC1864948@coredump.intra.peff.net","threadId":"52730","inReplyTo":"85e71c97-9e0a-863e-179f-a6e1f14365ce@voucoux.fr","subject":"Re: [PATCH] doc: add documentation for git log --no-follow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-01T11:16:36Z","receivedAt":"2020-02-01T11:16:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 01, 2020 at 12:24:31AM +0100, Étienne Servais wrote:\n\n> This feature was added by commit\n> 076c98372e (log: add \"log.follow\" configuration variable, 2015-07-07)\n> but remained undocumented.\n\nThat commit just touched the code; it was originally added by aebbcf5797\n(diff: accept --no-follow option, 2012-09-21). But I think the general\nintent of your patch is still valid.\n\n> Signed-off-by: Étienne Servais \n> ---\n> This is my first patch to git \\o/\n> Sent with thunderbird with help of format-patch'doc (So, it works!).\n\nYeah, everything looks good there (no whitespace damage, etc).\n\n> I couldn't figure if I shall merge the --no-follow doc with the follow\n> as is done for --no-decorate and --decorate just after.\n\nI think it would make more sense to just put it with --follow, like:\n\n [--no-]follow::\n\nwhich matches how --use-mailmap is defined, for instance.\n\n> @@ -205,7 +208,8 @@ log.follow::\n> \tIf `true`, `git log` will act as if the `--follow` option was used when\n> \ta single is given. This has the same limitations as `--follow`,\n> \ti.e. it cannot be used to follow multiple files and does not work well\n> -\ton non-linear history.\n> +\ton non-linear history. This setting can be disabled by the `--no-follow`\n> +\toption.\n\nI'm on the fence on this hunk. There are a number of config options that\ncan be canceled or overridden by command-line options. It's a normal\npattern for the \"--no\" variant to do so. So while it often doesn't hurt\nto give a pointer in the right direction, I don't know that we'd want to\nstart doing so in every such case.\n\n-Peff\n"},{"id":"391042","messageId":"20200203144055.GA4170731@coredump.intra.peff.net","threadId":"52730","inReplyTo":"85e71c97-9e0a-863e-179f-a6e1f14365ce@voucoux.fr","subject":"[PATCH] index-pack: downgrade twice-resolved REF_DELTA to die()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-03T14:40:55Z","receivedAt":"2020-02-03T14:40:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we're resolving a REF_DELTA, we compare-and-swap its type from\nREF_DELTA to whatever real type the base object has, as discussed in\nab791dd138 (index-pack: fix race condition with duplicate bases,\n2014-08-29). If the old type wasn't a REF_DELTA, we consider that a\nBUG(). But as discussed in that commit, we might see this case whenever\nwe try to resolve an object twice, which may happen because we have\nmultiple copies of the base object.\n\nSo this isn't a bug at all, but rather a sign that the input pack is\nbroken. And indeed, this case is triggered already in t5309.5 and\nt5309.6, which create packs with delta cycles and duplicate bases. But\nwe never noticed because those tests are marked expect_failure.\n\nThose tests were added by b2ef3d9ebb (test index-pack on packs with\nrecoverable delta cycles, 2013-08-23), which was leaving the door open\nfor cases that we theoretically _could_ handle. And when we see an\nalready-resolved object like this, in theory we could keep going after\nconfirming that the previously resolved child->real_type matches\nbase->obj->real_type. But:\n\n - enforcing the \"only resolve once\" rule here saves us from an\n infinite loop in other parts of the code. If we keep going, then the\n delta cycle in t5309.5 causes us to loop infinitely, as\n find_ref_delta_children() doesn't realize which objects have already\n been resolved. So there would be more changes needed to make this\n case work, and in the meantime we'd be worse off.\n\n - any pack that triggers this is broken anyway. It either has a\n duplicate base object, or it has a cycle which causes us to bring in\n a duplicate via --fix-thin. In either case, we'd end up rejecting\n the pack in write_idx_file(), which also detects duplicates.\n\nSo the tests have little value in documenting what we _could_ be doing\n(and have been neglected for 6+ years). Let's switch them to confirming\nthat we handle this case cleanly (and switch out the BUG() for a more\ninformative die() so that we do so).\n\nSigned-off-by: Jeff King \n---\nNoticed while running the tests in an environment that complains about\nSIGABRT deaths. Arguably the test suite should be looking for these even\nin test_expect_failure, but it would be a little convoluted to do so\n(e.g., teaching BUG() to write to a marker file, and then checking the\nfile). And I think we're better off phrasing things as much as possible\nas expect_success anyway.\n\n builtin/index-pack.c | 4 +++-\n t/t5309-pack-delta-cycles.sh | 8 ++++----\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 60a5591039..41a7c11c8e 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1003,7 +1003,9 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,\n \n \t\tif (!compare_and_swap_type(&child->real_type, OBJ_REF_DELTA,\n \t\t\t\t\t base->obj->real_type))\n-\t\t\tBUG(\"child->real_type != OBJ_REF_DELTA\");\n+\t\t\tdie(\"REF_DELTA at offset %\"PRIuMAX\" already resolved (duplicate base %s?)\",\n+\t\t\t (uintmax_t)child->idx.offset,\n+\t\t\t oid_to_hex(&base->obj->idx.oid));\n \n \t\tresolve_delta(child, base, result);\n \t\tif (base->ref_first == base->ref_last && base->ofs_last == -1)\ndiff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh\nindex 491556dad9..6c209ad45c 100755\n--- a/t/t5309-pack-delta-cycles.sh\n+++ b/t/t5309-pack-delta-cycles.sh\n@@ -62,13 +62,13 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '\n \ttest_must_fail git index-pack --fix-thin --stdin recoverable.pack &&\n \tpack_trailer recoverable.pack &&\n-\tgit index-pack --fix-thin --stdin When we're resolving a REF_DELTA, we compare-and-swap its type from\n> REF_DELTA to whatever real type the base object has, as discussed in\n> ab791dd138 (index-pack: fix race condition with duplicate bases,\n> 2014-08-29). If the old type wasn't a REF_DELTA, we consider that a\n> BUG(). But as discussed in that commit, we might see this case whenever\n> we try to resolve an object twice, which may happen because we have\n> multiple copies of the base object.\n>\n> So this isn't a bug at all, but rather a sign that the input pack is\n> broken. And indeed, this case is triggered already in t5309.5 and\n> t5309.6, which create packs with delta cycles and duplicate bases. But\n> we never noticed because those tests are marked expect_failure.\n>\n> Those tests were added by b2ef3d9ebb (test index-pack on packs with\n> recoverable delta cycles, 2013-08-23), which was leaving the door open\n> for cases that we theoretically _could_ handle. And when we see an\n> already-resolved object like this, in theory we could keep going after\n> confirming that the previously resolved child->real_type matches\n> base->obj->real_type. But:\n>\n> - enforcing the \"only resolve once\" rule here saves us from an\n> infinite loop in other parts of the code. If we keep going, then the\n> delta cycle in t5309.5 causes us to loop infinitely, as\n> find_ref_delta_children() doesn't realize which objects have already\n> been resolved. So there would be more changes needed to make this\n> case work, and in the meantime we'd be worse off.\n>\n> - any pack that triggers this is broken anyway. It either has a\n> duplicate base object, or it has a cycle which causes us to bring in\n> a duplicate via --fix-thin. In either case, we'd end up rejecting\n> the pack in write_idx_file(), which also detects duplicates.\n>\n> So the tests have little value in documenting what we _could_ be doing\n> (and have been neglected for 6+ years). Let's switch them to confirming\n> that we handle this case cleanly (and switch out the BUG() for a more\n> informative die() so that we do so).\n>\n> Signed-off-by: Jeff King \n> ---\n> Noticed while running the tests in an environment that complains about\n> SIGABRT deaths. Arguably the test suite should be looking for these even\n> in test_expect_failure, but it would be a little convoluted to do so\n> (e.g., teaching BUG() to write to a marker file, and then checking the\n> file). And I think we're better off phrasing things as much as possible\n> as expect_success anyway.\n>\n> builtin/index-pack.c | 4 +++-\n> t/t5309-pack-delta-cycles.sh | 8 ++++----\n> 2 files changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 60a5591039..41a7c11c8e 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1003,7 +1003,9 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,\n>\n> \t\tif (!compare_and_swap_type(&child->real_type, OBJ_REF_DELTA,\n> \t\t\t\t\t base->obj->real_type))\n> -\t\t\tBUG(\"child->real_type != OBJ_REF_DELTA\");\n> +\t\t\tdie(\"REF_DELTA at offset %\"PRIuMAX\" already resolved (duplicate base %s?)\",\n> +\t\t\t (uintmax_t)child->idx.offset,\n> +\t\t\t oid_to_hex(&base->obj->idx.oid));\n\nThis error message is informative, and matches what you described in the\npatch message.\n\n> \t\tresolve_delta(child, base, result);\n> \t\tif (base->ref_first == base->ref_last && base->ofs_last == -1)\n> diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh\n> index 491556dad9..6c209ad45c 100755\n> --- a/t/t5309-pack-delta-cycles.sh\n> +++ b/t/t5309-pack-delta-cycles.sh\n> @@ -62,13 +62,13 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '\n> \ttest_must_fail git index-pack --fix-thin --stdin '\n>\n> -test_expect_failure 'failover to an object in another pack' '\n> +test_expect_success 'failover to an object in another pack' '\n> \tclear_packs &&\n> \tgit index-pack --stdin -\tgit index-pack --stdin --fix-thin +\ttest_must_fail git index-pack --stdin --fix-thin '\n>\n> -test_expect_failure 'failover to a duplicate object in the same pack' '\n> +test_expect_success 'failover to a duplicate object in the same pack' '\n> \tclear_packs &&\n> \t{\n> \t\tpack_header 3 &&\n> @@ -77,7 +77,7 @@ test_expect_failure 'failover to a duplicate object in the same pack' '\n> \t\tpack_obj $A\n> \t} >recoverable.pack &&\n> \tpack_trailer recoverable.pack &&\n> -\tgit index-pack --fix-thin --stdin +\ttest_must_fail git index-pack --fix-thin --stdin '\n\nAnd these hunks all make sense, too.\n\n> test_done\n> --\n> 2.25.0.541.gdfd61ebb85\n\nThis looks great, thanks :-)\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"391052","messageId":"8d0dbaa2-938e-1797-98d7-3f9abeb5b863@voucoux.fr","threadId":"52730","inReplyTo":"20200201111636.GC1864948@coredump.intra.peff.net","subject":"Re: [PATCH] doc: add documentation for git log --no-follow","fromName":"Étienne Servais","fromEmail":"etienne.servais@voucoux.fr","sentAt":"2020-02-03T20:27:46Z","receivedAt":"2020-02-03T20:27:52Z","isPatch":true,"sender":{"key":"etienne.servais@voucoux.fr","avatar":null},"body":"Le 01/02/2020 à 12:16, Jeff King a écrit :\n> On Sat, Feb 01, 2020 at 12:24:31AM +0100, Étienne Servais wrote:\n> \n>> This feature was added by commit\n>> 076c98372e (log: add \"log.follow\" configuration variable, 2015-07-07)\n>> but remained undocumented.\n> \n> That commit just touched the code; it was originally added by aebbcf5797\n> (diff: accept --no-follow option, 2012-09-21). But I think the general\n> intent of your patch is still valid.\n\nGood catch. Corrected in upcoming patch v2\n\n>> I couldn't figure if I shall merge the --no-follow doc with the follow\n>> as is done for --no-decorate and --decorate just after.\n> \n> I think it would make more sense to just put it with --follow, like:\n> \n> [--no-]follow::\n> \n> which matches how --use-mailmap is defined, for instance.\n\nOk, I've turned it to (inspired by the --[no-]rename-empty doc)\n\n---follow::\n- Continue listing the history of a file beyond renames\n+--[no-]follow::\n+ Whether to continue listing the history of a file beyond renames\n (works only for a single file).\n\n\n> \n>> @@ -205,7 +208,8 @@ log.follow::\n>> \tIf `true`, `git log` will act as if the `--follow` option was used when\n>> \ta single is given. This has the same limitations as `--follow`,\n>> \ti.e. it cannot be used to follow multiple files and does not work well\n>> -\ton non-linear history.\n>> +\ton non-linear history. This setting can be disabled by the `--no-follow`\n>> +\toption.\n> \n> I'm on the fence on this hunk. There are a number of config options that\n> can be canceled or overridden by command-line options. It's a normal\n> pattern for the \"--no\" variant to do so. So while it often doesn't hurt\n> to give a pointer in the right direction, I don't know that we'd want to\n> start doing so in every such case.\n\nOK. I had just borrowed that sentence from notes.displayRef few lines below.\nSome config options doc follow this direction like log.abbrevCommit or log.mailmap.\n\nI'll follow you on this.\n\n\n-- \nÉtienne Servais\n"}]}