git/git-scm.com GH Issue Tracker

git/git-scm.com GH Issue Tracker

[Samuel Lijin]

I've went through a bunch of open issues on the git/git-scm.com repo
(specifically, everything after #600) and I think the bulk of them can
be closed.

I've taken the liberty of classifying them as shown below.

- Sam


# Irrelevant but someone should take a look

693


# Irrelevant to git-scm.com and should be closed
# Each of these had had someone comment on it saying as much

939 912 906 905 901 896 894 886 885 884 883 879 877 871 868 865 861 840
837 834 828 813 811 807 796 795 790 774 751 748 745 744 729 727 721 719
711 690 686 686 674 673 671 667 660 653 635 631 621 615 613 612 611 610
609 608


# Resolved, duplicate, or non-issue

936 875 863 858 856 839 786 785 761 760 754 752 736 723 720 704 684 683
675 663 662 661 657 651 649 640 637 634 633 628 623 616 614 605 602 601


# Relevant and should be kept open

929 890 859 855 854 826 812 808 804 787 777 768 747 715 703 701 695 694
678 668 665 649 646 639 620 617

Re: git/git-scm.com GH Issue Tracker

[Thomas Ferris Nicolaisen]

Adding Peff to cc as he is the current maintainer of the git-scm.com site/repo.

On Mon, Feb 6, 2017 at 7:15 AM, Samuel Lijin <sxlijin@gmail.com> wrote:
>
> I've taken the liberty of classifying them as shown below.
>

As a community member who cares a lot about that site, thank you! I
would love to contribute by reviewing your reviews, but personally
can't find the time (focusing free time on podcast production from
Git-Merge instead ;)).

Re: git/git-scm.com GH Issue Tracker

[Duy Nguyen]

On Mon, Feb 6, 2017 at 1:15 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> # Irrelevant but someone should take a look
>
> 693

To save people some time (and since i looked at it anyway), this is
about whether "warning in tree xxx: contains zero-padded file modes:
from fsck should be a warning or error. It is a warning now even
though "git -c transfer.fsckobjects=true clone" treats it as an error.
There are some discussions in the past [1] [2] about this.

There's also a question "And I failed to find in the documentation if
transfer.fsckobjects could be disabled per repository, can you confirm
it's not possible for now ?"

(sorry no answer from me)

[1] http://public-inbox.org/git/%3CCAEBDL5W3DL0v=TusuB7Vg-4bWdAJh5d2Psc1N0Qe+KK3bZH3=Q@mail.gmail.com%3E/
[2] http://public-inbox.org/git/%3C20100326215600.GA10910@spearce.org%3E/
-- 
Duy

Re: git/git-scm.com GH Issue Tracker

[Jeff King]

On Mon, Feb 06, 2017 at 12:15:08AM -0600, Samuel Lijin wrote:

> I've went through a bunch of open issues on the git/git-scm.com repo
> (specifically, everything after #600) and I think the bulk of them can
> be closed.
> 
> I've taken the liberty of classifying them as shown below.

Thanks, this is incredibly helpful. I'll close the appropriate ones you
identified.

-Peff

Re: git/git-scm.com GH Issue Tracker

[Jeff King]

On Mon, Feb 06, 2017 at 05:18:03PM +0700, Duy Nguyen wrote:

> On Mon, Feb 6, 2017 at 1:15 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> > # Irrelevant but someone should take a look
> >
> > 693
> 
> To save people some time (and since i looked at it anyway), this is
> about whether "warning in tree xxx: contains zero-padded file modes:
> from fsck should be a warning or error. It is a warning now even
> though "git -c transfer.fsckobjects=true clone" treats it as an error.
> There are some discussions in the past [1] [2] about this.

The bug that caused the trees is long-fixed. There's a question of
how severity levels should be handled in transfer.fsckObjects. By
default it treats everything as a reason to reject the object. Dscho
added configurable levels a few versions ago. It may be a good idea to
tweak the defaults to something more permissive[1].

> There's also a question "And I failed to find in the documentation if
> transfer.fsckobjects could be disabled per repository, can you confirm
> it's not possible for now ?"

I don't know why it wouldn't be, though note that it won't override
the operation-specific {receive,fetch}.fsckObjects.

-Peff

[1] If we had a more permissive set of defaults, it would probably make
    sense to turn on fsckObjects by default. Some of the checks are
    security-relevant, like disallowing trees with ".GIT",
    "../../etc/passwd", etc. Those _should_ be handled sanely by the
    rest of Git, but it serves as a belt-and-suspenders check, and also
    protects anybody with a buggy Git downstream from you.

    GitHub has had the feature turned on for ages, with a few caveats:

      - we loosened the zero-padded mode warning, because it was causing
	too many false positives

      - we loosened the timezone checks for the same reason; we've seen
	time zones that aren't exactly 4 characters before

      - we occasionally get complaints from people trying to push old
	histories with bogus committer idents. Usually a missing name or
	similar.

     So those are the ones we'd probably need to loosen off the bat, and
     they're all pretty harmless. But it would be a potential irritating
     regression for somebody if they have a history with other minor
     flaws, and Git suddenly starts refusing to clone it.

Re: git/git-scm.com GH Issue Tracker

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [1] If we had a more permissive set of defaults, it would probably make
>     sense to turn on fsckObjects by default. Some of the checks are
>     security-relevant, like disallowing trees with ".GIT",
>     "../../etc/passwd", etc. Those _should_ be handled sanely by the
>     rest of Git, but it serves as a belt-and-suspenders check, and also
>     protects anybody with a buggy Git downstream from you.

Yeah, we really should encourage people to turn it on.  Turning it
on by default is one way to do so, of course.

I think the reason why the transfer side is stricter than the local
checking [*1*] is because the problem in the local history is
already done and there is not much the user can do to fix it, while
objects that originate from outside world could be rejected to keep
the receiving repository clean.


>     GitHub has had the feature turned on for ages, with a few caveats:
>
>       - we loosened the zero-padded mode warning, because it was causing
> 	too many false positives
>
>       - we loosened the timezone checks for the same reason; we've seen
> 	time zones that aren't exactly 4 characters before
>
>       - we occasionally get complaints from people trying to push old
> 	histories with bogus committer idents. Usually a missing name or
> 	similar.

As long as we are sure that modern Git and its reimplementations no
longer create objects with this problems, I think loosening these
checks is perfectly sensible, as the only objects that trigger the
check will be the ones buried deep in the history made back when Git
was young.

Of course, that will make it easier to let broken objects created by
newer reimplementations of Git (and bugs in our code that cause us
to create such broken objects) go unnoticed, so we would want to
keep them at warn (not ignore) for the end-users.

>      So those are the ones we'd probably need to loosen off the bat, and
>      they're all pretty harmless. But it would be a potential irritating
>      regression for somebody if they have a history with other minor
>      flaws, and Git suddenly starts refusing to clone it.

[Footnote]

*1* ... which is what Jonathan Nieder originally wondered at
    https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=743227

Re: git/git-scm.com GH Issue Tracker

[Jeff King]

On Mon, Feb 06, 2017 at 12:49:34PM -0800, Junio C Hamano wrote:

> Of course, that will make it easier to let broken objects created by
> newer reimplementations of Git (and bugs in our code that cause us
> to create such broken objects) go unnoticed, so we would want to
> keep them at warn (not ignore) for the end-users.

Yes, sorry if that wasn't clear. By "loosen" I just meant to warn,
not ignore.

So a viable path forward is perhaps:

  1. Add fetch.fsck.* (and probably transfer.fsck.*) to match the
     receive-pack options.

  2. Go over the current list of default warning/error settings and make
     sure they are sensible.

  3. Add a "light" mode to transfer.fsckObjects and friends that blocks
     only errors, not warnings. Maybe setting the config bool to
     "errors-only" instead of "true" or something.

  4. (Optional) Default transfer.fsckObjects to "errors-only".

     The escape hatch is to set fsckObjects to "false", or to downgrade
     your specific problem to a warning via transfer.fsck.*.

-Peff

Re: git/git-scm.com GH Issue Tracker

[Samuel Lijin]

Finished going through and nailed the rest of the open issues!


# Irrelevant but it seems like someone should take a look

511 466


# Irrelevant to git-scm.com and should be closed

599 596 570 565 563 558 538 537 520 511 509 507 501 494 465


# Resolved, duplicate, or non-issue

596 593 592 588 587 585 583 576 575 573 572 547 546 543 540 539 529 521
516 515 504 503 502 496 491 490 476 473 470 467 463 460 456 454 451 413
377 265 257 95



# Relevant and should be kept open

597 595 591 586 578 544 532 518 513 512 500 493 466 448 416 410 381 379
140 13 12 11


That's all of them!

- Sam

On Mon, Feb 6, 2017 at 12:34 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 06, 2017 at 12:15:08AM -0600, Samuel Lijin wrote:
>
>> I've went through a bunch of open issues on the git/git-scm.com repo
>> (specifically, everything after #600) and I think the bulk of them can
>> be closed.
>>
>> I've taken the liberty of classifying them as shown below.
>
> Thanks, this is incredibly helpful. I'll close the appropriate ones you
> identified.
>
> -Peff