Permalink
Commits on Nov 13, 2017
  1. link_alt_odb_entries: make empty input a noop

    peff committed with gitster Nov 12, 2017
    If an empty string is passed to link_alt_odb_entries(), our
    loop finds no entries and we link nothing. But we still do
    some preparatory work to normalize the object directory
    path, even though we'll never look at the result. This
    triggers in basically every git process, since we feed the
    usually-empty ALTERNATE_DB_ENVIRONMENT to the function.
    
    Let's detect early that there's nothing to do and return.
    While we're at it, let's treat NULL the same as an empty
    string as a favor to our callers. That saves
    prepare_alt_odb() from having to cover this case.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Nov 3, 2017
  1. setup: avoid double slashes when looking for HEAD

    peff committed with gitster Nov 3, 2017
    Andrew Baumann reported that when called outside of any Git worktree,
    `git rev-parse --is-inside-work-tree` eventually tries to access
    `//HEAD`, i.e.  any `HEAD` file in the root directory, but with a double
    slash.
    
    This double slash is not only unintentional, but is allowed by the POSIX
    standard to have a special meaning. And most notably on Windows, it
    does, where it refers to a UNC path of the form `//server/share/`.
    
    As a consequence, afore-mentioned `rev-parse` call not only looks for
    the wrong thing, but it also causes serious delays, as Windows will try
    to access a server called `HEAD`.  Let's simply avoid the unintended
    double slash.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 21, 2017
  1. worktree: handle broken symrefs in find_shared_symref()

    peff committed with gitster Oct 19, 2017
    The refs_resolve_ref_unsafe() function may return NULL even
    with a REF_ISSYMREF flag if a symref points to a broken ref.
    As a result, it's possible for find_shared_symref() to
    segfault when it passes NULL to strcmp().
    
    This is hard to trigger for most code paths. We typically
    pass HEAD to the function as the symref to resolve, and
    programs like "git branch" will bail much earlier if HEAD
    isn't valid.
    
    I did manage to trigger it through one very obscure
    sequence:
    
      # You have multiple notes refs which conflict.
      git notes add -m base
      git notes --ref refs/notes/foo add -m foo
    
      # There's left-over cruft in NOTES_MERGE_REF that
      # makes it a broken symref (in this case we point
      # to a syntactically invalid ref).
      echo "ref: refs/heads/master.lock" >.git/NOTES_MERGE_REF
    
      # You try to merge the notes. We read the broken value in
      # order to complain that another notes-merge is
      # in-progress, but we segfault in find_shared_symref().
      git notes merge refs/notes/foo
    
    This is obviously silly and almost certainly impossible to
    trigger accidentally, but it does show that the bug is
    triggerable from at least one code path. In addition, it
    would trigger if we saw a transient filesystem error when
    resolving the pointed-to ref.
    
    We can fix this by treating NULL the same as a non-matching
    symref. Arguably we'd prefer to know if a symref points to
    "refs/heads/foo", but "refs/heads/foo" is broken. But
    refs_resolve_ref_unsafe() isn't capable of giving us that
    information, so this is the best we can do.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  2. log: handle broken HEAD in decoration check

    peff committed with gitster Oct 19, 2017
    The resolve_ref_unsafe() function may return NULL even with
    a REF_ISSYMREF flag if a symref points to a broken ref. As a
    result, it's possible for the decoration code's "is this
    branch the current HEAD" check to segfault when it passes
    the NULL to starts_with().
    
    This is unlikely in practice, since we can only reach this
    code if we already resolved HEAD to a matching sha1 earlier.
    But it's possible if HEAD racily becomes broken, or if
    there's a transient filesystem error.
    
    We can fix this by returning early in the broken case, since
    NULL could not possibly match any of our branch names.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  3. remote: handle broken symrefs

    peff committed with gitster Oct 19, 2017
    It's possible for resolve_ref_unsafe() to return NULL with a
    REF_ISSYMREF flag if a symref points to a broken ref.  In
    this case, the read_remote_branches() function will segfault
    passing the name to xstrdup().
    
    This is hard to trigger in practice, since this function is
    used as a callback to for_each_ref(), which will skip broken
    refs in the first place (so it would have to be broken
    racily, or for us to see a transient filesystem error).
    
    If we see such a racy broken outcome let's treat it as "not
    a symref". This is exactly the same thing that would happen
    in the non-racy case (our function would not be called at
    all, as for_each_ref would skip the broken symref).
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  4. test-ref-store: avoid passing NULL to printf

    peff committed with gitster Oct 19, 2017
    It's possible for resolve_ref_unsafe() to return NULL (e.g.,
    if we are reading and the ref does not exist), in which case
    we'll pass NULL to printf. On glibc systems this produces
    "(null)", but on others it may segfault.
    
    The tests don't expect any such case, but if we ever did
    trigger this, we would prefer to cleanly fail the test with
    unexpected input rather than segfault. Let's manually
    replace NULL with "(null)". The exact value doesn't matter,
    as it won't match any possible ref the caller could expect
    (and anyway, the exit code of the program will tell whether
    "ref" is valid or not).
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  5. diff: handle NULs in get_string_hash()

    peff committed with gitster Oct 19, 2017
    For computing moved lines, we feed the characters of each
    line into a hash. When we've been asked to ignore
    whitespace, then we pick each character using next_byte(),
    which returns -1 on end-of-string, which it determines using
    the start/end pointers we feed it.
    
    However our check of its return value treats "0" the same as
    "-1", meaning we'd quit if the string has an embedded NUL.
    This is unlikely to ever come up in practice since our line
    boundaries generally come from calling strlen() in the first
    place.
    
    But it was a bit surprising to me as a reader of the
    next_byte() code. And it's possible that we may one day feed
    this function with more exotic input, which otherwise works
    with arbitrary ptr/len pairs.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  6. diff: fix whitespace-skipping with --color-moved

    peff committed with gitster Oct 19, 2017
    The code for handling whitespace with --color-moved
    represents partial strings as a pair of pointers. There are
    two possible conventions for the end pointer:
    
      1. It points to the byte right after the end of the
         string.
    
      2. It points to the final byte of the string.
    
    But we seem to use both conventions in the code:
    
      a. we assign the initial pointers from the NUL-terminated
         string using (1)
    
      b. we eat trailing whitespace by checking the second
         pointer for isspace(), which needs (2)
    
      c. the next_byte() function checks for end-of-string with
         "if (cp > endp)", which is (2)
    
      d. in next_byte() we skip past internal whitespace with
         "while (cp < end)", which is (1)
    
    This creates fewer bugs than you might think, because there
    are some subtle interactions. Because of (a) and (c), we
    always return the NUL-terminator from next_byte(). But all
    of the callers of next_byte() happen to handle that
    gracefully.
    
    Because of the mismatch between (d) and (c), next_byte()
    could accidentally return a whitespace character right at
    endp. But because of the interaction of (a) and (b), we fail
    to actually chomp trailing whitespace, meaning our endp
    _always_ points to a NUL, canceling out the problem.
    
    But that does leave (b) as a real bug: when ignoring
    whitespace only at the end-of-line, we don't correctly trim
    it, and fail to match up lines.
    
    We can fix the whole thing by moving consistently to one
    convention. Since convention (1) is idiomatic in our code
    base, we'll pick that one.
    
    The existing "-w" and "-b" tests continue to pass, and a new
    "--ignore-space-at-eol" shows off the breakage we're fixing.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  7. t4015: test the output of "diff --color-moved -b"

    peff committed with gitster Oct 19, 2017
    Commit fa5ba2c (diff: fix infinite loop with
    --color-moved --ignore-space-change, 2017-10-12) added a
    test to make sure that "--color-moved -b" doesn't run
    forever, but the test in question doesn't actually have any
    moved lines in it.
    
    Let's scrap that test and add a variant of the existing
    "--color-moved -w" test, but this time we'll check that we
    find the move with whitespace changes, but not arbitrary
    whitespace additions.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  8. t4015: check "negative" case for "-w --color-moved"

    peff committed with gitster Oct 19, 2017
    We test that lines with whitespace changes are not found by
    "--color-moved" by default, but are found if "-w" is added.
    Let's add one more twist: a line that has non-whitespace
    changes should not be marked as a pure move.
    
    This is perhaps an obvious case for us to get right (and we
    do), but as we add more whitespace tests, they will form a
    pattern of "make sure this case is a move and this other
    case is not".
    
    Note that we have to add a line to our moved block, since
    having a too-small block doesn't trigger the "moved"
    heuristics.  And we also add a line of context to ensure
    that there's more context lines than moved lines (so the
    diff shows us moving the lines up, rather than moving the
    context down).
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  9. t4015: refactor --color-moved whitespace test

    peff committed with gitster Oct 19, 2017
    In preparation for testing several different whitespace
    options, let's split out the setup and cleanup steps of the
    whitespace test.
    
    While we're here, let's also switch to using "<<-" to indent
    our here-documents properly, and use q_to_tab to more
    explicitly mark where we expect whitespace to appear.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 17, 2017
  1. tag: respect color.ui config

    peff committed with gitster Oct 13, 2017
    Since 11b087a (ref-filter: consult want_color() before
    emitting colors, 2017-07-13), we expect that setting
    "color.ui" to "always" will enable color tag formats even
    without a tty.  As that commit was built on top of
    136c8c8 (color: check color.ui in git_default_config(),
    2017-07-13) from the same series, we didn't need to touch
    tag's config parsing at all.
    
    However, since we reverted 136c8c8, we now need to
    explicitly call git_color_default_config() to make this
    work.
    
    Let's do so, and also restore the test dropped in 0c88bf5
    (provide --color option for all ref-filter users,
    2017-10-03). That commit swapped out our "color.ui=always"
    test for "--color" in preparation for "always" going away.
    But since it is here to stay, we should test both cases.
    
    Note that for-each-ref also lost its color.ui support as
    part of reverting 136c8c8. But as a plumbing command, it
    should _not_ respect the color.ui config. Since it also
    gained a --color option in 0c88bf5, that's the correct
    way to ask it for color. We'll continue to test that, and
    confirm that "color.ui" is not respected.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  2. Revert "color: check color.ui in git_default_config()"

    peff committed with gitster Oct 13, 2017
    This reverts commit 136c8c8.
    
    That commit was trying to address a bug caused by 4c7f181
    (make color.ui default to 'auto', 2013-06-10), in which
    plumbing like diff-tree defaulted to "auto" color, but did
    not respect a "color.ui" directive to disable it.
    
    But it also meant that we started respecting "color.ui" set
    to "always". This was a known problem, but 4c7f181 argued
    that nobody ought to be doing that. However, that turned out
    to be wrong, and we got a number of bug reports related to
    "add -p" regressing in v2.14.2.
    
    Let's revert 136c8c8, fixing the regression to "add -p".
    This leaves the problem from 4c7f181 unfixed, but:
    
      1. It's a pretty obscure problem in the first place. I
         only noticed it while working on the color code, and we
         haven't got a single bug report or complaint about it.
    
      2. We can make a more moderate fix on top by respecting
         "never" but not "always" for plumbing commands. This
         is just the minimal fix to go back to the working state
         we had before v2.14.2.
    
    Note that this isn't a pure revert. We now have a test in
    t3701 which shows off the "add -p" regression. This can be
    flipped to success.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  3. Revert "t6006: drop "always" color config tests"

    peff committed with gitster Oct 13, 2017
    This reverts commit c5bdfe6.
    
    That commit was done primarily to prepare for the weakening
    of "always" in 6be4595 (color: make "always" the same as
    "auto" in config, 2017-10-03). But since we've now reverted
    6be4595, there's no need for us to remove "-c
    color.ui=always" from the tests. And in fact it's a good
    idea to restore these tests, to make sure that "always"
    continues to work.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  4. Revert "color: make "always" the same as "auto" in config"

    peff committed with gitster Oct 13, 2017
    This reverts commit 6be4595.
    
    That commit weakened the "always" setting of color config so
    that it acted as "auto". This was meant to solve regressions
    in v2.14.2 in which setting "color.ui=always" in the on-disk
    config broke scripts like add--interactive, because the
    plumbing diff commands began to generate color output.
    
    This was due to 136c8c8 (color: check color.ui in
    git_default_config(), 2017-07-13), which was in turn trying
    to fix issues caused by 4c7f181 (make color.ui default to
    'auto', 2013-06-10). But in weakening "always", we created
    even more problems, as people expect to be able to use "git
    -c color.ui=always" to force color (especially because some
    commands don't have their own --color flag). We can fix that
    by special-casing the command-line "-c", but now things are
    getting pretty confusing.
    
    Instead of piling hacks upon hacks, let's start peeling off
    the hacks. The first step is dropping the weakening of
    "always", which this revert does.
    
    Note that we could actually revert the whole series merged
    in by da15b78. Most of that
    series consists of preparations to the tests to handle the
    weakening of "-c color.ui=always". But it's worth keeping
    for a few reasons:
    
      - there are some other preparatory cleanups, like
        e433749 (test-terminal: set TERM=vt100, 2017-10-03)
    
      - it adds "--color" options more consistently in
        0c88bf5 (provide --color option for all ref-filter
        users, 2017-10-03)
    
      - some of the cases dropping "-c" end up being more robust
        and realistic tests, as in 01c94e9 (t7508: use
        test_terminal for color output, 2017-10-03)
    
      - the preferred tool for overriding config is "--color",
        and we should be modeling that consistently
    
    We can individually revert the few commits necessary to
    restore some useful tests (which will be done on top of this
    patch).
    
    Note that this isn't a pure revert; we'll keep the test
    added in t3701, but mark it as failure for now.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 16, 2017
  1. diff: fix infinite loop with --color-moved --ignore-space-change

    peff committed with gitster Oct 13, 2017
    The --color-moved code uses next_byte() to advance through
    the blob contents. When the user has asked to ignore
    whitespace changes, we try to collapse any whitespace change
    down to a single space.
    
    However, we enter the conditional block whenever we see the
    IGNORE_WHITESPACE_CHANGE flag, even if the next byte isn't
    whitespace.
    
    This means that the combination of "--color-moved and
    --ignore-space-change" was completely broken. Worse, because
    we return from next_byte() without having advanced our
    pointer, the function makes no forward progress in the
    buffer and loops infinitely.
    
    Fix this by entering the conditional only when we actually
    see whitespace. We can apply this also to the
    IGNORE_WHITESPACE change. That code path isn't buggy
    (because it falls through to returning the next
    non-whitespace byte), but it makes the logic more clear if
    we only bother to look at whitespace flags after seeing that
    the next byte is whitespace.
    
    Reported-by: Orgad Shaneh <orgads@gmail.com>
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 14, 2017
  1. revision: quit pruning diff more quickly when possible

    peff committed with gitster Oct 13, 2017
    When the revision traversal machinery is given a pathspec,
    we must compute the parent-diff for each commit to determine
    which ones are TREESAME. We set the QUICK diff flag to avoid
    looking at more entries than we need; we really just care
    whether there are any changes at all.
    
    But there is one case where we want to know a bit more: if
    --remove-empty is set, we care about finding cases where the
    change consists only of added entries (in which case we may
    prune the parent in try_to_simplify_commit()). To cover that
    case, our file_add_remove() callback does not quit the diff
    upon seeing an added entry; it keeps looking for other types
    of entries.
    
    But this means when --remove-empty is not set (and it is not
    by default), we compute more of the diff than is necessary.
    You can see this in a pathological case where a commit adds
    a very large number of entries, and we limit based on a
    broad pathspec. E.g.:
    
      perl -e '
        chomp(my $blob = `git hash-object -w --stdin </dev/null`);
        for my $a (1..1000) {
          for my $b (1..1000) {
            print "100644 $blob\t$a/$b\n";
          }
        }
      ' | git update-index --index-info
      git commit -qm add
    
      git rev-list HEAD -- .
    
    This case takes about 100ms now, but after this patch only
    needs 6ms. That's not a huge improvement, but it's easy to
    get and it protects us against even more pathological cases
    (e.g., going from 1 million to 10 million files would take
    ten times as long with the current code, but not increase at
    all after this patch).
    
    This is reported to minorly speed-up pathspec limiting in
    real world repositories (like the 100-million-file Windows
    repository), but probably won't make a noticeable difference
    outside of pathological setups.
    
    This patch actually covers the case without --remove-empty,
    and the case where we see only deletions. See the in-code
    comment for details.
    
    Note that we have to add a new member to the diff_options
    struct so that our callback can see the value of
    revs->remove_empty_trees. This callback parameter could be
    passed to the "add_remove" and "change" callbacks, but
    there's not much point. They already receive the
    diff_options struct, and doing it this way avoids having to
    update the function signature of the other callbacks
    (arguably the format_callback and output_prefix functions
    could benefit from the same simplification).
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 10, 2017
  1. write_entry: untangle symlink and regular-file cases

    peff committed with gitster Oct 9, 2017
    The write_entry() function switches on the mode of the entry
    we're going to write out. The cases for S_IFLNK and S_IFREG
    are lumped together. In earlier versions of the code, this
    made some sense. They have a shared preamble (which reads
    the blob content), a short type-specific body, and a shared
    conclusion (which writes out the file contents; always for
    S_IFREG and only sometimes for S_IFLNK).
    
    But over time this has grown to make less sense. The preamble
    now has conditional bits for each type, and the S_IFREG body
    has grown a lot more complicated. It's hard to follow the
    logic of which code is running for which mode.
    
    Let's give each mode its own case arm. We will still share
    the conclusion code, which means we now jump to it with a
    goto. Ideally we'd pull that shared code into its own
    function, but it touches so much internal state in the
    write_entry() function that the end result is actually
    harder to follow than the goto.
    
    While we're here, we'll touch up a few bits of whitespace to
    make the beginning and endings of the cases easier to read.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 9, 2017
  1. write_entry: avoid reading blobs in CE_RETRY case

    peff committed with gitster Oct 9, 2017
    When retrying a delayed filter-process request, we don't
    need to send the blob to the filter a second time. However,
    we read it unconditionally into a buffer, only to later
    throw away that buffer. We can make this more efficient by
    skipping the read in the first place when it isn't
    necessary.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  2. write_entry: fix leak when retrying delayed filter

    peff committed with gitster Oct 9, 2017
    When write_entry() retries a delayed filter request, we
    don't need to send the blob content to the filter again, and
    set the pointer to NULL. But doing so means we leak the
    contents we read earlier from read_blob_entry(). Let's make
    sure to free it before dropping the pointer.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 7, 2017
  1. refs_resolve_ref_unsafe: handle d/f conflicts for writes

    peff committed with gitster Oct 6, 2017
    If our call to refs_read_raw_ref() fails, we check errno to
    see if the ref is simply missing, or if we encountered a
    more serious error. If it's just missing, then in "write"
    mode (i.e., when RESOLVE_REFS_READING is not set), this is
    perfectly fine.
    
    However, checking for ENOENT isn't sufficient to catch all
    missing-ref cases. In the filesystem backend, we may also
    see EISDIR when we try to resolve "a" and "a/b" exists.
    Likewise, we may see ENOTDIR if we try to resolve "a/b" and
    "a" exists. In both of those cases, we know that our
    resolved ref doesn't exist, but we return an error (rather
    than reporting the refname and returning a null sha1).
    
    This has been broken for a long time, but nobody really
    noticed because the next step after resolving without the
    READING flag is usually to lock the ref and write it. But in
    both of those cases, the write will fail with the same
    errno due to the directory/file conflict.
    
    There are two cases where we can notice this, though:
    
      1. If we try to write "a" and there's a leftover directory
         already at "a", even though there is no ref "a/b". The
         actual write is smart enough to move the empty "a" out
         of the way.
    
         This is reasonably rare, if only because the writing
         code has to do an independent resolution before trying
         its write (because the actual update_ref() code handles
         this case fine). The notes-merge code does this, and
         before the fix in the prior commit t3308 erroneously
         expected this case to fail.
    
      2. When resolving symbolic refs, we typically do not use
         the READING flag because we want to resolve even
         symrefs that point to unborn refs. Even if those unborn
         refs could not actually be written because of d/f
         conflicts with existing refs.
    
         You can see this by asking "git symbolic-ref" to report
         the target of a symref pointing past a d/f conflict.
    
    We can fix the problem by recognizing the other "missing"
    errnos and treating them like ENOENT. This should be safe to
    do even for callers who are then going to actually write the
    ref, because the actual writing process will fail if the d/f
    conflict is a real one (and t1404 checks these cases).
    
    Arguably this should be the responsibility of the
    files-backend to normalize all "missing ref" errors into
    ENOENT (since something like EISDIR may not be meaningful at
    all to a database backend). However other callers of
    refs_read_raw_ref() may actually care about the distinction;
    putting this into resolve_ref() is the minimal fix for now.
    
    The new tests in t1401 use git-symbolic-ref, which is the
    most direct way to check the resolution by itself.
    Interestingly we actually had a test that setup this case
    already, but we only used it to verify that the funny state
    could be overwritten, not that it could be resolved.
    
    We also add a new test in t3200, as "branch -m" was the
    original motivation for looking into this. What happens is
    this:
    
      0. HEAD is pointing to branch "a"
    
      1. The user asks to rename "a" to "a/b".
    
      2. We create "a/b" and delete "a".
    
      3. We then try to update any worktree HEADs that point to
         the renamed ref (including the main repo HEAD). To do
         that, we have to resolve each HEAD. But now our HEAD is
         pointing at "a", and we get EISDIR due to the loose
         "a/b". As a result, we think there is no HEAD, and we
         do not update it. It now points to the bogus "a".
    
    Interestingly this case used to work, but only accidentally.
    Before 31824d1 (branch: fix branch renaming not updating
    HEADs correctly, 2017-08-24), we'd update any HEAD which we
    couldn't resolve. That was wrong, but it papered over the
    fact that we were incorrectly failing to resolve HEAD.
    
    So while the bug demonstrated by the git-symbolic-ref is
    quite old, the regression to "branch -m" is recent.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  2. t3308: create a real ref directory/file conflict

    peff committed with gitster Oct 6, 2017
    A test in t3308 wants to make sure that we don't
    accidentally merge into "refs/notes/dir" when it exists as a
    directory, so it does:
    
      mkdir .git/refs/notes/dir
      git -c core.notesRef=refs/notes/dir merge ...
    
    and expects the second command to fail. But that
    understimates the refs code, which is smart enough to remove
    useless directories in the refs hierarchy. The test
    succeeded only because of a bug which prevented resolving
    refs/notes/dir for writing, even though an actual ref update
    would succeed.
    
    In preparation for fixing that bug, let's switch to creating
    a real ref in refs/notes/dir, which is a more realistic
    situation.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 6, 2017
  1. sha1_loose_object_info: handle errors from unpack_sha1_rest

    peff committed with gitster Oct 5, 2017
    When a caller of sha1_object_info_extended() sets the
    "contentp" field in object_info, we call unpack_sha1_rest()
    but do not check whether it signaled an error.
    
    This causes two problems:
    
      1. We pass back NULL to the caller via the contentp field,
         but the function returns "0" for success. A caller
         might reasonably expect after a successful return that
         it can access contentp without a NULL check and
         segfault.
    
         As it happens, this is impossible to trigger in the
         current code. There is exactly one caller which uses
         contentp, read_object(). And the only thing it does
         after a successful call is to return the content
         pointer to its caller, using NULL as a sentinel for
         errors. So in effect it converts the success code from
         sha1_object_info_extended() back into an error!
    
         But this is still worth addressing avoid problems for
         future users of "contentp".
    
      2. Callers of unpack_sha1_rest() are expected to close the
         zlib stream themselves on error. Which means that we're
         leaking the stream.
    
    The problem in (1) comes from from c84a1f3 (sha1_file:
    refactor read_object, 2017-06-21), which added the contentp
    field.  Before that, we called unpack_sha1_rest() via
    unpack_sha1_file(), which directly used the NULL to signal
    an error.
    
    But note that the leak in (2) is actually older than that.
    The original unpack_sha1_file() directly returned the result
    of unpack_sha1_rest() to its caller, when it should have
    been closing the zlib stream itself on error.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commits on Oct 4, 2017
  1. path.c: fix uninitialized memory access

    peff committed with gitster Oct 3, 2017
    In cleanup_path we're passing in a char array, run a memcmp on it, and
    run through it without ever checking if something is in the array in the
    first place.  This can lead us to access uninitialized memory, for
    example in t5541-http-push-smart.sh test 7, when run under valgrind:
    
    ==4423== Conditional jump or move depends on uninitialised value(s)
    ==4423==    at 0x242FA9: cleanup_path (path.c:35)
    ==4423==    by 0x242FA9: mkpath (path.c:456)
    ==4423==    by 0x256CC7: refname_match (refs.c:364)
    ==4423==    by 0x26C181: count_refspec_match (remote.c:1015)
    ==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)
    ==4423==    by 0x26C181: check_push_refs (remote.c:1409)
    ==4423==    by 0x2ABB4D: transport_push (transport.c:870)
    ==4423==    by 0x186703: push_with_options (push.c:332)
    ==4423==    by 0x18746D: do_push (push.c:409)
    ==4423==    by 0x18746D: cmd_push (push.c:566)
    ==4423==    by 0x1183E0: run_builtin (git.c:352)
    ==4423==    by 0x11973E: handle_builtin (git.c:539)
    ==4423==    by 0x11973E: run_argv (git.c:593)
    ==4423==    by 0x11973E: main (git.c:698)
    ==4423==  Uninitialised value was created by a heap allocation
    ==4423==    at 0x4C2CD8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==4423==    by 0x4C2F195: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==4423==    by 0x2C196B: xrealloc (wrapper.c:137)
    ==4423==    by 0x29A30B: strbuf_grow (strbuf.c:66)
    ==4423==    by 0x29A30B: strbuf_vaddf (strbuf.c:277)
    ==4423==    by 0x242F9F: mkpath (path.c:454)
    ==4423==    by 0x256CC7: refname_match (refs.c:364)
    ==4423==    by 0x26C181: count_refspec_match (remote.c:1015)
    ==4423==    by 0x26C181: match_explicit_lhs (remote.c:1126)
    ==4423==    by 0x26C181: check_push_refs (remote.c:1409)
    ==4423==    by 0x2ABB4D: transport_push (transport.c:870)
    ==4423==    by 0x186703: push_with_options (push.c:332)
    ==4423==    by 0x18746D: do_push (push.c:409)
    ==4423==    by 0x18746D: cmd_push (push.c:566)
    ==4423==    by 0x1183E0: run_builtin (git.c:352)
    ==4423==    by 0x11973E: handle_builtin (git.c:539)
    ==4423==    by 0x11973E: run_argv (git.c:593)
    ==4423==    by 0x11973E: main (git.c:698)
    ==4423==
    
    Avoid this by using skip_prefix(), which knows not to go beyond the
    end of the string.
    
    Reported-by: Thomas Gummerer <t.gummerer@gmail.com>
    Signed-off-by: Jeff King <peff@peff.net>
    Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  2. t7301: use test_terminal to check color

    peff committed with gitster Oct 3, 2017
    This test wants to confirm that "clean -i" shows color
    output. Using test_terminal gives us a more realistic
    environment than "color.ui=always", and prepares us for the
    behavior of "always" changing in a future patch.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  3. t4015: use --color with --color-moved

    peff committed with gitster Oct 3, 2017
    The tests for --color-moved write their output to a file,
    but doing so suppresses color output under "auto". Right now
    this is solved by running the whole script under
    "color.diff=always". In preparation for the behavior of
    "always" changing, let's explicitly enable color.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  4. color: make "always" the same as "auto" in config

    peff committed with gitster Oct 3, 2017
    It can be handy to use `--color=always` (or it's synonym
    `--color`) on the command-line to convince a command to
    produce color even if it's stdout isn't going to the
    terminal or a pager.
    
    What's less clear is whether it makes sense to set config
    variables like color.ui to `always`. For a one-shot like:
    
      git -c color.ui=always ...
    
    it's potentially useful (especially if the command doesn't
    directly support the `--color` option). But setting `always`
    in your on-disk config is much muddier, as you may be
    surprised when piped commands generate colors (and send them
    to whatever is consuming the pipe downstream).
    
    Some people have done this anyway, because:
    
      1. The documentation for color.ui makes it sound like
         using `always` is a good idea, when you almost
         certainly want `auto`.
    
      2. Traditionally not every command (and especially not
         plumbing) respected color.ui in the first place. So
         the confusion came up less frequently than it might
         have.
    
    The situation changed in 136c8c8 (color: check color.ui
    in git_default_config(), 2017-07-13), which negated point
    (2): now scripts using only plumbing commands (like
    add-interactive) are broken by this setting.
    
    That commit was fixing real issues (e.g., by making
    `color.ui=never` work, since `auto` is the default), so we
    don't want to just revert it.  We could turn `always` into a
    noop in plumbing commands, but that creates a hard-to-explain
    inconsistency between the plumbing and other commands.
    
    Instead, let's just turn `always` into `auto` for all config.
    This does break the "one-shot" config shown above, but again,
    we're probably better to have simple and consistent rules than
    to try to special-case command-line config.
    
    There is one place where `always` should retain its meaning:
    on the command line, `--color=always` should continue to be
    the same as `--color`, overriding any isatty checks. Since the
    command-line parser also depends on git_config_colorbool(), we
    can use the existence of the "var" string to deterine whether
    we are serving the command-line or the config.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  5. provide --color option for all ref-filter users

    peff committed with gitster Oct 3, 2017
    When ref-filter learned about want_color() in 11b087a
    (ref-filter: consult want_color() before emitting colors,
    2017-07-13), it became useful to be able to turn colors off
    and on for specific commands. For git-branch, you can do so
    with --color/--no-color.
    
    But for git-for-each-ref and git-tag, the other users of
    ref-filter, you have no option except to tweak the
    "color.ui" config setting. Let's give both of these commands
    the usual color command-line options.
    
    This is a bit more obvious as a method for overriding the
    config. And it also prepares us for the behavior of "always"
    changing (so that we are still left with a way of forcing
    color when our output goes to a non-terminal).
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  6. t3205: use --color instead of color.branch=always

    peff committed with gitster Oct 3, 2017
    To test the color output, we must convince "git branch" to
    write colors to a non-terminal. We do that now by setting
    the color config to "always".  In preparation for the
    behavior of "always" changing, let's switch to using the
    "--color" command-line option, which is more direct.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  7. t3203: drop "always" color test

    peff committed with gitster Oct 3, 2017
    In preparation for the behavior of "always" changing to
    match "auto", we can simply drop this test. We already check
    other forms (like "--color") independently.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  8. t6006: drop "always" color config tests

    peff committed with gitster Oct 3, 2017
    We test the %C() format placeholders with a variety of
    color-inducing options, including "--color" and
    "-c color.ui=always". In preparation for the behavior of
    "always" changing, we need to do something with those
    "always" tests.
    
    We can drop ones that expect "always" to turn on color even
    to a file, as that will become a synonym for "auto", which
    is already tested.
    
    For the "--no-color" test, we need to make sure that color
    would otherwise be shown. To do this, we can use
    test_terminal, which enables colors in the default setup.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  9. t7502: use diff.noprefix for --verbose test

    peff committed with gitster Oct 3, 2017
    To check that "status -v" respects diff config, we set
    "color.diff" and look at the output of "status". We could
    equally well use any diff config. Since color output depends
    on a lot of other factors (like whether stdout is a tty, and
    how we interpret "always"), let's use a more mundane option.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  10. t7508: use test_terminal for color output

    peff committed with gitster Oct 3, 2017
    This script tests the output of status with various formats
    when color is enabled. It uses the "always" setting so that
    the output is valid even though we capture it in a file.
    Using test_terminal gives us a more realistic environment,
    and prepares us for the behavior of "always" changing.
    
    Arguably we are testing less than before, since "auto" is
    already the default, and we can no longer tell if the config
    is actually doing anything.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  11. t3701: use test-terminal to collect color output

    peff committed with gitster Oct 3, 2017
    When testing whether "add -p" can generate colors, we set
    color.ui to "always". This isn't a very good test, as in the
    real-world a user typically has "auto" coupled with stdout
    going to a terminal (and it's plausible that this could mask
    a real bug in add--interactive if we depend on plumbing's
    isatty check).
    
    Let's switch to test_terminal, which gives us a more
    realistic environment. This also prepare us for future
    changes to the "always" color option.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
  12. t4015: prefer --color to -c color.diff=always

    peff committed with gitster Oct 3, 2017
    t4015 contains many color-related tests which need to
    override the "is stdout a tty" check. They do so by setting
    the color.diff config, but we can accomplish the same with
    the --color option. Besides being shorter to type, switching
    will prepare us for upcoming changes to "always" when see it
    in config.
    
    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>