What's cooking in git.git (Jun 2016, #05; Thu, 16)

What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

Let's start a new cycle by rewinding the tip of 'next' soonish.  I
expect I'd eject a few premature topics out of 'next' while doing
so.  There are many topics that need input from the list (look for
'?' in this document) to decide either to drop them or to move them
forward.

You can find the changes described here in the integration branches
of the repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

--------------------------------------------------
[New Topics]

* ap/git-svn-propset-doc (2016-06-15) 1 commit
 - git-svn: document the 'git svn propset' command

 "git svn propset" subcommand that was added in 2.3 days is
 documented now.

 Will merge to 'next'.


* jk/add-i-diff-compact-heuristics (2016-06-16) 1 commit
 - add--interactive: respect diff.compactionHeuristic

 "git add -i/-p" learned to honor diff.compactionHeuristic
 experimental knob, so that the user can work on the same hunk split
 as "git diff" output.

 Will merge to 'next'.


* jk/big-and-old-archive-tar (2016-06-16) 2 commits
 - archive-tar: write extended headers for far-future mtime
 - archive-tar: write extended headers for file sizes >= 8GB

 "git archive" learned to handle files that are larger than 8GB and
 commits far in the future than expressible by the traditional US-TAR
 format.

 Will merge to 'next'.


* jk/gpg-interface-cleanup (2016-06-16) 7 commits
 - gpg-interface: check gpg signature creation status
 - sign_buffer: use pipe_command
 - verify_signed_buffer: use pipe_command
 - run-command: add pipe_command helper
 - verify_signed_buffer: use tempfile object
 - verify_signed_buffer: drop pbuf variable
 - gpg-interface: use child_process.args

 A new run-command API function pipe_command() is introduced to
 sanely feed data to the standard input while capturing data from
 the standard output and the standard error of an external process,
 which is cumbersome to hand-roll correctly without deadlocking.

 The codepath to sign data in a prepared buffer with GPG has been
 updated to use this API to read from the status-fd to check for
 errors (instead of relying on GPG's exit status).

 Will merge to 'next'.


* lf/sideband-returns-void (2016-06-16) 2 commits
 - upload-pack.c: make send_client_data() return void
 - sideband.c: make send_sideband() return void

 A small internal API cleanup.

 Will merge to 'next'.


* nd/graph-width-padded (2016-06-16) 2 commits
 - pretty.c: support <direction>|(<negative number>) forms
 - pretty: pass graph width to pretty formatting for use in '%>|(N)'

 "log --graph --format=" learned that "%>|(N)" specifies the width
 relative to the terminal's left edge, not relative to the area to
 draw text that is to the right of the ancestry-graph section.  It
 also now accepts negative N that means the column limit is relative
 to the right border.

 Will merge to 'next'.


* dn/gpg-doc (2016-06-16) 1 commit
 - Documentation: GPG capitalization

 The documentation tries to consistently spell "GPG"; when
 referring to the specific program name, "gpg" is used.

 Will merge to 'next'.


* jk/bisect-show-tree (2016-06-16) 1 commit
 - bisect: always call setup_revisions after init_revisions

 "git bisect" makes an internal call to "git diff-tree" when
 bisection finds the culprit, but this call did not initialize the
 data structure to pass to the diff-tree API correctly.

 Will merge to 'next'.

--------------------------------------------------
[Stalled]

* mj/log-show-signature-conf (2016-06-06) 2 commits
 - log: "--no-show-signature" commmand-line option
 - log: add "log.showsignature" configuration variable

 "git log" learns log.showSignature configuration variable, and a
 command line option "--no-show-signature" to countermand it.

 The order of the commits in the topic need to be reversed.
 Expecting a reroll.


* sb/bisect (2016-04-15) 22 commits
 - SQUASH???
 - bisect: get back halfway shortcut
 - bisect: compute best bisection in compute_relevant_weights()
 - bisect: use a bottom-up traversal to find relevant weights
 - bisect: prepare for different algorithms based on find_all
 - bisect: rename count_distance() to compute_weight()
 - bisect: make total number of commits global
 - bisect: introduce distance_direction()
 - bisect: extract get_distance() function from code duplication
 - bisect: use commit instead of commit list as arguments when appropriate
 - bisect: replace clear_distance() by unique markers
 - bisect: use struct node_data array instead of int array
 - bisect: get rid of recursion in count_distance()
 - bisect: make algorithm behavior independent of DEBUG_BISECT
 - bisect: make bisect compile if DEBUG_BISECT is set
 - bisect: plug the biggest memory leak
 - bisect: add test for the bisect algorithm
 - t6030: generalize test to not rely on current implementation
 - t: use test_cmp_rev() where appropriate
 - t/test-lib-functions.sh: generalize test_cmp_rev
 - bisect: allow 'bisect run' if no good commit is known
 - bisect: write about `bisect next` in documentation

 The internal algorithm used in "git bisect" to find the next commit
 to check has been optimized greatly.

 Expecting a reroll.
 ($gmane/291163)


* sg/completion-updates (2016-02-28) 21 commits
 . completion: cache the path to the repository
 . completion: extract repository discovery from __gitdir()
 . completion: don't guard git executions with __gitdir()
 . completion: consolidate silencing errors from git commands
 . completion: don't use __gitdir() for git commands
 . completion: respect 'git -C <path>'
 . completion: fix completion after 'git -C <path>'
 . completion: don't offer commands when 'git --opt' needs an argument
 . rev-parse: add '--absolute-git-dir' option
 . completion: list short refs from a remote given as a URL
 . completion: don't list 'HEAD' when trying refs completion outside of a repo
 . completion: list refs from remote when remote's name matches a directory
 . completion: respect 'git --git-dir=<path>' when listing remote refs
 . completion: fix most spots not respecting 'git --git-dir=<path>'
 . completion: ensure that the repository path given on the command line exists
 . completion tests: add tests for the __git_refs() helper function
 . completion tests: check __gitdir()'s output in the error cases
 . completion tests: consolidate getting path of current working directory
 . completion tests: make the $cur variable local to the test helper functions
 . completion tests: don't add test cruft to the test repository
 . completion: improve __git_refs()'s in-code documentation

 Will be rerolled.
 ($gmane/287839)


* nd/icase (2016-02-15) 12 commits
 - grep.c: reuse "icase" variable
 - diffcore-pickaxe: support case insensitive match on non-ascii
 - diffcore-pickaxe: "share" regex error handling code
 - grep/pcre: support utf-8
 - gettext: add is_utf8_locale()
 - grep/pcre: prepare locale-dependent tables for icase matching
 - grep/icase: avoid kwsset when -F is specified
 - grep/icase: avoid kwsset on literal non-ascii strings
 - test-regex: expose full regcomp() to the command line
 - test-regex: isolate the bug test code
 - grep: break down an "if" stmt in preparation for next changes
 - grep: allow -F -i combination

 "git grep -i" has been taught to fold case in non-ascii locales.

 This really needs review.  What it attempts to achieve is
 worthwhile, I would think ($gmane/286137).


* ec/annotate-deleted (2015-11-20) 1 commit
 - annotate: skip checking working tree if a revision is provided

 Usability fix for annotate-specific "<file> <rev>" syntax with deleted
 files.

 Waiting for review.


* dg/subtree-rebase-test (2016-01-19) 1 commit
 - contrib/subtree: Add a test for subtree rebase that loses commits

 Reviewed up to v5.
 Will be rerolled.
 ($gmane/284426)


* dk/gc-more-wo-pack (2016-01-13) 4 commits
 - gc: clean garbage .bitmap files from pack dir
 - t5304: ensure non-garbage files are not deleted
 - t5304: test .bitmap garbage files
 - prepare_packed_git(): find more garbage

 Follow-on to dk/gc-idx-wo-pack topic, to clean up stale
 .bitmap and .keep files.

 Waiting for a reroll.
 ($gmane/284368).


* jc/diff-b-m (2015-02-23) 5 commits
 . WIPWIP
 . WIP: diff-b-m
 - diffcore-rename: allow easier debugging
 - diffcore-rename.c: add locate_rename_src()
 - diffcore-break: allow debugging

 "git diff -B -M" produced incorrect patch when the postimage of a
 completely rewritten file is similar to the preimage of a removed
 file; such a resulting file must not be expressed as a rename from
 other place.

 The fix in this patch is broken, unfortunately.
 Will discard.

--------------------------------------------------
[Cooking]

* lv/status-say-working-tree-not-directory (2016-06-09) 1 commit
 - Use "working tree" instead of "working directory" for git status

 "git status" used to say "working directory" when it meant "working
 tree".

 Will merge to 'next'.


* jk/parseopt-string-list (2016-06-13) 3 commits
 - blame,shortlog: don't make local option variables static
 - interpret-trailers: don't duplicate option strings
 - parse_opt_string_list: stop allocating new strings
 (this branch is used by jk/string-list-static-init.)

 The command line argument parsing that uses OPT_STRING_LIST() often
 made a copy of the argv[] element, which was unnecessary.

 Will merge to 'next'.


* jk/repack-keep-unreachable (2016-06-14) 3 commits
 - repack: extend --keep-unreachable to loose objects
 - repack: add --keep-unreachable option
 - repack: document --unpack-unreachable option

 "git repack" learned the "--keep-unreachable" option, which sends
 loose unreachable objects to a pack instead of leaving them loose.
 This helps heuristics based on the number of loose objects
 (e.g. "gc --auto").

 Will merge to 'next'.


* lf/recv-sideband-cleanup (2016-06-13) 1 commit
 - sideband.c: refactor recv_sideband()

 Code simplification.  It however loses the atomicity of the output
 9ac13ec9 (atomic write for sideband remote messages, 2006-10-11)
 tried to add to an once-much-simpler codebase.

 Expecting a reroll.


* nd/test-lib-httpd-show-error-log-in-verbose (2016-06-13) 1 commit
 - lib-httpd.sh: print error.log on error

 Debugging aid.

 Will merge to 'next'.


* pc/occurred (2016-06-10) 2 commits
 - config.c: fix misspelt "occurred" in an error message
 - refs.h: fix misspelt "occurred" in a comment

 Will merge to 'next'.


* sb/submodule-clone-retry (2016-06-13) 2 commits
 - submodule update: continue when a clone fails
 - submodule--helper: initial clone learns retry logic
 (this branch uses sb/submodule-recommend-shallowness.)

 "git submodule update" that drives many "git clone" could
 eventually hit flaky servers/network conditions on one of the
 submodules; the command learned to retry the attempt.

 Will merge to 'next'.


* jc/blame-reverse (2016-06-14) 2 commits
 - blame: dwim "blame --reverse OLD" as "blame --reverse OLD.."
 - blame: improve diagnosis for "--reverse NEW"

 It is a common mistake to say "git blame --reverse OLD path",
 expecting that the command line is dwimmed as if asking how lines
 in path in an old revision OLD have survived up to the current
 commit.

 Any supporters?  Otherwise will drop.


* jc/deref-tag (2016-06-14) 1 commit
 - blame, line-log: do not loop around deref_tag()

 Code clean-up.

 Will merge to 'next'.


* jk/fetch-prune-doc (2016-06-14) 1 commit
 - fetch: document that pruning happens before fetching

 Will merge to 'next'.


* km/fetch-do-not-free-remote-name (2016-06-14) 1 commit
 - builtin/fetch.c: don't free remote->name after fetch

 Will merge to 'next'.


* nb/gnome-keyring-build (2016-06-14) 1 commit
 - gnome-keyring: Don't hard-code pkg-config executable

 Build improvements for gnome-keyring (in contrib/)

 Will merge to 'next'.


* pb/strbuf-read-file-doc (2016-06-14) 1 commit
 - strbuf: describe the return value of strbuf_read_file

 Will merge to 'next'.


* nd/shallow-deepen (2016-06-13) 27 commits
 - fetch, upload-pack: --deepen=N extends shallow boundary by N commits
 - upload-pack: add get_reachable_list()
 - upload-pack: split check_unreachable() in two, prep for get_reachable_list()
 - t5500, t5539: tests for shallow depth excluding a ref
 - clone: define shallow clone boundary with --shallow-exclude
 - fetch: define shallow boundary with --shallow-exclude
 - upload-pack: support define shallow boundary by excluding revisions
 - refs: add expand_ref()
 - t5500, t5539: tests for shallow depth since a specific date
 - clone: define shallow clone boundary based on time with --shallow-since
 - fetch: define shallow boundary with --shallow-since
 - upload-pack: add deepen-since to cut shallow repos based on time
 - shallow.c: implement a generic shallow boundary finder based on rev-list
 - fetch-pack: use a separate flag for fetch in deepening mode
 - fetch-pack.c: mark strings for translating
 - fetch-pack: use a common function for verbose printing
 - fetch-pack: use skip_prefix() instead of starts_with()
 - upload-pack: move rev-list code out of check_non_tip()
 - upload-pack: make check_non_tip() clean things up on error
 - upload-pack: tighten number parsing at "deepen" lines
 - upload-pack: use skip_prefix() instead of starts_with()
 - upload-pack: move "unshallow" sending code out of deepen()
 - upload-pack: remove unused variable "backup"
 - upload-pack: move "shallow" sending code out of deepen()
 - upload-pack: move shallow deepen code out of receive_needs()
 - transport-helper.c: refactor set_helper_option()
 - remote-curl.c: convert fetch_git() to use argv_array

 The existing "git fetch --depth=<n>" option was hard to use
 correctly when making the history of an existing shallow clone
 deeper.  A new option, "--deepen=<n>", has been added to make this
 easier to use.  "git clone" also learned "--shallow-since=<date>"
 and "--shallow-exclude=<tag>" options to make it easier to specify
 "I am interested only in the recent N months worth of history" and
 "Give me only the history since that version".

 Rerolled.
 Needs review.  What this topic attempts to achieve is worthwhile, I
 would think.


* jk/avoid-unbounded-alloca (2016-06-07) 1 commit
 - tree-diff: avoid alloca for large allocations

 Will merge to 'next'.


* jk/send-pack-stdio (2016-06-10) 2 commits
 - write_or_die: remove the unused write_or_whine() function
 - send-pack: use buffered I/O to talk to pack-objects

 Code clean-up.

 Will merge to 'next'.


* pb/commit-editmsg-path (2016-06-09) 1 commit
 - builtin/commit.c: memoize git-path for COMMIT_EDITMSG

 Code clean-up.

 Will merge to 'next'.


* wd/userdiff-css (2016-06-03) 1 commit
  (merged to 'next' on 2016-06-06 at 536102f)
 + userdiff: add built-in pattern for CSS

 Update the funcname definition to support css files.

 Will merge to 'master'.


* jc/attr-more (2016-06-09) 8 commits
 - attr.c: outline the future plans by heavily commenting
 - attr.c: always pass check[] to collect_some_attrs()
 - attr.c: introduce empty_attr_check_elems()
 - attr.c: correct ugly hack for git_all_attrs()
 - attr.c: rename a local variable check
 - fixup! d5ad6c13
 - attr.c: pass struct git_attr_check down the callchain
 - attr.c: add push_stack() helper
 (this branch uses jc/attr; is tangled with sb/pathspec-label and sb/submodule-default-paths.)

 The beginning of long and tortuous journey to clean-up attribute
 subsystem implementation.

 Needs to be redone.


* jk/rev-list-count-with-bitmap (2016-06-03) 2 commits
  (merged to 'next' on 2016-06-06 at dd9b30f)
 + rev-list: disable bitmaps when "-n" is used with listing objects
 + rev-list: "adjust" results of "--count --use-bitmap-index -n"

 "git rev-list --count" whose walk-length is limited with "-n"
 option did not work well with the counting optimized to look at the
 bitmap index.

 Will merge to 'master'.


* mh/ref-iterators (2016-06-03) 13 commits
  (merged to 'next' on 2016-06-06 at c8e79dc)
 + for_each_reflog(): reimplement using iterators
 + dir_iterator: new API for iterating over a directory tree
 + for_each_reflog(): don't abort for bad references
 + do_for_each_ref(): reimplement using reference iteration
 + refs: introduce an iterator interface
 + ref_resolves_to_object(): new function
 + entry_resolves_to_object(): rename function from ref_resolves_to_object()
 + get_ref_cache(): only create an instance if there is a submodule
 + remote rm: handle symbolic refs correctly
 + delete_refs(): add a flags argument
 + refs: use name "prefix" consistently
 + do_for_each_ref(): move docstring to the header file
 + refs: remove unnecessary "extern" keywords
 (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled with mh/update-ref-errors.)

 The API to iterate over all the refs (i.e. for_each_ref(), etc.)
 has been revamped.

 Will merge to 'master'.


* ew/mboxrd-format-am (2016-06-06) 3 commits
 - am: support --patch-format=mboxrd
 - mailsplit: support unescaping mboxrd messages
 - pretty: support "mboxrd" output format

 Teach format-patch and mailsplit (hence "am") how a line that
 happens to begin with "From " in the e-mail message is quoted with
 ">", so that these lines can be restored to their original shape.

 Will merge to 'next'.


* lf/receive-pack-auto-gc-to-client (2016-06-06) 1 commit
 - receive-pack: send auto-gc output over sideband 2

 Allow messages that are generated by auto gc during "git push" on
 the receiving end to be explicitly passed back to the sending end
 over sideband, so that they are shown with "remote: " prefix to
 avoid confusing the users.

 Will merge to 'next'.


* mg/cherry-pick-multi-on-unborn (2016-06-06) 1 commit
 - cherry-pick: allow to pick to unborn branches

 "git cherry-pick A" worked on an unborn branch, but "git
 cherry-pick A..B" didn't.

 Will merge to 'next'.


* sg/reflog-past-root (2016-06-06) 1 commit
 - reflog: continue walking the reflog past root commits

 "git reflog" stopped upon seeing an entry that denotes a branch
 creation event (aka "unborn"), which made it appear as if the
 reflog was truncated.

 Will merge to 'next'.


* tb/complete-status (2016-06-10) 3 commits
 - completion: add git status
 - completion: add __git_get_option_value helper
 - completion: factor out untracked file modes into a variable

 The completion script (in contrib/) learned to complete "git
 status" options.

 Any further comments?  Otherwise will merge to 'next'.


* tr/doc-tt (2016-06-08) 4 commits
 - doc: change configuration variables format
 - doc: more consistency in environment variables format
 - doc: change environment variables format
 - doc: clearer rule about formatting literals

 The documentation set has been updated so that literal commands,
 configuration variables and environment variables are consistently
 typeset in fixed-width font and bold in manpages.

 Will merge to 'next'.


* vs/prompt-avoid-unset-variable (2016-06-06) 1 commit
 - git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION, $short_sha

 The git-prompt scriptlet (in contrib/) was not friendly with those
 who uses "set -u", which has been fixed.

 Will merge to 'next'.


* rj/compat-regex-size-max-fix (2016-06-06) 1 commit
 - regex: fix a SIZE_MAX macro redefinition warning

 A compilation fix.

 Will merge to 'next'.


* et/add-chmod-x (2016-06-07) 1 commit
 - add: add --chmod=+x / --chmod=-x options

 "git update-index --add --chmod=+x file" may be usable as an escape
 hatch, but not a friendly thing to force for people who do need to
 use it regularly.  "git add --chmod=+x file" can be used instead.

 Will merge to 'next'.


* mh/connect (2016-06-06) 10 commits
 - connect: [host:port] is legacy for ssh
 - connect: move ssh command line preparation to a separate function
 - connect: actively reject git:// urls with a user part
 - connect: change the --diag-url output to separate user and host
 - connect: make parse_connect_url() return the user part of the url as a separate value
 - connect: group CONNECT_DIAG_URL handling code
 - connect: make parse_connect_url() return separated host and port
 - connect: re-derive a host:port string from the separate host and port variables
 - connect: call get_host_and_port() earlier
 - connect: document why we sometimes call get_port after get_host_and_port

 Rewrite Git-URL parsing routine (hopefully) without changing any
 behaviour.

 Will merge to 'next'???


* aq/upload-pack-use-parse-options (2016-05-31) 1 commit
  (merged to 'next' on 2016-06-06 at 505f1ee)
 + upload-pack.c: use parse-options API

 "git upload-pack" command has been updated to use the parse-options
 API.

 Will merge to 'master'.


* jc/clear-pathspec (2016-06-02) 1 commit
  (merged to 'next' on 2016-06-06 at 9e7e291)
 + pathspec: rename free_pathspec() to clear_pathspec()

 We usually call a function that clears the contents a data
 structure X without freeing the structure itself clear_X(), and
 call a function that does clear_X() and also frees it free_X().
 free_pathspec() function has been renamed to clear_pathspec()
 to avoid confusion.

 Will merge to 'master'.


* sb/submodule-recommend-shallowness (2016-05-27) 2 commits
  (merged to 'next' on 2016-05-31 at 1ee161c)
 + submodule update: learn `--[no-]recommend-shallow` option
 + submodule-config: keep shallow recommendation around
 (this branch is used by sb/submodule-clone-retry.)

 An upstream project can make a recommendation to make only a
 shallow clone for some submodules in the .gitmodules file it ship.

 Will merge to 'master'.


* tb/convert-peek-in-index (2016-06-07) 3 commits
 - correct ce_compare_data() in a middle of a merge
 - read-cache: factor out get_sha1_from_index() helper
 - convert: unify the "auto" handling of CRLF

 Needs review.


* va/i18n-even-more (2016-06-07) 38 commits
 - i18n: branch: mark comment when editing branch description for translation
 - i18n: unmark die messages for translation
 - i18n: submodule: escape shell variables inside eval_gettext
 - i18n: submodule: join strings marked for translation
 - i18n: init-db: join message pieces
 - i18n: remote: allow translations to reorder message
 - i18n: remote: mark URL fallback text for translation
 - i18n: standardise messages
 - i18n: sequencer: add period to error message
 - i18n: merge: change command option help to lowercase
 - i18n: merge: mark messages for translation
 - i18n: notes: mark options for translation
 - i18n: notes: mark strings for translation
 - i18n: transport-helper.c: change N_() call to _()
 - i18n: bisect: mark strings for translation
 - t5523: use test_i18ngrep for negation
 - t4153: fix negated test_i18ngrep call
 - t9003: become resilient to GETTEXT_POISON
 - tests: unpack-trees: update to use test_i18n* functions
 - tests: use test_i18n* functions to suppress false positives
 - i18n: setup: mark strings for translation
 - i18n: rebase-interactive: mark comments of squash for translation
 - i18n: rebase-interactive: mark here-doc strings for translation
 - i18n: rebase-interactive: mark strings for translation
 - i18n: git-sh-setup.sh: mark strings for translation
 - t6030: update to use test_i18ncmp
 - i18n: bisect: simplify error message for i18n
 - i18n: rebase: mark placeholder for translation
 - i18n: rebase: fix marked string to use eval_gettext variant
 - merge-octupus: use die shell function from git-sh-setup.sh
 - i18n: merge-octopus: mark messages for translation
 - i18n: sequencer: mark string for translation
 - i18n: sequencer: mark entire sentences for translation
 - i18n: transport: mark strings for translation
 - i18n: advice: internationalize message for conflicts
 - i18n: advice: mark string about detached head for translation
 - i18n: builtin/remote.c: fix mark for translation
 - Merge branch 'jc/t2300-setup' into HEAD

 More markings of messages for i18n, with updates to various tests
 to pass GETTEXT_POISON tests.

 One patch from the original submission dropped due to conflicts
 with other topics in flight.

 Will merge to 'next'.


* jg/dash-is-last-branch-in-worktree-add (2016-05-31) 1 commit
  (merged to 'next' on 2016-06-02 at 3959ef6)
 + worktree: allow "-" short-hand for @{-1} in add command

 "git worktree add" learned that '-' can be used as a short-hand for
 "@{-1}", the previous branch.

 Will merge to 'master'.


* dk/blame-move-no-reason-for-1-line-context (2016-05-29) 1 commit
 - blame: require 0 context lines while finding moved lines with -M

 "git blame -M" missed a single line that was moved within the file.

 We may want to squash a test or two to this commit.  Volunteers?


* nd/worktree-lock (2016-06-13) 6 commits
 - worktree.c: find_worktree() search by path suffix
 - worktree: add "unlock" command
 - worktree: add "lock" command
 - worktree.c: add is_worktree_locked()
 - worktree.c: add is_main_worktree()
 - worktree.c: add find_worktree()
 (this branch uses nd/worktree-cleanup-post-head-protection.)

 "git worktree prune" protected worktrees that are marked as
 "locked" by creating a file in a known location.  "git worktree"
 command learned a dedicated command pair to create and remoev such
 a file, so that the users do not have to do this with editor.

 Is everybody happy with this version?
 If so, will merge to 'next'.


* et/pretty-format-c-auto (2016-05-27) 1 commit
  (merged to 'next' on 2016-05-31 at 1e9c920)
 + format_commit_message: honor `color=auto` for `%C(auto)`

 %C(auto) in a custom format string that commands in `git log`
 family takes unconditionally turned the color on, ignoring
 --no-color or --color=auto with output not connected to a tty;
 this was corrected to make the format truly behave as "auto".

 Will merge to 'master'.


* ew/daemon-socket-keepalive (2016-05-25) 1 commit
  (merged to 'next' on 2016-05-31 at c32acf1)
 + daemon: enable SO_KEEPALIVE for all sockets

 When "git daemon" is run without --[init-]timeout specified, a
 connection from a client that silently goes offline can hang around
 for a long time, wasting resources.  The socket-level KEEPALIVE has
 been enabled to allow the OS to notice such failed connections.

 Will merge to 'master'.


* jk/upload-pack-hook (2016-06-02) 7 commits
 - upload-pack: provide a hook for running pack-objects
 - t1308: do not get fooled by symbolic links to the source tree
 - config: add a notion of "scope"
 - config: return configset value for current_config_ functions
 - config: set up config_source for command-line config
 - git_config_parse_parameter: refactor cleanup code
 - git_config_with_options: drop "found" counting

 Allow a custom "git upload-pack" replacement to respond to
 "fetch/clone" request.

 Still under discussion???
 ($gmane/295705).


* rs/xdiff-hunk-with-func-line (2016-06-09) 9 commits
  (merged to 'next' on 2016-06-10 at 9ff9ba8)
 + xdiff: fix merging of appended hunk with -W
  (merged to 'next' on 2016-06-02 at 0c2e335)
 + grep: -W: don't extend context to trailing empty lines
 + t7810: add test for grep -W and trailing empty context lines
 + xdiff: don't trim common tail with -W
 + xdiff: -W: don't include common trailing empty lines in context
 + xdiff: ignore empty lines before added functions with -W
 + xdiff: handle appended chunks better with -W
 + xdiff: factor out match_func_rec()
 + t4051: rewrite, add more tests

 "git show -W" (extend hunks to cover the entire function, delimited
 by lines that match the "funcname" pattern) used to show the entire
 file when a change added an entire function at the end of the file,
 which has been fixed.

 Will merge to 'master'.


* sb/submodule-misc-cleanups (2016-05-25) 1 commit
  (merged to 'next' on 2016-05-31 at 0d07b9c)
 + submodule update: make use of the existing fetch_in_submodule function

 Minor simplification.

 Will merge to 'master'.


* sb/submodule-default-paths (2016-06-14) 8 commits
 - completion: clone can recurse into submodules
 - clone: add --init-submodule=<pathspec> switch
 - submodule update: add `--init-default-path` switch
 - Merge branch 'sb/pathspec-label' into sb/submodule-default-paths
 - Merge branch 'jc/attr' into sb/submodule-default-paths
 - Merge branch 'sb/clone-shallow-passthru' into sb/submodule-default-paths
 - Merge branch 'sb/submodule-deinit-all' into sb/submodule-default-paths
 - Merge branch 'sb/submodule-parallel-update' into sb/submodule-default-paths
 (this branch uses jc/attr and sb/pathspec-label; is tangled with jc/attr-more.)

 Will hold.


* ah/no-verify-signature-with-pull-rebase (2016-05-20) 1 commit
  (merged to 'next' on 2016-05-31 at 30add83)
 + pull: warn on --verify-signatures with --rebase

 "git pull --rebase --verify-signature" learned to warn the user
 that "--verify-signature" is a no-op.

 Will merge to 'master'.


* ep/http-curl-trace (2016-05-24) 2 commits
 - imap-send.c: introduce the GIT_TRACE_CURL enviroment variable
 - http.c: implement the GIT_TRACE_CURL environment variable

 HTTP transport gained an option to produce more detailed debugging
 trace.

 Will merge to 'next'.


* jc/attr (2016-05-25) 18 commits
  (merged to 'next' on 2016-05-31 at 5b2f08b)
 + attr: support quoting pathname patterns in C style
 + attr: expose validity check for attribute names
 + attr: add counted string version of git_attr()
 + attr: add counted string version of git_check_attr()
 + attr: retire git_check_attrs() API
 + attr: convert git_check_attrs() callers to use the new API
 + attr: convert git_all_attrs() to use "struct git_attr_check"
 + attr: (re)introduce git_check_attr() and struct git_attr_check
 + attr: rename function and struct related to checking attributes
 + attr.c: plug small leak in parse_attr_line()
 + attr.c: tighten constness around "git_attr" structure
 + attr.c: simplify macroexpand_one()
 + attr.c: mark where #if DEBUG ends more clearly
 + attr.c: complete a sentence in a comment
 + attr.c: explain the lack of attr-name syntax check in parse_attr()
 + attr.c: update a stale comment on "struct match_attr"
 + attr.c: use strchrnul() to scan for one line
 + commit.c: use strchrnul() to scan for one line
 (this branch is used by jc/attr-more, sb/pathspec-label and sb/submodule-default-paths.)

 The attributes API has been updated so that it can later be
 optimized using the knowledge of which attributes are queried.

 Will eject from 'next'.
 The updated API needs further rethinking.


* cc/apply-introduce-state (2016-06-06) 50 commits
  (merged to 'next' on 2016-06-06 at 9f6bdcb)
 + builtin/apply: remove misleading comment on lock_file field
  (merged to 'next' on 2016-06-03 at 1ab0cf9)
 + builtin/apply: move 'newfd' global into 'struct apply_state'
 + builtin/apply: add 'lock_file' pointer into 'struct apply_state'
 + builtin/apply: move applying patches into apply_all_patches()
 + builtin/apply: move 'state' check into check_apply_state()
 + builtin/apply: move 'symlink_changes' global into 'struct apply_state'
 + builtin/apply: move 'fn_table' global into 'struct apply_state'
 + builtin/apply: move 'state_linenr' global into 'struct apply_state'
 + builtin/apply: move 'max_change' and 'max_len' into 'struct apply_state'
 + builtin/apply: move 'ws_ignore_action' into 'struct apply_state'
 + builtin/apply: move 'ws_error_action' into 'struct apply_state'
 + builtin/apply: move 'applied_after_fixing_ws' into 'struct apply_state'
 + builtin/apply: move 'squelch_whitespace_errors' into 'struct apply_state'
 + builtin/apply: remove whitespace_option arg from set_default_whitespace_mode()
 + builtin/apply: move 'whitespace_option' into 'struct apply_state'
 + builtin/apply: move 'whitespace_error' global into 'struct apply_state'
 + builtin/apply: move 'root' global into 'struct apply_state'
 + builtin/apply: move 'p_value_known' global into 'struct apply_state'
 + builtin/apply: move 'p_value' global into 'struct apply_state'
 + builtin/apply: move 'has_include' global into 'struct apply_state'
 + builtin/apply: move 'limit_by_name' global into 'struct apply_state'
 + builtin/apply: move 'patch_input_file' global into 'struct apply_state'
 + builtin/apply: move 'apply' global into 'struct apply_state'
 + builtin/apply: move 'p_context' global into 'struct apply_state'
 + builtin/apply: move 'fake_ancestor' global into 'struct apply_state'
 + builtin/apply: move 'line_termination' global into 'struct apply_state'
 + builtin/apply: move 'unsafe_paths' global into 'struct apply_state'
 + builtin/apply: move 'no_add' global into 'struct apply_state'
 + builtin/apply: move 'threeway' global into 'struct apply_state'
 + builtin/apply: move 'summary' global into 'struct apply_state'
 + builtin/apply: move 'numstat' global into 'struct apply_state'
 + builtin/apply: move 'diffstat' global into 'struct apply_state'
 + builtin/apply: move 'cached' global into 'struct apply_state'
 + builtin/apply: move 'allow_overlap' global into 'struct apply_state'
 + builtin/apply: move 'update_index' global into 'struct apply_state'
 + builtin/apply: move 'apply_verbosely' global into 'struct apply_state'
 + builtin/apply: move 'apply_with_reject' global into 'struct apply_state'
 + builtin/apply: move 'apply_in_reverse' global into 'struct apply_state'
 + builtin/apply: move 'check_index' global into 'struct apply_state'
 + builtin/apply: move 'check' global into 'struct apply_state'
 + builtin/apply: move 'unidiff_zero' global into 'struct apply_state'
 + builtin/apply: move 'state' init into init_apply_state()
 + builtin/apply: introduce 'struct apply_state' to start libifying
 + builtin/apply: move 'read_stdin' global into cmd_apply()
 + builtin/apply: move 'options' variable into cmd_apply()
 + builtin/apply: extract line_by_line_fuzzy_match() from match_fragment()
 + builtin/apply: avoid local variable shadowing 'len' parameter
 + builtin/apply: avoid parameter shadowing 'linenr' global
 + builtin/apply: avoid parameter shadowing 'p_value' global
 + builtin/apply: make gitdiff_verify_name() return void

 The "git apply" standalone program is being libified; this is the
 first step to move many state variables into a structure that can
 be explicitly (re)initialized to make the machinery callable more
 than once.

 The next step that moves some remaining state variables into the
 structure and turns die()s into an error return that propagates up
 to the caller is not queued yet but in flight.  It would be good to
 review the above first and give the remainder of the series a solid
 base to build on.

 Will hold.


* pb/bisect (2016-05-24) 3 commits
 - bisect--helper: `write_terms` shell function in C
 - bisect: rewrite `check_term_format` shell function in C
 - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

 Beginning of GSoC "git bisect" project.

 I know another topic is getting rerolled many times on top of this;
 are people happy with these three patches?  If so, will merge to
 'next'.


* sb/pathspec-label (2016-06-03) 6 commits
  (merged to 'next' on 2016-06-03 at 362f097)
 + pathspec: disable preload-index when attribute pathspec magic is in use
 + pathspec: allow escaped query values
  (merged to 'next' on 2016-05-31 at e72b604)
 + pathspec: allow querying for attributes
 + pathspec: move prefix check out of the inner loop
 + pathspec: move long magic parsing out of prefix_pathspec
 + Documentation: fix a typo
 (this branch is used by sb/submodule-default-paths; uses jc/attr; is tangled with jc/attr-more.)

 The pathspec mechanism learned ":(attr:X)$pattern" pathspec magic
 to limit paths that match $pattern further by attribute settings.
 The preload-index mechanism is disabled when the new pathspec magic
 is in use (at least for now), because the attribute subsystem is
 not thread-ready.

 Will eject from 'next'.
 The jc/attr topic that this depends on needs to be redone.


* nd/worktree-cleanup-post-head-protection (2016-05-24) 6 commits
 - worktree: simplify prefixing paths
 - worktree: avoid 0{40}, too many zeroes, hard to read
 - worktree.c: use is_dot_or_dotdot()
 - git-worktree.txt: keep subcommand listing in alphabetical order
 - worktree.c: rewrite mark_current_worktree() to avoid strbuf
 - completion: support git-worktree
 (this branch is used by nd/worktree-lock.)

 Further preparatory clean-up for "worktree" feature.

 Will merge to 'next'.


* mh/split-under-lock (2016-05-13) 33 commits
  (merged to 'next' on 2016-06-03 at 2e71330)
 + lock_ref_sha1_basic(): only handle REF_NODEREF mode
 + commit_ref_update(): remove the flags parameter
 + lock_ref_for_update(): don't resolve symrefs
 + lock_ref_for_update(): don't re-read non-symbolic references
 + refs: resolve symbolic refs first
 + ref_transaction_update(): check refname_is_safe() at a minimum
 + unlock_ref(): move definition higher in the file
 + lock_ref_for_update(): new function
 + add_update(): initialize the whole ref_update
 + verify_refname_available(): adjust constness in declaration
 + refs: don't dereference on rename
 + refs: allow log-only updates
 + delete_branches(): use resolve_refdup()
 + ref_transaction_commit(): correctly report close_ref() failure
 + ref_transaction_create(): disallow recursive pruning
 + refs: make error messages more consistent
 + lock_ref_sha1_basic(): remove unneeded local variable
 + read_raw_ref(): move docstring to header file
 + read_raw_ref(): improve docstring
 + read_raw_ref(): rename symref argument to referent
 + read_raw_ref(): clear *type at start of function
 + read_raw_ref(): rename flags argument to type
 + ref_transaction_commit(): remove local variable n
 + rename_ref(): remove unneeded local variable
 + commit_ref_update(): write error message to *err, not stderr
 + refname_is_safe(): insist that the refname already be normalized
 + refname_is_safe(): don't allow the empty string
 + refname_is_safe(): use skip_prefix()
 + remove_dir_recursively(): add docstring
 + safe_create_leading_directories(): improve docstring
 + read_raw_ref(): don't get confused by an empty directory
 + commit_ref(): if there is an empty dir in the way, delete it
 + t1404: demonstrate a bug resolving references
 (this branch is used by mh/ref-iterators, mh/ref-store and mh/update-ref-errors.)

 Further preparatory work on the refs API before the pluggable
 backend series can land.

 Will merge to 'master'.


* ew/fast-import-unpack-limit (2016-05-29) 2 commits
  (merged to 'next' on 2016-05-29 at af32aba)
 + fast-import: invalidate pack_id references after loosening
  (merged to 'next' on 2016-05-11 at ffd4efb)
 + fast-import: implement unpack limit

 "git fast-import" learned the same performance trick to avoid
 creating too small a packfile as "git fetch" and "git push" have,
 using *.unpackLimit configuration.

 Will merge to 'master'.


* jc/send-email-skip-backup (2016-04-12) 1 commit
 - send-email: detect and offer to skip backup files

 A careless invocation of "git send-email directory/" after editing
 0001-change.patch with an editor often ends up sending both
 0001-change.patch and its backup file, 0001-change.patch~, causing
 embarrassment and a minor confusion.  Detect such an input and
 offer to skip the backup files when sending the patches out.

 Will merge to 'next'.
 Perhaps people will enhance it more once it gains more visibility.


* kn/ref-filter-branch-list (2016-05-17) 17 commits
 - branch: implement '--format' option
 - branch: use ref-filter printing APIs
 - branch, tag: use porcelain output
 - ref-filter: allow porcelain to translate messages in the output
 - ref-filter: add `:dir` and `:base` options for ref printing atoms
 - ref-filter: make remote_ref_atom_parser() use refname_atom_parser_internal()
 - ref-filter: introduce symref_atom_parser() and refname_atom_parser()
 - ref-filter: introduce refname_atom_parser_internal()
 - ref-filter: make "%(symref)" atom work with the ':short' modifier
 - ref-filter: add support for %(upstream:track,nobracket)
 - ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
 - ref-filter: introduce format_ref_array_item()
 - ref-filter: move get_head_description() from branch.c
 - ref-filter: modify "%(objectname:short)" to take length
 - ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
 - ref-filter: include reference to 'used_atom' within 'atom_value'
 - ref-filter: implement %(if), %(then), and %(else) atoms

 The code to list branches in "git branch" has been consolidated
 with the more generic ref-filter API.

 Rerolled.
 This also really needs review.


* dt/index-helper (2016-05-20) 20 commits
 - index-helper: indexhelper.exitafter config
 - trace: measure where the time is spent in the index-heavy operations
 - index-helper: optionally automatically run
 - index-helper: autorun mode
 - index-helper: don't run if already running
 - index-helper: kill mode
 - watchman: add a config option to enable the extension
 - unpack-trees: preserve index extensions
 - update-index: enable/disable watchman support
 - index-helper: use watchman to avoid refreshing index with lstat()
 - watchman: support watchman to reduce index refresh cost
 - read-cache: add watchman 'WAMA' extension
 - index-helper: log warnings
 - index-helper: add --detach
 - daemonize(): set a flag before exiting the main process
 - index-helper: add --strict
 - index-helper: new daemon for caching index and related stuff
 - pkt-line: add gentle version of packet_write
 - read-cache: allow to keep mmap'd memory after reading
 - read-cache.c: fix constness of verify_hdr()

 A new "index-helper" daemon has been introduced to give newly
 spawned Git process a quicker access to the data in the index, and
 optionally interface with the watchman daemon to further reduce the
 refresh cost.

 Are people happy with this version?  If so will merge to 'next'.
 ($gmane/295106).


* jc/bundle (2016-03-03) 6 commits
 - index-pack: --clone-bundle option
 - Merge branch 'jc/index-pack' into jc/bundle
 - bundle v3: the beginning
 - bundle: keep a copy of bundle file name in the in-core bundle header
 - bundle: plug resource leak
 - bundle doc: 'verify' is not about verifying the bundle

 The beginning of "split bundle", which could be one of the
 ingredients to allow "git clone" traffic off of the core server
 network to CDN.

 While I think it would make it easier for people to experiment and
 build on if the topic is merged to 'next', I am at the same time a
 bit reluctant to merge an unproven new topic that introduces a new
 file format, which we may end up having to support til the end of
 time.  Comments?


* jc/merge-drop-old-syntax (2015-04-29) 1 commit
 - merge: drop 'git merge <message> HEAD <commit>' syntax

 Stop supporting "git merge <message> HEAD <commit>" syntax that has
 been deprecated since October 2007, and issues a deprecation
 warning message since v2.5.0.

 It has been reported that git-gui still uses the deprecated syntax,
 which needs to be fixed before this final step can proceed.
 ($gmane/282594)

--------------------------------------------------
[Discarded]

* js/am-3-merge-recursive-direct (2015-10-12) 2 commits
 . am: make a direct call to merge_recursive
 . merge_recursive_options: introduce the "gently" flag

 The merge_recursive_generic() function has been made a bit safer to
 call from inside a process.  "git am -3" was taught to make a direct
 call to the function when falling back to three-way merge.

 Being able to make a direct call would be good in general, but as a
 performance thing, the change needs to be backed up by numbers.

 I haven't gone through the "gently" change with fine toothed comb;
 I can see that the change avoids calling die(), but I haven't made
 sure that the program states (e.g. what's in the in-core index) are
 adjusted sensibly when it returns to the caller instead of dying,
 or the codepaths that used to die() are free of resource leaks.
 The original code certainly did not care the program states at the
 point of dying exactly because it knew it is going to exit, but now
 they have to care, and they need to be audited.

 Has a lot of conflict when merged to 'pu' these days, so let's
 eject it from our tree for now.

 Will discard.
 ($gmane/292205)


* nd/i-t-a-commitable (2016-06-06) 3 commits
 . commit: don't count i-t-a entries when checking if the new commit is empty
 . Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
 . diff.h: extend "flags" field to 64 bits because we're out of bits

 "rm .git/index && git add -N * && git commit" allows you to create
 an empty commit without --allow-empty; attempt to forbid it.

 Retracted.
 Will discard.
 ($gmane/297362)


* jc/merge-impossible-no-commit (2016-04-26) 2 commits
 . merge: warn --no-commit merge when no new commit is created
 . merge: do not contaminate option_commit with --squash

 "git merge --no-commit" silently succeeded when there is no need to
 create any commit, either when you are more recent than the commit
 you tried to merge, or you can fast-forward to the commit you tried
 to merge.  The command gives a warning message in such cases.

 Just tying loose ends in a discussion.  Unless somebody else
 champions this topic, I'll drop it.

 Will discard.


* bc/cocci-object-id (2016-06-06) 8 commits
 . merge-recursive: convert merge_recursive_generic to object_id
 . merge-recursive: convert leaf functions to use struct object_id
 . merge-recursive: convert struct merge_file_info to object_id
 . merge-recursive: convert struct stage_data to use object_id
 . Rename struct diff_filespec's sha1_valid member.
 . Convert struct diff_filespec to struct object_id
 . Apply standard object_id Coccinelle transformations.
 . Add basic Coccinelle transforms.

 Move from "unsigned char [20]" to "struct object_id" continues,
 with help from an automated tool.

 Will discard.
 ($gmane/296749)


* az/p4-bare-no-rebase (2016-02-19) 1 commit
 - git-p4.py: Don't try to rebase on submit from bare repository

 "git p4 submit" attempts to do a rebase, which would fail if done
 in a bare repository.  Not doing this rebase would paper over the
 failure, which is what this patch does, but it is unclear what the
 side effect of not rebasing is.

 Retracted.
 ($gmane/296722)

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Pranit Bauva]

Hey Junio,

On Fri, Jun 17, 2016 at 8:50 AM, Junio C Hamano <gitster@pobox.com> wrote:

> * pb/bisect (2016-05-24) 3 commits
>  - bisect--helper: `write_terms` shell function in C
>  - bisect: rewrite `check_term_format` shell function in C
>  - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
>
>  Beginning of GSoC "git bisect" project.
>
>  I know another topic is getting rerolled many times on top of this;
>  are people happy with these three patches?  If so, will merge to
>  'next'.


This matches the last re-roll[1] I did which looks good to me. Thanks!
Though I was under the impression that the whole topic would be merged
to next after the completion of the GSoC project because the test
suite seems lacking at a few places (pointed out by Eric). So I had
intended to do the coverage tests after the completion so that I could
use the gcov rather than setting up a coverage tool for shell scripts.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/295518

Regards,
Pranit Bauva

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Vasco Almeida]

Junio C Hamano <gitster <at> pobox.com> writes:
> * va/i18n-even-more (2016-06-07) 38 commits
>  - i18n: branch: mark comment when editing branch description for translation
>  - i18n: unmark die messages for translation
>  - i18n: submodule: escape shell variables inside eval_gettext
>  - i18n: submodule: join strings marked for translation
>  - i18n: init-db: join message pieces
[snip]
>  - i18n: builtin/remote.c: fix mark for translation
>  - Merge branch 'jc/t2300-setup' into HEAD

Patch "i18n: init-db: join message pieces" breaks test
t0204-gettext-reencode-sanity.sh because it changes message id used in
Icelandic translation used on that test.
Please, don't merge this series, I'm going to send a re-roll (v5).

Discussion: http://mid.gmane.org/1562644.BnkjqA6nsN@linux-omuo

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
> 
> ...
> 
> * mh/split-under-lock (2016-05-13) 33 commits
>  (merged to 'next' on 2016-06-03 at 2e71330)
> + lock_ref_sha1_basic(): only handle REF_NODEREF mode
> + commit_ref_update(): remove the flags parameter
> + lock_ref_for_update(): don't resolve symrefs
> + lock_ref_for_update(): don't re-read non-symbolic references
> + refs: resolve symbolic refs first
> + ref_transaction_update(): check refname_is_safe() at a minimum
> + unlock_ref(): move definition higher in the file
> + lock_ref_for_update(): new function
> + add_update(): initialize the whole ref_update
> + verify_refname_available(): adjust constness in declaration
> + refs: don't dereference on rename
> + refs: allow log-only updates
> + delete_branches(): use resolve_refdup()
> + ref_transaction_commit(): correctly report close_ref() failure
> + ref_transaction_create(): disallow recursive pruning
> + refs: make error messages more consistent
> + lock_ref_sha1_basic(): remove unneeded local variable
> + read_raw_ref(): move docstring to header file
> + read_raw_ref(): improve docstring
> + read_raw_ref(): rename symref argument to referent
> + read_raw_ref(): clear *type at start of function
> + read_raw_ref(): rename flags argument to type
> + ref_transaction_commit(): remove local variable n
> + rename_ref(): remove unneeded local variable
> + commit_ref_update(): write error message to *err, not stderr
> + refname_is_safe(): insist that the refname already be normalized
> + refname_is_safe(): don't allow the empty string
> + refname_is_safe(): use skip_prefix()
> + remove_dir_recursively(): add docstring
> + safe_create_leading_directories(): improve docstring
> + read_raw_ref(): don't get confused by an empty directory
> + commit_ref(): if there is an empty dir in the way, delete it
> + t1404: demonstrate a bug resolving references
> (this branch is used by mh/ref-iterators, mh/ref-store and mh/update-ref-errors.)
> 
> Further preparatory work on the refs API before the pluggable
> backend series can land.
> 
> Will merge to 'master'.

This topic seems break two git-p4 tests (t9801 and t9803) on next:
https://travis-ci.org/git/git/jobs/137333785

According to git bisect the commit "ref_transaction_update(): 
check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
(scroll all the way down to see the bisecting)

- Lars

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> ...
>> 
>> * mh/split-under-lock (2016-05-13) 33 commits
>>  (merged to 'next' on 2016-06-03 at 2e71330)
>> + lock_ref_sha1_basic(): only handle REF_NODEREF mode
>> + ...
>> + t1404: demonstrate a bug resolving references
>> (this branch is used by mh/ref-iterators, mh/ref-store and mh/update-ref-errors.)
>> 
>> Further preparatory work on the refs API before the pluggable
>> backend series can land.
>> 
>> Will merge to 'master'.
>
> This topic seems break two git-p4 tests (t9801 and t9803) on next:
> https://travis-ci.org/git/git/jobs/137333785
>
> According to git bisect the commit "ref_transaction_update(): 
> check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
> (scroll all the way down to see the bisecting)

Thanks.  Let's hold it and all of its dependents to see what is
going on.

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty]

On 06/17/2016 05:20 AM, Junio C Hamano wrote:
> [...]
> * mh/ref-iterators (2016-06-03) 13 commits
>   (merged to 'next' on 2016-06-06 at c8e79dc)
>  + for_each_reflog(): reimplement using iterators
>  + dir_iterator: new API for iterating over a directory tree
>  + for_each_reflog(): don't abort for bad references
>  + do_for_each_ref(): reimplement using reference iteration
>  + refs: introduce an iterator interface
>  + ref_resolves_to_object(): new function
>  + entry_resolves_to_object(): rename function from ref_resolves_to_object()
>  + get_ref_cache(): only create an instance if there is a submodule
>  + remote rm: handle symbolic refs correctly
>  + delete_refs(): add a flags argument
>  + refs: use name "prefix" consistently
>  + do_for_each_ref(): move docstring to the header file
>  + refs: remove unnecessary "extern" keywords
>  (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled with mh/update-ref-errors.)
> 
>  The API to iterate over all the refs (i.e. for_each_ref(), etc.)
>  has been revamped.
> 
>  Will merge to 'master'.

It would be preferable (though not critical) to use the promised v3,
which I just sent [1]. This includes some minor improvements, described
here [2]. This is also available from my GitHub fork [3] as branch
"ref-iterators".

> * mh/split-under-lock (2016-05-13) 33 commits
>   (merged to 'next' on 2016-06-03 at 2e71330)
>  + lock_ref_sha1_basic(): only handle REF_NODEREF mode
>  + commit_ref_update(): remove the flags parameter
>  + lock_ref_for_update(): don't resolve symrefs
>  + lock_ref_for_update(): don't re-read non-symbolic references
>  + refs: resolve symbolic refs first
>  + ref_transaction_update(): check refname_is_safe() at a minimum
>  + unlock_ref(): move definition higher in the file
>  + lock_ref_for_update(): new function
>  + add_update(): initialize the whole ref_update
>  + verify_refname_available(): adjust constness in declaration
>  + refs: don't dereference on rename
>  + refs: allow log-only updates
>  + delete_branches(): use resolve_refdup()
>  + ref_transaction_commit(): correctly report close_ref() failure
>  + ref_transaction_create(): disallow recursive pruning
>  + refs: make error messages more consistent
>  + lock_ref_sha1_basic(): remove unneeded local variable
>  + read_raw_ref(): move docstring to header file
>  + read_raw_ref(): improve docstring
>  + read_raw_ref(): rename symref argument to referent
>  + read_raw_ref(): clear *type at start of function
>  + read_raw_ref(): rename flags argument to type
>  + ref_transaction_commit(): remove local variable n
>  + rename_ref(): remove unneeded local variable
>  + commit_ref_update(): write error message to *err, not stderr
>  + refname_is_safe(): insist that the refname already be normalized
>  + refname_is_safe(): don't allow the empty string
>  + refname_is_safe(): use skip_prefix()
>  + remove_dir_recursively(): add docstring
>  + safe_create_leading_directories(): improve docstring
>  + read_raw_ref(): don't get confused by an empty directory
>  + commit_ref(): if there is an empty dir in the way, delete it
>  + t1404: demonstrate a bug resolving references
>  (this branch is used by mh/ref-iterators, mh/ref-store and mh/update-ref-errors.)
> 
>  Further preparatory work on the refs API before the pluggable
>  backend series can land.
> 
>  Will merge to 'master'.

Please make sure to pick up the important bugfix discussed here [4],
which is integrated into branch "split-under-lock" on my GitHub fork [3].

Michael

[1] http://thread.gmane.org/gmane.comp.version-control.git/297625
[2]
http://thread.gmane.org/gmane.comp.version-control.git/296322/focus=296883
[3] https://github.com/mhagger/git
[4] http://article.gmane.org/gmane.comp.version-control.git/297174

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty]

On 06/18/2016 12:05 AM, Lars Schneider wrote:
> 
>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> ...
>>
>> * mh/split-under-lock (2016-05-13) 33 commits
>>  (merged to 'next' on 2016-06-03 at 2e71330)
>> [...]
> 
> This topic seems break two git-p4 tests (t9801 and t9803) on next:
> https://travis-ci.org/git/git/jobs/137333785
> 
> According to git bisect the commit "ref_transaction_update(): 
> check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
> (scroll all the way down to see the bisecting)

Thanks for the bug report. I'll look into this as soon as I have the chance.

Do you happen to know if there is a way to get a copy of p4 without
paying for it so that I can run the tests locally?

Given the commit that you bisected to, one likely possibility is that
the test is trying to create a reference with an unsafe name, in the
sense of `refname_is_safe()`. Is that possible? Do you happen to know
what Git reference names that test case wants to create?

Michael

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 06/17/2016 05:20 AM, Junio C Hamano wrote:
>> [...]
>> * mh/ref-iterators (2016-06-03) 13 commits
>>   (merged to 'next' on 2016-06-06 at c8e79dc)
>>  + ...
>>  (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled with mh/update-ref-errors.)
>> 
>>  The API to iterate over all the refs (i.e. for_each_ref(), etc.)
>>  has been revamped.
>> 
>>  Will merge to 'master'.
>
> It would be preferable (though not critical) to use the promised v3,
> which I just sent [1]. This includes some minor improvements, described
> here [2]. This is also available from my GitHub fork [3] as branch
> "ref-iterators".
>
>> * mh/split-under-lock (2016-05-13) 33 commits
>>   (merged to 'next' on 2016-06-03 at 2e71330)
>>  + lock_ref_sha1_basic(): only handle REF_NODEREF mode
>>  + ...
>>  Will merge to 'master'.
>
> Please make sure to pick up the important bugfix discussed here [4],
> which is integrated into branch "split-under-lock" on my GitHub fork [3].

Good timing. I was planning to kick split-under-lock and any of its
dependents temporarily out of 'next', so that fixes can choose not
to be incremental, and dependent topics can be rebased on top of the
fixed fondation.  Even if we do incremental, [4] is not sufficient
material for me to write a log message for.

So people who reviewed what has been in 'next' can revisit [4] and
give review comments, while I could just pick up the history
mentioned there, i.e.

    git checkout pu
    git pull git://github.com/mhagger/git +split-under-lock:mh/split-under-lock

and we can start from there?

Thanks.

>
> Michael
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/297625
> [2]
> http://thread.gmane.org/gmane.comp.version-control.git/296322/focus=296883
> [3] https://github.com/mhagger/git
> [4] http://article.gmane.org/gmane.comp.version-control.git/297174

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty]

On 06/18/2016 12:05 AM, Lars Schneider wrote:
> 
>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> ...
>>
>> * mh/split-under-lock (2016-05-13) 33 commits
>>  (merged to 'next' on 2016-06-03 at 2e71330)
>> + lock_ref_sha1_basic(): only handle REF_NODEREF mode
>> + commit_ref_update(): remove the flags parameter
>> + lock_ref_for_update(): don't resolve symrefs
>> + lock_ref_for_update(): don't re-read non-symbolic references
>> + refs: resolve symbolic refs first
>> + ref_transaction_update(): check refname_is_safe() at a minimum
>> + unlock_ref(): move definition higher in the file
>> + lock_ref_for_update(): new function
>> + add_update(): initialize the whole ref_update
>> + verify_refname_available(): adjust constness in declaration
>> + refs: don't dereference on rename
>> + refs: allow log-only updates
>> + delete_branches(): use resolve_refdup()
>> + ref_transaction_commit(): correctly report close_ref() failure
>> + ref_transaction_create(): disallow recursive pruning
>> + refs: make error messages more consistent
>> + lock_ref_sha1_basic(): remove unneeded local variable
>> + read_raw_ref(): move docstring to header file
>> + read_raw_ref(): improve docstring
>> + read_raw_ref(): rename symref argument to referent
>> + read_raw_ref(): clear *type at start of function
>> + read_raw_ref(): rename flags argument to type
>> + ref_transaction_commit(): remove local variable n
>> + rename_ref(): remove unneeded local variable
>> + commit_ref_update(): write error message to *err, not stderr
>> + refname_is_safe(): insist that the refname already be normalized
>> + refname_is_safe(): don't allow the empty string
>> + refname_is_safe(): use skip_prefix()
>> + remove_dir_recursively(): add docstring
>> + safe_create_leading_directories(): improve docstring
>> + read_raw_ref(): don't get confused by an empty directory
>> + commit_ref(): if there is an empty dir in the way, delete it
>> + t1404: demonstrate a bug resolving references
>> (this branch is used by mh/ref-iterators, mh/ref-store and mh/update-ref-errors.)
>>
>> Further preparatory work on the refs API before the pluggable
>> backend series can land.
>>
>> Will merge to 'master'.
> 
> This topic seems break two git-p4 tests (t9801 and t9803) on next:
> https://travis-ci.org/git/git/jobs/137333785
> 
> According to git bisect the commit "ref_transaction_update(): 
> check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
> (scroll all the way down to see the bisecting)
> 
> - Lars
> 

Lars,

According to [1], something in that test seems to have been trying to run

    git update-ref -d git-p4-tmp/6

Similarly in the other failed test.

Because `update-ref` doesn't do DWIM for reference names, this is *not*
expanded to `refs/heads/git-p4-tmp/6` or something. Previously this
command would have quietly failed to do anything. But after
"ref_transaction_update(): check refname_is_safe() at a minimum", `git
update-ref` notices that `git/p4/tmp/6` is not a safe refname (according
to `refname_is_safe()` [2]), and correctly fails with an error message.

Even before this change, Git didn't allow such references to be created
or updated. So I think this test failure is revealing an error in `git
p4 clone` that went undetected before this change.

Please let me know whether you agree. If so, it is realistic to fix
`git-p4` promptly? This failure is currently blocking
mh/split-under-lock, so if `git-p4` can't be fixed, then I'd have to
either disable t9801 and t9803 in this patch series, or omit the
`refname_is_safe()` check.

In the interest of backwards compatibility, I considered making `git
update-ref -d` continue to fail silently for NOOP operations with unsafe
refnames (one of the requirements being that no old_oid is specified).
But I think that would be giving the wrong signal to scripts that are
doing something that is invalid but pausible, like trying to delete the
reference `../$(basename $PWD)/refs/heads/foo`. Such scripts would be
misled into thinking the deletion was successful. And yet treating
plausibly-sensible requests differently than obviously bogus requests
seems like a path to madness.

Michael

[1] https://travis-ci.org/git/git/jobs/137333785#L2025-L2026
[2]
https://github.com/mhagger/git/blob/7a418f3a17b95746eb94cfd55f4fe0385d058777/refs.c#L121-L151

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty]

On 06/18/2016 08:20 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> On 06/17/2016 05:20 AM, Junio C Hamano wrote:
>>> [...]
>>> * mh/ref-iterators (2016-06-03) 13 commits
>>>   (merged to 'next' on 2016-06-06 at c8e79dc)
>>>  + ...
>>>  (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled with mh/update-ref-errors.)
>>>
>>>  The API to iterate over all the refs (i.e. for_each_ref(), etc.)
>>>  has been revamped.
>>>
>>>  Will merge to 'master'.
>>
>> It would be preferable (though not critical) to use the promised v3,
>> which I just sent [1]. This includes some minor improvements, described
>> here [2]. This is also available from my GitHub fork [3] as branch
>> "ref-iterators".
>>
>>> * mh/split-under-lock (2016-05-13) 33 commits
>>>   (merged to 'next' on 2016-06-03 at 2e71330)
>>>  + lock_ref_sha1_basic(): only handle REF_NODEREF mode
>>>  + ...
>>>  Will merge to 'master'.
>>
>> Please make sure to pick up the important bugfix discussed here [4],
>> which is integrated into branch "split-under-lock" on my GitHub fork [3].
> 
> Good timing. I was planning to kick split-under-lock and any of its
> dependents temporarily out of 'next', so that fixes can choose not
> to be incremental, and dependent topics can be rebased on top of the
> fixed fondation.  Even if we do incremental, [4] is not sufficient
> material for me to write a log message for.
> 
> So people who reviewed what has been in 'next' can revisit [4] and
> give review comments, while I could just pick up the history
> mentioned there, i.e.
> 
>     git checkout pu
>     git pull git://github.com/mhagger/git +split-under-lock:mh/split-under-lock
> 
> and we can start from there?

Sure. The branches in my GitHub fork already include all of the
improvements and fixes that I know of, and the only outstanding issue is
the one that Lars mentioned in this thread (which I believe to be a
problem in git-p4).

BTW, there are still no conflicts between these branches
(split-under-lock, update-ref-errors, ref-iterators, and ref-store) and
current master. Therefore, I don't see a need to rebase them onto
master. But if you would prefer that I do so, just let me know.

Michael

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 19 Jun 2016, at 09:59, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 
> On 06/18/2016 12:05 AM, Lars Schneider wrote:
>> 
>>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> ...
>>> 
>>> * mh/split-under-lock (2016-05-13) 33 commits
>>> ...
>>> 
>>> Further preparatory work on the refs API before the pluggable
>>> backend series can land.
>>> 
>>> Will merge to 'master'.
>> 
>> This topic seems break two git-p4 tests (t9801 and t9803) on next:
>> https://travis-ci.org/git/git/jobs/137333785
>> 
>> According to git bisect the commit "ref_transaction_update(): 
>> check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
>> https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
>> (scroll all the way down to see the bisecting)
>> 
>> - Lars
>> 
> 
> Lars,
> 
> According to [1], something in that test seems to have been trying to run
> 
>    git update-ref -d git-p4-tmp/6
> 
> Similarly in the other failed test.
> 
> Because `update-ref` doesn't do DWIM for reference names, this is *not*
> expanded to `refs/heads/git-p4-tmp/6` or something. Previously this
> command would have quietly failed to do anything. But after
> "ref_transaction_update(): check refname_is_safe() at a minimum", `git
> update-ref` notices that `git/p4/tmp/6` is not a safe refname (according
> to `refname_is_safe()` [2]), and correctly fails with an error message.

All errors seem to be related to the Git-P4 branch import. I am no expert
in that area because the branch import never worked for me (and I am puzzled
to some extend how it is supposed to work given the differences how branches
work in Git and P4).

This is the offending call:
https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/git-p4.py#L3464

This is only a cleanup call and we could make all tests work if we remove the
cleanup and also the "cleanup successful check":
https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9801-git-p4-branch.sh#L303
https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9801-git-p4-branch.sh#L355

I am a bit surprised that we do not see other errors given the fact 
that the branch name is clearly invalid:
https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9803-git-p4-shell-metachars.sh#L102

I see two ways to proceed:

(1) We remove the cleanup.

(2) We sanitize the branch names (e.g. by removing invalid characters).
@Michael: Is there a function to "sanitize" a given branch name already?

Option 1 is trivial and option 2 (my preference) shouldn't be too hard. 
But maybe Luke has some insights since he added the "branch with shell char" 
test in 52a4880.


> Even before this change, Git didn't allow such references to be created
> or updated. So I think this test failure is revealing an error in `git
> p4 clone` that went undetected before this change.
> 
> Please let me know whether you agree. If so, it is realistic to fix
> `git-p4` promptly? This failure is currently blocking
> mh/split-under-lock, so if `git-p4` can't be fixed, then I'd have to
> either disable t9801 and t9803 in this patch series, or omit the
> `refname_is_safe()` check.
I am looking into option 2.

> 
> In the interest of backwards compatibility, I considered making `git
> update-ref -d` continue to fail silently for NOOP operations with unsafe
> refnames (one of the requirements being that no old_oid is specified).
> But I think that would be giving the wrong signal to scripts that are
> doing something that is invalid but pausible, like trying to delete the
> reference `../$(basename $PWD)/refs/heads/foo`. Such scripts would be
> misled into thinking the deletion was successful. And yet treating
> plausibly-sensible requests differently than obviously bogus requests
> seems like a path to madness.
Agreed!

Cheers,
Lars

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 19 Jun 2016, at 17:04, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
> 
>> On 19 Jun 2016, at 09:59, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> 
>> On 06/18/2016 12:05 AM, Lars Schneider wrote:
>>> 
>>>> On 17 Jun 2016, at 05:20, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 
>>>> ...
>>>> 
>>>> * mh/split-under-lock (2016-05-13) 33 commits
>>>> ...
>>>> 
>>>> Further preparatory work on the refs API before the pluggable
>>>> backend series can land.
>>>> 
>>>> Will merge to 'master'.
>>> 
>>> This topic seems break two git-p4 tests (t9801 and t9803) on next:
>>> https://travis-ci.org/git/git/jobs/137333785
>>> 
>>> According to git bisect the commit "ref_transaction_update(): 
>>> check refname_is_safe() at a minimum" (3da1f3) introduces the problem: 
>>> https://s3.amazonaws.com/archive.travis-ci.org/jobs/138457628/log.txt
>>> (scroll all the way down to see the bisecting)
>>> 
>>> - Lars
>>> 
>> 
>> Lars,
>> 
>> According to [1], something in that test seems to have been trying to run
>> 
>>   git update-ref -d git-p4-tmp/6
>> 
>> Similarly in the other failed test.
>> 
>> Because `update-ref` doesn't do DWIM for reference names, this is *not*
>> expanded to `refs/heads/git-p4-tmp/6` or something. Previously this
>> command would have quietly failed to do anything. But after
>> "ref_transaction_update(): check refname_is_safe() at a minimum", `git
>> update-ref` notices that `git/p4/tmp/6` is not a safe refname (according
>> to `refname_is_safe()` [2]), and correctly fails with an error message.
> 
> All errors seem to be related to the Git-P4 branch import. I am no expert
> in that area because the branch import never worked for me (and I am puzzled
> to some extend how it is supposed to work given the differences how branches
> work in Git and P4).
> 
> This is the offending call:
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/git-p4.py#L3464
> 
> This is only a cleanup call and we could make all tests work if we remove the
> cleanup and also the "cleanup successful check":
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9801-git-p4-branch.sh#L303
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9801-git-p4-branch.sh#L355
> 
> I am a bit surprised that we do not see other errors given the fact 
> that the branch name is clearly invalid:
> https://github.com/git/git/blob/05219a1276341e72d8082d76b7f5ed394b7437a4/t/t9803-git-p4-shell-metachars.sh#L102
> 
> I see two ways to proceed:
> 
> (1) We remove the cleanup.
> 
> (2) We sanitize the branch names (e.g. by removing invalid characters).
> @Michael: Is there a function to "sanitize" a given branch name already?
> 
> Option 1 is trivial and option 2 (my preference) shouldn't be too hard. 
> But maybe Luke has some insights since he added the "branch with shell char" 
> test in 52a4880.
> 
> 
>> Even before this change, Git didn't allow such references to be created
>> or updated. So I think this test failure is revealing an error in `git
>> p4 clone` that went undetected before this change.
>> 
>> Please let me know whether you agree. If so, it is realistic to fix
>> `git-p4` promptly? This failure is currently blocking
>> mh/split-under-lock, so if `git-p4` can't be fixed, then I'd have to
>> either disable t9801 and t9803 in this patch series, or omit the
>> `refname_is_safe()` check.
> I am looking into option 2.

After looking more into it I realized that the character "\$" in the branch 
name is not even the problem. The git-p4 temp refs are just not located
under refs/heads.

This seems to fix the issue:

--- a/git-p4.py
+++ b/git-p4.py
@@ -2274,7 +2274,7 @@ class P4Sync(Command, P4UserMap):
         self.useClientSpec_from_options = False
         self.clientSpecDirs = None
         self.tempBranches = []
-        self.tempBranchLocation = "git-p4-tmp"
+        self.tempBranchLocation = "refs/heads/git-p4-tmp"
         self.largeFileSystem = None
 
         if gitConfig('git-p4.largeFileSystem'):
-- 


@Luke: Would that be an acceptable solution?

Thanks,
Lars


> 
>> 
>> In the interest of backwards compatibility, I considered making `git
>> update-ref -d` continue to fail silently for NOOP operations with unsafe
>> refnames (one of the requirements being that no old_oid is specified).
>> But I think that would be giving the wrong signal to scripts that are
>> doing something that is invalid but pausible, like trying to delete the
>> reference `../$(basename $PWD)/refs/heads/foo`. Such scripts would be
>> misled into thinking the deletion was successful. And yet treating
>> plausibly-sensible requests differently than obviously bogus requests
>> seems like a path to madness.
> Agreed!
> 
> Cheers,
> Lars

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> Good timing. I was planning to kick split-under-lock and any of its
>> dependents temporarily out of 'next', so that fixes can choose not
>> to be incremental, and dependent topics can be rebased on top of the
>> fixed fondation.  Even if we do incremental, [4] is not sufficient
>> material for me to write a log message for.
>> 
>> So people who reviewed what has been in 'next' can revisit [4] and
>> give review comments, while I could just pick up the history
>> mentioned there, i.e.
>> 
>>     git checkout pu
>>     git pull git://github.com/mhagger/git +split-under-lock:mh/split-under-lock
>> 
>> and we can start from there?
>
> Sure. The branches in my GitHub fork already include all of the
> improvements and fixes that I know of, and the only outstanding issue is
> the one that Lars mentioned in this thread (which I believe to be a
> problem in git-p4).
>
> BTW, there are still no conflicts between these branches
> (split-under-lock, update-ref-errors, ref-iterators, and ref-store) and
> current master. Therefore, I don't see a need to rebase them onto
> master. But if you would prefer that I do so, just let me know.

If the updated split-under-lock itself can build on the same
upstream commit, then there is no reason to rebase it on top of v2.9
or anything newer.

Your other topics queued in my tree build on the version of
split-under-lock I have (e.g. mh/ref-store uses ref-iterators and
split-under-lock).  When split-under-lock gets updated, they need to
be rebuilt on top of the updated version, and that is what I meant
by "dependent topics can be rebased on top".

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> According to [1], something in that test seems to have been trying to run
>
>     git update-ref -d git-p4-tmp/6
>
> Similarly in the other failed test.

Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is
not a place to have a ref.  It will not participate in reachability
analysis and will end up losing the referents.

Perhaps placing it under refs/git-p4-tmp would fix it (both in
git-p4 and in tests)?

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

> This seems to fix the issue:
>
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -2274,7 +2274,7 @@ class P4Sync(Command, P4UserMap):
>          self.useClientSpec_from_options = False
>          self.clientSpecDirs = None
>          self.tempBranches = []
> -        self.tempBranchLocation = "git-p4-tmp"
> +        self.tempBranchLocation = "refs/heads/git-p4-tmp"
>          self.largeFileSystem = None
>  
>          if gitConfig('git-p4.largeFileSystem'):

Anywhere in refs/ would be OK, but don't you need to adjust the
test, too?

Even though I do not use git-p4, I'd imagine that I would be upset
if temporary refs that are used only during sync contaminated the
set of local branches I have, if I were a user of git-p4.  Would it
make sense to use "refs/git-p4-tmp" or something instead?

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 19 Jun 2016, at 20:13, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Lars Schneider <larsxschneider@gmail.com> writes:
> 
>> This seems to fix the issue:
>> 
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2274,7 +2274,7 @@ class P4Sync(Command, P4UserMap):
>>         self.useClientSpec_from_options = False
>>         self.clientSpecDirs = None
>>         self.tempBranches = []
>> -        self.tempBranchLocation = "git-p4-tmp"
>> +        self.tempBranchLocation = "refs/heads/git-p4-tmp"
>>         self.largeFileSystem = None
>> 
>>         if gitConfig('git-p4.largeFileSystem'):
> 
> Anywhere in refs/ would be OK, but don't you need to adjust the
> test, too?
> 
> Even though I do not use git-p4, I'd imagine that I would be upset
> if temporary refs that are used only during sync contaminated the
> set of local branches I have, if I were a user of git-p4.  Would it
> make sense to use "refs/git-p4-tmp" or something instead?
Yes, "refs/git-p4-tmp" would work equally well.

- Lars

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 19 Jun 2016, at 20:49, Lars Schneider <larsxschneider@gmail.com> wrote:
> 
> 
>> On 19 Jun 2016, at 20:13, Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Lars Schneider <larsxschneider@gmail.com> writes:
>> 
>>> This seems to fix the issue:
>>> 
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -2274,7 +2274,7 @@ class P4Sync(Command, P4UserMap):
>>>        self.useClientSpec_from_options = False
>>>        self.clientSpecDirs = None
>>>        self.tempBranches = []
>>> -        self.tempBranchLocation = "git-p4-tmp"
>>> +        self.tempBranchLocation = "refs/heads/git-p4-tmp"
>>>        self.largeFileSystem = None
>>> 
>>>        if gitConfig('git-p4.largeFileSystem'):
>> 
>> Anywhere in refs/ would be OK, but don't you need to adjust the
>> test, too?
>> 
>> Even though I do not use git-p4, I'd imagine that I would be upset
>> if temporary refs that are used only during sync contaminated the
>> set of local branches I have, if I were a user of git-p4.  Would it
>> make sense to use "refs/git-p4-tmp" or something instead?
> Yes, "refs/git-p4-tmp" would work equally well.

Plus, you are right. A minor test adjustment is necessary (although
the tests pass without adjustment). I will post a full patch.

- Lars

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> According to [1], something in that test seems to have been trying to run
>>
>>     git update-ref -d git-p4-tmp/6
>>
>> Similarly in the other failed test.
>
> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is
> not a place to have a ref.  It will not participate in reachability
> analysis and will end up losing the referents.
>
> Perhaps placing it under refs/git-p4-tmp would fix it (both in
> git-p4 and in tests)?

Oh, another thing.  If these refs are meant to be transient, they
are likely to be per worktree, if "git worktree" managed multiple
worktrees that share the same set of branches and tags are in use.

I recall we carved out one hierarchy under refs/ as "not shared
across worktrees" (was that refs/worktree/ hierarchy?  I didn't
check but please do so when the patch actually is written), and
that hierarchy is the appropriate thing to use for this, I think.

Thanks.

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Torsten Bögershausen]

There is a conflict in pu:
"jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index"

(And currently pu didn't compile)

(I will hopefully be able to do a separate review of the smudge/clean patch)

(And joeyh@joeyh.name is not reachable from web.de)

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 20 Jun 2016, at 01:51, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> 
>>> According to [1], something in that test seems to have been trying to run
>>> 
>>>    git update-ref -d git-p4-tmp/6
>>> 
>>> Similarly in the other failed test.
>> 
>> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is
>> not a place to have a ref.  It will not participate in reachability
>> analysis and will end up losing the referents.
>> 
>> Perhaps placing it under refs/git-p4-tmp would fix it (both in
>> git-p4 and in tests)?
> 
> Oh, another thing.  If these refs are meant to be transient, they
> are likely to be per worktree, if "git worktree" managed multiple
> worktrees that share the same set of branches and tags are in use.
> 
> I recall we carved out one hierarchy under refs/ as "not shared
> across worktrees" (was that refs/worktree/ hierarchy?  I didn't
> check but please do so when the patch actually is written), and
> that hierarchy is the appropriate thing to use for this, I think.

Thanks for the hint. It looks like as if the "per worktree" decision
is made in "ref.c:466" "is_per_worktree_ref":
https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470

In ce414b3 "refs/bisect" was added to a list of prefixes that are
per worktree. I could easily add "refs/git-p4-tmp" to this list but
I don't think that would be a good solution. I would prefer to go with 
your suggestion and add "refs/worktree" to the prefix list and then use
"refs/worktree/git-p4-tmp".

Later on we could move "refs/bisect" to "refs/worktree/bisect" and
simplify the "is_per_worktree_ref" code, too.

Thoughts?

Thanks,
Lars

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> There is a conflict in pu:
> "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index"
>
> (And currently pu didn't compile)

Thanks for a report, but didn't I mention this breakage in the
What's cooking report already?

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

Torsten Bögershausen wrote:
> There is a conflict in pu:
> "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index"
> 
> (And currently pu didn't compile)

I'm sending a v4 of jh/clean-smudge-annex that is rebased on top of
tb/convert-peek-in-index to fix this.

> (I will hopefully be able to do a separate review of the smudge/clean patch)

Would be appreciated. It'll be 2 weeks until I can work on this any more.

> (And joeyh@joeyh.name is not reachable from web.de)

I'd like to fix whatever's broken; you could send details out of band to
joeyhess@gmail.com

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty]

On 06/20/2016 09:57 AM, Lars Schneider wrote:
> 
>> On 20 Jun 2016, at 01:51, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>
>>>> According to [1], something in that test seems to have been trying to run
>>>>
>>>>    git update-ref -d git-p4-tmp/6
>>>>
>>>> Similarly in the other failed test.
>>>
>>> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is
>>> not a place to have a ref.  It will not participate in reachability
>>> analysis and will end up losing the referents.
>>>
>>> Perhaps placing it under refs/git-p4-tmp would fix it (both in
>>> git-p4 and in tests)?

I have to say, I'm still confused about how the old code could have
worked at all. For quite some time, Git hasn't allowed references with
weird names like `git-p4-tmp/6` to be created, so how could there be any
references there that need to be deleted? It seems to me that one of the
following must be true:

* This feature (i.e., whatever needs to create the temporary
  references) has been broken for a long time, which would imply that
  there are no tests for it.

* The references are created in some bogus way, like writing files
  directly to the filesystem, rather than using `git update-ref`.
  This is broken and will be even more broken soon when we add
  support for different reference storage backends.

* Such references are never actually created and never actually
  needed. This would imply that the cleanup code is unnecessary.

* The temporary references are created using `git branch`, and are
  thus actually named like `refs/heads/git-p4-tmp/6`. In this case
  the deletion code wasn't cleaning them up right, but the feature
  might have otherwise worked correctly.

* I'm wrong about Git not allowing references like `git-p4-tmp/6` to
  be created, in which case I'd like to know what code path allows it
  so that I can fix it.

Any of these possibilities would mean that your proposed fix is wrong or
incomplete.

>> Oh, another thing.  If these refs are meant to be transient, they
>> are likely to be per worktree, if "git worktree" managed multiple
>> worktrees that share the same set of branches and tags are in use.
>>
>> I recall we carved out one hierarchy under refs/ as "not shared
>> across worktrees" (was that refs/worktree/ hierarchy?  I didn't
>> check but please do so when the patch actually is written), and
>> that hierarchy is the appropriate thing to use for this, I think.
> 
> Thanks for the hint. It looks like as if the "per worktree" decision
> is made in "ref.c:466" "is_per_worktree_ref":
> https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470
> 
> In ce414b3 "refs/bisect" was added to a list of prefixes that are
> per worktree. I could easily add "refs/git-p4-tmp" to this list but
> I don't think that would be a good solution. I would prefer to go with 
> your suggestion and add "refs/worktree" to the prefix list and then use
> "refs/worktree/git-p4-tmp".
> 
> Later on we could move "refs/bisect" to "refs/worktree/bisect" and
> simplify the "is_per_worktree_ref" code, too.

The other part of making `refs/bisect` per-worktree is this horrible
hack here [1].

I'd like to request that the change for the p4 temprefs be made in two
steps:

1. Write references to `refs/git-p4-tmp` or whatever, without
   worrying about making them per-worktree.

2. Carve out a per-worktree namespace, bikeshed about its name and
   semantics, make sure it works correctly, then finally move the
   p4 temporary references and eventually the bisect references there.

The reason is that step 1 is low-risk, could be made quickly, and is
enough to unblock mh/split-under-lock and the other patch series that
depend on it. Step 2, on the other hand, could take quite a while, and
its implementation might benefit from changes to reference handling that
are in the pipeline (e.g., perhaps the horrible hack can be dispensed
with). Meanwhile, as far as I understand these references are transient,
so there are no backwards-compatibility considerations that make
renaming them twice more cumbersome than renaming them once.

Thanks,
Michael

[1]
https://github.com/git/git/blob/ab7797dbe95fff38d9265869ea367020046db118/refs/files-backend.c#L176-L192

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Torsten Bögershausen]

On 22/06/16 23:09, Joey Hess wrote:
> Torsten Bögershausen wrote:
>> There is a conflict in pu:
>> "jh/clean-smudge-annex" does not work together with "tb/convert-peek-in-index"
>>
>> (And currently pu didn't compile)
> I'm sending a v4 of jh/clean-smudge-annex that is rebased on top of
> tb/convert-peek-in-index to fix this.
>
>> (I will hopefully be able to do a separate review of the smudge/clean patch)
> Would be appreciated. It'll be 2 weeks until I can work on this any more.
>
>> (And joeyh@joeyh.name is not reachable from web.de)
> I'd like to fix whatever's broken; you could send details out of band to
> joeyhess@gmail.com
>
Currently there is one comment:
The (new) usage of assert() in sha1_file.c:
   assert(would_convert_to_git_filter_fd(path));

The thing is that we need to check the file system to find .gitatttibutes,
even if we just did it 1 nanosecond ago.

So the .gitattributes is done 3 times:
-1 would_convert_to_git_filter_fd(
-2 assert(would_convert_to_git_filter_fd(path));
-3 convert.c/convert_to_git_filter_fd()

The only situation where this could be useful is when the .gitattributes
change between -1 and -2,
but then they would have changed between -1 and -3, and convert.c
will die().

Does it make sense to remove -2 ?

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Lars Schneider]

> On 23 Jun 2016, at 09:32, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 
> On 06/20/2016 09:57 AM, Lars Schneider wrote:
>> 
>>> On 20 Jun 2016, at 01:51, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Junio C Hamano <gitster@pobox.com> writes:
>>> 
>>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>> 
>>>>> According to [1], something in that test seems to have been trying to run
>>>>> 
>>>>>   git update-ref -d git-p4-tmp/6
>>>>> 
>>>>> Similarly in the other failed test.
>>>> 
>>>> Ah, OK, that would try mucking with .git/git-p4-tmp/6 but that is
>>>> not a place to have a ref.  It will not participate in reachability
>>>> analysis and will end up losing the referents.
>>>> 
>>>> Perhaps placing it under refs/git-p4-tmp would fix it (both in
>>>> git-p4 and in tests)?
> 
> I have to say, I'm still confused about how the old code could have
> worked at all. For quite some time, Git hasn't allowed references with
> weird names like `git-p4-tmp/6` to be created, so how could there be any
> references there that need to be deleted? It seems to me that one of the
> following must be true:
> 
> * This feature (i.e., whatever needs to create the temporary
>  references) has been broken for a long time, which would imply that
>  there are no tests for it.
> 
> * The references are created in some bogus way, like writing files
>  directly to the filesystem, rather than using `git update-ref`.
>  This is broken and will be even more broken soon when we add
>  support for different reference storage backends.
> 
> * Such references are never actually created and never actually
>  needed. This would imply that the cleanup code is unnecessary.
> 
> * The temporary references are created using `git branch`, and are
>  thus actually named like `refs/heads/git-p4-tmp/6`. In this case
>  the deletion code wasn't cleaning them up right, but the feature
>  might have otherwise worked correctly.
> 
> * I'm wrong about Git not allowing references like `git-p4-tmp/6` to
>  be created, in which case I'd like to know what code path allows it
>  so that I can fix it.
> 
> Any of these possibilities would mean that your proposed fix is wrong or
> incomplete.

Although I know git-p4 quite well, I am certainly no expert for the git-p4
branch import. I also pointed out in my previous reply ($gmane/297703)
that the git-p4 branch import never worked successfully for me. I always
thought the reason is that my P4 branch history is a bit messy. The clean 
git-p4 branch test cases work fine, though.


>>> Oh, another thing.  If these refs are meant to be transient, they
>>> are likely to be per worktree, if "git worktree" managed multiple
>>> worktrees that share the same set of branches and tags are in use.
>>> 
>>> I recall we carved out one hierarchy under refs/ as "not shared
>>> across worktrees" (was that refs/worktree/ hierarchy?  I didn't
>>> check but please do so when the patch actually is written), and
>>> that hierarchy is the appropriate thing to use for this, I think.
>> 
>> Thanks for the hint. It looks like as if the "per worktree" decision
>> is made in "ref.c:466" "is_per_worktree_ref":
>> https://github.com/git/git/blob/3dc84b0c19932ec9947ca4936b6bfd6421ccb1b4/refs.c#L466-L470
>> 
>> In ce414b3 "refs/bisect" was added to a list of prefixes that are
>> per worktree. I could easily add "refs/git-p4-tmp" to this list but
>> I don't think that would be a good solution. I would prefer to go with 
>> your suggestion and add "refs/worktree" to the prefix list and then use
>> "refs/worktree/git-p4-tmp".
>> 
>> Later on we could move "refs/bisect" to "refs/worktree/bisect" and
>> simplify the "is_per_worktree_ref" code, too.
> 
> The other part of making `refs/bisect` per-worktree is this horrible
> hack here [1].
Thanks for the pointer!


> I'd like to request that the change for the p4 temprefs be made in two
> steps:
> 
> 1. Write references to `refs/git-p4-tmp` or whatever, without
>   worrying about making them per-worktree.
> 
> 2. Carve out a per-worktree namespace, bikeshed about its name and
>   semantics, make sure it works correctly, then finally move the
>   p4 temporary references and eventually the bisect references there.
> 
> The reason is that step 1 is low-risk, could be made quickly, and is
> enough to unblock mh/split-under-lock and the other patch series that
> depend on it. Step 2, on the other hand, could take quite a while, and
> its implementation might benefit from changes to reference handling that
> are in the pipeline (e.g., perhaps the horrible hack can be dispensed
> with). Meanwhile, as far as I understand these references are transient,
> so there are no backwards-compatibility considerations that make
> renaming them twice more cumbersome than renaming them once.
Agreed! I will post the patch in a few seconds.

Cheers,
Lars


> 
> Thanks,
> Michael
> 
> [1]
> https://github.com/git/git/blob/ab7797dbe95fff38d9265869ea367020046db118/refs/files-backend.c#L176-L192

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I'd like to request that the change for the p4 temprefs be made in two
> steps:
>
> 1. Write references to `refs/git-p4-tmp` or whatever, without
>    worrying about making them per-worktree.
>
> 2. Carve out a per-worktree namespace, bikeshed about its name and
>    semantics, make sure it works correctly, then finally move the
>    p4 temporary references and eventually the bisect references there.

Hasn't 2. already happened long time ago?  I thought that the
direction was that bisect will have to be grandfathered to stay
there, and new things would hang beneath a single per-worktree
hierarchy.

I would have expected that your 1. would be a request to use
refs/worktrees/git-p4-tmp.

... goes and looks ...

Ahh, I did misremember the discussions.

http://thread.gmane.org/gmane.comp.version-control.git/276628
http://thread.gmane.org/gmane.comp.version-control.git/276960/focus=276963

It appears that in the latter, a more generic refs/worktrees/ was
somehow postponed, and only bisect was made per-worktree to fix the
immediate breakage.

> The reason is that step 1 is low-risk, could be made quickly, and is
> enough to unblock mh/split-under-lock and the other patch series that
> depend on it. Step 2, on the other hand, could take quite a while, and
> its implementation might benefit from changes to reference handling that
> are in the pipeline (e.g., perhaps the horrible hack can be dispensed
> with). Meanwhile, as far as I understand these references are transient,
> so there are no backwards-compatibility considerations that make
> renaming them twice more cumbersome than renaming them once.

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Michael Haggerty]

On 06/23/2016 09:32 AM, Michael Haggerty wrote:
> [...]
> I have to say, I'm still confused about how the old code could have
> worked at all. For quite some time, Git hasn't allowed references with
> weird names like `git-p4-tmp/6` to be created, so how could there be any
> references there that need to be deleted? It seems to me that one of the
> following must be true:
> [...]
> * I'm wrong about Git not allowing references like `git-p4-tmp/6` to
>   be created, in which case I'd like to know what code path allows it
>   so that I can fix it.

I was indeed wrong.

Reference names are vetted by `check_refname_format()` on creation, but
that function knows and enforces nothing about the `refs/` namespace or
the ALL_CAPS convention for top-level references. Moreover, the relevant
caller passes the `REFNAME_ALLOW_ONELEVEL` option, which has semantics
that are fairly useless as far as I can tell. A casual reader might
assume that `REFNAME_ALLOW_ONELEVEL` allows one-level reference only if
they satisfy the special `ALL_CAPS` convention, but in fact it doesn't
enforce the convention. (I suppose that `REFNAME_ALLOW_ONELEVEL` was
meant for checking partial reference names, for example to vet "foo"
that the caller wants to pass to `git branch`, which automatically turns
it into `refs/heads/foo`.)

In summary, `check_refname_format()` is in some ways less strict than
`refname_is_safe()`. For example, it allows reference names like
`git-p4-tmp/6` or even `git-p4-tmp`. With current master:

    $ git update-ref git-p4-tmp HEAD
    $ echo $?
    0
    $ git rev-parse git-p4-tmp
    cf4c2cfe52be5bd973a4838f73a35d3959ce2f43
    $ git update-ref -d git-p4-tmp
    $ echo $?
    0

I don't know what I was thinking. Long ago, when I refactored and
documented check_refname_format(), I realized that the checks are
surprisingly lax and that the `REFNAME_ALLOW_ONELEVEL` option is
misleading. But I was new to the project and wasn't brave or energetic
enough to do something about it. Meanwhile I guess I forgot that it
never got fixed. Commit

d0f810f0 2014-09-03 refs.c: allow listing and deleting badly named refs

, which introduced `refname_is_safe()`, perhaps muddled the situation by
adding a "fallback" check that is not strictly laxer than the main check.

Where does that leave us?

* It is currently possible to create and delete references with
  names that are neither within `refs/` nor ALL_CAPS (though not
  remotely).

* Such references do not participate in reachability checks, so
  they have to be considered semi-broken.

* Users could create chaos (though not remotely) by creating a
  loose "reference" whose name coincides with that of another
  file under `$GIT_DIR`.
  (`git update-ref objects/info/alternates HEAD` anyone?)

* `git-p4` is apparently the only code within the Git project that
  was making use of this questionable freedom.

* Presumably there is user code in the wild that creates and uses
  such references.

I think we need to get this under control, but given that we've allowed
such references (albeit partly broken) for a long time, we probably need
to provide an escape hatch to aid the transition. I'm skeptical that the
mh/split-under-lock patch series is the best place to start such a
campaign. On the other hand, I don't want that patch series to open up
any new avenues to creating references that escape `$GIT_DIR`.

Let me think for a bit about the next step. Input is welcome.

In any case, I think that Lars's patch to `git-p4` is a good thing.

Michael

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> (I suppose that `REFNAME_ALLOW_ONELEVEL` was
> meant for checking partial reference names, for example to vet "foo"
> that the caller wants to pass to `git branch`, which automatically turns
> it into `refs/heads/foo`.)

Correct.

> In summary, `check_refname_format()` is in some ways less strict than
> `refname_is_safe()`. For example, it allows reference names like
> `git-p4-tmp/6` or even `git-p4-tmp`.

Again, correct.

> I don't know what I was thinking. Long ago, when I refactored and
> documented check_refname_format(), I realized that the checks are
> surprisingly lax and that the `REFNAME_ALLOW_ONELEVEL` option is
> misleading. But I was new to the project and wasn't brave or energetic
> enough to do something about it. Meanwhile I guess I forgot that it
> never got fixed. Commit
>
> d0f810f0 2014-09-03 refs.c: allow listing and deleting badly named refs
>
> , which introduced `refname_is_safe()`, perhaps muddled the situation by
> adding a "fallback" check that is not strictly laxer than the main check.
>
> Where does that leave us?
>
> * It is currently possible to create and delete references with
>   names that are neither within `refs/` nor ALL_CAPS (though not
>   remotely).
>
> * Such references do not participate in reachability checks, so
>   they have to be considered semi-broken.
>
> * Users could create chaos (though not remotely) by creating a
>   loose "reference" whose name coincides with that of another
>   file under `$GIT_DIR`.
>   (`git update-ref objects/info/alternates HEAD` anyone?)

All correct.

> * `git-p4` is apparently the only code within the Git project that
>   was making use of this questionable freedom.

True.

> * Presumably there is user code in the wild that creates and uses
>   such references.

I doubt it, if they value their data.  .git/p4-tmp or .git/p4-tmp/6
are not just "partly broken"--they are just as transient as things
like MERGE_HEAD in that the objects pointed by them are subject to
be pruned.

> I think we need to get this under control, but given that we've allowed
> such references (albeit partly broken) for a long time, we probably need
> to provide an escape hatch to aid the transition. I'm skeptical that the
> mh/split-under-lock patch series is the best place to start such a
> campaign. On the other hand, I don't want that patch series to open up
> any new avenues to creating references that escape `$GIT_DIR`.
>
> Let me think for a bit about the next step. Input is welcome.
>
> In any case, I think that Lars's patch to `git-p4` is a good thing.

Thanks.

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Joey Hess]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

Torsten Bögershausen wrote re jh/clean-smudge-annex:
> The thing is that we need to check the file system to find .gitatttibutes,
> even if we just did it 1 nanosecond ago.
> 
> So the .gitattributes is done 3 times:
> -1 would_convert_to_git_filter_fd(
> -2 assert(would_convert_to_git_filter_fd(path));
> -3 convert.c/convert_to_git_filter_fd()
> 
> The only situation where this could be useful is when the .gitattributes
> change between -1 and -2,
> but then they would have changed between -1 and -3, and convert.c
> will die().
> 
> Does it make sense to remove -2 ?

There's less redundant work going on than at first seems, because 
.gitattribute files are only read once and cached. Verified by strace.

So, the redundant work is only in the processing that convert_attrs() does
of the cached .gitattributes.

Notice that there was a similar redundant triple call to convert_attrs()
before my patch set:

1. index_fd checks would_convert_to_git_filter_fd
2. index_stream_convert_blob does assert(would_convert_to_git_filter_fd(path))
   (Again redundantly since 1. is its only caller and has already
   checked.)
3. in convert_to_git_filter_fd

If convert_attrs() is somehow expensive, it might be worth passing a
struct conv_attrs * into the functions that currently call
convert_attrs(). But it does not look expensive to me.

I think it would be safe enough to remove both asserts, at least as the
code is structured now.

-- 
see shy jo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Torsten Bögershausen]

On 07/13/2016 12:20 AM, Joey Hess wrote:
> Torsten Bögershausen wrote re jh/clean-smudge-annex:
>> The thing is that we need to check the file system to find .gitatttibutes,
>> even if we just did it 1 nanosecond ago.
>>
>> So the .gitattributes is done 3 times:
>> -1 would_convert_to_git_filter_fd(
>> -2 assert(would_convert_to_git_filter_fd(path));
>> -3 convert.c/convert_to_git_filter_fd()
>>
>> The only situation where this could be useful is when the .gitattributes
>> change between -1 and -2,
>> but then they would have changed between -1 and -3, and convert.c
>> will die().
>>
>> Does it make sense to remove -2 ?
>
> There's less redundant work going on than at first seems, because
> .gitattribute files are only read once and cached. Verified by strace.
>
OK, I think I missed that work (not enough time for Git at the moment)
Junio, please help me out, do we have a cache here now?
I tried to figure out that following your attr branch, but failed.
> So, the redundant work is only in the processing that convert_attrs() does
> of the cached .gitattributes.
>
> Notice that there was a similar redundant triple call to convert_attrs()
> before my patch set:
>
> 1. index_fd checks would_convert_to_git_filter_fd
> 2. index_stream_convert_blob does assert(would_convert_to_git_filter_fd(path))
>    (Again redundantly since 1. is its only caller and has already
>    checked.)
> 3. in convert_to_git_filter_fd
>
> If convert_attrs() is somehow expensive, it might be worth passing a
> struct conv_attrs * into the functions that currently call
> convert_attrs(). But it does not look expensive to me.
I have that on the list, but seems to be uneccesary now.
>
> I think it would be safe enough to remove both asserts, at least as the
> code is structured now.
>
OK.

Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

>> There's less redundant work going on than at first seems, because
>> .gitattribute files are only read once and cached. Verified by strace.
>>
> OK, I think I missed that work (not enough time for Git at the moment)
> Junio, please help me out, do we have a cache here now?

The call convert_attrs() makes to the attribute subsystem is:

    git_check_attr(path, NUM_CONV_ATTRS, ccheck)

Conceptually, this call has to do the following, and for the very
first call that is actually what it does:

 1. Read all the relevant attrubutes files.  If you are asking for
    "a/x/1", we need to read $GIT_DIR/info/attributes,
    ".gitattributes", "a/.gitattributes" and "a/x/.gitattributes".

 2. Find matching patterns that cover "a/x/1", and pick up the
    attribute definition from the above.

If you have asked for "a/x/1", it is very likely that you would next
ask for "a/x/2" (think: "git checkout a/x"), and we can realize that
exactly the same set of attributes files apply to that path.  So an
obvious optimization is to cache the result of the first step.

In addition, it is also likely that you would later ask for "a/y/3"
before asking for "b/z/4" (think: "git add .").  A part of the step
1. that was done when you asked for "a/x/1" and then was reused when
you asked for "a/x/2" can further be reused for this request, by
discarding only what was read from "a/x/.gitattributes" and reading
only from "a/y/.gitattributes".

The above two optimizations are done in prepare_attr_stack().

Unfortunately this is one of the three reasons why the attribute
subsystem is not thread-ready.  I.e. there is only one global cache,
so if you spawn two threads to speed up "git add ." by handing paths
[a-m]* to one and [n-z]* to the other, they would thrash the cache
and making it ineffective (even if we protect the cache with mutex,
which obviously has not been done).

I earlier started looking into this, but the effort stalled.  But
for a single-thread use, the attributes read from the filesystem are
cached, and the cache is designed to perform well as long as you
make requests in-order.

To make the attribute look-up thread-ready, the attribute cache
needs to become per-thread.

Orthogonal of the threading issue, there is another posssible
optimization that is not there yet.  The cache can be tied to what
is in ccheck[] to further reduce the size of the cache, making step
2. a lot cheaper.  Currently in step 1. we read and keep everything,
but if we tie the cache to the contents of ccheck[], we can read and
ignore entries we read in step 1. that does not talk about the
attributes ccheck[] is interested in.  My plan is to either

 (1) make the cache per-thread, limit the reading done in 1. to
     ccheck[], but invalidate the cache when a different set of
     attributes are asked; or

 (2) make the cache per <thread, ccheck[]>.