1. 21 Sep, 2018 2 commits
    • Derrick Stolee's avatar
      commit-reach: properly peel tags · b67f6b26
      Derrick Stolee authored
      The can_all_from_reach_with_flag() algorithm was refactored in 4fbcca4e
      "commit-reach: make can_all_from_reach... linear" but incorrectly
      assumed that all objects provided were commits. During a fetch
      negotiation, ok_to_give_up() in upload-pack.c may provide unpeeled tags
      to the 'from' array. The current code creates a segfault.
      
      Add a direct call to can_all_from_reach_with_flag() in 'test-tool reach'
      and add a test in t6600-test-reach.sh that demonstrates this segfault.
      
      Correct the issue by peeling tags when investigating the initial list
      of objects in the 'from' array.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: Derrick Stolee's avatarDerrick Stolee <dstolee@microsoft.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      b67f6b26
    • Duy Nguyen's avatar
      add: do not accept pathspec magic 'attr' · 84d938b7
      Duy Nguyen authored
      Commit b0db7046 (pathspec: allow querying for attributes -
      2017-03-13) adds new pathspec magic 'attr' but only with
      match_pathspec(). "git add" has some pathspec related code that still
      does not know about 'attr' and will bail out:
      
          $ git add ':(attr:foo)'
          fatal: BUG:dir.c:1584: unsupported magic 40
      
      A better solution would be making this code support 'attr'. But I
      don't know how much work is needed (I'm not familiar with this new
      magic). For now, let's simply reject this magic with a friendlier
      message:
      
          $ git add ':(attr:foo)'
          fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'
      
      Update t6135 so that the expected error message is from the
      "graceful" rejection codepath, not "oops, we were supposed to reject
      the request to trigger this magic" codepath.
      
      Reported-by: smaudet@sebastianaudet.com
      Signed-off-by: Duy Nguyen's avatarNguyễn Thái Ngọc Duy <pclouds@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      84d938b7
  2. 14 Sep, 2018 1 commit
  3. 13 Sep, 2018 2 commits
    • Jonathan Tan's avatar
      fetch-object: set exact_oid when fetching · e6830201
      Jonathan Tan authored
      fetch_objects() currently does not set exact_oid in struct ref when
      invoking transport_fetch_refs(). If the server supports ref-in-want,
      fetch_pack() uses this field to determine whether a wanted ref should be
      requested as a "want-ref" line or a "want" line; without the setting of
      exact_oid, the wrong line will be sent.
      
      Set exact_oid, so that the correct line is sent.
      Signed-off-by: default avatarJonathan Tan <jonathantanmy@google.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      e6830201
    • Elijah Newren's avatar
      sequencer: fix --allow-empty-message behavior, make it smarter · a3ec9eaf
      Elijah Newren authored
      In commit b00bf1c9 ("git-rebase: make --allow-empty-message the
      default", 2018-06-27), several arguments were given for transplanting
      empty commits without halting and asking the user for confirmation on
      each commit.  These arguments were incomplete because the logic clearly
      assumed the only cases under consideration were transplanting of commits
      with empty messages (see the comment about "There are two sources for
      commits with empty messages).  It didn't discuss or even consider
      rewords, squashes, etc. where the user is explicitly asked for a new
      commit message and provides an empty one.  (My bad, I totally should
      have thought about that at the time, but just didn't.)
      
      Rewords and squashes are significantly different, though, as described
      by SZEDER:
      
          Let's suppose you start an interactive rebase, choose a commit to
          squash, save the instruction sheet, rebase fires up your editor, and
          then you notice that you mistakenly chose the wrong commit to
          squash.  What do you do, how do you abort?
      
          Before [that commit] you could clear the commit message, exit the
          editor, and then rebase would say "Aborting commit due to empty
          commit message.", and you get to run 'git rebase --abort', and start
          over.
      
          But [since that commit, ...] saving the commit message as is would
          let rebase continue and create a bunch of unnecessary objects, and
          then you would have to use the reflog to return to the pre-rebase
          state.
      
      Also, he states:
      
          The instructions in the commit message template, which is shown for
          'reword' and 'squash', too, still say...
      
          # Please enter the commit message for your changes. Lines starting
          # with '#' will be ignored, and an empty message aborts the commit.
      
      These are sound arguments that when editing commit messages during a
      sequencer operation, that if the commit message is empty then the
      operation should halt and ask the user to correct.  The arguments in
      commit b00bf1c9 (referenced above) still apply when transplanting
      previously created commits with empty commit messages, so the sequencer
      should not halt for those.
      
      Furthermore, all rationale so far applies equally for cherry-pick as for
      rebase.  Therefore, make the code default to --allow-empty-message when
      transplanting an existing commit, and to default to halting when the
      user is asked to edit a commit message and provides an empty one -- for
      both rebase and cherry-pick.
      Signed-off-by: Elijah Newren's avatarElijah Newren <newren@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      a3ec9eaf
  4. 12 Sep, 2018 4 commits
    • Elijah Newren's avatar
      update-ref: allow --no-deref with --stdin · d345e9fb
      Elijah Newren authored
      If passed both --no-deref and --stdin, update-ref would error out with a
      general usage message that did not at all suggest these options were
      incompatible.  The manpage for update-ref did suggest through its
      synopsis line that --no-deref and --stdin were incompatible, but it sadly
      also incorrectly suggested that -d and --no-deref were incompatible.  So
      the help around the --no-deref option is buggy in a few ways.
      
      The --stdin option did provide a different mechanism for avoiding
      dereferencing symbolic-refs: adding a line reading
        option no-deref
      before every other directive in the input.  (Technically, if the user
      wants to do the extra work of first determining which refs they want to
      update or delete are symbolic, then they only need to put the extra
      "option no-deref" lines before the updates of those refs.  But in some
      cases, that's more work than just adding the "option no-deref" before
      every other directive.)
      
      It's easier to allow the user to just pass --no-deref along with --stdin
      in order to tell update-ref that the user doesn't want any symbolic ref
      to be dereferenced.  It also makes the update-ref documentation simpler.
      Implement that, and update the documentation to match.
      Signed-off-by: Elijah Newren's avatarElijah Newren <newren@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      d345e9fb
    • Gábor Szeder's avatar
      t0090: disable GIT_TEST_SPLIT_INDEX for the test checking split index · 456d7cd3
      Gábor Szeder authored
      The test 'switching trees does not invalidate shared index' in
      't0090-cache-tree.sh' is about verifying the behaviour of the split
      index feature, therefore it should be in full control of when index
      splitting is performed, like all the tests in 't1700-split-index.sh'.
      
      Unset GIT_TEST_SPLIT_INDEX for this test to avoid unintended random
      index splitting.
      Signed-off-by: Gábor Szeder's avatarSZEDER Gábor <szeder.dev@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      456d7cd3
    • Gábor Szeder's avatar
      t1700-split-index: drop unnecessary 'grep' · acdee9e9
      Gábor Szeder authored
      The test 'disable split index' in 't1700-split-index.sh' runs the
      following pipeline:
      
        cmd | grep <pattern> | sed s///
      
      Drop that 'grep' from the pipeline, and let 'sed' take over its
      duties.
      Signed-off-by: Gábor Szeder's avatarSZEDER Gábor <szeder.dev@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      acdee9e9
    • Derrick Stolee's avatar
      t3206-range-diff.sh: cover single-patch case · cdc067c3
      Derrick Stolee authored
      The commit 40ce4160 "format-patch: allow --range-diff to apply to
      a lone-patch" added the ability to see a range-diff as commentary
      after the commit message of a single patch series (i.e. [PATCH]
      instead of [PATCH X/N]). However, this functionality was not
      covered by a test case.
      
      Add a simple test case that checks that a range-diff is written as
      commentary to the patch.
      Signed-off-by: Derrick Stolee's avatarDerrick Stolee <dstolee@microsoft.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      cdc067c3
  5. 11 Sep, 2018 6 commits
  6. 08 Sep, 2018 1 commit
    • Jonathan Nieder's avatar
      Revert "Merge branch 'sb/submodule-core-worktree'" · f178c13f
      Jonathan Nieder authored
      This reverts commit 7e25437d, reversing
      changes made to 00624d60.
      
      v2.19.0-rc0~165^2~1 (submodule: ensure core.worktree is set after
      update, 2018-06-18) assumes an "absorbed" submodule layout, where the
      submodule's Git directory is in the superproject's .git/modules/
      directory and .git in the submodule worktree is a .git file pointing
      there.  In particular, it uses $GIT_DIR/modules/$name to find the
      submodule to find out whether it already has core.worktree set, and it
      uses connect_work_tree_and_git_dir if not, resulting in
      
      	fatal: could not open sub/.git for writing
      
      The context behind that patch: v2.19.0-rc0~165^2~2 (submodule: unset
      core.worktree if no working tree is present, 2018-06-12) unsets
      core.worktree when running commands like "git checkout
      --recurse-submodules" to switch to a branch without the submodule.  If
      a user then uses "git checkout --no-recurse-submodules" to switch back
      to a branch with the submodule and runs "git submodule update", this
      patch is needed to ensure that commands using the submodule directly
      are aware of the path to the worktree.
      
      It is late in the release cycle, so revert the whole 3-patch series.
      We can try again later for 2.20.
      Reported-by: default avatarAllan Sandfeld Jensen <allan.jensen@qt.io>
      Helped-by: Stefan Beller's avatarStefan Beller <sbeller@google.com>
      Signed-off-by: default avatarJonathan Nieder <jrnieder@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      f178c13f
  7. 07 Sep, 2018 1 commit
    • Max A.K.'s avatar
      http-backend: allow empty CONTENT_LENGTH · 574c513e
      Max A.K. authored
      According to RFC3875, empty environment variable is equivalent to unset,
      and for CONTENT_LENGTH it should mean zero body to read.
      
      However, unset CONTENT_LENGTH is also used for chunked encoding to indicate
      reading until EOF. At least, the test "large fetch-pack requests can be split
      across POSTs" from t5551 starts faliing, if unset or empty CONTENT_LENGTH is
      treated as zero length body. So keep the existing behavior as much as possible.
      
      Add a test for the case.
      Reported-By: Jelmer Vernooij's avatarJelmer Vernooij <jelmer@jelmer.uk>
      Signed-off-by: Max A.K.'s avatarMax Kirillov <max@max630.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      574c513e
  8. 05 Sep, 2018 1 commit
    • Jeff King's avatar
      reopen_tempfile(): truncate opened file · 6c003d6f
      Jeff King authored
      We provide a reopen_tempfile() function, which is in turn
      used by reopen_lockfile().  The idea is that a caller may
      want to rewrite the tempfile without letting go of the lock.
      And that's what our one caller does: after running
      add--interactive, "commit -p" will update the cache-tree
      extension of the index and write out the result, all while
      holding the lock.
      
      However, because we open the file with only the O_WRONLY
      flag, the existing index content is left in place, and we
      overwrite it starting at position 0. If the new index after
      updating the cache-tree is smaller than the original, those
      final bytes are not overwritten and remain in the file. This
      results in a corrupt index, since those cruft bytes are
      interpreted as part of the trailing hash (or even as an
      extension, if there are enough bytes).
      
      This bug actually pre-dates reopen_tempfile(); the original
      code from 9c4d6c02 (cache-tree: Write updated cache-tree
      after commit, 2014-07-13) has the same bug, and those lines
      were eventually refactored into the tempfile module. Nobody
      noticed until now for two reasons:
      
       - the bug can only be triggered in interactive mode
         ("commit -p" or "commit -i")
      
       - the size of the index must shrink after updating the
         cache-tree, which implies a non-trivial deletion. Notice
         that the included test actually has to create a 2-deep
         hierarchy. A single level is not enough to actually cause
         shrinkage.
      
      The fix is to truncate the file before writing out the
      second index. We can do that at the caller by using
      ftruncate(). But we shouldn't have to do that. There is no
      other place in Git where we want to open a file and
      overwrite bytes, making reopen_tempfile() a confusing and
      error-prone interface. Let's pass O_TRUNC there, which gives
      callers the same state they had after initially opening the
      file or lock.
      
      It's possible that we could later add a caller that wants
      something else (e.g., to open with O_APPEND). But this is
      the only caller we've had in the history of the codebase.
      Let's punt on doing anything more clever until another one
      comes along.
      Reported-by: Luc Van Oostenryck's avatarLuc Van Oostenryck <luc.vanoostenryck@gmail.com>
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      6c003d6f
  9. 04 Sep, 2018 3 commits
    • Johannes Schindelin's avatar
      rebase -i: be careful to wrap up fixup/squash chains · 10d2f354
      Johannes Schindelin authored
      When an interactive rebase was stopped at the end of a fixup/squash
      chain, the user might have edited the commit manually before continuing
      (with either `git rebase --skip` or `git rebase --continue`, it does not
      really matter which).
      
      We need to be very careful to wrap up the fixup/squash chain also in
      this scenario: otherwise the next fixup/squash chain would try to pick
      up where the previous one was left.
      Signed-off-by: Johannes Schindelin's avatarJohannes Schindelin <johannes.schindelin@gmx.de>
      10d2f354
    • Johannes Schindelin's avatar
      rebase -i --autosquash: demonstrate a problem skipping the last squash · 2f3eb68f
      Johannes Schindelin authored
      The `git commit --squash` command can be used not only to amend commit
      messages and changes, but also to record notes for an upcoming rebase.
      
      For example, when the author information of a given commit is incorrect,
      a user might call `git commit --allow-empty -m "Fix author" --squash
      <commit>`, to remind them to fix that during the rebase. When the editor
      would pop up, the user would simply delete the commit message to abort
      the rebase at this stage, fix the author information, and continue with
      `git rebase --skip`. (This is a real-world example from the rebase of
      Git for Windows onto v2.19.0-rc1.)
      
      However, there is a bug in `git rebase` that will cause the squash
      message *not* to be forgotten in this case. It will therefore be reused
      in the next fixup/squash chain (if any).
      
      This patch adds a test case to demonstrate this breakage.
      Signed-off-by: Johannes Schindelin's avatarJohannes Schindelin <johannes.schindelin@gmx.de>
      2f3eb68f
    • Jeff King's avatar
      t5310: test delta reuse with bitmaps · c0d61dfc
      Jeff King authored
      Commit 6a1e32d5 (pack-objects: reuse on-disk deltas for
      thin "have" objects, 2018-08-21) taught pack-objects a new
      optimization trick. Since this wasn't meant to change
      user-visible behavior, but only produce smaller packs more
      quickly, testing focused on t/perf/p5311.
      
      However, since people don't run perf tests very often, we
      should make sure that the feature is exercised in the
      regular test suite. This patch does so.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      c0d61dfc
  10. 31 Aug, 2018 4 commits
    • Ævar Arnfjörð Bjarmason's avatar
      fetch: stop clobbering existing tags without --force · 0bc8d71b
      Ævar Arnfjörð Bjarmason authored
      Change "fetch" to treat "+" in refspecs (aka --force) to mean we
      should clobber a local tag of the same name.
      
      This changes the long-standing behavior of "fetch" added in
      853a3697 ("[PATCH] Multi-head fetch.", 2005-08-20). Before this
      change, all tag fetches effectively had --force enabled. See the
      git-fetch-script code in fast_forward_local() with the comment:
      
          > Tags need not be pointing at commits so there is no way to
          > guarantee "fast-forward" anyway.
      
      That commit and the rest of the history of "fetch" shows that the
      "+" (--force) part of refpecs was only conceived for branch updates,
      while tags have accepted any changes from upstream unconditionally and
      clobbered the local tag object. Changing this behavior has been
      discussed as early as 2011[1].
      
      The current behavior doesn't make sense to me, it easily results in
      local tags accidentally being clobbered. We could namespace our tags
      per-remote and not locally populate refs/tags/*, but as with my
      97716d21 ("fetch: add a --prune-tags option and fetch.pruneTags
      config", 2018-02-09) it's easier to work around the current
      implementation than to fix the root cause.
      
      So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
      "fetch" now only clobbers the tag if either "+" is provided as part of
      the refspec, or if "--force" is provided on the command-line.
      
      This also makes it nicely symmetrical with how "tag" itself works when
      creating tags. I.e. we refuse to clobber any existing tags unless
      "--force" is supplied. Now we can refuse all such clobbering, whether
      it would happen by clobbering a local tag with "tag", or by fetching
      it from the remote with "fetch".
      
      Ref updates outside refs/{tags,heads/* are still still not symmetrical
      with how "git push" works, as discussed in the recently changed
      pull-fetch-param.txt documentation. This change brings the two
      divergent behaviors more into line with one another. I don't think
      there's any reason "fetch" couldn't fully converge with the behavior
      used by "push", but that's a topic for another change.
      
      One of the tests added in 31b808a0 ("clone --single: limit the fetch
      refspec to fetched branch", 2012-09-20) is being changed to use
      --force where a clone would clobber a tag. This changes nothing about
      the existing behavior of the test.
      
      1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/Signed-off-by: Ævar Arnfjörð Bjarmason's avatarÆvar Arnfjörð Bjarmason <avarab@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      0bc8d71b
    • Ævar Arnfjörð Bjarmason's avatar
      fetch tests: add a test for clobbering tag behavior · 6b0b0677
      Ævar Arnfjörð Bjarmason authored
      The test suite only incidentally (and unintentionally) tested for the
      current behavior of eager tag clobbering on "fetch". This is a
      followup to 380efb65 ("push tests: assert re-pushing annotated
      tags", 2018-07-31) which tests for it explicitly.
      Signed-off-by: Ævar Arnfjörð Bjarmason's avatarÆvar Arnfjörð Bjarmason <avarab@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      6b0b0677
    • Ævar Arnfjörð Bjarmason's avatar
      push tests: use spaces in interpolated string · 253b3d4f
      Ævar Arnfjörð Bjarmason authored
      The quoted -m'msg' option would mean the same as -mmsg when passed
      through the test_force_push_tag helper. Let's instead use a string
      with spaces in it, to have a working example in case we need to pass
      other whitespace-delimited arguments to git-tag.
      Signed-off-by: Ævar Arnfjörð Bjarmason's avatarÆvar Arnfjörð Bjarmason <avarab@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      253b3d4f
    • Ævar Arnfjörð Bjarmason's avatar
      push tests: make use of unused $1 in test description · f08fb8df
      Ævar Arnfjörð Bjarmason authored
      Fix up a logic error in 380efb65 ("push tests: assert re-pushing
      annotated tags", 2018-07-31), where the $tag_type_description variable
      was assigned to but never used, unlike in the subsequently added
      companion test for fetches in 2d216a7ef6 ("fetch tests: add a test for
      clobbering tag behavior", 2018-04-29).
      Signed-off-by: Ævar Arnfjörð Bjarmason's avatarÆvar Arnfjörð Bjarmason <avarab@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      f08fb8df
  11. 30 Aug, 2018 14 commits
    • Jeff King's avatar
      t5303: use printf to generate delta bases · 18f60f2d
      Jeff King authored
      The exact byte count of the delta base file is important.
      The test-delta helper will feed it to patch_delta(), which
      will barf if it doesn't match the size byte given in the
      delta. Using "echo" may end up with unexpected line endings
      on some platforms (e.g,. "\r\n" instead of just "\n").
      
      This actually wouldn't cause the test to fail (since we
      already expect test-delta to complain about these bogus
      deltas), but would mean that we're not exercising the code
      we think we are.
      
      Let's use printf instead (which we already trust to give us
      byte-perfect output when we generate the deltas).
      
      While we're here, let's tighten the 5-byte result size used
      in the "truncated copy parameters" test. This just needs to
      have enough room to attempt to parse the bogus copy command,
      meaning 2 is sufficient. Using 5 was arbitrary and just
      copied from the base size; since those no longer match, it's
      simply confusing. Let's use a more meaningful number.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      18f60f2d
    • Jeff King's avatar
      patch-delta: handle truncated copy parameters · 9514b0b2
      Jeff King authored
      When we see a delta command instructing us to copy bytes
      from the base, we have to read the offset and size from the
      delta stream. We do this without checking whether we're at
      the end of the stream, meaning we may read past the end of
      the buffer.
      
      In practice this isn't exploitable in any interesting way
      because:
      
        1. Deltas are always in packfiles, so we have at least a
           20-byte trailer that we'll end up reading.
      
        2. The worst case is that we try to perform a nonsense
           copy from the base object into the result, based on
           whatever was in the pack stream next. In most cases
           this will simply fail due to our bounds-checks against
           the base or the result.
      
           But even if you carefully constructed a pack stream for
           which it succeeds, it wouldn't perform any delta
           operation that you couldn't have simply included in a
           non-broken form.
      
      But obviously it's poor form to read past the end of the
      buffer we've been given. Unfortunately there's no easy way
      to do a single length check, since the number of bytes we
      need depends on the number of bits set in the initial
      command byte. So we'll just check each byte as we parse. We
      can hide the complexity in a macro; it's ugly, but not as
      ugly as writing out each individual conditional.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Reviewed-by: Nicolas Pitre's avatarNicolas Pitre <nico@fluxnic.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      9514b0b2
    • Jann Horn's avatar
      patch-delta: consistently report corruption · fa72f90e
      Jann Horn authored
      When applying a delta, if we see an opcode that cannot be
      fulfilled (e.g., asking to write more bytes than the
      destination has left), we break out of our parsing loop but
      don't signal an explicit error. We rely on the sanity check
      after the loop to see if we have leftover delta bytes or
      didn't fill our result buffer.
      
      This can silently ignore corruption when the delta buffer
      ends with a bogus command and the destination buffer is
      already full. Instead, let's jump into the error handler
      directly when we see this case.
      
      Note that the tests also cover the "bad opcode" case, which
      already handles this correctly.
      Signed-off-by: default avatarJann Horn <jannh@google.com>
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Reviewed-by: Nicolas Pitre's avatarNicolas Pitre <nico@fluxnic.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      fa72f90e
    • Jann Horn's avatar
      patch-delta: fix oob read · 21870efc
      Jann Horn authored
      If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
      `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
      into `dst_buf`.
      
      This is not an exploitable bug because triggering the bug increments the
      `data` pointer beyond `top`, causing the `data != top` sanity check after
      the loop to trigger and discard the destination buffer - which means that
      the result of the out-of-bounds read is never used for anything.
      Signed-off-by: default avatarJann Horn <jannh@google.com>
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Reviewed-by: Nicolas Pitre's avatarNicolas Pitre <nico@fluxnic.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      21870efc
    • Jeff King's avatar
      t5303: test some corrupt deltas · 9caf0107
      Jeff King authored
      We don't have any tests that specifically check boundary
      cases in patch_delta(). It obviously gets exercised by tests
      which read from packfiles, but it's hard to create packfiles
      with bogus deltas.
      
      So let's cover some obvious boundary cases:
      
        1. commands that overflow the result buffer
      
           a. literal content from the delta
      
           b. copies from a base
      
        2. commands where the source isn't large enough
      
           a. literal content from a truncated delta
      
           b. copies that need more bytes than the base has
      
        3. copy commands who parameters are truncated
      
      And indeed, we have problems with both 2a and 3. I've marked
      these both as expect_failure, though note that because they
      involve reading past the end of a buffer, they will
      typically only be caught when run under valgrind or ASan.
      
      There's one more test here, too, which just applies a basic
      delta. Since all of the other tests expect failure and we
      don't otherwise use "test-tool delta" in the test suite,
      this gives a sanity check that the tool works at all.
      
      These are based on an earlier patch by Jann Horn
      <jannh@google.com>.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Reviewed-by: Nicolas Pitre's avatarNicolas Pitre <nico@fluxnic.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      9caf0107
    • Jeff King's avatar
      test-delta: read input into a heap buffer · d65930c5
      Jeff King authored
      We currently read the input to test-delta by mmap()-ing it.
      However, memory-checking tools like valgrind and ASan are
      less able to detect reads/writes past the end of an mmap'd
      buffer, because the OS is likely to give us extra bytes to
      pad out the final page size. So instead, let's read into a
      heap buffer.
      
      As a bonus, this also makes it possible to write tests with
      empty bases, as mmap() will complain about a zero-length
      map.
      
      This is based on a patch by Jann Horn <jannh@google.com>
      which actually aligned the data at the end of a page, and
      followed it with another page marked with mprotect(). That
      would detect problems even without a tool like ASan, but it
      was significantly more complex and may have introduced
      portability problems. By comparison, this approach pushes
      the complexity onto existing memory-checking tools.
      
      Note that this could be done even more simply by using
      strbuf_read_file(), but that would defeat the purpose:
      strbufs generally overallocate (and at the very least
      include a trailing NUL which we do not care about), which
      would defeat most memory checkers.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Reviewed-by: Nicolas Pitre's avatarNicolas Pitre <nico@fluxnic.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      d65930c5
    • Eric Sunshine's avatar
      worktree: delete .git/worktrees if empty after 'remove' · 3a540433
      Eric Sunshine authored
      For cleanliness, "git worktree prune" deletes the .git/worktrees
      directory if it is empty after pruning is complete.
      
      For consistency, make "git worktree remove <path>" likewise delete
      .git/worktrees if it is empty after the removal.
      Signed-off-by: Eric Sunshine's avatarEric Sunshine <sunshine@sunshineco.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      3a540433
    • Eric Sunshine's avatar
      worktree: teach 'remove' to override lock when --force given twice · f4143101
      Eric Sunshine authored
      For consistency with "add -f -f" and "move -f -f" which override
      the lock on a worktree, allow "remove -f -f" to do so, as well, as a
      convenience.
      Signed-off-by: Eric Sunshine's avatarEric Sunshine <sunshine@sunshineco.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      f4143101
    • Eric Sunshine's avatar
      worktree: teach 'move' to override lock when --force given twice · 68a6b3a1
      Eric Sunshine authored
      For consistency with "add -f -f", which allows a missing but locked
      worktree path to be re-used, allow "move -f -f" to override a lock,
      as well, as a convenience.
      Signed-off-by: Eric Sunshine's avatarEric Sunshine <sunshine@sunshineco.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      68a6b3a1
    • Eric Sunshine's avatar
      worktree: teach 'add' to respect --force for registered but missing path · e19831c9
      Eric Sunshine authored
      For safety, "git worktree add <path>" will refuse to add a new
      worktree at <path> if <path> is already associated with a worktree
      entry, even if <path> is missing (for instance, has been deleted or
      resides on non-mounted removable media or network share). The typical
      way to re-create a worktree at <path> in such a situation is either to
      prune all "broken" entries ("git worktree prune") or to selectively
      remove the worktree entry manually ("git worktree remove <path>").
      
      However, neither of these approaches ("prune" nor "remove") is
      especially convenient, and they may be unsuitable for scripting when a
      tool merely wants to re-use a worktree if it exists or create it from
      scratch if it doesn't (much as a tool might use "mkdir -p" to re-use
      or create a directory).
      
      Therefore, teach 'add' to respect --force as a convenient way to
      re-use a path already associated with a worktree entry if the path is
      non-existent. For a locked worktree, require --force to be specified
      twice.
      Signed-off-by: Eric Sunshine's avatarEric Sunshine <sunshine@sunshineco.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      e19831c9
    • Eric Sunshine's avatar
      worktree: disallow adding same path multiple times · cb56f55c
      Eric Sunshine authored
      A given path should only ever be associated with a single registered
      worktree. This invariant is enforced by refusing to create a new
      worktree at a given path if that path already exists. For example:
      
          $ git worktree add -q --detach foo
          $ git worktree add -q --detach foo
          fatal: 'foo' already exists
      
      However, the check can be fooled, and the invariant broken, if the
      path is missing. Continuing the example:
      
          $ rm -fr foo
          $ git worktree add -q --detach foo
          $ git worktree list
          ...      eadebfe [master]
          .../foo  eadebfe (detached HEAD)
          .../foo  eadebfe (detached HEAD)
      
      This "corruption" leads to the unfortunate situation in which the
      worktree can not be removed:
      
          $ git worktree remove foo
          fatal: validation failed, cannot remove working tree: '.../foo'
          does not point back to '.git/worktrees/foo'
      
      Nor can the bogus entry be pruned:
      
          $ git worktree prune -v
          $ git worktree list
          ...      eadebfe [master]
          .../foo  eadebfe (detached HEAD)
          .../foo  eadebfe (detached HEAD)
      
      without first deleting the worktree directory manually:
      
          $ rm -fr foo
          $ git worktree prune -v
          Removing .../foo: gitdir file points to non-existent location
          Removing .../foo1: gitdir file points to non-existent location
          $ git worktree list
          ...  eadebfe [master]
      
      or by manually deleting the worktree entry in .git/worktrees.
      
      To address this problem, upgrade "git worktree add" validation to
      allow worktree creation only if the given path is not already
      associated with an existing worktree (even if the path itself is
      non-existent), thus preventing such bogus worktree entries from being
      created in the first place.
      Reported-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: Eric Sunshine's avatarEric Sunshine <sunshine@sunshineco.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      cb56f55c
    • Eric Sunshine's avatar
      worktree: don't die() in library function find_worktree() · 4c5fa9e6
      Eric Sunshine authored
      Callers don't expect library function find_worktree() to die(); they
      expect it to return the named worktree if found, or NULL if not.
      Although find_worktree() itself never invokes die(), it calls
      real_pathdup() with 'die_on_error' incorrectly set to 'true', thus will
      die() indirectly if the user-provided path is not to real_pathdup()'s
      liking. This can be observed, for instance, with any git-worktree
      command which searches for an existing worktree:
      
          $ git worktree unlock foo
          fatal: 'foo' is not a working tree
          $ git worktree unlock foo/bar
          fatal: Invalid path '.../foo': No such file or directory
      
      The first error message is the expected one from "git worktree unlock"
      not finding the specified worktree; the second is from find_worktree()
      invoking real_pathdup() incorrectly and die()ing prematurely.
      
      Aside from the inconsistent error message between the two cases, this
      bug hasn't otherwise been a serious problem since existing callers all
      die() anyhow when the worktree can't be found. However, that may not be
      true of callers added in the future, so fix find_worktree() to avoid
      die()ing.
      Signed-off-by: Eric Sunshine's avatarEric Sunshine <sunshine@sunshineco.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      4c5fa9e6
    • Elijah Newren's avatar
      am: avoid directory rename detection when calling recursive merge machinery · 6aba117d
      Elijah Newren authored
      Let's say you have the following three trees, where Base is from one commit
      behind either master or branch:
      
         Base  : bar_v1, foo/{file1, file2, file3}
         branch: bar_v2, foo/{file1, file2},       goo/file3
         master: bar_v3, foo/{file1, file2, file3}
      
      Using git-am (or am-based rebase) to apply the changes from branch onto
      master results in the following tree:
      
         Result: bar_merged, goo/{file1, file2, file3}
      
      This is not what users want; they did not rename foo/ -> goo/, they only
      renamed one file within that directory.  The reason this happens is am
      constructs fake trees (via build_fake_ancestor()) of the following form:
      
         Base_bfa  : bar_v1, foo/file3
         branch_bfa: bar_v2, goo/file3
      
      Combining these two trees with master's tree:
      
         master: bar_v3, foo/{file1, file2, file3},
      
      You can see that merge_recursive_generic() would see branch_bfa as renaming
      foo/ -> goo/, and master as just adding both foo/file1 and foo/file2.  As
      such, it ends up with goo/{file1, file2, file3}
      
      The core problem is that am does not have access to the original trees; it
      can only construct trees using the blobs involved in the patch.  As such,
      it is not safe to perform directory rename detection within am -3.
      Signed-off-by: Elijah Newren's avatarElijah Newren <newren@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      6aba117d
    • Elijah Newren's avatar
      t3401: add another directory rename testcase for rebase and am · e7588c96
      Elijah Newren authored
      Similar to commit 16346883 ("t3401: add directory rename testcases for
      rebase and am", 2018-06-27), add another testcase for directory rename
      detection.  This new testcase differs in that it showcases a situation
      where no directory rename was performed, but which some backends
      incorrectly detect.
      
      As with the other testcase, run this in conjunction with each of the
      types of rebases:
        git-rebase--interactive
        git-rebase--am
        git-rebase--merge
      and also use the same testcase for
        git am --3way
      Reported-by: corristo's avatarNikolay Kasyanov <corrmage@gmail.com>
      Signed-off-by: Elijah Newren's avatarElijah Newren <newren@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      e7588c96
  12. 29 Aug, 2018 1 commit
    • René Scharfe's avatar
      mailinfo: support format=flowed · 3aa4d81f
      René Scharfe authored
      Add best-effort support for patches sent using format=flowed (RFC 3676).
      Remove leading spaces ("unstuff"), remove soft line breaks (indicated
      by space + newline), but leave the signature separator (dash dash space
      newline) alone.
      
      Warn in git am when encountering a format=flowed patch, because any
      trailing spaces would most probably be lost, as the sending MUA is
      encouraged to remove them when preparing the email.
      
      Provide a test patch formatted by Mozilla Thunderbird 60 using its
      default configuration.  It reuses the contents of the file mailinfo.c
      before and after this patch.
      Signed-off-by: default avatarRene Scharfe <l.s.r@web.de>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      3aa4d81f