What's cooking in git.git (Apr 2013, #05; Mon, 15)

What's cooking in git.git (Apr 2013, #05; Mon, 15)

[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'.

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

--------------------------------------------------
[Graduated to "master"]

* jk/diff-algo-finishing-touches (2013-04-05) 2 commits
  (merged to 'next' on 2013-04-11 at af83b2b)
 + diff: allow unstuck arguments with --diff-algorithm
 + git-merge(1): document diff-algorithm option to merge-recursive

 "git diff --diff-algorithm algo" is also understood as "git diff
 --diff-algorithm=algo".


* jk/diff-graph-submodule-summary (2013-04-05) 1 commit
  (merged to 'next' on 2013-04-11 at 70dfa8d)
 + submodule: print graph output next to submodule log

 Make "git diff --graph" work better with submodule log output.


* jk/http-error-messages (2013-04-06) 9 commits
  (merged to 'next' on 2013-04-11 at 7a03981)
 + http: drop http_error function
 + remote-curl: die directly with http error messages
 + http: re-word http error message
 + http: simplify http_error helper function
 + remote-curl: consistently report repo url for http errors
 + remote-curl: always show friendlier 404 message
 + remote-curl: let servers override http 404 advice
 + remote-curl: show server content on http errors
 + http: add HTTP_KEEP_ERROR option

 Improve error reporting from the http transfer clients.


* jk/show-branch-strbuf (2013-04-06) 1 commit
  (merged to 'next' on 2013-04-11 at 7a20aa5)
 + show-branch: use strbuf instead of static buffer

 "git show-branch" was not prepared to show a very long run of
 ancestor operators e.g. foobar^2~2^2^2^2...^2~4 correctly.


* lf/bundle-with-tip-wo-message (2013-04-07) 1 commit
  (merged to 'next' on 2013-04-11 at bb9f869)
 + bundle: Accept prerequisites without commit messages

 "git bundle" did not like a bundle created using a commit without
 any message as its one of the prerequistes.


* po/help-guides (2013-04-03) 5 commits
  (merged to 'next' on 2013-04-04 at 3d99b28)
 + doc: include --guide option description for "git help"
 + help: mention -a and -g option, and 'git help <concept>' usage.
 + builtin/help.c: add list_common_guides_help() function
 + builtin/help.c: add --guide option
 + builtin/help.c: split "-a" processing into two

 "git help" learned "-g" option to show the list of guides just like
 list of commands are given with "-a".
 * po/help-guides (2013-04-12) 1 commit
 - help: mark common_guides[] as translatable

 Finishing touches.


* rt/commentchar-fmt-merge-msg (2013-04-07) 2 commits
  (merged to 'next' on 2013-04-11 at 6af638b)
 + fmt-merge-msg: use core.commentchar in tag signatures completely
 + fmt-merge-msg: respect core.commentchar in people credits

 The new core.commentchar configuration was not applied to a few
 places.


* tr/perl-keep-stderr-open (2013-04-04) 2 commits
  (merged to 'next' on 2013-04-07 at 04f737a)
 + t9700: do not close STDERR
 + perl: redirect stderr to /dev/null instead of closing

 Closing (not redirecting to /dev/null) the standard error stream is
 not a very smart thing to do.  Later open may return file
 descriptor #2 for unrelated purpose, and error reporting code may
 write into them.

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

* kb/status-ignored-optim-2 (2013-04-15) 14 commits
 . dir.c: git-status --ignored: don't scan the work tree twice
 . dir.c: git-status --ignored: don't scan the work tree three times
 . dir.c: git-status: avoid is_excluded checks for tracked files
 . dir.c: replace is_path_excluded with now equivalent is_excluded API
 . dir.c: unify is_excluded and is_path_excluded APIs
 . dir.c: move prep_exclude
 . dir.c: factor out parts of last_exclude_matching for later reuse
 . dir.c: git-clean -d -X: don't delete tracked directories
 . dir.c: make 'git-status --ignored' work within leading directories
 . dir.c: git-status --ignored: don't list empty directories as ignored
 . dir.c: git-ls-files --directories: don't hide empty directories
 . dir.c: git-status --ignored: don't list empty ignored directories
 . dir.c: git-status --ignored: don't list files in ignored directories
 . dir.c: git-status --ignored: don't drop ignored directories

 Rerolls kb/status-ignored-optim topic (reverted from 'next').  Not
 merged to 'pu' as it heavily interferes with as/check-ignore topic.


* fc/branch-upstream-color (2013-04-15) 1 commit
  (merged to 'next' on 2013-04-15 at 2fc50fd)
 + branch: colour upstream branches

 Add more colors to "git branch -vv" output.

 Will merge to 'master'.


* jk/commit-info-slab (2013-04-13) 3 commits
 - commit-slab: introduce a macro to define a slab for new type
 - commit-slab: avoid large realloc
 - commit: allow associating auxiliary info on-demand

 Technology demonstration to show a way we could use unbound number
 of flag bits on commit objects.


* jk/test-trash (2013-04-14) 2 commits
  (merged to 'next' on 2013-04-15 at 15a6624)
 + t/test-lib.sh: drop "$test" variable
 + t/test-lib.sh: fix TRASH_DIRECTORY handling

 Fix longstanding issues with the test harness when used with --root=<there>
 option.


* lf/read-blob-data-from-index (2013-04-15) 3 commits
  (merged to 'next' on 2013-04-15 at 09f92c6)
 + convert.c: Remove duplicate code
 + Add size parameter to read_blob_data_from_index_path()
 + Add public function read_blob_data_from_index_path()

 Reduce duplicated code between convert.c and attr.c.

 Will merge to 'master'.


* mv/ssl-ftp-curl (2013-04-12) 1 commit
  (merged to 'next' on 2013-04-15 at 7fdada6)
 + Support FTP-over-SSL/TLS for regular FTP

 Does anybody really use commit walkers over ftp???

 Will merge to 'master'.


* as/check-ignore (2013-04-11) 5 commits
 - Documentation: add caveats about I/O buffering for check-{attr,ignore}
 - check-ignore: allow incremental streaming of queries via --stdin
 - check-ignore: move setup into cmd_check_ignore()
 - check-ignore: add -n / --non-matching option
 - t0008: remove duplicated test fixture data

 Enhance "check-ignore" (1.8.2 update) to work more like "check-attr"
 over bidi-pipes.


* mh/packed-refs-various (2013-04-15) 33 commits
 - refs: handle the main ref_cache specially
 - refs: change do_for_each_*() functions to take ref_cache arguments
 - pack_one_ref(): do some cheap tests before a more expensive one
 - pack_one_ref(): use write_packed_entry() to do the writing
 - pack_one_ref(): use function peel_entry()
 - refs: inline function do_not_prune()
 - pack_refs(): change to use do_for_each_entry()
 - refs: use same lock_file object for both ref-packing functions
 - pack_one_ref(): rename "path" parameter to "refname"
 - pack-refs: merge code from pack-refs.{c,h} into refs.{c,h}
 - pack-refs: rename handle_one_ref() to pack_one_ref()
 - refs: extract a function write_packed_entry()
 - repack_without_ref(): write peeled refs in the rewritten file
 - t3211: demonstrate loss of peeled refs if a packed ref is deleted
 - refs: change how packed refs are deleted
 - search_ref_dir(): return an index rather than a pointer
 - repack_without_ref(): silence errors for dangling packed refs
 - t3210: test for spurious error messages for dangling packed refs
 - refs: change the internal reference-iteration API
 - refs: extract a function peel_entry()
 - peel_ref(): fix return value for non-peelable, not-current reference
 - peel_object(): give more specific information in return value
 - refs: extract function peel_object()
 - refs: extract a function ref_resolves_to_object()
 - repack_without_ref(): use function get_packed_ref()
 - peel_ref(): use function get_packed_ref()
 - get_packed_ref(): return a ref_entry
 - do_for_each_ref_in_dirs(): remove dead code
 - refs: define constant PEELED_LINE_LENGTH
 - refs: document how current_ref is used
 - refs: document do_for_each_ref() and do_one_ref()
 - refs: document the fields of struct ref_value
 - refs: document flags constants REF_*

 Updates reading and updating packed-refs file, correcting corner
 case bugs.


* jk/remote-helper-with-signed-tags (2013-04-15) 3 commits
 - transport-helper: add 'signed-tags' capability
 - transport-helper: pass --signed-tags=warn-strip to fast-export
 - fast-export: add --signed-tags=warn-strip mode

 Allows remote-helpers to declare they can handle signed tags, and
 issue a warning when using those that don't.

 Comments?


* jn/config-ignore-inaccessible (2013-04-15) 1 commit
 - config: allow inaccessible configuration under $HOME

 When $HOME is misconfigured to point at an unreadable directory, we
 used to complain and die. This loosens the check.

 I do not think we agreed that this is a good idea, though.


* jn/gitweb-install-doc (2013-04-15) 1 commit
 - gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM

 Reword gitweb configuration instrutions.

 Will merge to 'next'.


* jx/i18n-branch-error-messages (2013-04-15) 1 commit
 - i18n: branch: mark strings for translation

 Will merge to 'master'.


* nd/checkout-keep-sparse (2013-04-15) 1 commit
 - checkout: add --ignore-skip-worktree-bits in sparse checkout mode

 Make the initial "sparse" selection of the paths more sticky across
 "git checkout".

 Will merge to 'next'.


* ta/glossary (2013-04-15) 4 commits
 - glossary: improve definitions of refspec and pathspec
 - The name of the hash function is "SHA-1", not "SHA1"
 - glossary: improve description of SHA-1 related topics
 - glossary: remove outdated/misleading/irrelevant entries

 Will merge to 'next'.


* th/bisect-final-log (2013-04-15) 1 commit
 - bisect: Store first bad commit as comment in log file

 Will merge to 'next'.

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

* nd/pretty-formats (2013-04-01) 12 commits
 - pretty: support %>> that steal trailing spaces
 - pretty: support truncating in %>, %< and %><
 - pretty: support padding placeholders, %< %> and %><
 - pretty: add %C(auto) for auto-coloring on the next placeholder
 - pretty: two phase conversion for non utf-8 commits
 - utf8: keep NULs in reencode_string()
 - pretty: get the correct encoding for --pretty:format=%e
 - pretty: save commit encoding from logmsg_reencode if the caller needs it
 - utf8.c: add utf8_strnwidth() with the ability to skip ansi sequences
 - utf8.c: move display_mode_esc_sequence_len() for use by other functions
 - pretty: share code between format_decoration and show_decorations
 - pretty-formats.txt: wrap long lines

 A mixed bag of a bugfix and two fun enhancements on pretty formats
 placeholder.

 Expecting a reroll.


* jc/format-patch (2013-02-21) 2 commits
 - format-patch: --inline-single
 - format-patch: rename "no_inline" field

 A new option to send a single patch to the standard output to be
 appended at the bottom of a message.  I personally have no need for
 this, but it was easy enough to cobble together.  Tests, docs and
 stripping out more MIMEy stuff are left as exercises to interested
 parties.

 Not ready for inclusion.

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

* ap/strbuf-humanize (2013-04-10) 2 commits
  (merged to 'next' on 2013-04-14 at 66d7af5)
 + count-objects: add -H option to humanize sizes
 + strbuf: create strbuf_humanise_bytes() to show byte sizes

 Teach "--human-readable" aka "-H" option to "git count-objects" to
 show various large numbers in Ki/Mi/GiB scaled as necessary.

 I've decided to let this topic supersede mc/count-objects-kibibytes.
 Human users will get an even easier output with "-H" and by not
 changing the output without an explicit option we do not have to
 break third-party tools that may have been reading from the output
 of this command.


* as/clone-reference-with-gitfile (2013-04-09) 2 commits
  (merged to 'next' on 2013-04-15 at ab0d128)
 + clone: Allow repo using gitfile as a reference
 + clone: Fix error message for reference repository

 "git clone" did not work if a repository pointed at by the
 "--reference" option is a gitfile that points at another place.

 Waiting for comments.


* fc/transport-helper-error-reporting (2013-04-11) 3 commits
 - transport-helper: improve push messages
 - transport-helper: mention helper name when it dies
 - transport-helper: report errors properly

 Rerolled enough times.  In-code comments may want to be further
 extended to explain tricky parts, but seems to be ready otherwise.

 Will merge to 'next'.


* jk/doc-http-backend (2013-04-13) 3 commits
 - doc/http-backend: match query-string in apache half-auth example
 - doc/http-backend: give some lighttpd config examples
 - doc/http-backend: clarify "half-auth" repo configuration

 Improve documentation to illustrate "push authenticated, fetch
 anonymous" configuration for smart HTTP servers.

 Will merge to 'next'.


* jk/gitweb-utf8 (2013-04-08) 4 commits
 - gitweb: Fix broken blob action parameters on blob/commitdiff pages
 - gitweb: Don't append ';js=(0|1)' to external links
 - gitweb: Make feed title valid utf8
 - gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain, and patch

 Various fixes to gitweb.

 Waiting for a reroll after a review.


* jk/submodule-subdirectory-ok (2013-04-10) 2 commits
 - submodule: drop the top-level requirement
 - rev-parse: add --prefix option

 Allow various subcommands of "git submodule" to be run not from the
 top of the working tree of the superproject.

 Waiting for comments.


* kb/co-orphan-suggestion-short-sha1 (2013-04-08) 1 commit
  (merged to 'next' on 2013-04-14 at 8caf7fd)
 + checkout: abbreviate hash in suggest_reattach

 Update the informational message when "git checkout" leaves the
 detached head state.

 Will merge to 'master'.


* mv/sequencer-pick-error-diag (2013-04-11) 1 commit
 - cherry-pick: make sure all input objects are commits

 "git cherry-pick $blob $tree" is diagnosed as a nonsense.

 Will merge to 'next'.


* rs/empty-archive (2013-04-10) 1 commit
  (merged to 'next' on 2013-04-15 at eab39bc)
 + t5004: fix issue with empty archive test and bsdtar

 Implementations of "tar" of BSD descend have found to have trouble
 with reading an otherwise empty tar archive with pax headers and
 causes an unnecessary test failure.

 Will merge to 'master'.


* th/t9903-symlinked-workdir (2013-04-11) 1 commit
  (merged to 'next' on 2013-04-15 at f062dc6)
 + t9903: Don't fail when run from path accessed through symlink

 Will merge to 'master'.


* fc/completion (2013-04-14) 8 commits
  (merged to 'next' on 2013-04-14 at a509746)
 + completion: small optimization
 + completion: inline __gitcomp_1 to its sole callsite
 + completion: get rid of compgen
 + completion: add __gitcomp_nl tests
 + completion: add new __gitcompadd helper
 + completion: get rid of empty COMPREPLY assignments
 + completion: trivial test improvement
 + completion: add more cherry-pick options

 Will merge to 'master'.


* jk/daemon-user-doc (2013-04-12) 1 commit
  (merged to 'next' on 2013-04-14 at 56c08ff)
 + doc: clarify that "git daemon --user=<user>" option does not export HOME=~user

 Will merge to 'master'.


* fc/send-email-annotate (2013-04-14) 7 commits
  (merged to 'next' on 2013-04-14 at 4af1076)
 + rebase-am: explicitly disable cover-letter
 + format-patch: trivial cleanups
 + format-patch: add format.coverLetter configuration variable
 + log: update to OPT_BOOL
 + format-patch: refactor branch name calculation
 + format-patch: improve head calculation for cover-letter
 + send-email: make annotate configurable

 Allows format-patch --cover-letter to be configurable; the most
 notable is the "auto" mode to create cover-letter only for multi
 patch series.

 Will merge to 'master'.


* fc/remote-hg (2013-04-11) 21 commits
 - remote-hg: activate graphlog extension for hg_log()
 - remote-hg: fix bad file paths
 - remote-hg: document location of stored hg repository
 - remote-hg: fix bad state issue
 - remote-hg: add 'insecure' option
 - remote-hg: add simple mail test
 - remote-hg: add basic author tests
 - remote-hg: show more proper errors
 - remote-hg: force remote push
 - remote-hg: push to the appropriate branch
 - remote-hg: update tags globally
 - remote-hg: update remote bookmarks
 - remote-hg: refactor export
 - remote-hg: split bookmark handling
 - remote-hg: redirect buggy mercurial output
 - remote-hg: trivial test cleanups
 - remote-hg: make sure fake bookmarks are updated
 - remote-hg: fix for files with spaces
 - remote-hg: properly report errors on bookmark pushes
 - remote-hg: add missing config variable in doc
 - remote-hg: trivial cleanups

 Rerolled.

 Waiting for comments.


* jk/http-dumb-namespaces (2013-04-09) 1 commit
  (merged to 'next' on 2013-04-15 at 4bfa834)
 + http-backend: respect GIT_NAMESPACE with dumb clients

 Allow smart-capable HTTP servers to be restricted via the
 GIT_NAMESPACE mechanism when talking with commit-walker clients
 (they already do so when talking with smart HTTP clients).

 Will merge to 'master'.


* jl/submodule-mv (2013-04-11) 4 commits
 - rm: delete .gitmodules entry of submodules removed from the work tree
 - Teach mv to update the path entry in .gitmodules for moved submodules
 - Teach mv to move submodules using a gitfile
 - Teach mv to move submodules together with their work trees

 "git mv A B" when moving a submodule A does "the right thing",
 inclusing relocating its working tree and adjusting the paths in
 the .gitmodules file.

 Will merge to 'next'.


* jc/detached-head-doc (2013-04-05) 1 commit
  (merged to 'next' on 2013-04-14 at 24b9271)
 + glossary: extend "detached HEAD" description

 Will merge to 'master'.


* jk/merge-tree-added-identically (2013-04-08) 1 commit
  (merged to 'next' on 2013-04-15 at 35fd4b9)
 + merge-tree: don't print entries that match "local"

 The resolution of some corner cases by "git merge-tree" were
 inconsistent between top-of-the-tree and in a subdirectory.

 Will merge to 'master'.


* jn/add-2.0-u-A-sans-pathspec (2013-04-03) 6 commits
 - git add: -u/-A now affects the entire working tree
  (merged to 'next' on 2013-04-05 at eae93ef)
 + add -A: only show pathless 'add -A' warning when changes exist outside cwd
 + add -u: only show pathless 'add -u' warning when changes exist outside cwd
 + add: make warn_pathless_add() a no-op after first call
 + add: add a blank line at the end of pathless 'add [-u|-A]' warning
 + add: make pathless 'add [-u|-A]' warning a file-global function

 "git add -u/-A" without any pathspec traditionally limited its
 operation to the current directory when run from a subdirectory,
 but in Git 2.0, they will affect the entire working tree.  Start
 training users to explicitly say "." or ":/" to smooth out the
 transition hump with the earlier parts of this series, and flip the
 default as the final step.

 Will merge to 'master' the early bits and cook the rest in 'next' until Git 2.0.


* tr/packed-object-info-wo-recursion (2013-03-27) 3 commits
  (merged to 'next' on 2013-03-29 at b1c3858)
 + sha1_file: remove recursion in unpack_entry
 + Refactor parts of in_delta_base_cache/cache_or_unpack_entry
 + sha1_file: remove recursion in packed_object_info

 Attempts to reduce the stack footprint of sha1_object_info()
 and unpack_entry() codepaths.

 Will merge to 'master'.


* nd/magic-pathspecs (2013-03-31) 45 commits
 . Rename field "raw" to "_raw" in struct pathspec
 . pathspec: support :(glob) syntax
 . pathspec: make --literal-pathspecs disable pathspec magic
 . pathspec: support :(literal) syntax for noglob pathspec
 . Kill limit_pathspec_to_literal() as it's only used by parse_pathspec()
 . parse_pathspec: preserve prefix length via PATHSPEC_PREFIX_ORIGIN
 . parse_pathspec: make sure the prefix part is wildcard-free
 . tree-diff: remove the use of pathspec's raw[] in follow-rename codepath
 . Remove match_pathspec() in favor of match_pathspec_depth()
 . Remove init_pathspec() in favor of parse_pathspec()
 . Remove diff_tree_{setup,release}_paths
 . Convert common_prefix() to use struct pathspec
 . Convert add_files_to_cache to take struct pathspec
 . Convert {read,fill}_directory to take struct pathspec
 . Convert refresh_index to take struct pathspec
 . Convert report_path_error to take struct pathspec
 . checkout: convert read_tree_some to take struct pathspec
 . Convert unmerge_cache to take struct pathspec
 . Convert run_add_interactive to use struct pathspec
 . Convert read_cache_preload() to take struct pathspec
 . reset: convert to use parse_pathspec
 . add: convert to use parse_pathspec
 . check-ignore: convert to use parse_pathspec
 . archive: convert to use parse_pathspec
 . ls-files: convert to use parse_pathspec
 . rm: convert to use parse_pathspec
 . checkout: convert to use parse_pathspec
 . rerere: convert to use parse_pathspec
 . status: convert to use parse_pathspec
 . commit: convert to use parse_pathspec
 . clean: convert to use parse_pathspec
 . Guard against new pathspec magic in pathspec matching code
 . parse_pathspec: support prefixing original patterns
 . parse_pathspec: support stripping/checking submodule paths
 . parse_pathspec: support stripping submodule trailing slashes
 . parse_pathspec: a special flag for max_depth feature
 . Convert some get_pathspec() calls to parse_pathspec()
 . parse_pathspec: add PATHSPEC_PREFER_{CWD,FULL}
 . parse_pathspec: save original pathspec for reporting
 . Add parse_pathspec() that converts cmdline args to struct pathspec
 . pathspec: add copy_pathspec
 . pathspec: i18n-ize error strings in pathspec parsing code
 . Move struct pathspec and related functions to pathspec.[ch]
 . clean: remove unused variable "seen"
 . setup.c: check that the pathspec magic ends with ")"

 Migrate the rest of codebase to use "struct pathspec" more.


* jc/add-2.0-delete-default (2013-03-08) 3 commits
 - git add <pathspec>... defaults to "-A"
  (merged to 'next' on 2013-04-05 at 199442e)
 + git add: start preparing for "git add <pathspec>..." to default to "-A"
 + builtin/add.c: simplify boolean variables

 In Git 2.0, "git add pathspec" will mean "git add -A pathspec".  If
 you did this in a working tree that tracks dir/lost and dir/another:

 $ rm dir/lost
 $ edit dir/another
 $ git add dir

 The last step will not only notices and records updated
 dir/another, but also notices and records the removal of dir/lost
 in the index.

 Start training the users for this change to say --no-all when they
 want to ignore the removal to smooth the transition hump.

 Will merge to 'master' the early bits and cook the rest in 'next' until Git 2.0.


* tr/line-log (2013-04-12) 11 commits
  (merged to 'next' on 2013-04-15 at 504559e)
 + log -L: improve comments in process_all_files()
 + log -L: store the path instead of a diff_filespec
 + log -L: test merge of parallel modify/rename
 + t4211: pass -M to 'git log -M -L...' test
  (merged to 'next' on 2013-04-05 at 5afb00c)
 + log -L: fix overlapping input ranges
 + log -L: check range set invariants when we look it up
  (merged to 'next' on 2013-04-01 at 5be920c)
 + Speed up log -L... -M
 + log -L: :pattern:file syntax to find by funcname
 + Implement line-history search (git log -L)
 + Export rewrite_parents() for 'log -L'
 + Refactor parse_loc


* jc/push-2.0-default-to-simple (2013-04-03) 13 commits
 - push: switch default from "matching" to "simple"
  (merged to 'next' on 2013-04-05 at 1b42c19)
 + t5570: do not assume the "matching" push is the default
 + t5551: do not assume the "matching" push is the default
 + t5550: do not assume the "matching" push is the default
 + t9401: do not assume the "matching" push is the default
 + t9400: do not assume the "matching" push is the default
 + t7406: do not assume the "matching" push is the default
 + t5531: do not assume the "matching" push is the default
 + t5519: do not assume the "matching" push is the default
 + t5517: do not assume the "matching" push is the default
 + t5516: do not assume the "matching" push is the default
 + t5505: do not assume the "matching" push is the default
 + t5404: do not assume the "matching" push is the default

 Update the test suite that still assumed the push.default will
 forever be 'matching'.  In Git 2.0, that will no longer be the
 case.

 Will merge to 'master' the early bits and cook the rest in 'next' until Git 2.0.

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

* fc/transport-helper-waitpid (2013-04-07) 3 commits
 . SQUASH???
 . transport-helper: check if remote helper is alive
 . [EXPLAIN BETTER] run-command: add new check_command helper

 fc/transport-helper-error-reporting supersedes this topic.


* jc/gg (2013-04-08) 3 commits
 . commit: add get_commit_encoding()
 . commit: rename parse_commit_date()
 . commit: shrink "indegree" field
 (this branch uses jc/decorate.)


* mc/count-objects-kibibytes (2013-04-14) 2 commits
  (merged to 'next' on 2013-04-14 at ff03f2b)
 + Revert "count-objects: output "KiB" instead of "kilobytes""
  (merged to 'next' on 2013-04-05 at f4e50e8)
 + count-objects: output "KiB" instead of "kilobytes"

 The command reports the total diskspace used to store loose objects
 in kibibytes, but it was labelled as "kilobytes".  The number now
 is shown with "KiB", e.g. "6750 objects, 50928 KiB".

 If you have scripts that decide when to run "git repack" by parsing
 the output from "git count-objects", this release may break them.
 Sorry about that.  One of the scripts shipped by git-core itself
 also had to be adjusted.  You may want to consider updating such
 scripts to always call "git gc --auto" to let it decide when to
 repack for you.

 Discarded.


* jc/decorate (2013-04-07) 2 commits
 - decorate: add "clear_decoration()"
 - decorate: document API
 (this branch is used by jc/gg.)

 Discarded.


* kb/status-ignored-optim (2013-03-19) 8 commits
  (merged to 'next' on 2013-04-01 at 0c12ed9)
 + dir.c: git-status: avoid is_excluded checks for tracked files
 + dir.c: replace is_path_excluded with now equivalent is_excluded API
 + dir.c: unify is_excluded and is_path_excluded APIs
 + dir.c: move prep_exclude and factor out parts of last_exclude_matching
 + dir.c: git-status --ignored: don't list empty directories as ignored
 + dir.c: git-status --ignored: don't list empty ignored directories
 + dir.c: git-status --ignored: don't list files in ignored directories
 + dir.c: git-status --ignored: don't drop ignored directories

 "git status --ignored" had many corner case bugs.  Also the command
 has been optimized by taking advantage of the fact that paths that
 are already known to the index do not have to be checked against
 the .gitignore mechanism most of the time.

 Discarded.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Mon, Apr 15, 2013 at 3:28 PM, Junio C Hamano <gitster@pobox.com> wrote:

> * fc/remote-hg (2013-04-11) 21 commits
>  - remote-hg: activate graphlog extension for hg_log()
>  - remote-hg: fix bad file paths
>  - remote-hg: document location of stored hg repository
>  - remote-hg: fix bad state issue
>  - remote-hg: add 'insecure' option
>  - remote-hg: add simple mail test
>  - remote-hg: add basic author tests
>  - remote-hg: show more proper errors
>  - remote-hg: force remote push
>  - remote-hg: push to the appropriate branch
>  - remote-hg: update tags globally
>  - remote-hg: update remote bookmarks
>  - remote-hg: refactor export
>  - remote-hg: split bookmark handling
>  - remote-hg: redirect buggy mercurial output
>  - remote-hg: trivial test cleanups
>  - remote-hg: make sure fake bookmarks are updated
>  - remote-hg: fix for files with spaces
>  - remote-hg: properly report errors on bookmark pushes
>  - remote-hg: add missing config variable in doc
>  - remote-hg: trivial cleanups
>
>  Rerolled.
>
>  Waiting for comments.

>From whom?


And about this:
http://mid.gmane.org/1365638832-9000-3-git-send-email-felipe.contreras@gmail.com

I think it's a disservice to git users to not consider this a "cooking
patch", specially since it's only the commit message somebody was
worried about. But whatever, don't.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> And about this:
> http://mid.gmane.org/1365638832-9000-3-git-send-email-felipe.contreras@gmail.com

What about it?  Is that the one you said you are going to reroll?

I do not recall the details of Peff's complaints, but re-reading the
log message of the patch itself, seeing "correctly" twice is not
satisfactory.  As you very well know, a bug description that says
"This does not correctly work!" and stops there is not as useful as
a description that defines what "correct" behaviour is expected.

If one of them said "update correctly to record what was pushed" or
something like that, that should be sufficient.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote:

> [Graduated to "master"]
> [...]
> * jk/http-error-messages (2013-04-06) 9 commits
>   (merged to 'next' on 2013-04-11 at 7a03981)
>  + http: drop http_error function
>  + remote-curl: die directly with http error messages
>  + http: re-word http error message
>  + http: simplify http_error helper function
>  + remote-curl: consistently report repo url for http errors
>  + remote-curl: always show friendlier 404 message
>  + remote-curl: let servers override http 404 advice
>  + remote-curl: show server content on http errors
>  + http: add HTTP_KEEP_ERROR option
> 
>  Improve error reporting from the http transfer clients.

I had not been keeping tabs on the progress of this topic, and was
surprised to see it in master already. It hadn't gotten any comments, so
I sort of assumed I would need to re-post to get interest. I don't mind,
but...

...the tip of your current master does not currently pass the test
suite[1]. I bisected the problem to "show server content on http
errors" from the above topic, but haven't figure it out past that. I
typically run "make test" before submitting, so I'm guessing it is an
interaction with another topic that graduated around the same time
(though it's also possible that I just failed to test after the last
rebase).

I'll investigate further, but it may be a few hours.

-Peff

[1] I know you always test master before pushing it out, but I suspect
    you do not run the GIT_TEST_HTTPD tests. The failures are in t5541
    and t5551.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Mon, Apr 15, 2013 at 6:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> And about this:
>> http://mid.gmane.org/1365638832-9000-3-git-send-email-felipe.contreras@gmail.com
>
> What about it?  Is that the one you said you are going to reroll?

At first, but then I changed my mind.

> I do not recall the details of Peff's complaints, but re-reading the
> log message of the patch itself, seeing "correctly" twice is not
> satisfactory.  As you very well know, a bug description that says
> "This does not correctly work!" and stops there is not as useful as
> a description that defines what "correct" behaviour is expected.

  The subject is: transport-helper: update remote helper namespace

Clearly, that's the correct behavior. Why would anybody send a change
that does something other than the correct behavior?

---
When pushing, the remote namespace is updated correctly
(e.g. refs/origin/master), but not the remote helper's
(e.g. refs/testgit/origin/master).
---

So it should be clear now: the remote namespace refs/origin/master is
updated, but not the remote helper's namespace
refs/testgit/origin/master, which is what I already said. I don't know
what more do you expect. When you push 'refs/heads/master' to origin,
you expect 'refs/remotes/origin/master' to point to the same commit,
same with 'refs/testgit/origin/master', why would you expect to point
somewhere else?

> If one of them said "update correctly to record what was pushed" or
> something like that, that should be sufficient.

Sure, it's still under the definition of "cooking" in my mind. Anyway,
I'm not feeling a great urge to resubmit this patch just because of
the commit message, which I think it's perfectly fine. There's more
interesting things work on.

Personally I feel it's preferable to fix the actual issue that is
already present rather than hold it because the commit message might
or might not be enough for hypothetical point in the future.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Øyvind A. Holm]

On 16 April 2013 01:25, Jeff King <peff@peff.net> wrote:
> On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote:
> > [Graduated to "master"]
> > [...]
> > * jk/http-error-messages (2013-04-06) 9 commits
> >   (merged to 'next' on 2013-04-11 at 7a03981)
> > [...]
>
> ...the tip of your current master does not currently pass the test
> suite[1].
> [...]
>
> [1] I know you always test master before pushing it out, but I suspect
>     you do not run the GIT_TEST_HTTPD tests. The failures are in t5541
>     and t5551.

Ah, that explains why the test suite passed here, I built a new
version an hour ago from current master (v1.8.2.1-418-gaec3f77,
2013-04-15 12:45:15 -0700), and no errors were found. I build new gits
almost every day for testing purposes (master, and next and maint very
often) on several machines with different setups, and of course also
to have the newest version. I'd like to run as many tests as possible.
Is there any list of environment variables or make directives
available to enable most of them?

Regards,
Øyvind

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Mon, Apr 15, 2013 at 07:25:32PM -0400, Jeff King wrote:

> On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote:
> 
> > * jk/http-error-messages (2013-04-06) 9 commits
> [...]
> ...the tip of your current master does not currently pass the test
> suite[1]. I bisected the problem to "show server content on http
> errors" from the above topic, but haven't figure it out past that. I
> typically run "make test" before submitting, so I'm guessing it is an
> interaction with another topic that graduated around the same time
> (though it's also possible that I just failed to test after the last
> rebase).

This patch on top of jk/http-error-messages fixes it.

-- >8 --
Subject: [PATCH] http: set curl FAILONERROR each time we select a handle

Because we reuse curl handles for multiple requests, the
setup of a handle happens in two stages: stable, global
setup and per-request setup. The lifecycle of a handle is
something like:

  1. get_curl_handle; do basic global setup that will last
     through the whole program (e.g., setting the user
     agent, ssl options, etc)

  2. get_active_slot; set up a per-request baseline (e.g.,
     clearing the read/write functions, making it a GET
     request, etc)

  3. perform the request with curl_*_perform functions

  4. goto step 2 to perform another request

Breaking it down this way means we can avoid doing global
setup from step (1) repeatedly, but we still finish step (2)
with a predictable baseline setup that callers can rely on.

Until commit 6d052d7 (http: add HTTP_KEEP_ERROR option,
2013-04-05), setting curl's FAILONERROR option was a global
setup; we never changed it. However, 6d052d7 introduced in
option where some requests might turn off FAILONERROR. Later
requests using the same handle would have the option
unexpectedly turned off, which meant they would not notice
http failures at all.

This could easily be seen in the test-suite for the
"half-auth" cases of t5541 and t5551. The initial requests
turned off FAILONERROR, which meant it was erroneously off
for the rpc POST. That worked fine for a successful request,
but meant that we failed to react properly to the HTTP 401
(instead, we treated whatever the server handed us as a
successful message body).

The solution is simple: now that FAILONERROR is a
per-request setting, we move it to get_active_slot to make
sure it is reset for each request.

Signed-off-by: Jeff King <peff@peff.net>
---
Hmph. I have no idea how this ever passed the tests, so I can only
assume that I screwed up in running them. I even recall considering this
issue while writing the patches, but I mixed up which of get_curl_handle
and get_active_slot it needed to be in when I did so.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 58c063c..48d4ff6 100644
--- a/http.c
+++ b/http.c
@@ -282,7 +282,6 @@ static CURL *get_curl_handle(void)
 #endif
 	if (ssl_cainfo != NULL)
 		curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
-	curl_easy_setopt(result, CURLOPT_FAILONERROR, 1);
 
 	if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
 		curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
@@ -506,6 +505,7 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
+	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
 	if (http_auth.password)
 		init_curl_http_auth(slot->curl);
 
-- 
1.8.2.8.g44e4c28

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Tue, Apr 16, 2013 at 01:49:13AM +0200, Øyvind A. Holm wrote:

> > [1] I know you always test master before pushing it out, but I suspect
> >     you do not run the GIT_TEST_HTTPD tests. The failures are in t5541
> >     and t5551.
> 
> Ah, that explains why the test suite passed here, I built a new
> version an hour ago from current master (v1.8.2.1-418-gaec3f77,
> 2013-04-15 12:45:15 -0700), and no errors were found. I build new gits
> almost every day for testing purposes (master, and next and maint very
> often) on several machines with different setups, and of course also
> to have the newest version. I'd like to run as many tests as possible.
> Is there any list of environment variables or make directives
> available to enable most of them?

I don't think there's a master list anywhere. The simplest thing is
probably to run the whole suite and grep for skipped tests, each of
which should usually explain their reasoning for the skip. Like:

  make test | grep '# skip'

Note that this will turn up more than just environment variables to set.
It will also turn up missing programs you might need to install to run
the tests (e.g., we do not do subversion tests if svn is not installed).
Which is a good thing if you are trying for complete test coverage. But
it will also turn up useless things that are just fundamental to your
platform (e.g., you cannot do both the case-sensitive and the
case-insensitive filesystem tests).

Most of the tests are on by default, unless necessary programs are
missing or you explicitly disable them. GIT_TEST_HTTPD and
GIT_TEST_GIT_DAEMON seem to be the exceptions.

-Peff

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Eric Sunshine]

On Mon, Apr 15, 2013 at 8:30 PM, Jeff King <peff@peff.net> wrote:
> Subject: [PATCH] http: set curl FAILONERROR each time we select a handle
>
> Until commit 6d052d7 (http: add HTTP_KEEP_ERROR option,
> 2013-04-05), setting curl's FAILONERROR option was a global
> setup; we never changed it. However, 6d052d7 introduced in

s/in/an/

> option where some requests might turn off FAILONERROR. Later
> requests using the same handle would have the option
> unexpectedly turned off, which meant they would not notice
> http failures at all.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Drew Northup]

n Mon, Apr 15, 2013 at 4:28 PM, Junio C Hamano <gitster@pobox.com> wrote:

> * jn/gitweb-install-doc (2013-04-15) 1 commit
>  - gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM
>
>  Reword gitweb configuration instrutions.
>
>  Will merge to 'next'.

While the re-worded text is easier on the eyes it fails to note why we
don't just dump our idiosyncratic way of doing things and just make
the system-wide defaults act individually. This information is useful
to system administrators, as it explains what is actually going on.

--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> So it should be clear now: the remote namespace refs/origin/master is
> updated, but not the remote helper's namespace
> refs/testgit/origin/master, which is what I already said. I don't know
> what more do you expect. When you push 'refs/heads/master' to origin,
> you expect 'refs/remotes/origin/master' to point to the same commit,
> same with 'refs/testgit/origin/master', why would you expect to point
> somewhere else?

Let me play somebody who comes later and wonders about this exchange
three months down the road...

You mention three refs/ here.  Do they live in the same repository?
Any Git person is expected to know refs/heads/master, which is "my
local branch I have worked on and I am pushing".  It also is easy to
guess what "refs/remotes/origin/master" is, even though we are not
talking about a usual Git remote.  It is to keep track of the remote
behind the helper we are pushing into, and is updated to pretend as
if we fetched immediately from the place we just pushed.  The latter
being in sync with what we pushed is something that can naturally be
expected.

Now, what is this third "refs/testgit/origin/master" thing?  Is it
expected to always be the same as "refs/remotes/origin/master"?  If
that is the case, why do we even need such a redundant information
in the first place?

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Mon, Apr 15, 2013 at 11:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> So it should be clear now: the remote namespace refs/origin/master is
>> updated, but not the remote helper's namespace
>> refs/testgit/origin/master, which is what I already said. I don't know
>> what more do you expect. When you push 'refs/heads/master' to origin,
>> you expect 'refs/remotes/origin/master' to point to the same commit,
>> same with 'refs/testgit/origin/master', why would you expect to point
>> somewhere else?
>
> Let me play somebody who comes later and wonders about this exchange
> three months down the road...
>
> You mention three refs/ here.  Do they live in the same repository?
> Any Git person is expected to know refs/heads/master, which is "my
> local branch I have worked on and I am pushing".  It also is easy to
> guess what "refs/remotes/origin/master" is, even though we are not
> talking about a usual Git remote.  It is to keep track of the remote
> behind the helper we are pushing into, and is updated to pretend as
> if we fetched immediately from the place we just pushed.  The latter
> being in sync with what we pushed is something that can naturally be
> expected.
>
> Now, what is this third "refs/testgit/origin/master" thing?  Is it
> expected to always be the same as "refs/remotes/origin/master"?  If
> that is the case, why do we even need such a redundant information
> in the first place?

Answering that question is beyond the scope of the commit message. The
purpose of the commit message is not to educate people about the
current design of transport-helpers, we have
Documentation/gitremote-helpers.txt for that. We also have discussions
in the mailing list.

The untold answer is 'if you have to ask, you don't understand this
code', but if you must, the short answer is: because it doesn't work
otherwise.

Yes, in theory not using a remote helper ref namespace should work,
but it doesn't, and I sent patches to try to fix this behavior, but
those, along with this particular patch, where ignored. So I gave up
on those patches that tried to fix the behavior for more corner cases
and tried to push the simplest one I could find, still finding
trouble. I tried to document that in t/t5801-remote-helpers.sh,
although to be honest, the tests that pass can be hardly be considered
proper behavior. This is of course, for import/export, which are the
only operations I'm familiar with (maybe the namespace makes sense for
push/fetch operations.

So basically I'm just tired of explaining the same things over and
over again, and if you try to explain every single detail, I think any
reasonable person would reach the conclusion that the design doesn't
really make sense. But the only person that seems to be trying to
explain it and improve it seems to be me.

The commit message is no place to explain all these subtleties, it
would be huge and would need to refer to plenty of mails in the
mailing list.

I could send the whole patch series I have, and then it might become
clearer why not having the refspec doesn't work, but then ,the chances
of this patch not getting through would be higher, as the last time I
sent such series, it didn't go through.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Clearly, that's the correct behavior. Why would anybody send a change
> that does something other than the correct behavior?

Along the same lines, why would anyone write broken code?  Nobody does,
right?

If anyone reads that commit message in more than a few weeks, then it's
because some of the code is *broken*.  So the reader is investigating a
situation where there must be a flaw somewhere, and trying to pin down
the source.  Having access to the thinking behind each commit means s/he
can more easily verify whether that thinking was correct and still
applies.

And your commit messages do nothing towards that end.


A cursory look^W^Wreview of the messages in fc/remote-hg:

    remote-hg: fix bad file paths
    
    Mercurial allows absolute file paths, and Git doesn't like that.

Only describes the problem; no reasoning as to what the chosen solution
is or why it is correct.  (I can at least infer the former from the
code, but not the latter.)

    remote-hg: show more proper errors
    
    When cloning or pushing fails, we don't want to show a stack-trace.

So what do we show?

It also seems that you do not actually use the import you add, or do
you?

    remote-hg: force remote push
    
    Ideally we shouldn't do this, as it's not recommended in mercurial
    documentation, but there's no other way to push multiple bookmarks (on
    the same branch), which would be the behavior most similar to git.
    
    At the same time, add a configuration option for the people that don't
    want to risk creating new remote heads.

This one, for a change, says what it does but doesn't say what problem
it fixes.

I'll refrain from commenting on all the one-line messages, and just
point at this one:

    remote-hg: trivial test cleanups

In $DAYJOB the advice is to avoid "trivial" (and similarly "obvious"):
either it *is* trivial, in which case you don't need to point that out,
or you're just trying to handwave over the fact that it's not.  Like
this:

 git_clone () {
-       hg -R $1 bookmark -f -r tip master &&
        git clone -q "hg::$PWD/$1" $2
 }

Not knowing the code I can only conjecture, but surely there was a
reason that the hg call lived in a function called git_clone?  And
surely there must be a good reason why it is no longer needed?


My personal favorite however is this one:

    remote-bzr: improve tag handling
    
    revision_history() is deprecated and doesn't do what we want (revno
    instead of dotted_revno?).

I don't even know how to parse that question mark.  Does it actually ask
a question?  Does it mean to imply, by the intonation suggested by a
question mark, "how could anyone ever have been so silly as to use a
revno instead of a dotted_revno"?


By the way, it's easy to find similarly helpful messages in git.git in
the old days.  One that I remember stumbling across was:

    Add the --color-words option to the diff options family
    
    With this option, the changed words are shown inline. For example,
    if a file containing "This is foo" is changed to "This is bar", the diff
    will now show "This is " in plain text, "foo" in red, and "bar" in green.

How could it not be obvious how it achieves this to anyone who has read
the ~170 lines of code it adds?

Luckily *that* code was correct and feature-complete right from the
start, so nobody ever had to actually read it to figure out what's going
on.

But that was back in 2006.  I should think that git.git has improved
since; when I wrote my first patches in 2008, I was impressed with the
readable history and extensive reviews.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The solution is simple: now that FAILONERROR is a
> per-request setting, we move it to get_active_slot to make
> sure it is reset for each request.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Hmph. I have no idea how this ever passed the tests, so I can only
> assume that I screwed up in running them. I even recall considering this
> issue while writing the patches, but I mixed up which of get_curl_handle
> and get_active_slot it needed to be in when I did so.

Thanks.

>  http.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 58c063c..48d4ff6 100644
> --- a/http.c
> +++ b/http.c
> @@ -282,7 +282,6 @@ static CURL *get_curl_handle(void)
>  #endif
>  	if (ssl_cainfo != NULL)
>  		curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
> -	curl_easy_setopt(result, CURLOPT_FAILONERROR, 1);
>  
>  	if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
>  		curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
> @@ -506,6 +505,7 @@ struct active_request_slot *get_active_slot(void)
>  	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
>  	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
> +	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
>  	if (http_auth.password)
>  		init_curl_http_auth(slot->curl);
Thanks

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Clearly, that's the correct behavior. Why would anybody send a change
>> that does something other than the correct behavior?
>
> Along the same lines, why would anyone write broken code?  Nobody does,
> right?

Yes, I should change the subject to:

  transport-helper: update remote helper namespace, because that's
exactly the thing we DON'T want to do, the purpose of this patch is to
mess up everything

Suree. I'm willing and knowingly introducing a change that goes
diametrically opposite to what we want.

> If anyone reads that commit message in more than a few weeks, then it's
> because some of the code is *broken*.

That is irrelevant. Junio said the correct behavior was not described,
when if fact it clearly is. Whether or not the patch has a bug in it
is irrelevant to the fact that the correct behavior is described or
not.

> So the reader is investigating a
> situation where there must be a flaw somewhere, and trying to pin down
> the source.  Having access to the thinking behind each commit means s/he
> can more easily verify whether that thinking was correct and still
> applies.

Sure, and where is the thinking not clear? The remote helper ref is
not updated, so we do update it. How is that not clear?

> And your commit messages do nothing towards that end.

Oh, it does. You just don't understand how remote-helper works.

> A cursory look^W^Wreview of the messages in fc/remote-hg:

[skipping irrelevant comments]

I'm sorry, did you actually hit an issue that required to look at the
commit message to understand where the issue came from? No? Then I
won't bother with hypotheticals.

If you want to waste your time, by all means, rewrite all my commit
messages with essays that nobody will ever read. I'm not going to do
that for some hypothetical case that will never happen. I'm not going
to waste my time.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Sure, and where is the thinking not clear? The remote helper ref is
> not updated, so we do update it. How is that not clear?

Sure, between "leaving it untouched, keeping the stale value" and
"updating it to match what was pushed", everybody would know you
mean the latter when you say "correctly update".  There is no third
option "updating it to match a random commit that is related to but
is not exactly the same as what was pushed" to be correct.

What I felt unclear was _why_ both of these two (remote and testgit)
have to get updated.  In other words, "correctly update it" because
"without doing so, these bad things X, Y and Z will happen".

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Tue, Apr 16, 2013 at 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Sure, and where is the thinking not clear? The remote helper ref is
>> not updated, so we do update it. How is that not clear?
>
> Sure, between "leaving it untouched, keeping the stale value" and
> "updating it to match what was pushed", everybody would know you
> mean the latter when you say "correctly update".  There is no third
> option "updating it to match a random commit that is related to but
> is not exactly the same as what was pushed" to be correct.
>
> What I felt unclear was _why_ both of these two (remote and testgit)
> have to get updated.  In other words, "correctly update it" because
> "without doing so, these bad things X, Y and Z will happen".

The bad thing that would happen is that it won't be up-to-date.

If you don't know what an outdated ref causes, then you don't know
what transport-helper does with it, and if you don't know that, why
are you bothering trying to review this patch? Is the purpose of a
patch to educate people?

Here it goes. The remote helper ref is going to be used to tell
fast-export which refs to negate (e.g. ^refs/testgit/origin/master),
so that extra commits are not generated, which the remote helper
should ignore anyway, because it should already have marks for those.
So doing two consecutive pushes, would push the commits twice.

It's worth noting this is the first time anybody asks what is the
negative effect of this not getting fixed.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Phil Hord]

On Tue, Apr 16, 2013 at 3:48 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Here it goes. The remote helper ref is going to be used to tell
> fast-export which refs to negate (e.g. ^refs/testgit/origin/master),
> so that extra commits are not generated, which the remote helper
> should ignore anyway, because it should already have marks for those.
> So doing two consecutive pushes, would push the commits twice.
>
> It's worth noting this is the first time anybody asks what is the
> negative effect of this not getting fixed.

Yes, but what is noteworthy about it is that you did not include that
in your commit message to begin with.  This is the commit message
request from Documentation/SubmittingPatches:

    The body should provide a meaningful commit message, which:

      . explains the problem the change tries to solve, iow, what is wrong
        with the current code without the change.

      . justifies the way the change solves the problem, iow, why the
        result with the change is better.

      . alternate solutions considered but discarded, if any.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Phil Hord]

On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> A cursory look^W^Wreview of the messages in fc/remote-hg:
>
> [skipping irrelevant comments]
>
> I'm sorry, did you actually hit an issue that required to look at the
> commit message to understand where the issue came from? No? Then I
> won't bother with hypotheticals.
>
> If you want to waste your time, by all means, rewrite all my commit
> messages with essays that nobody will ever read. I'm not going to do
> that for some hypothetical case that will never happen. I'm not going
> to waste my time.

This is not a hypothetical.  Almost every time I bisect a regression
in git.git, I find the commit message tells me exactly why the commit
did what it did and what the expected result was.  I find this to be
amazingly useful.  Do I need to show you real instances of that
happening? No.  I promise it did, though.

Of course, 99% of the commit messages may never be useful to me or
anyone else.  But we do not eschew them altogether.  The 1% I have to
rely on are nearly always helpful and clear, and that is the part I
care about.

If you will not waste your time to write a decent commit message, why
do you waste our time asking us to review and accept ill-defined
patches?  Here, of course, I use the royal "us" as I do not review
your patches.  I do not know why that is; I suppose you patch things
outside of my interests, but it may also be that your patches are
simply incomprehensible by design.

Phil

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Tue, Apr 16, 2013 at 5:34 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Tue, Apr 16, 2013 at 3:48 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Here it goes. The remote helper ref is going to be used to tell
>> fast-export which refs to negate (e.g. ^refs/testgit/origin/master),
>> so that extra commits are not generated, which the remote helper
>> should ignore anyway, because it should already have marks for those.
>> So doing two consecutive pushes, would push the commits twice.
>>
>> It's worth noting this is the first time anybody asks what is the
>> negative effect of this not getting fixed.
>
> Yes, but what is noteworthy about it is that you did not include that
> in your commit message to begin with.  This is the commit message
> request from Documentation/SubmittingPatches:

And yet, nobody asked for that.

Anyway, drop the patch then.

-- 
Felipe Contreras

"What's cooking" between #05 and #06

[Junio C Hamano]

Quick incremental report on tonight's integration status.

> * jk/http-error-messages (2013-04-06) 9 commits
>   (merged to 'next' on 2013-04-11 at 7a03981)
>  ...
>  + http: add HTTP_KEEP_ERROR option
>
>  Improve error reporting from the http transfer clients.

Peff posted an update to this to fix a regression, which is parked
on 'next' for tonight.

> * kb/status-ignored-optim-2 (2013-04-15) 14 commits
>  ...
>
>  Rerolls kb/status-ignored-optim topic (reverted from 'next').  Not
>  merged to 'pu' as it heavily interferes with as/check-ignore topic.

This still has not been merged to 'pu' for the same reason.

> * jk/remote-helper-with-signed-tags (2013-04-15) 3 commits
>  - transport-helper: add 'signed-tags' capability
>  - transport-helper: pass --signed-tags=warn-strip to fast-export
>  - fast-export: add --signed-tags=warn-strip mode

There were some comments on the noisiness of the warning output, but
it appears that everybody involved in the area is basically happy
with the direction this series goes in, so I'll expect a reroll and
then merge it to 'next'.

> * jn/gitweb-install-doc (2013-04-15) 1 commit
>  - gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM
>
>  Reword gitweb configuration instrutions.

This hasn't been merged to 'next'.

There are a few competing proposals to further update it.  I am
inclined to take Jonathan's version.

> * nd/pretty-formats (2013-04-01) 12 commits
> ...

Updated the one in 'pu' with the reroll.

> * fc/transport-helper-error-reporting (2013-04-11) 3 commits
>  - transport-helper: improve push messages
>  - transport-helper: mention helper name when it dies
>  - transport-helper: report errors properly
>
>  Rerolled enough times.  In-code comments may want to be further
>  extended to explain tricky parts, but seems to be ready otherwise.
>
>  Will merge to 'next'.

This is in 'next' with the last one (missing from the above list)
with slightly tweaked commit log message.

> * jk/submodule-subdirectory-ok (2013-04-10) 2 commits
>  - submodule: drop the top-level requirement
>  - rev-parse: add --prefix option
>
>  Allow various subcommands of "git submodule" to be run not from the
>  top of the working tree of the superproject.
>
>  Waiting for comments.

Any submodule users wants to weigh in?  The code looked fine, but I
do not heavily use it (and the repository with a submodule I have, I
do not have a "subdirectory" ;-, so I am a bad guinea pig).


> * mv/sequencer-pick-error-diag (2013-04-11) 1 commit
>  - cherry-pick: make sure all input objects are commits
>
>  "git cherry-pick $blob $tree" is diagnosed as a nonsense.
>
>  Will merge to 'next'.

This, with the help with a fix to a long-standing issue in the
command line parser for revisions from Thomas, is now in 'next'.

> * fc/remote-hg (2013-04-11) 21 commits
> ...
>  Rerolled.
>
>  Waiting for comments.

I think this has gone as far as it can go with the people who are
interested in reviewing this series on the list.  It is in 'next'
now.

> * nd/magic-pathspecs (2013-03-31) 45 commits
> ...
>
>  Migrate the rest of codebase to use "struct pathspec" more.

Still out of 'pu' for the same reason as yesterday (and as
kb/status-ignored-optim topic).

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> ...  Almost every time I bisect a regression
> in git.git, I find the commit message tells me exactly why the commit
> did what it did and what the expected result was.  I find this to be
> amazingly useful.
> ...
> Of course, 99% of the commit messages may never be useful to me or
> anyone else.  But we do not eschew them altogether.  The 1% I have to
> rely on are nearly always helpful and clear, and that is the part I
> care about.

It is nice to be appreciated for our efforts by somebody every once
in a while ;-)

Thanks.

Re: "What's cooking" between #05 and #06

[John Keeping]

On Tue, Apr 16, 2013 at 04:52:14PM -0700, Junio C Hamano wrote:
> > * jk/remote-helper-with-signed-tags (2013-04-15) 3 commits
> >  - transport-helper: add 'signed-tags' capability
> >  - transport-helper: pass --signed-tags=warn-strip to fast-export
> >  - fast-export: add --signed-tags=warn-strip mode
> 
> There were some comments on the noisiness of the warning output, but
> it appears that everybody involved in the area is basically happy
> with the direction this series goes in, so I'll expect a reroll and
> then merge it to 'next'.

What do you expect to change in the reroll?  The only comments I've seen
have been about the warning output it seems to me that we've agreed to
leave that as it is.  Have I missed something?

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Lukas Fleischer]

On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote:
> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.
> [...]
> --------------------------------------------------
> [New Topics]
> [...]
> * lf/read-blob-data-from-index (2013-04-15) 3 commits
>   (merged to 'next' on 2013-04-15 at 09f92c6)
>  + convert.c: Remove duplicate code
>  + Add size parameter to read_blob_data_from_index_path()
>  + Add public function read_blob_data_from_index_path()
> 
>  Reduce duplicated code between convert.c and attr.c.
> 
>  Will merge to 'master'.

Not sure if you care but the commit messages of these are all wrong now
that you squashed your API fix into the first commit. They all refer to
read_blob_data_from_index_path() instead of read_blob_data_from_index()
and most of the details mentioned in the first commit of this series no
longer apply...

Just saying :)

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Thomas Rast]

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

> * jc/add-2.0-delete-default (2013-03-08) 3 commits
>  - git add <pathspec>... defaults to "-A"
>   (merged to 'next' on 2013-04-05 at 199442e)
>  + git add: start preparing for "git add <pathspec>..." to default to "-A"
>  + builtin/add.c: simplify boolean variables
>
>  In Git 2.0, "git add pathspec" will mean "git add -A pathspec".  If
>  you did this in a working tree that tracks dir/lost and dir/another:
>
>  $ rm dir/lost
>  $ edit dir/another
>  $ git add dir
>
>  The last step will not only notices and records updated
>  dir/another, but also notices and records the removal of dir/lost
>  in the index.
>
>  Start training the users for this change to say --no-all when they
>  want to ignore the removal to smooth the transition hump.
>
>  Will merge to 'master' the early bits and cook the rest in 'next' until Git 2.0.

The warning triggers in some cases where it shouldn't, relating to
submodules:

  $ git submodule add gitosis@git.csa.inf.ethz.ch:domjudge.git domjudge
  Adding existing repo at 'domjudge' to the index                                                        
  warning: In Git 2.0, 'git add <pathspec>...' will also update the                                      
  index for paths removed from the working tree that match                                               
  the given pathspec. If you want to 'add' only changed                                                  
  or newly created paths, say 'git add --no-all <pathspec>...' instead.

It also seems to hint that the problem is with giving a 'pathspec', but
in fact in the case of a "proper" pathspec (that isn't an existing path)
it does *not* trigger, even though it probably should:

  $ git ls-files
  foo
  $ rm foo
  $ git add 'f*'
  $ git status
  # On branch master
  # Changes not staged for commit:
  #   (use "git add/rm <file>..." to update what will be committed)
  #   (use "git checkout -- <file>..." to discard changes in working directory)
  #
  #       deleted:    foo
  #
  no changes added to commit (use "git add" and/or "git commit -a")
  $ git add -A 'f*'
  $ git status
  # On branch master
  # Changes to be committed:
  #   (use "git reset HEAD <file>..." to unstage)
  #
  #       deleted:    foo
  #
  # Untracked files not listed (use -u option to show untracked files)

That's of course assuming that you want to unconditionally make -A the
default.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Lukas Fleischer <git@cryptocrack.de> writes:

> Not sure if you care but the commit messages of these are all wrong now
> that you squashed your API fix into the first commit. They all refer to
> read_blob_data_from_index_path()...

Ouch; thanks for noticing.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> The warning triggers in some cases where it shouldn't, relating to
> submodules:
>
>   $ git submodule add gitosis@git.csa.inf.ethz.ch:domjudge.git domjudge
>   Adding existing repo at 'domjudge' to the index
>   warning: In Git 2.0, 'git add <pathspec>...' will also update
>   the index for paths removed from the working tree that match
>   the given pathspec. If you want to 'add' only changed
>   or newly created paths, say 'git add --no-all <pathspec>...' instead.

Good one.  So "add" used internally there needs to say --no-add?

> It also seems to hint that the problem is with giving a 'pathspec', but
> in fact in the case of a "proper" pathspec (that isn't an existing path)
> it does *not* trigger, even though it probably should:

We have seen users who explicitly say:

	git add dir

after removing dir/del and adding dir/ins got surprised that we do
not notice removal of dir/del without "add -A".  And it is fairly
straight-forward to check and warn for such a case.

> That's of course assuming that you want to unconditionally make -A the
> default.

I thought what the warning text says is what we decided to do
eventually.  Not "unconditionally", but "with <pathspec>", but not
"only with pathspec that exactly matches an existing path".  It
appears that we would better discuss and decide such details
further, so let's revert the "warn early" bits from master and kick
the topic back.

Re: "What's cooking" between #05 and #06

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Tue, Apr 16, 2013 at 04:52:14PM -0700, Junio C Hamano wrote:
>> > * jk/remote-helper-with-signed-tags (2013-04-15) 3 commits
>> >  - transport-helper: add 'signed-tags' capability
>> >  - transport-helper: pass --signed-tags=warn-strip to fast-export
>> >  - fast-export: add --signed-tags=warn-strip mode
>> 
>> There were some comments on the noisiness of the warning output, but
>> it appears that everybody involved in the area is basically happy
>> with the direction this series goes in, so I'll expect a reroll and
>> then merge it to 'next'.
>
> What do you expect to change in the reroll?  The only comments I've seen
> have been about the warning output it seems to me that we've agreed to
> leave that as it is.  Have I missed something?

You missed the sender timestamp of the message you are responding
to, and that of the discussion we later agreed there is nothing to
change ;-)

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Thomas Rast]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> The warning triggers in some cases where it shouldn't, relating to
>> submodules:
>>
>>   $ git submodule add gitosis@git.csa.inf.ethz.ch:domjudge.git domjudge
>>   Adding existing repo at 'domjudge' to the index
>>   warning: In Git 2.0, 'git add <pathspec>...' will also update
>>   the index for paths removed from the working tree that match
>>   the given pathspec. If you want to 'add' only changed
>>   or newly created paths, say 'git add --no-all <pathspec>...' instead.
>
> Good one.  So "add" used internally there needs to say --no-add?

I think the logic in git-add needs to learn about submodules.  The same
warning later trigger when you later say 'git add submoduledir', even
though that obviously doesn't walk inside the submodule.

>> It also seems to hint that the problem is with giving a 'pathspec', but
>> in fact in the case of a "proper" pathspec (that isn't an existing path)
>> it does *not* trigger, even though it probably should:
>
> We have seen users who explicitly say:
>
> 	git add dir
>
> after removing dir/del and adding dir/ins got surprised that we do
> not notice removal of dir/del without "add -A".  And it is fairly
> straight-forward to check and warn for such a case.

I can see that problem, but along the same lines, why shouldn't I have
an expectation that when I say 'git add "*.py"' it removes stuff that I
have removed?  That's what I tried to show with the f?o example.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> I can see that problem, but along the same lines, why shouldn't I have
> an expectation that when I say 'git add "*.py"' it removes stuff that I
> have removed?

You _should_ have that expectation.

If it does not remove with the code that has been prepared for 2.0
(that is a bit beyond 'next'), then it is a big problem, but I think
it does remove the removed python source without "-A", as long as
you give a pathspec "*.py" (with quotes around it) that match it.

I think it is just the warning code avoiding extra complexity and
overhead, if you are talking about not getting warning in the
pre-2.0 step that is in 'next'.  Patches are very much welcomed,
especially the ones that come before I get around to it ;-)

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> I can see that problem, but along the same lines, why shouldn't I have
>> an expectation that when I say 'git add "*.py"' it removes stuff that I
>> have removed?
>
> You _should_ have that expectation.
>
> If it does not remove with the code that has been prepared for 2.0
> (that is a bit beyond 'next'), then it is a big problem, but I think
> it does remove the removed python source without "-A", as long as
> you give a pathspec "*.py" (with quotes around it) that match it.
>
> I think it is just the warning code avoiding extra complexity and
> overhead, if you are talking about not getting warning in the
> pre-2.0 step that is in 'next'.  Patches are very much welcomed,
> especially the ones that come before I get around to it ;-)

I took a brief look at the code, and as you said "add" needs to know
about submodules, and the best fix looks to me to take the same
approach Jonathan came up with to de-noise the "add -u/-A" topic.

That is, to scan the working tree to actually see if we would record
removals to the index in 2.0, but not remove them in this current
version, and give the warning when the differences in the behaviours
matter.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Tue, Apr 16, 2013 at 5:45 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> A cursory look^W^Wreview of the messages in fc/remote-hg:
>>
>> [skipping irrelevant comments]
>>
>> I'm sorry, did you actually hit an issue that required to look at the
>> commit message to understand where the issue came from? No? Then I
>> won't bother with hypotheticals.
>>
>> If you want to waste your time, by all means, rewrite all my commit
>> messages with essays that nobody will ever read. I'm not going to do
>> that for some hypothetical case that will never happen. I'm not going
>> to waste my time.
>
> This is not a hypothetical.  Almost every time I bisect a regression
> in git.git, I find the commit message tells me exactly why the commit
> did what it did and what the expected result was.  I find this to be
> amazingly useful.  Do I need to show you real instances of that
> happening? No.  I promise it did, though.

Yes please. Show me one of the instances where you hit a bisect with
any of the remote-hg commits mentioned above by Thomas Rast.

> Of course, 99% of the commit messages may never be useful to me or
> anyone else.  But we do not eschew them altogether.  The 1% I have to
> rely on are nearly always helpful and clear, and that is the part I
> care about.

And how do you know this will be part of the 1%? You don't. How many
times have you tracked regressions in transport helper's import/export
functionality? How many times in remote-hg? How many times has
*anybody* done so?

> If you will not waste your time to write a decent commit message, why
> do you waste our time asking us to review and accept ill-defined
> patches?

Because it *fixes a problem*. And a commit essay doesn't fix any,
because nobody will ever go back in history and wonder, hey, what is
up with this commit. If somebody does, then I will accept that commit
essays are always a must. But it won't happen.

> Here, of course, I use the royal "us" as I do not review
> your patches.  I do not know why that is; I suppose you patch things
> outside of my interests, but it may also be that your patches are
> simply incomprehensible by design.

Yeah, but that's the thing, if you don't understand the code the
patches are changing, then how can you know the commit message is
sufficient to figure things out when a regression is found? You don't.
You can't.

Let's face the truth, you are advocating for stopping progress on the
name that something might happen sometime in the feature, although
most likely won't. When in reality, it just won't.

And you are not saying "it would be nice to have full commit essay",
you are saying: "without a commit essay this patch should NOT be
merged", even more "without a commit essay this patch should NOT be
considered a cooking patch".

I think the commit message is fine, you don't. So YOU go ahead and
write the proper one. If you don't, all you are doing is being an
impediment to progress.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Wed, Apr 17, 2013 at 11:14:42AM -0700, Junio C Hamano wrote:

> > I think it is just the warning code avoiding extra complexity and
> > overhead, if you are talking about not getting warning in the
> > pre-2.0 step that is in 'next'.  Patches are very much welcomed,
> > especially the ones that come before I get around to it ;-)
> 
> I took a brief look at the code, and as you said "add" needs to know
> about submodules, and the best fix looks to me to take the same
> approach Jonathan came up with to de-noise the "add -u/-A" topic.
> 
> That is, to scan the working tree to actually see if we would record
> removals to the index in 2.0, but not remove them in this current
> version, and give the warning when the differences in the behaviours
> matter.

Yeah, I had the same thought, as this warning has been bugging me for
the last day or two. The worst part about it is that I finally trained
myself to type "git add ." to silence the _other_ warning, and now it
triggers this one. :)

-Peff

Re: "What's cooking" between #05 and #06

[Jens Lehmann]

Am 17.04.2013 01:52, schrieb Junio C Hamano:
>> * jk/submodule-subdirectory-ok (2013-04-10) 2 commits
>>  - submodule: drop the top-level requirement
>>  - rev-parse: add --prefix option
>>
>>  Allow various subcommands of "git submodule" to be run not from the
>>  top of the working tree of the superproject.
>>
>>  Waiting for comments.
> 
> Any submodule users wants to weigh in?  The code looked fine, but I
> do not heavily use it (and the repository with a submodule I have, I
> do not have a "subdirectory" ;-, so I am a bad guinea pig).

I like it, as it gets rid of the top-level requirement. But from
my testing it looks like we're not quite there yet.

'summary' and 'status' behave as if they were run in the toplevel
directory, while a "git status" shows all filenames relative to the
current directory. Me thinks 'summary' and 'status' (and all other
submodule commands) should behave like status and print relative
paths too. I'm not really sure yet how $sm_path should behave for
'foreach', but I suspect having it relative to the current
directory would be the way to go (which it currently isn't).

When "submodule add" is run with a relative path it is relative to
the top-level directory, which I find confusing (and won't play
well with shell completion).

'deinit .' doesn't deinit submodules above the current directory
(but prints the path relative to top-level) while 'init' will
initialize all submodules known to the superproject.

So this is a good start, but it looks like there is some work left
to do before this can hit master.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> And how do you know this will be part of the 1%? You don't. How many
> times have you tracked regressions in transport helper's import/export
> functionality? How many times in remote-hg? How many times has
> *anybody* done so?

The last point makes it all the more important to have a good
history [*1*]. An area that no developer rarely touches with a little
user base can stay dormant for a long time, and when people do need
to hunt for an ancient bug or to enhance the existing feature to
support a new use case without breaking the old use case, the
original author may not be around, lost interest, or no longer uses
his own creation.

The code left behind tells us what the author thought was the best
way to solve his problem, but it does not clearly define what the
problem he tried to solve was, within what constraint he had to find
a solution for it, and why he thought that the solution was the best
(or sometimes "only") one.  Log and in-code comments are to explain
such things that are beyond how the code works and what it does.


[Footnote]

*1* In this message, I am not judging if the depth of your writing
    for the particular change is deep enough. It depends on how well
    the reader knows the area, and there is no single right answer
    to that question.
    
    Incidentally that is why we tend to err on the more descriptive
    side. The next person your commit will help may not know the
    area as well as you do and has to figure things out on his
    own. You are helping him by being descriptive.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I had the same thought, as this warning has been bugging me for
> the last day or two. The worst part about it is that I finally trained
> myself to type "git add ." to silence the _other_ warning, and now it
> triggers this one. :)

So here is the "reworked" one on top of what is in 'next'.

It introduces a bit of conflict with the "add -u/-A" topic, so I am
not ready to push out the integration result yet.

-- >8 --
Subject: [PATCH] git add: rework the logic to warn "git add <pathspec>..." default change

The earlier logic to warn against "git add subdir" that is run
without "-A" or "--no-all" was only to check any <pathspec> given
exactly spells a directory name that (still) exists on the
filesystem.  This had number of problems:

 * "git add '*dir'" (note that the wildcard is hidden from the
   shell) would not trigger the warning.

 * "git add '*.py'" would behave differently between the current
   version of Git and Git 2.0 for the same reason as "subdir", but
   would not trigger the warning.

 * "git add dir" for a submodule "dir" would just update the index
   entry for the submodule "dir" without ever recursing into it, and
   use of "-A" or "--no-all" would matter.  But the logic only
   checks the directory-ness of "dir" and gives an unnecessary
   warning.

Rework the logic to detect the case where the behaviour will be
different in Git 2.0, and issue a warning only when it matters.
Even with the code before this warning, "git add subdir" will have
to traverse the directory in order to find _new_ files the index
does not know about _anyway_, so we can do this check without adding
an extra pass to find if <pathspec> matches any removed file.

This essentially updates the "add_files_to_cache()" public API to
"update_files_in_cache()" API that is internal to "git add", because
with the "--all" option, the function is no longer about "adding"
paths to the cache, but is also used to remove them.

There are other callers of the former from "checkout" (used when
"checkout -m" prepares the temporary tree that represents the local
modifications to be merged) and "commit" ("commit --include" that
picks up local changes in addition to what is in the index).  Since
ADD_CACHE_IGNORE_ERRORS (aka "--no-all") is not used by either of
them, once dust settles after Git 2.0 and the warning becomes
unnecessary, we may want to unify these two functions again.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c | 64 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index f8f6c9e..4242bce 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,6 +26,9 @@ static int take_worktree_changes;
 struct update_callback_data {
 	int flags;
 	int add_errors;
+
+	/* only needed for 2.0 transition preparation */
+	int warn_add_would_remove;
 };
 
 static int fix_unmerged_status(struct diff_filepair *p,
@@ -49,6 +52,17 @@ static int fix_unmerged_status(struct diff_filepair *p,
 		return DIFF_STATUS_MODIFIED;
 }
 
+static void warn_add_would_remove(const char *path)
+{
+	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
+		  "index for paths removed from the working tree that match\n"
+		  "the given pathspec. If you want to 'add' only changed\n"
+		  "or newly created paths, say 'git add --no-all <pathspec>...'"
+		  " instead.\n\n"
+		  "'%s' would be removed from the index without --no-all."),
+		path);
+}
+
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
@@ -70,6 +84,10 @@ static void update_callback(struct diff_queue_struct *q,
 			}
 			break;
 		case DIFF_STATUS_DELETED:
+			if (data->warn_add_would_remove) {
+				warn_add_would_remove(path);
+				data->warn_add_would_remove = 0;
+			}
 			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
 				break;
 			if (!(data->flags & ADD_CACHE_PRETEND))
@@ -81,20 +99,27 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+static void update_files_in_cache(const char *prefix, const char **pathspec,
+				  struct update_callback_data *data)
 {
-	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	init_pathspec(&rev.prune_data, pathspec);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
-	data.flags = flags;
-	data.add_errors = 0;
-	rev.diffopt.format_callback_data = &data;
+	rev.diffopt.format_callback_data = data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
+}
+
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
+{
+	struct update_callback_data data;
+
+	memset(&data, 0, sizeof(data));
+	data.flags = flags;
+	update_files_in_cache(prefix, pathspec, &data);
 	return !!data.add_errors;
 }
 
@@ -354,18 +379,6 @@ static void warn_pathless_add(const char *option_name, const char *short_name) {
 		option_name, short_name);
 }
 
-static int directory_given(int argc, const char **argv)
-{
-	struct stat st;
-
-	while (argc--) {
-		if (!lstat(*argv, &st) && S_ISDIR(st.st_mode))
-			return 1;
-		argv++;
-	}
-	return 0;
-}
-
 int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
@@ -378,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	char *seen = NULL;
 	const char *option_with_implicit_dot = NULL;
 	const char *short_option_with_implicit_dot = NULL;
+	struct update_callback_data update_data;
 
 	git_config(add_config, NULL);
 
@@ -403,15 +417,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Warn when "git add pathspec..." was given without "-u" or "-A"
-	 * and pathspec... contains a directory name.
+	 * and pathspec... covers a removed path.
 	 */
-	if (!take_worktree_changes && addremove_explicit < 0 &&
-	    directory_given(argc, argv))
-		warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
-			  "index for paths removed from the working tree that match\n"
-			  "the given pathspec. If you want to 'add' only changed\n"
-			  "or newly created paths, say 'git add --no-all <pathspec>...'"
-			  " instead."));
+	memset(&update_data, 0, sizeof(update_data));
+	if (!take_worktree_changes && addremove_explicit < 0)
+		update_data.warn_add_would_remove = 1;
 
 	if (!take_worktree_changes && addremove_explicit < 0 && argc)
 		/*
@@ -508,8 +518,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	exit_status |= add_files_to_cache(prefix, pathspec, flags);
+	update_data.flags = flags;
+	update_files_in_cache(prefix, pathspec, &update_data);
 
+	exit_status |= !!update_data.add_errors;
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
 
-- 
1.8.2.1-552-g964983e

[PATCH] git add <pathspec>... defaults to "-A"

[Junio C Hamano]

Make "git add <pathspec>..." notice paths that have been removed
from the working tree, i.e. a synonym to "git add -A <pathspec>...".

Given that "git add <pathspec>" is to update the index with the
state of the named part of the working tree as a whole, it makes it
more intuitive, and also makes it possible to simplify the advice we
give while marking the paths the user finished resolving conflicts
with.  We used to say "to record removal as a resolution, remove the
path from the working tree and say 'git rm'; for all other cases,
edit the path in the working tree and say 'git add'", but we can now
say "update the path in the working tree and say 'git add'" instead.

As promised, this merges the temporary update_files_in_cache() helper
function back to add_files_to_cache() function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * So this comes on top of the previous update that will sit at
   jc/add-2.0-delete-default~1 and replaces the one previouly at its
   tip.

 Documentation/git-add.txt | 18 +++++++++------
 builtin/add.c             | 57 ++++++++---------------------------------------
 2 files changed, 20 insertions(+), 55 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 5c501a2..5bd0791 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -53,8 +53,14 @@ OPTIONS
 	Files to add content from.  Fileglobs (e.g. `*.c`) can
 	be given to add all matching files.  Also a
 	leading directory name (e.g. `dir` to add `dir/file1`
-	and `dir/file2`) can be given to add all files in the
-	directory, recursively.
+	and `dir/file2`) can be given to update the index to
+	match the current state of the directory as a whole (e.g.
+	specifying `dir` will record not just a file `dir/file1`
+	modified in the working tree, a file `dir/file2` added to
+	the working tree, but also a file `dir/file3` removed from
+	the working tree.  Note that older versions of 	Git used
+	to ignore removed files; use `--no-all` option if you want
+	to add modified or new files but ignore removed	ones.
 
 -n::
 --dry-run::
@@ -127,11 +133,9 @@ of Git, hence the form without <pathspec> should not be used.
 	files that have been removed from the working tree.  This
 	option is a no-op when no <pathspec> is used.
 +
-This option is primarily to help the current users of Git, whose
-"git add <pathspec>..." ignores removed files.  In future versions
-of Git, "git add <pathspec>..." will be a synonym to "git add -A
-<pathspec>..." and "git add --no-all <pathspec>..." will behave like
-today's "git add <pathspec>...", ignoring removed files.
+This option is primarily to help users who are used to older
+versions of Git, whose "git add <pathspec>..." was a synonym
+to "git add --no-all <pathspec>...", i.e. ignored removed files.
 
 -N::
 --intent-to-add::
diff --git a/builtin/add.c b/builtin/add.c
index 4242bce..21c685f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -26,9 +26,6 @@ static int take_worktree_changes;
 struct update_callback_data {
 	int flags;
 	int add_errors;
-
-	/* only needed for 2.0 transition preparation */
-	int warn_add_would_remove;
 };
 
 static int fix_unmerged_status(struct diff_filepair *p,
@@ -52,17 +49,6 @@ static int fix_unmerged_status(struct diff_filepair *p,
 		return DIFF_STATUS_MODIFIED;
 }
 
-static void warn_add_would_remove(const char *path)
-{
-	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
-		  "index for paths removed from the working tree that match\n"
-		  "the given pathspec. If you want to 'add' only changed\n"
-		  "or newly created paths, say 'git add --no-all <pathspec>...'"
-		  " instead.\n\n"
-		  "'%s' would be removed from the index without --no-all."),
-		path);
-}
-
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
@@ -84,10 +70,6 @@ static void update_callback(struct diff_queue_struct *q,
 			}
 			break;
 		case DIFF_STATUS_DELETED:
-			if (data->warn_add_would_remove) {
-				warn_add_would_remove(path);
-				data->warn_add_would_remove = 0;
-			}
 			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
 				break;
 			if (!(data->flags & ADD_CACHE_PRETEND))
@@ -99,27 +81,20 @@ static void update_callback(struct diff_queue_struct *q,
 	}
 }
 
-static void update_files_in_cache(const char *prefix, const char **pathspec,
-				  struct update_callback_data *data)
+int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
 {
+	struct update_callback_data data;
 	struct rev_info rev;
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 	init_pathspec(&rev.prune_data, pathspec);
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = update_callback;
-	rev.diffopt.format_callback_data = data;
+	data.flags = flags;
+	data.add_errors = 0;
+	rev.diffopt.format_callback_data = &data;
 	rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
 	run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);
-}
-
-int add_files_to_cache(const char *prefix, const char **pathspec, int flags)
-{
-	struct update_callback_data data;
-
-	memset(&data, 0, sizeof(data));
-	data.flags = flags;
-	update_files_in_cache(prefix, pathspec, &data);
 	return !!data.add_errors;
 }
 
@@ -298,7 +273,7 @@ N_("The following paths are ignored by one of your .gitignore files:\n");
 static int verbose, show_only, ignored_too, refresh_only;
 static int ignore_add_errors, intent_to_add, ignore_missing;
 
-#define ADDREMOVE_DEFAULT 0 /* Change to 1 in Git 2.0 */
+#define ADDREMOVE_DEFAULT 1
 static int addremove = ADDREMOVE_DEFAULT;
 static int addremove_explicit = -1; /* unspecified */
 
@@ -391,7 +366,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	char *seen = NULL;
 	const char *option_with_implicit_dot = NULL;
 	const char *short_option_with_implicit_dot = NULL;
-	struct update_callback_data update_data;
 
 	git_config(add_config, NULL);
 
@@ -415,20 +389,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
 
-	/*
-	 * Warn when "git add pathspec..." was given without "-u" or "-A"
-	 * and pathspec... covers a removed path.
-	 */
-	memset(&update_data, 0, sizeof(update_data));
-	if (!take_worktree_changes && addremove_explicit < 0)
-		update_data.warn_add_would_remove = 1;
-
 	if (!take_worktree_changes && addremove_explicit < 0 && argc)
-		/*
-		 * Turn "git add pathspec..." to "git add -A pathspec..."
-		 * in Git 2.0 but not yet
-		 */
-		; /* addremove = 1; */
+		/* Turn "git add pathspec..." to "git add -A pathspec..." */
+		addremove = 1;
 
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
@@ -518,10 +481,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
-	update_data.flags = flags;
-	update_files_in_cache(prefix, pathspec, &update_data);
+	exit_status |= add_files_to_cache(prefix, pathspec, flags);
 
-	exit_status |= !!update_data.add_errors;
 	if (add_new_files)
 		exit_status |= add_files(&dir, flags);
 
-- 
1.8.2.1-552-g964983e

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Wed, Apr 17, 2013 at 6:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> And how do you know this will be part of the 1%? You don't. How many
>> times have you tracked regressions in transport helper's import/export
>> functionality? How many times in remote-hg? How many times has
>> *anybody* done so?
>
> The last point makes it all the more important to have a good
> history [*1*]. An area that no developer rarely touches with a little
> user base can stay dormant for a long time, and when people do need
> to hunt for an ancient bug or to enhance the existing feature to
> support a new use case without breaking the old use case, the
> original author may not be around, lost interest, or no longer uses
> his own creation.

You are going in circles, I said such situation was *HYPOTHETICAL*,
Phil Hord said it wasn't, and now you are bringing back more
hypothetical examples, which I would gladly address, as soon as you
accept they are HYPOTHETICAL.

Now, how about you answer the questions about the *REAL* situations
Phil Hord mentioned?

* How many times have you tracked regressions in transport helper's
import/export functionality?

Hint: zero.

* How many times in remote-hg?

Hint: zero.

* How many times has *anybody* done so?

Hint: other than me, quite possibly zero.

And then, before we consider this *hypothetical* situation, it might
be worth noticing what commit this hypothetical person would hit if
you do *not* apply this patch, and what the commit message says:

---
remote-helpers: add support for an export command

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Yeah, well, glad you didn't apply my patch, wouldn't want to mess up
the code that was clearly explained by that commit message.

And before you rationalize the above commit, because maybe the
functionality was described in the documentation, it wasn't:

 transport-helper.c | 132
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 120 insertions(+), 12 deletions(-)

If you do apply my patch, it turns out even the shortest version of my
commit message already gives more information to this *hypothetical*
developer person.

> [Footnote]
>
> *1* In this message, I am not judging if the depth of your writing
>     for the particular change is deep enough. It depends on how well
>     the reader knows the area, and there is no single right answer
>     to that question.
>
>     Incidentally that is why we tend to err on the more descriptive
>     side. The next person your commit will help may not know the
>     area as well as you do and has to figure things out on his
>     own. You are helping him by being descriptive.

I partially agree with this, but I think documenting the nuts and
bolts of transport-helper would be better in done in code,
documentation, tests, and mailing list analysis. And in all those
respects, I believe I've done a more than adequate job.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Matthieu Moy]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> * How many times have you tracked regressions in transport helper's
> import/export functionality?
>
> Hint: zero.

The real question to make the situation non-hypothetical would actually
be "how many times did you track a regression that bisected down to
*this particular commit*". Any regression that ends up on another commit
is irrelevant.

I guess you realize how stupid my argument is. But how is yours
different? You do realize that your claim that nobody is ever going to
bisect down to your commit is as hypothetical as other people's claim
(if you think it is not, then try to point us a proof that nobody is
ever going to need a good message in the future to understand what I
mean).

We're trying to make all the code and all the commits clean. It seems to
be a consensus here that review is good. I see no reason to purposely
make some commits less good than others based on the fact that they may
not be used in the future.

Search your favorite search engine for "broken window principle" to get
more arguments in this direction.

> * How many times has *anybody* done so?
>
> Hint: other than me, quite possibly zero.

If you want to be the only developer, and avoid being disturbed by
others, then why are you pushing your changes to git.git? Why are you
even discussing on this list?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: "What's cooking" between #05 and #06

[John Keeping]

On Wed, Apr 17, 2013 at 11:25:07PM +0200, Jens Lehmann wrote:
> I like it, as it gets rid of the top-level requirement. But from
> my testing it looks like we're not quite there yet.
> 
> 'summary' and 'status' behave as if they were run in the toplevel
> directory, while a "git status" shows all filenames relative to the
> current directory. Me thinks 'summary' and 'status' (and all other
> submodule commands) should behave like status and print relative
> paths too. I'm not really sure yet how $sm_path should behave for
> 'foreach', but I suspect having it relative to the current
> directory would be the way to go (which it currently isn't).
>
> When "submodule add" is run with a relative path it is relative to
> the top-level directory, which I find confusing (and won't play
> well with shell completion).

This confused me for a bit because I was sure I handled this, but I see
I missed relative submodules URLs.  So the path at which to put the
submodule is correct, but the path from which to clone is not.

> 'deinit .' doesn't deinit submodules above the current directory
> (but prints the path relative to top-level) while 'init' will
> initialize all submodules known to the superproject.

I can't see how this happens.  'init' uses module_list which has been
updated to handle relative paths.  So I expect 'git submodule init .' to
work correctly here.  I would expect either of them to act on all
submodules when given no extra arguments.

> So this is a good start, but it looks like there is some work left
> to do before this can hit master.

Thanks for the feedback.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Thu, Apr 18, 2013 at 2:44 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> * How many times have you tracked regressions in transport helper's
>> import/export functionality?
>>
>> Hint: zero.
>
> The real question to make the situation non-hypothetical would actually
> be "how many times did you track a regression that bisected down to
> *this particular commit*". Any regression that ends up on another commit
> is irrelevant.
>
> I guess you realize how stupid my argument is. But how is yours
> different?

I did not make any argument (stupid or otherwise), I made I claim; I
won't waste my time with hypotheticals.

> You do realize that your claim that nobody is ever going to
> bisect down to your commit is as hypothetical as other people's claim
> (if you think it is not, then try to point us a proof that nobody is
> ever going to need a good message in the future to understand what I
> mean).

Yeah, they are both hypotheticals, the only difference is that your
claim is very easy to prove; all you need is *ONE* example.

But I'm very happy to withdraw my claim, as long as you withdraw your
claim as well, and we go back to the default position: we don't know
if anybody will every look at these commit messages again.

> We're trying to make all the code and all the commits clean. It seems to
> be a consensus here that review is good. I see no reason to purposely
> make some commits less good than others based on the fact that they may
> not be used in the future.

You have to prove first that they are "less good", and the best way to
do that is provide commit messages of your own, if you do that, they
can be used instead, but if you don't, what do you propose to do? Drop
the patches?

> Search your favorite search engine for "broken window principle" to get
> more arguments in this direction.

More like broken windows hypothesis, which is not without its critics.

>> * How many times has *anybody* done so?
>>
>> Hint: other than me, quite possibly zero.
>
> If you want to be the only developer, and avoid being disturbed by
> others, then why are you pushing your changes to git.git? Why are you
> even discussing on this list?

Doesn't matter, it's still *HYPOTHETICAL* that anybody will every hit
this in a bisect.

Now, if you agree it's all hypothetical, the next rational thing to do
is risk analysis: how likely is it to happen, and what would be the
impact if it does? The answer to both questions is: close to *ZERO*.
So, considering the nature of these patches (a remote-helper in the
contrib area that is relatively new), and the active developers (me),
I'd say it's much more important to get the fixes in, than to document
every little quirk, detail and reasoning behind them. It's the balance
I think it's best at this point, and it is my time, and it is my
decision what I do with it.

It might also help to compare oranges with oranges, and with regards
to remote-hg transport helpers, I do believe the one in
contrib/remote-helpers has the best commit messages:

msysgit's remote-hg:
---
commit 6bbd5365988d63780acc2ab407878eef8c19b47c
Author: Sverre Rabbelier <srabbelier@gmail.com>
Date:   Sun Aug 22 01:22:14 2010 -0500

    git_remote_helpers: add fastimport library

 git_remote_helpers/fastimport/__init__.py     |   0
 git_remote_helpers/fastimport/commands.py     | 469
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git_remote_helpers/fastimport/dates.py        |  79 +++++++++++
 git_remote_helpers/fastimport/errors.py       | 182 +++++++++++++++++++++++++
 git_remote_helpers/fastimport/head_tracker.py |  47 +++++++
 git_remote_helpers/fastimport/helpers.py      |  88 +++++++++++++
 git_remote_helpers/fastimport/idmapfile.py    |  65 +++++++++
 git_remote_helpers/fastimport/parser.py       | 621
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git_remote_helpers/fastimport/processor.py    | 222
+++++++++++++++++++++++++++++++
 git_remote_helpers/setup.py                   |   3 +-
 10 files changed, 1775 insertions(+), 1 deletion(-)
---

gitifyhg:
--
commit 4b364563cd705dc5e69082e6b80d304fe50b9c9c
Author: Alex Sydell <alex@dropbox.com>
Date:   Sat Mar 23 23:46:33 2013 -0700

    Report correct (instead of unknown) hashes when importing refs into git

 gitifyhg/gitifyhg.py   | 27 +++++++++++++++++++++------
 gitifyhg/hgimporter.py | 20 ++------------------
 gitifyhg/util.py       | 44 ++++++++++++++++++++++++++++++++++++++++++++
 test/test_push.py      | 31 +++++++++++++++++++++++++++----
 4 files changed, 94 insertions(+), 28 deletions(-)
---

And of course, the best place to discuss the lack of good commit
messages, is in the patches themselves, which are after all, sent to
the mailing list for everyone to review.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> I think the commit message is fine, you don't. So YOU go ahead and
> write the proper one. If you don't, all you are doing is being an
> impediment to progress.

Hey Felipe.  Let's get a few things straightened out first:

- We all act in our selfish interests, and write code to scratch our
personal itches.  I don't write code or commit messages for anyone
else, and neither should you.

- However, we're not working in isolation.  We have this giant mailing
list where we all post our patches.  It's like a bazaar where we
compete against other patches for developer attention and potential
reviewers.  In other words, it's a free market, and we're selling our
product: if it fails to sell, will you blame the market or your
product?  I write clear code and beautiful commit messages exactly for
this reason: I'm fighting for attention!

- We have to learn to interoperate with others' code and conventions,
if we want to be part of the community.  That doesn't mean that we
drown out our individuality, but it means that a our patch series has
to conform to some minimal, loose, and evolving standard.  Now, you
can argue that many of the existing conventions are outdated (I do it
all the time), but it cannot change overnight.  Your influence on the
community will show up over an extended period of time.

- We are not an old enterprise who blame breakages on a few
individuals, and fire them.  We're a community where all of us are
equally responsible for all parts of the code.  I am as responsible
for the remote-hg code in master as you are, as I had every
opportunity to review it when the patch series came up on the list.  I
might have chosen not to, but that doesn't relieve me of
responsibility.

-  We don't practice division of labour.  There are no managers,
"testing people", "documentation people", "code-writing people",
"commit-message writing people" etc.  Everyone has to do some portion
of all these tasks, although we try to keep the boring work/ technical
debt to a minimum.  Don't ask other people to write commit messages
for your code.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Thu, Apr 18, 2013 at 4:19 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> I think the commit message is fine, you don't. So YOU go ahead and
>> write the proper one. If you don't, all you are doing is being an
>> impediment to progress.
>
> Hey Felipe.  Let's get a few things straightened out first:
>
> - We all act in our selfish interests, and write code to scratch our
> personal itches.  I don't write code or commit messages for anyone
> else, and neither should you.
>
> - However, we're not working in isolation.  We have this giant mailing
> list where we all post our patches.  It's like a bazaar where we
> compete against other patches for developer attention and potential
> reviewers.  In other words, it's a free market, and we're selling our
> product: if it fails to sell, will you blame the market or your
> product?  I write clear code and beautiful commit messages exactly for
> this reason: I'm fighting for attention!

Except the customers are not git developers, it's git users. Git
developers rejecting patches because of the commit message is akin to
distributors rejecting products because they don't like the
transportation packages; they are only hurting themselves, by hurting
their customers.

> - We have to learn to interoperate with others' code and conventions,
> if we want to be part of the community.  That doesn't mean that we
> drown out our individuality, but it means that a our patch series has
> to conform to some minimal, loose, and evolving standard.  Now, you
> can argue that many of the existing conventions are outdated (I do it
> all the time), but it cannot change overnight.  Your influence on the
> community will show up over an extended period of time.

And the only way it can change is by discussing.

The only one that gets bitten by fixes not getting merged are git
users, not me. So if a discussion of a commit message impedes the
merging of the commit, I don't get affected, but when we have agreed
to disagree on what constitutes a good message, and the patch is still
on hold, then there's a problem.

> - We are not an old enterprise who blame breakages on a few
> individuals, and fire them.  We're a community where all of us are
> equally responsible for all parts of the code.  I am as responsible
> for the remote-hg code in master as you are, as I had every
> opportunity to review it when the patch series came up on the list.  I
> might have chosen not to, but that doesn't relieve me of
> responsibility.

I don't think so. Unless you added your Signed-off-by, you are not.

> -  We don't practice division of labour.  There are no managers,
> "testing people", "documentation people", "code-writing people",
> "commit-message writing people" etc.  Everyone has to do some portion
> of all these tasks, although we try to keep the boring work/ technical
> debt to a minimum.  Don't ask other people to write commit messages
> for your code.

I am not. Neither should they ask me to write the commit messages they
want. They can make *suggestions*, and I can reject them.

When two persons have different ideas, often times both are wrong, and
the middle-ground is best, but sometimes a person reaches the
middle-ground, and sometimes one person was right from the start.

But when everyone shares the *assumption* that there is never a commit
message that is too long, you know the wrestling mat of ideas is
rigged. I wonder if I should write a commit message as long as a book
chapter for a one-liner, only to prove a point, but I'm honestly
afraid that it would be committed as is.

And remember what started the conversation; do you think a patch with
a possibly incomplete commit message should not be merged to pu
(proposed updates), shouldn't even be mentioned in the "what's
cooking" mail, and thus shouldn't even be considered "cooking"?

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> Except the customers are not git developers, it's git users. Git
> developers rejecting patches because of the commit message is akin to
> distributors rejecting products because they don't like the
> transportation packages; they are only hurting themselves, by hurting
> their customers.

Huh?  I certainly don't develop for some "git users" I don't even know
or care about.  In this order of precedence, my customers are:

1. Me.
2. People who develop git.git, whom I have to cooperate with.
3. People who exercise git heavily like the linux.git community, as
opposed to some little projects that operate using pull requests on
GitHub.
...
137. People who incidentally choose to use git.
138. People who incidentally choose to use git, but aren't on Linux.

I don't know if Junio or the others share this view, but this is how I
personally operate and I'm very happy.

And nobody is hurting anyone else.  Someone wrote some code, and
failed to sell it to the community.  That's all happened.

> The only one that gets bitten by fixes not getting merged are git
> users, not me. So if a discussion of a commit message impedes the
> merging of the commit, I don't get affected, but when we have agreed
> to disagree on what constitutes a good message, and the patch is still
> on hold, then there's a problem.

I think the whole issue of whether your commit message conforms to
some transcendental standard is orthogonal to the issue.  Is your
patch getting attention?  Has it attracted reviewers, and turned up on
the latest "What's cooking"?

> I don't think so. Unless you added your Signed-off-by, you are not.

Okay, so your view differs.

> I am not. Neither should they ask me to write the commit messages they
> want. They can make *suggestions*, and I can reject them.

Ofcourse you have a right to reject suggestions.  The question at the
end of the day doesn't change: did you manage to get people to read
your patch?

> When two persons have different ideas, often times both are wrong, and
> the middle-ground is best, but sometimes a person reaches the
> middle-ground, and sometimes one person was right from the start.

https://yourlogicalfallacyis.com/middle-ground :)

> But when everyone shares the *assumption* that there is never a commit
> message that is too long, you know the wrestling mat of ideas is
> rigged. I wonder if I should write a commit message as long as a book
> chapter for a one-liner, only to prove a point, but I'm honestly
> afraid that it would be committed as is.

I'm with you, and don't share that assumption.  I'm not accusing you
of writing commit messages that don't conform to some "transcendental
standard" either: I didn't look at your patches in the first place,
because the simple Signed-off-by: one-liner in the body didn't really
make me want to read it.

> And remember what started the conversation; do you think a patch with
> a possibly incomplete commit message should not be merged to pu
> (proposed updates), shouldn't even be mentioned in the "what's
> cooking" mail, and thus shouldn't even be considered "cooking"?

It's irrelevant what I think or others think.  The point is that it
wasn't mentioned.  Now, why wasn't it mentioned?  Is it because Junio
and the community hate you, and are conspiring against getting your
code merged?  Or is it because it didn't catch anyone's eye, and Junio
was waiting for it to happen (as always)?

TL;DR version: Your goal in submitting a patch is to sell it to other
people in the community.  If enough people like your patch, it gets
merged (but that is only the second step).  Your goal is not to fix
problems for some unknown "users", or argue about some "transcendental
standard".  Ofcourse the community shares some view about what a patch
should look like, but you can mould those expectations gradually.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Thu, Apr 18, 2013 at 5:27 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> Except the customers are not git developers, it's git users. Git
>> developers rejecting patches because of the commit message is akin to
>> distributors rejecting products because they don't like the
>> transportation packages; they are only hurting themselves, by hurting
>> their customers.
>
> Huh?  I certainly don't develop for some "git users" I don't even know
> or care about.  In this order of precedence, my customers are:
>
> 1. Me.
> 2. People who develop git.git, whom I have to cooperate with.
> 3. People who exercise git heavily like the linux.git community, as
> opposed to some little projects that operate using pull requests on
> GitHub.
> ...
> 137. People who incidentally choose to use git.
> 138. People who incidentally choose to use git, but aren't on Linux.

As they are for most open source developers on the planet, not me. I
believe a project is nothing without its users.

> And nobody is hurting anyone else.  Someone wrote some code, and
> failed to sell it to the community.  That's all happened.

If remote-hg wasn't available for users, they would be hurt; if stash
wasn't available, if rebase --interactive didn't exist, if there was
no msysgit, if it wasn't so fast, if the object model wasn't so simple
and extensible; users would be hurt. And if users didn't have all
these, there would be less users, and if there were less users, there
would be less developers, and mercurial might have been more popular,
and most repositories you have to work on would be in mercurial, and
you might be developing mercurial right now.

But I won't bother trying to convince you that no project is more
important than its users (in the words of Linus Torvalds), because
most people don't see the big picture.

>> I don't think so. Unless you added your Signed-off-by, you are not.
>
> Okay, so your view differs.
>
>> I am not. Neither should they ask me to write the commit messages they
>> want. They can make *suggestions*, and I can reject them.
>
> Ofcourse you have a right to reject suggestions.  The question at the
> end of the day doesn't change: did you manage to get people to read
> your patch?

No, at the end of the day what matters is: did the users benefit from this?

The answer for this particular patch is no, and it's not my fault.

>> When two persons have different ideas, often times both are wrong, and
>> the middle-ground is best, but sometimes a person reaches the
>> middle-ground, and sometimes one person was right from the start.
>
> https://yourlogicalfallacyis.com/middle-ground :)

Yeah, but I didn't claim that, I said sometimes one person was right
from the start, no middle-ground.

>> But when everyone shares the *assumption* that there is never a commit
>> message that is too long, you know the wrestling mat of ideas is
>> rigged. I wonder if I should write a commit message as long as a book
>> chapter for a one-liner, only to prove a point, but I'm honestly
>> afraid that it would be committed as is.
>
> I'm with you, and don't share that assumption.

s/everyone/almost everyone/

> I'm not accusing you
> of writing commit messages that don't conform to some "transcendental
> standard" either: I didn't look at your patches in the first place,
> because the simple Signed-off-by: one-liner in the body didn't really
> make me want to read it.
>
>> And remember what started the conversation; do you think a patch with
>> a possibly incomplete commit message should not be merged to pu
>> (proposed updates), shouldn't even be mentioned in the "what's
>> cooking" mail, and thus shouldn't even be considered "cooking"?
>
> It's irrelevant what I think or others think.  The point is that it
> wasn't mentioned.  Now, why wasn't it mentioned?  Is it because Junio
> and the community hate you, and are conspiring against getting your
> code merged?  Or is it because it didn't catch anyone's eye, and Junio
> was waiting for it to happen (as always)?

My abridged version of the story is: because Jeff King pointed to an
area of improvement he wasn't even strongly attached to, I agreed to
resubmit, Junio saw that, then I changed my mind, Junio probably
didn't see that, and then he forgot about it.

Then, since it's taboo to suggest that a concise commit message is
fine, a discussion sprung.

> TL;DR version: Your goal in submitting a patch is to sell it to other
> people in the community.

I disagree.

> If enough people like your patch, it gets
> merged (but that is only the second step).  Your goal is not to fix
> problems for some unknown "users", or argue about some "transcendental
> standard".

I disagree.

> Ofcourse the community shares some view about what a patch
> should look like, but you can mould those expectations gradually.

If experience is any guide, doesn't look like that. But I've noticed
that after many months that a patch has been sent people realize that
it's more important to get the damn issue fixed than to have a hugely
verbose commit message...

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Ramkumar Ramachandra]

Since you disagreed with the rest, I'll only respond to this part:

Felipe Contreras wrote:
> But I won't bother trying to convince you that no project is more
> important than its users (in the words of Linus Torvalds), because
> most people don't see the big picture.

I didn't say otherwise.  What I'm saying is: my personal incentive to
write code does not prioritize the supposed benefit of some unknown
"user" somewhere on the planet above everything else.  My personal
incentive prioritizes me, and my immediate circle (ie. the git
community).  The benefit propagates outwards to extended circles until
it reaches the people I care least about: incidental end-users.
That's how people are connected: how can I care about distant unknown
people I'm not connected to?  The people in the outermost circles
benefit the least, because they didn't get a say in the development.
All they can do is write a rant about it on their blog, and hope that
it gets fixed someday.

You just ditched us, the inner circle of people who care about your
work the most, and are instead trying to convince us that we're
hurting some unknown hypothetical "users" by not merging your code
immediately.

If you think these users are more important to you than we are, then
why are you posting your code on this mailing list?  Start your own
project that's focused on satisfying these users.  It doesn't even
need to be open source or have a community of reviewers, because all
you care about are users.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Ramkumar Ramachandra]

Okay, one more segment needs to be responded to.

Felipe Contreras wrote:
> If remote-hg wasn't available for users, they would be hurt; if stash
> wasn't available, if rebase --interactive didn't exist, if there was
> no msysgit, if it wasn't so fast, if the object model wasn't so simple
> and extensible; users would be hurt. And if users didn't have all
> these, there would be less users, and if there were less users, there
> would be less developers, and mercurial might have been more popular,
> and most repositories you have to work on would be in mercurial, and
> you might be developing mercurial right now.

Flawed logic.

A large number of users doesn't automatically imply good software with
lots of features, or even a great development community.  A great
development community leads to great software.  And great software
leads to lots of users.  Sure, there's a feedback loop pushing users
to become developers; but it doesn't start with users of vaporware,
leading to more developers joining the effort to turn that vaporware
into a great product.

Life doesn't begin with users.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Thu, Apr 18, 2013 at 6:31 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Since you disagreed with the rest, I'll only respond to this part:
>
> Felipe Contreras wrote:
>> But I won't bother trying to convince you that no project is more
>> important than its users (in the words of Linus Torvalds), because
>> most people don't see the big picture.
>
> I didn't say otherwise.  What I'm saying is: my personal incentive to
> write code does not prioritize the supposed benefit of some unknown
> "user" somewhere on the planet above everything else.  My personal
> incentive prioritizes me, and my immediate circle (ie. the git
> community).  The benefit propagates outwards to extended circles until
> it reaches the people I care least about: incidental end-users.

If the people that matter most are given the worst prioritization, it
means the prioritization is wrong.

> That's how people are connected: how can I care about distant unknown
> people I'm not connected to?

It's called empathy.

> The people in the outermost circles
> benefit the least, because they didn't get a say in the development.
> All they can do is write a rant about it on their blog, and hope that
> it gets fixed someday.

To the detriment of the project.

> You just ditched us, the inner circle of people who care about your
> work the most, and are instead trying to convince us that we're
> hurting some unknown hypothetical "users" by not merging your code
> immediately.

The users are real, the developers that will look retroatcively to the
commit message of this patch are not.

> If you think these users are more important to you than we are, then
> why are you posting your code on this mailing list?

What other way is there for this code to reach the users?

> Start your own
> project that's focused on satisfying these users.

Start a new project so I can include a patch that hasn't made it yet
into the "what's cooking" in one week? That's ridiculous.

> It doesn't even
> need to be open source or have a community of reviewers, because all
> you care about are users.

Who said *all* that matters are the users? And even if somebody did,
ultimately a closed source proprietary software doesn't benefit the
users, so either way it has to be open and active to benefit the
users.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Thu, Apr 18, 2013 at 6:46 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Okay, one more segment needs to be responded to.
>
> Felipe Contreras wrote:
>> If remote-hg wasn't available for users, they would be hurt; if stash
>> wasn't available, if rebase --interactive didn't exist, if there was
>> no msysgit, if it wasn't so fast, if the object model wasn't so simple
>> and extensible; users would be hurt. And if users didn't have all
>> these, there would be less users, and if there were less users, there
>> would be less developers, and mercurial might have been more popular,
>> and most repositories you have to work on would be in mercurial, and
>> you might be developing mercurial right now.
>
> Flawed logic.
>
> A large number of users doesn't automatically imply good software with
> lots of features, or even a great development community.  A great
> development community leads to great software.  And great software
> leads to lots of users.  Sure, there's a feedback loop pushing users
> to become developers; but it doesn't start with users of vaporware,
> leading to more developers joining the effort to turn that vaporware
> into a great product.
>
> Life doesn't begin with users.

Nobody knows how life began, and it doesn't matter now, what matters
is how life evolves. It doesn't matter if the chicken was first, or
the egg, what matters is that if all the chickens and eggs are gone,
there won't be more.

Plenty of projects have died because they stopped caring about their
users, and without users there's no new developers, and the old
developers eventually move on, and all the literary quality of commit
messages have no eyes to see it.

I repeat: no project is more important than its users.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Wed, Apr 17, 2013 at 06:39:06PM -0700, Junio C Hamano wrote:

> Subject: [PATCH] git add: rework the logic to warn "git add <pathspec>..." default change
>
> [...]
>
> Rework the logic to detect the case where the behaviour will be
> different in Git 2.0, and issue a warning only when it matters.
> Even with the code before this warning, "git add subdir" will have
> to traverse the directory in order to find _new_ files the index
> does not know about _anyway_, so we can do this check without adding
> an extra pass to find if <pathspec> matches any removed file.

Thanks, I think this is looking much better.

A few minor nits on the message itself:

> +static void warn_add_would_remove(const char *path)
> +{
> +	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
> +		  "index for paths removed from the working tree that match\n"
> +		  "the given pathspec. If you want to 'add' only changed\n"
> +		  "or newly created paths, say 'git add --no-all <pathspec>...'"
> +		  " instead.\n\n"

This wrapping looks funny in the actual output, due to the extra
"warning:" and the lack of newline before "instead":

  warning: In Git 2.0, 'git add <pathspec>...' will also update the
  index for paths removed from the working tree that match
  the given pathspec. If you want to 'add' only changed
  or newly created paths, say 'git add --no-all <pathspec>...' instead.

Wrapping it like this ends up with a much more natural-looking
paragraph:

  warning: In Git 2.0, 'git add <pathspec>...' will also update the
  index for paths removed from the working tree that match the given
  pathspec. If you want to 'add' only changed or newly created paths,
  say 'git add --no-all <pathspec>...' instead.

> +		  "'%s' would be removed from the index without --no-all."),
> +		path);

I think mentioning the filename is a good thing; the original message
left me scratching my head and wondering "so, did you add it or not?".
I still think your "would be" is unnecessarily confusing, though. It is
"would be in Git 2.0 without --no-all, but we did not now". Which makes
sense when you think about it, but it took me a moment to parse.

Perhaps we can be more direct with something like:

  warning: did not stage removal of 'foo'

or perhaps the present tense "not staging removal of..." would be
better.

I also think it makes sense to show every path that is affected, not
just the first one, to be clear about what was done (and what _would_
have been done in Git 2.0).

A patch with all of the suggestions together is below. I still think the
multi-line warning block looks ugly. I kind of like the way advise()
puts "hint:" on each line. I wonder if we should do the same here.

diff --git a/builtin/add.c b/builtin/add.c
index 4242bce..aae550a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -52,15 +52,19 @@ static void warn_add_would_remove(const char *path)
 		return DIFF_STATUS_MODIFIED;
 }
 
+static const char *add_would_remove_warning = N_(
+/* indent for "warning: " */
+         "In Git 2.0, 'git add <pathspec>...' will also update the\n"
+"index for paths removed from the working tree that match the given\n"
+"pathspec. If you want to 'add' only changed or newly created paths,\n"
+"say 'git add --no-all <pathspec>...' instead.\n");
+
 static void warn_add_would_remove(const char *path)
 {
-	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
-		  "index for paths removed from the working tree that match\n"
-		  "the given pathspec. If you want to 'add' only changed\n"
-		  "or newly created paths, say 'git add --no-all <pathspec>...'"
-		  " instead.\n\n"
-		  "'%s' would be removed from the index without --no-all."),
-		path);
+	static int warned_once;
+	if (!warned_once++)
+		warning(_(add_would_remove_warning));
+	warning("did not stage removal of '%s'", path);
 }
 
 static void update_callback(struct diff_queue_struct *q,
@@ -84,10 +88,8 @@ static void update_callback(struct diff_queue_struct *q,
 			}
 			break;
 		case DIFF_STATUS_DELETED:
-			if (data->warn_add_would_remove) {
+			if (data->warn_add_would_remove)
 				warn_add_would_remove(path);
-				data->warn_add_would_remove = 0;
-			}
 			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
 				break;
 			if (!(data->flags & ADD_CACHE_PRETEND))

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +static const char *add_would_remove_warning = N_(
> +/* indent for "warning: " */
> +         "In Git 2.0, 'git add <pathspec>...' will also update the\n"
> +"index for paths removed from the working tree that match the given\n"
> +"pathspec. If you want to 'add' only changed or newly created paths,\n"
> +"say 'git add --no-all <pathspec>...' instead.\n");
> +
>  static void warn_add_would_remove(const char *path)
>  {
> -	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
> -		  "index for paths removed from the working tree that match\n"
> -		  "the given pathspec. If you want to 'add' only changed\n"
> -		  "or newly created paths, say 'git add --no-all <pathspec>...'"
> -		  " instead.\n\n"
> -		  "'%s' would be removed from the index without --no-all."),
> -		path);
> +	static int warned_once;
> +	if (!warned_once++)
> +		warning(_(add_would_remove_warning));
> +	warning("did not stage removal of '%s'", path);
>  }

Would "add --dry-run" say this, too?

>  static void update_callback(struct diff_queue_struct *q,
> @@ -84,10 +88,8 @@ static void update_callback(struct diff_queue_struct *q,
>  			}
>  			break;
>  		case DIFF_STATUS_DELETED:
> -			if (data->warn_add_would_remove) {
> +			if (data->warn_add_would_remove)
>  				warn_add_would_remove(path);
> -				data->warn_add_would_remove = 0;
> -			}
>  			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
>  				break;
>  			if (!(data->flags & ADD_CACHE_PRETEND))

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Thu, Apr 18, 2013 at 10:51:12AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +static const char *add_would_remove_warning = N_(
> > +/* indent for "warning: " */
> > +         "In Git 2.0, 'git add <pathspec>...' will also update the\n"
> > +"index for paths removed from the working tree that match the given\n"
> > +"pathspec. If you want to 'add' only changed or newly created paths,\n"
> > +"say 'git add --no-all <pathspec>...' instead.\n");
> > +
> >  static void warn_add_would_remove(const char *path)
> >  {
> > -	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
> > -		  "index for paths removed from the working tree that match\n"
> > -		  "the given pathspec. If you want to 'add' only changed\n"
> > -		  "or newly created paths, say 'git add --no-all <pathspec>...'"
> > -		  " instead.\n\n"
> > -		  "'%s' would be removed from the index without --no-all."),
> > -		path);
> > +	static int warned_once;
> > +	if (!warned_once++)
> > +		warning(_(add_would_remove_warning));
> > +	warning("did not stage removal of '%s'", path);
> >  }
> 
> Would "add --dry-run" say this, too?

It probably makes sense to continue to have the warning in the dry-run
case, but it may make sense to tweak it grammatically when we are in
dry-run mode. Saying "would stage removal" is technically correct, but I
think it is somewhat ambiguous: would git do it if we were not in a
--dry-run, or would git do it if it were Git 2.0?

Doing it as:

  warning: not staging removal of '%s'

could work for both cases. Something like "not considering" (or another
synonym for "considering") might be even more accurate. It is not just
that we did not stage it; it is what we did not even consider it an item
for staging under the current rules.

Note that the "not staging" warnings may potentially be interspersed
with the normal dry-run output. I think that's OK. But another
alternative would be to collect the paths and then print:

  warning: In Git 2.0, ...

  The following deleted paths were not considered under the current
  rule. Use "git add -A" to stage their removal now.

    foo
    bar

-Peff

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> could work for both cases. Something like "not considering" (or another
> synonym for "considering") might be even more accurate. It is not just
> that we did not stage it; it is what we did not even consider it an item
> for staging under the current rules.

Yes, "not considering" is much more sensible, while side-stepping
the dryrun issue.  Or

       warning("ignoring removal of '%s'")

> Note that the "not staging" warnings may potentially be interspersed
> with the normal dry-run output. I think that's OK.

As long as the top-text makes it clear what "not considering" (or
"ignoring") in the following text means, I think it is fine.

But I think we are doing users a disservice by listing tons of
paths.  Where the difference of versions matters _most_ is when the
user has tons of removed paths in the working tree.  Either with one
warning per path, or a block of collected paths at the end, we are
scrolling the more important part of the message up.

That was why I originally showed one path as an example and stopped
there.  Perhaps it is a better solution to keep that behaviour and
rephrase the message to say that we ignored removal of paths like
this one '%s' you lost from the working tree but it will change in
Git 2.0 and you will better learn to use the --no-all option now.

The inter-topic conflicts between stages of three "add in Git 2.0"
topics is getting cumbersome even with the help from rerere, so I'd
like to merge their preparatory steps as I have them now to 'next'
and merge them down to 'master' first, and start applying tweaks
from there, or something.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Phil Hord]

On Wed, Apr 17, 2013 at 2:50 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Tue, Apr 16, 2013 at 5:45 PM, Phil Hord <phil.hord@gmail.com> wrote:
>> On Tue, Apr 16, 2013 at 3:04 PM, Felipe Contreras

>>> If you want to waste your time, by all means, rewrite all my commit
>>> messages with essays that nobody will ever read. I'm not going to do
>>> that for some hypothetical case that will never happen. I'm not going
>>> to waste my time.
>>
>> This is not a hypothetical.  Almost every time I bisect a regression
>> in git.git, I find the commit message tells me exactly why the commit
>> did what it did and what the expected result was.  I find this to be
>> amazingly useful.  Do I need to show you real instances of that
>> happening? No.  I promise it did, though.
>
> Yes please. Show me one of the instances where you hit a bisect with
> any of the remote-hg commits mentioned above by Thomas Rast.

I made no such claim.  In fact, I have never bisected to any
remote-hg-related commit.  I fail to see the relevance of this
qualifier, though.

P

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Thu, Apr 18, 2013 at 11:16:33AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > could work for both cases. Something like "not considering" (or another
> > synonym for "considering") might be even more accurate. It is not just
> > that we did not stage it; it is what we did not even consider it an item
> > for staging under the current rules.
> 
> Yes, "not considering" is much more sensible, while side-stepping
> the dryrun issue.  Or
> 
>        warning("ignoring removal of '%s'")

I like that much better than either of my suggestions.

> > Note that the "not staging" warnings may potentially be interspersed
> > with the normal dry-run output. I think that's OK.
> 
> As long as the top-text makes it clear what "not considering" (or
> "ignoring") in the following text means, I think it is fine.

Agreed, and I think the current text is fine for that (though neither of
us is the best judge at this point of how a less familiar user would
interpret it).

> But I think we are doing users a disservice by listing tons of
> paths.  Where the difference of versions matters _most_ is when the
> user has tons of removed paths in the working tree.  Either with one
> warning per path, or a block of collected paths at the end, we are
> scrolling the more important part of the message up.

I'm not sure I agree. Even with a handful, it made me wonder why one was
mentioned and not others. That _could_ be cleared up by rewording (i.e.,
making it clear that this is an example, and there may be more). But
somehow listing them is what I would expect. Perhaps because it gives
the user a clue about what to do next; they ask themselves "did I want
those updated or not?".

In the orphaned-commit message when leaving a detached HEAD, we collect
the answer, say "you are leaving N commits", and show the first 5 five
of them, with an ellipsis at the end if we didn't show them all.  Would
it makes sense to do that here?

Yet another alternative would be to print a warning for each path, but
hold the main warning for the end, so that it is the first thing the
user sees.  That has the added bonus that regular "--dry-run" output
will not scroll it away, either.

-Peff

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> But I think we are doing users a disservice by listing tons of
>> paths.  Where the difference of versions matters _most_ is when the
>> user has tons of removed paths in the working tree.  Either with one
>> warning per path, or a block of collected paths at the end, we are
>> scrolling the more important part of the message up.
>
> I'm not sure I agree. Even with a handful, it made me wonder why one was
> mentioned and not others. That _could_ be cleared up by rewording (i.e.,
> making it clear that this is an example, and there may be more). But
> somehow listing them is what I would expect. Perhaps because it gives
> the user a clue about what to do next; they ask themselves "did I want
> those updated or not?".
>
> In the orphaned-commit message when leaving a detached HEAD, we collect
> the answer, say "you are leaving N commits", and show the first 5 five
> of them, with an ellipsis at the end if we didn't show them all.  Would
> it makes sense to do that here?

Because this is to help people who are _used_ to seeing "git add"
not take the removals into account, I doubt that "Did I want those
updated or not?  Let me see the details of them." will be the
question they will be asking [*1*].

I dunno.


[Footnote]

*1* "I know I didn't want to include these removals to the index,
but I learned today that in later Git I should make myself more
clear if I want to keep doing so; thanks for letting me know.", or
"I've long been assuming that I have to say 'git add' and 'git rm'
separately, but I learned today that I can say 'add --all', and in
later Git I do not even have to; thanks for letting me know." are
the two reactions I expected.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Thu, Apr 18, 2013 at 02:37:53PM -0700, Junio C Hamano wrote:

> Because this is to help people who are _used_ to seeing "git add"
> not take the removals into account, I doubt that "Did I want those
> updated or not?  Let me see the details of them." will be the
> question they will be asking [*1*].
> 
> I dunno.
> 
> 
> [Footnote]
> 
> *1* "I know I didn't want to include these removals to the index,
> but I learned today that in later Git I should make myself more
> clear if I want to keep doing so; thanks for letting me know.", or
> "I've long been assuming that I have to say 'git add' and 'git rm'
> separately, but I learned today that I can say 'add --all', and in
> later Git I do not even have to; thanks for letting me know." are
> the two reactions I expected.

I am expecting a reaction more like "Hmm, I never thought about it
before. Does that make sense to me or not? Let me think about which
paths it pertains to and decide".

Which I admit is no more likely than the scenarios you outlined, but it
is close to what I thought the first time I saw the warning.

-Peff

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am expecting a reaction more like "Hmm, I never thought about it
> before. Does that make sense to me or not? Let me think about which
> paths it pertains to and decide".

Let's step back and re-review the main text.

It currently says:

    In Git 2.0, 'git add <pathspec>...' will also update the
    index for paths removed from the working tree that match
    the given pathspec. If you want to 'add' only changed
    or newly created paths, say 'git add --no-all <pathspec>...'
    instead.

This was written for the old "we may want to warn" logic that did
not even check if we would be omitting a removal.  The new logic
will show the text _only_ when the difference matters, we have an
opportunity to tighten it a lot, for example:

    You ran 'git add' with neither '-A (--all)' or '--no-all', whose
    behaviour will change in Git 2.0 with respect to paths you
    removed from your working tree.

    * 'git add --no-all <pathspec>', which is the current default,
      ignores paths you removed from your working tree.

    * 'git add --all <pathspec>' will let you also record the
      removals.

    The removed paths (e.g. '%s') are ignored with this version of Git.
    Run 'git status' to remind yourself what paths you have removed
    from your working tree.

or something?

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Thu, Apr 18, 2013 at 3:06 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Wed, Apr 17, 2013 at 2:50 PM, Felipe Contreras

>> Yes please. Show me one of the instances where you hit a bisect with
>> any of the remote-hg commits mentioned above by Thomas Rast.
>
> I made no such claim.  In fact, I have never bisected to any
> remote-hg-related commit.  I fail to see the relevance of this
> qualifier, though.

Here, this is what you said:

You:
> Me:
>> [skipping irrelevant comments]
>>
>> I'm sorry, did you actually hit an issue that required to look at the
>> commit message to understand where the issue came from? No? Then I
>> won't bother with hypotheticals.
>>
>> If you want to waste your time, by all means, rewrite all my commit
>> messages with essays that nobody will ever read. I'm not going to do
>> that for some hypothetical case that will never happen. I'm not going
>> to waste my time.
>
> This is not a hypothetical.

If something is not hypothetical, it's real, which means it actually
happened, but then you said you never made the claim that it did. So
what is it? Either it did happen, or it didn't; you cannot have your
cake and eat it.

If you are going to change your claims on the fly, and deny you ever
made them, I don't see much point in discussing with you.

Cheers.

-- 
Felipe Contreras

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Thu, Apr 18, 2013 at 03:10:29PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I am expecting a reaction more like "Hmm, I never thought about it
> > before. Does that make sense to me or not? Let me think about which
> > paths it pertains to and decide".
> 
> Let's step back and re-review the main text.

Good idea. I was very caught up in the existing message and what it made
me expect, and not in what we are trying to accomplish overall.

> It currently says:
> 
>     In Git 2.0, 'git add <pathspec>...' will also update the
>     index for paths removed from the working tree that match
>     the given pathspec. If you want to 'add' only changed
>     or newly created paths, say 'git add --no-all <pathspec>...'
>     instead.
> 
> This was written for the old "we may want to warn" logic that did
> not even check if we would be omitting a removal.  The new logic
> will show the text _only_ when the difference matters, we have an
> opportunity to tighten it a lot, for example:
> 
>     You ran 'git add' with neither '-A (--all)' or '--no-all', whose
>     behaviour will change in Git 2.0 with respect to paths you
>     removed from your working tree.
> 
>     * 'git add --no-all <pathspec>', which is the current default,
>       ignores paths you removed from your working tree.
> 
>     * 'git add --all <pathspec>' will let you also record the
>       removals.
> 
>     The removed paths (e.g. '%s') are ignored with this version of Git.
>     Run 'git status' to remind yourself what paths you have removed
>     from your working tree.
> 
> or something?

Yes, I like that much better. It reads more clearly than the original,
and it is more obvious why we are mentioning the path at all.

And I think the hint of "git status" is good. I had considered before
that the user would simply run "git status" after the message to get
more data, but I didn't want to rely on them knowing to do that.
Actually mentioning it is a good solution. :)

Thanks for pointing us in the right direction.

-Peff

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jonathan Nieder]

Junio C Hamano wrote:

>     You ran 'git add' with neither '-A (--all)' or '--no-all', whose
>     behaviour will change in Git 2.0 with respect to paths you
>     removed from your working tree.
>
>     * 'git add --no-all <pathspec>', which is the current default,
>       ignores paths you removed from your working tree.
>
>     * 'git add --all <pathspec>' will let you also record the
>       removals.
>
>     The removed paths (e.g. '%s') are ignored with this version of Git.
>     Run 'git status' to remind yourself what paths you have removed
>     from your working tree.
>
> or something?

That looks good. :)

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>>     You ran 'git add' with neither '-A (--all)' or '--no-all', whose
>>     behaviour will change in Git 2.0 with respect to paths you
>>     removed from your working tree.
>>
>>     * 'git add --no-all <pathspec>', which is the current default,
>>       ignores paths you removed from your working tree.
>>
>>     * 'git add --all <pathspec>' will let you also record the
>>       removals.
>>
>>     The removed paths (e.g. '%s') are ignored with this version of Git.
>>     Run 'git status' to remind yourself what paths you have removed
>>     from your working tree.
>>
>> or something?
>
> That looks good. :)

I think the direction may be good but the above is too tall to be
the final version. of the message.  Somebody good at phrasing needs
to trim it down without losing the essense.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Phil Hord]

On Thu, Apr 18, 2013 at 7:48 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Thu, Apr 18, 2013 at 3:06 PM, Phil Hord <phil.hord@gmail.com> wrote:
>> On Wed, Apr 17, 2013 at 2:50 PM, Felipe Contreras
>
>>> Yes please. Show me one of the instances where you hit a bisect with
>>> any of the remote-hg commits mentioned above by Thomas Rast.
>>
>> I made no such claim.  In fact, I have never bisected to any
>> remote-hg-related commit.  I fail to see the relevance of this
>> qualifier, though.
>
> Here, this is what you said:
>
> You:
>> Me:
>>> [skipping irrelevant comments]
>>>
>>> I'm sorry, did you actually hit an issue that required to look at the
>>> commit message to understand where the issue came from? No? Then I
>>> won't bother with hypotheticals.
>>>
>>> If you want to waste your time, by all means, rewrite all my commit
>>> messages with essays that nobody will ever read. I'm not going to do
>>> that for some hypothetical case that will never happen. I'm not going
>>> to waste my time.
>>
>> This is not a hypothetical.
>
> If something is not hypothetical, it's real, which means it actually
> happened, but then you said you never made the claim that it did. So
> what is it?

My claim:

   I bisected to a commit whose commit message helped me deduce its entirety.


Your fanciful interpretation, which I denied:

  "Show me one of the instances where you hit a bisect with
  any of the remote-hg commits mentioned above by Thomas Rast."


I have never bisected to any commit related to remote-hg, and neither
did I ever claim to.  I do not know where you got such a ridiculous
qualifier as this to append to my statement.


> Either it did happen, or it didn't;

It did.  Where "it" is my actual claim, that I bisected to a commit
whose commit message helped me deduce its entirety.

But also, it didn't, where "it" is your preposterous interpretation of
my interest and/or experiences with remote-hg commits.

You seem only to want to argue, Felipe.  I have neither time nor
interest in pig-wrestling, myself.

Phil

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King]

On Fri, Apr 19, 2013 at 10:25:46AM -0700, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Junio C Hamano wrote:
> >
> >>     You ran 'git add' with neither '-A (--all)' or '--no-all', whose
> >>     behaviour will change in Git 2.0 with respect to paths you
> >>     removed from your working tree.
> >>
> >>     * 'git add --no-all <pathspec>', which is the current default,
> >>       ignores paths you removed from your working tree.
> >>
> >>     * 'git add --all <pathspec>' will let you also record the
> >>       removals.
> >>
> >>     The removed paths (e.g. '%s') are ignored with this version of Git.
> >>     Run 'git status' to remind yourself what paths you have removed
> >>     from your working tree.
> >>
> >> or something?
> >
> > That looks good. :)
> 
> I think the direction may be good but the above is too tall to be
> the final version. of the message.  Somebody good at phrasing needs
> to trim it down without losing the essense.

Hmph. I actually like it as it is. It says:

  1. Here's what triggered this warning (removed paths without -A).

  2. Here is how you tell git what you want to do (--all/--no-all)

  3. Here is how you get more information about what you wanted to do
     (mention one such path, point to "git status").

I would not want to cut out any of those three. You could perhaps cut
out the bullet points in the middle, which reduces (2), but the user may
be able to figure it out from the first sentence. However, I like the
explicitness of those bullet points (and I prefer them to a wall of text
which is more daunting to read).

-Peff

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 19, 2013 at 10:25:46AM -0700, Junio C Hamano wrote:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>> 
>> > Junio C Hamano wrote:
>> >
>> >>     You ran 'git add' with neither '-A (--all)' or '--no-all', whose
>> >>     behaviour will change in Git 2.0 with respect to paths you
>> >>     removed from your working tree.
>> >>
>> >>     * 'git add --no-all <pathspec>', which is the current default,
>> >>       ignores paths you removed from your working tree.
>> >>
>> >>     * 'git add --all <pathspec>' will let you also record the
>> >>       removals.
>> >>
>> >>     The removed paths (e.g. '%s') are ignored with this version of Git.
>> >>     Run 'git status' to remind yourself what paths you have removed
>> >>     from your working tree.
>> >>
>> >> or something?
>> >
>> > That looks good. :)
>> 
>> I think the direction may be good but the above is too tall to be
>> the final version. of the message.  Somebody good at phrasing needs
>> to trim it down without losing the essense.
>
> Hmph. I actually like it as it is. It says:
>
>   1. Here's what triggered this warning (removed paths without -A).
>
>   2. Here is how you tell git what you want to do (--all/--no-all)
>
>   3. Here is how you get more information about what you wanted to do
>      (mention one such path, point to "git status").
>
> I would not want to cut out any of those three. You could perhaps cut
> out the bullet points in the middle, which reduces (2), but the user may
> be able to figure it out from the first sentence. However, I like the
> explicitness of those bullet points (and I prefer them to a wall of text
> which is more daunting to read).

I think we are saying the same thing.  I do agree with you that
these three points are "the essense" we do not want to lose.

Shrinking the first paragraph and the last paragraph to less than
three lines and each bullet point a single liner each was the kind
of change I had in mind.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Fri, Apr 19, 2013 at 4:07 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Thu, Apr 18, 2013 at 7:48 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

>> If something is not hypothetical, it's real, which means it actually
>> happened, but then you said you never made the claim that it did. So
>> what is it?
>
> My claim:
>
>    I bisected to a commit whose commit message helped me deduce its entirety.

What a bold claim! That changes everything! Without your valuable
input this discussion would have gone nowhere!

This claim is absolutely worthless, nobody denies that somebody at
some point in time did bisect a commit and a read it's commit message,
and claiming what nobody denies makes as much sense as fighting the
wind.

Thanks for wasting all of our times.

> Your fanciful interpretation, which I denied:
>
>   "Show me one of the instances where you hit a bisect with
>   any of the remote-hg commits mentioned above by Thomas Rast."
>
>
> I have never bisected to any commit related to remote-hg, and neither
> did I ever claim to.  I do not know where you got such a ridiculous
> qualifier as this to append to my statement.

That's *EXACTLY* the topic you replied to. Thomas Rast pointed to
remote-hg commits, I asked if he hit an actual issue that required to
look at the commit message, or such issue was hypothetical.

Then you come along and say it isn't, Well, what isn't? If it's not
the *EXACT* topic we were talking about.

>> Either it did happen, or it didn't;
>
> It did.  Where "it" is my actual claim, that I bisected to a commit
> whose commit message helped me deduce its entirety.

So "it" is absolutely unrelated to what we were talking about, and
"it" was something nobody cared about, nor did help one iota to move
the conversation forward.

Thanks, thanks a lot.

> But also, it didn't, where "it" is your preposterous interpretation of
> my interest and/or experiences with remote-hg commits.
>
> You seem only to want to argue, Felipe.  I have neither time nor
> interest in pig-wrestling, myself.

No, I don't want to argue, specially with people that either a) deny
that they argued what they argued, or b) argue with pointless obvious
claims about something that is not being discussed. Whether it's
intellectual dishonesty, or plain madness, I'm not interested.

Hitting an issue that required anybody to look at the commit messages
of the commits that Thomas Rast mentioned, is a hypothetical
situation. *Period*.

Cheers.

-- 
Felipe Contreras

jc/add-2.0-delete-default (Re: What's cooking in git.git (Apr 2013, #05; Mon, 15))

[Jonathan Nieder]

> On Fri, Apr 19, 2013 at 10:25:46AM -0700, Junio C Hamano wrote:
>>> Junio C Hamano wrote:

>>>>     You ran 'git add' with neither '-A (--all)' or '--no-all', whose
>>>>     behaviour will change in Git 2.0 with respect to paths you
>>>>     removed from your working tree.
>>>>
>>>>     * 'git add --no-all <pathspec>', which is the current default,
>>>>       ignores paths you removed from your working tree.
>>>>
>>>>     * 'git add --all <pathspec>' will let you also record the
>>>>       removals.
>>>>
>>>>     The removed paths (e.g. '%s') are ignored with this version of Git.
>>>>     Run 'git status' to remind yourself what paths you have removed
>>>>     from your working tree.
>>>>
>>>> or something?
[...]
>>                                     Somebody good at phrasing needs
>> to trim it down without losing the essense.

By the way, it was mentioned on IRC that the above is a bit odd for
a different reason: the option --no-all that maintains the old behavior
is not intuitively named.

How about something like this?

	warning: "git add" run on path with files removed (e.g., '%s')
	hint: use "git add --ignore-removals <pathspec>" to ignore removals
	hint: or "git add --no-ignore-removals <pathspec>" to notice them
	hint: --ignore-removals is the default but this will change soon
	hint: see git-add(1) for details

Then the --ignore-removals option could be added using a patch like
the following.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-add.txt | 24 +++++++++++++++---------
 builtin/add.c             | 28 +++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index b0944e57..8607cf37 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,7 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'git add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
-	  [--edit | -e] [--all | [--update | -u]] [--intent-to-add | -N]
+	  [--edit | -e] [--update | -u] [--no-ignore-removals | -A]
+	  [--intent-to-add | -N]
 	  [--refresh] [--ignore-errors] [--ignore-missing] [--]
 	  [<pathspec>...]
 
@@ -109,17 +110,22 @@ If no <pathspec> is given, the current version of Git defaults to
 and its subdirectories. This default will change in a future version
 of Git, hence the form without <pathspec> should not be used.
 
+--ignore-removals::
+--no-ignore-removals::
 -A::
 --all::
-	Update the index not only where the working tree has a file
-	matching <pathspec> but also where the index already has an
-	entry.	This adds, modifies, and removes index entries to
-	match the working tree.
+	Update the index only where the working tree has a file
+	matching <pathspec>.  This adds and modifies index entries
+	to match the working tree but ignores removed files.
 +
-If no <pathspec> is given, the current version of Git defaults to
-"."; in other words, update all files in the current directory
-and its subdirectories. This default will change in a future version
-of Git, hence the form without <pathspec> should not be used.
+This is currently the default.  Git 2.0 will change the default
+to --no-ignore-removals.
++
+With --no-ignore-removals (and its historical synonyms `-A` and
+`--all`), if no <pathspec> is given, the current version of Git
+defaults to "."; in other words, update all files in the current
+directory and its subdirectories. This default will change in a future
+version of Git, hence the form without <pathspec> should not be used.
 
 -N::
 --intent-to-add::
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8f..4a4e71ad 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,6 +28,14 @@ struct update_callback_data {
 	int add_errors;
 };
 
+static int parse_opt_neg_tertiary(const struct option *opt, const char *arg,
+				  int unset)
+{
+	int *target = opt->value;
+	*target = unset ? 1 : 2;
+	return 0;
+}
+
 static int fix_unmerged_status(struct diff_filepair *p,
 			       struct update_callback_data *data)
 {
@@ -271,7 +279,8 @@ static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
 static int verbose = 0, show_only = 0, ignored_too = 0, refresh_only = 0;
-static int ignore_add_errors, addremove, intent_to_add, ignore_missing = 0;
+static int ignore_add_errors, intent_to_add, ignore_missing = 0;
+static int ignore_removals, addremove;
 
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only, N_("dry run")),
@@ -283,10 +292,12 @@ static struct option builtin_add_options[] = {
 	OPT__FORCE(&ignored_too, N_("allow adding otherwise ignored files")),
 	OPT_BOOLEAN('u', "update", &take_worktree_changes, N_("update tracked files")),
 	OPT_BOOLEAN('N', "intent-to-add", &intent_to_add, N_("record only the fact that the path will be added later")),
-	OPT_BOOLEAN('A', "all", &addremove, N_("add changes from all tracked and untracked files")),
+	OPT_UYN( 0 , "ignore-removals", &ignore_removals, N_("do not record removal of tracked files")),
 	OPT_BOOLEAN( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
 	OPT_BOOLEAN( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
 	OPT_BOOLEAN( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
+	{ OPTION_CALLBACK, 'A', "all", &ignore_removals, NULL, N_("synonym for --no-ignore-removals"),
+	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, &parse_opt_neg_tertiary },
 	OPT_END(),
 };
 
@@ -377,8 +388,19 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	argc--;
 	argv++;
 
+	if (ignore_removals == 2) {	/* --no-ignore-removals, or -A */
+		addremove = 1;
+		ignore_removals = 0;
+	}
+	if (ignore_removals && take_worktree_changes)
+		/*
+		 * NEEDSWORK: "git add -u --ignore-removals" should mean
+		 * "git diff --diff-filter=M | git apply --cached"
+		 */
+		die(_("--ignore-removals cannot be used with --update"));
 	if (addremove && take_worktree_changes)
-		die(_("-A and -u are mutually incompatible"));
+		/* -u --no-ignore-removals is the same as -u */
+		addremove = 0;
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
 	if (addremove) {
-- 
1.8.2.1

Re: jc/add-2.0-delete-default (Re: What's cooking in git.git (Apr 2013, #05; Mon, 15))

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> How about something like this?
>
> 	warning: "git add" run on path with files removed (e.g., '%s')
> 	hint: use "git add --ignore-removals <pathspec>" to ignore removals
> 	hint: or "git add --no-ignore-removals <pathspec>" to notice them
> 	hint: --ignore-removals is the default but this will change soon
> 	hint: see git-add(1) for details
>
> Then the --ignore-removals option could be added using a patch like
> the following.

adding ignore-removals as a synonym (and keeping it) would be a good
idea.

We would still need to carry --all and --no-all that have been with
us ever since we added "-A" option, though.

Re: jc/add-2.0-delete-default (Re: What's cooking in git.git (Apr 2013, #05; Mon, 15))

[Junio C Hamano]

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

>> Then the --ignore-removals option could be added using a patch like
>> the following.
>
> adding ignore-removals as a synonym (and keeping it) would be a good
> idea.
>
> We would still need to carry --all and --no-all that have been with
> us ever since we added "-A" option, though.

The final step to turn "-A" the default will be held back until Git 2.0 release,
but I've inserted the following patch before that step.

I am thinking that it would be a good idea to merge up to this step
to 'master' tomorrow, and have you guys tweak it further on 'master'
with a patch like the one I am responding to, before the 1.8.3
final.  We will have to tweak the 2.0 endgame version as we go but
that is outside 'next' for now, so it should be manageable.

-- >8 --
Subject: [PATCH] git add: rephrase the "removal will cease to be ignored" warning

Now the logic to decide when to warn has been tightened, we know the
user is in a situation where the current and future behaviours will
be different.  Spell out what happens with these two versions and
how to explicitly ask for the behaviour, and suggest "git status" as
a way to inspect the current status.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 4242bce..20f459a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -52,15 +52,22 @@ static int fix_unmerged_status(struct diff_filepair *p,
 		return DIFF_STATUS_MODIFIED;
 }
 
+static const char *add_would_remove_warning = N_(
+	"You ran 'git add' with neither '-A (--all)' or '--no-all', whose\n"
+"behaviour will change in Git 2.0 with respect to paths you removed from\n"
+"your working tree. Paths like '%s' that are\n"
+"removed are ignored with this version of Git.\n"
+"\n"
+"* 'git add --no-all <pathspec>', which is the current default, ignores\n"
+"  paths you removed from your working tree.\n"
+"\n"
+"* 'git add --all <pathspec>' will let you also record the removals.\n"
+"\n"
+"Run 'git status' to check the paths you removed from your working tree.\n");
+
 static void warn_add_would_remove(const char *path)
 {
-	warning(_("In Git 2.0, 'git add <pathspec>...' will also update the\n"
-		  "index for paths removed from the working tree that match\n"
-		  "the given pathspec. If you want to 'add' only changed\n"
-		  "or newly created paths, say 'git add --no-all <pathspec>...'"
-		  " instead.\n\n"
-		  "'%s' would be removed from the index without --no-all."),
-		path);
+	warning(_(add_would_remove_warning), path);
 }
 
 static void update_callback(struct diff_queue_struct *q,
-- 
1.8.2.1-650-g3c8b519

[PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

Applying Jonathan's idea on top of the early part that has graduated
to 'master', here is to add "--ignore-removal" (which is a more
natural way to say "--no-all") and use it in the warning message.

Junio C Hamano (2):
  git add: --ignore-removal is a better named --no-all
  git add: rephrase -A/--no-all warning

 Documentation/git-add.txt | 10 ++++++----
 builtin/add.c             | 23 +++++++++++++++++------
 2 files changed, 23 insertions(+), 10 deletions(-)

-- 
1.8.2.1-683-g39c426e

[PATCH 1/2] git add: --ignore-removal is a better named --no-all

[Junio C Hamano]

In the historical context of "git add --all ." that tells the
command to pay attention to "all kinds of changes" (implying
"without ignoring removals"), the option "--no-all" to countermand
it may have made some sense, but because we will be making "--all"
the default when a pathspec is given, it makes more sense to rename
the option to a more explicit "--ignore-removal".  The "-all" option
naturally becomes its negation: "--no-ignore-removal".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-add.txt | 10 ++++++----
 builtin/add.c             | 11 +++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 5c501a2..48754cb 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -9,9 +9,9 @@ SYNOPSIS
 --------
 [verse]
 'git add' [-n] [-v] [--force | -f] [--interactive | -i] [--patch | -p]
-	  [--edit | -e] [--[no-]all | [--update | -u]] [--intent-to-add | -N]
-	  [--refresh] [--ignore-errors] [--ignore-missing] [--]
-	  [<pathspec>...]
+	  [--edit | -e] [--[no-]all | --[no-]ignore-removal | [--update | -u]]
+	  [--intent-to-add | -N] [--refresh] [--ignore-errors] [--ignore-missing]
+	  [--] [<pathspec>...]
 
 DESCRIPTION
 -----------
@@ -111,6 +111,7 @@ of Git, hence the form without <pathspec> should not be used.
 
 -A::
 --all::
+--no-ignore-removal::
 	Update the index not only where the working tree has a file
 	matching <pathspec> but also where the index already has an
 	entry.	This adds, modifies, and removes index entries to
@@ -122,6 +123,7 @@ and its subdirectories. This default will change in a future version
 of Git, hence the form without <pathspec> should not be used.
 
 --no-all::
+--ignore-removal::
 	Update the index by adding new files that are unknown to the
 	index and files modified in the working tree, but ignore
 	files that have been removed from the working tree.  This
@@ -130,7 +132,7 @@ of Git, hence the form without <pathspec> should not be used.
 This option is primarily to help the current users of Git, whose
 "git add <pathspec>..." ignores removed files.  In future versions
 of Git, "git add <pathspec>..." will be a synonym to "git add -A
-<pathspec>..." and "git add --no-all <pathspec>..." will behave like
+<pathspec>..." and "git add --ignore-removal <pathspec>..." will behave like
 today's "git add <pathspec>...", ignoring removed files.
 
 -N::
diff --git a/builtin/add.c b/builtin/add.c
index 54cd2d4..aefbc45 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -382,6 +382,13 @@ static int ignore_add_errors, intent_to_add, ignore_missing;
 static int addremove = ADDREMOVE_DEFAULT;
 static int addremove_explicit = -1; /* unspecified */
 
+static int ignore_removal_cb(const struct option *opt, const char *arg, int unset)
+{
+	/* if we are told to ignore, we are not adding removals */
+	*(int *)opt->value = !unset ? 0 : 1;
+	return 0;
+}
+
 static struct option builtin_add_options[] = {
 	OPT__DRY_RUN(&show_only, N_("dry run")),
 	OPT__VERBOSE(&verbose, N_("be verbose")),
@@ -393,6 +400,10 @@ static struct option builtin_add_options[] = {
 	OPT_BOOL('u', "update", &take_worktree_changes, N_("update tracked files")),
 	OPT_BOOL('N', "intent-to-add", &intent_to_add, N_("record only the fact that the path will be added later")),
 	OPT_BOOL('A', "all", &addremove_explicit, N_("add changes from all tracked and untracked files")),
+	{ OPTION_CALLBACK, 0, "ignore-removal", &addremove_explicit,
+	  NULL /* takes no arguments */,
+	  N_("ignore paths removed in the working tree (same as --no-all)"),
+	  PARSE_OPT_NOARG, ignore_removal_cb },
 	OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
 	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
 	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
-- 
1.8.2.1-683-g39c426e

[PATCH 2/2] git add: rephrase -A/--no-all warning

[Junio C Hamano]

With the synonym "--ignore-removal" for "--no-all", we can rephrase
the Git 2.0 transition warning message in a more natural way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/add.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index aefbc45..c55615b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -97,13 +97,13 @@ static int fix_unmerged_status(struct diff_filepair *p,
 }
 
 static const char *add_would_remove_warning = N_(
-	"You ran 'git add' with neither '-A (--all)' or '--no-all', whose\n"
-"behaviour will change in Git 2.0 with respect to paths you removed from\n"
-"your working tree. Paths like '%s' that are\n"
-"removed are ignored with this version of Git.\n"
+	"You ran 'git add' with neither '-A (--all)' or '--ignore-removal',\n"
+"whose behaviour will change in Git 2.0 with respect to paths you removed.\n"
+"Paths like '%s' that are\n"
+"removed from your working tree are ignored with this version of Git.\n"
 "\n"
-"* 'git add --no-all <pathspec>', which is the current default, ignores\n"
-"  paths you removed from your working tree.\n"
+"* 'git add --ignore-removal <pathspec>', which is the current default,\n"
+"  ignores paths you removed from your working tree.\n"
 "\n"
 "* 'git add --all <pathspec>' will let you also record the removals.\n"
 "\n"
-- 
1.8.2.1-683-g39c426e

[PATCH 3/2] git add <pathspec>... defaults to "-A"

[Junio C Hamano]

Make "git add <pathspec>..." notice paths that have been removed
from the working tree, i.e. a synonym to "git add -A <pathspec>...".

Given that "git add <pathspec>" is to update the index with the
state of the named part of the working tree as a whole, it makes it
more intuitive, and also makes it possible to simplify the advice we
give while marking the paths the user finished resolving conflicts
with.  We used to say "to record removal as a resolution, remove the
path from the working tree and say 'git rm'; for all other cases,
edit the path in the working tree and say 'git add'", but we can now
say "update the path in the working tree and say 'git add'" instead.

As promised, this merges the temporary update_files_in_cache() helper
function back to add_files_to_cache() function.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This comes on top of the previous two meant for 1.8.3, but should
   wait until Git 2.0.  You can see that it still has conflicts with
   Jonathan's jn/add-2.0-u-A-sans-pathspec topic, but there is a
   resolution already prepared on 'pu'.

 Documentation/git-add.txt | 18 +++++++++++-------
 builtin/add.c             | 43 ++++---------------------------------------
 2 files changed, 15 insertions(+), 46 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 48754cb..77ad391 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -53,8 +53,14 @@ OPTIONS
 	Files to add content from.  Fileglobs (e.g. `*.c`) can
 	be given to add all matching files.  Also a
 	leading directory name (e.g. `dir` to add `dir/file1`
-	and `dir/file2`) can be given to add all files in the
-	directory, recursively.
+	and `dir/file2`) can be given to update the index to
+	match the current state of the directory as a whole (e.g.
+	specifying `dir` will record not just a file `dir/file1`
+	modified in the working tree, a file `dir/file2` added to
+	the working tree, but also a file `dir/file3` removed from
+	the working tree.  Note that older versions of 	Git used
+	to ignore removed files; use `--no-all` option if you want
+	to add modified or new files but ignore removed	ones.
 
 -n::
 --dry-run::
@@ -129,11 +135,9 @@ of Git, hence the form without <pathspec> should not be used.
 	files that have been removed from the working tree.  This
 	option is a no-op when no <pathspec> is used.
 +
-This option is primarily to help the current users of Git, whose
-"git add <pathspec>..." ignores removed files.  In future versions
-of Git, "git add <pathspec>..." will be a synonym to "git add -A
-<pathspec>..." and "git add --ignore-removal <pathspec>..." will behave like
-today's "git add <pathspec>...", ignoring removed files.
+This option is primarily to help users who are used to older
+versions of Git, whose "git add <pathspec>..." was a synonym
+to "git add --no-all <pathspec>...", i.e. ignored removed files.
 
 -N::
 --intent-to-add::
diff --git a/builtin/add.c b/builtin/add.c
index c55615b..22c5ff5 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -28,9 +28,6 @@ struct update_callback_data {
 	int add_errors;
 	const char *implicit_dot;
 	size_t implicit_dot_len;
-
-	/* only needed for 2.0 transition preparation */
-	int warn_add_would_remove;
 };
 
 static const char *option_with_implicit_dot;
@@ -96,24 +93,6 @@ static int fix_unmerged_status(struct diff_filepair *p,
 		return DIFF_STATUS_MODIFIED;
 }
 
-static const char *add_would_remove_warning = N_(
-	"You ran 'git add' with neither '-A (--all)' or '--ignore-removal',\n"
-"whose behaviour will change in Git 2.0 with respect to paths you removed.\n"
-"Paths like '%s' that are\n"
-"removed from your working tree are ignored with this version of Git.\n"
-"\n"
-"* 'git add --ignore-removal <pathspec>', which is the current default,\n"
-"  ignores paths you removed from your working tree.\n"
-"\n"
-"* 'git add --all <pathspec>' will let you also record the removals.\n"
-"\n"
-"Run 'git status' to check the paths you removed from your working tree.\n");
-
-static void warn_add_would_remove(const char *path)
-{
-	warning(_(add_would_remove_warning), path);
-}
-
 static void update_callback(struct diff_queue_struct *q,
 			    struct diff_options *opt, void *cbdata)
 {
@@ -151,10 +130,6 @@ static void update_callback(struct diff_queue_struct *q,
 			}
 			break;
 		case DIFF_STATUS_DELETED:
-			if (data->warn_add_would_remove) {
-				warn_add_would_remove(path);
-				data->warn_add_would_remove = 0;
-			}
 			if (data->flags & ADD_CACHE_IGNORE_REMOVAL)
 				break;
 			if (!(data->flags & ADD_CACHE_PRETEND))
@@ -378,7 +353,7 @@ N_("The following paths are ignored by one of your .gitignore files:\n");
 static int verbose, show_only, ignored_too, refresh_only;
 static int ignore_add_errors, intent_to_add, ignore_missing;
 
-#define ADDREMOVE_DEFAULT 0 /* Change to 1 in Git 2.0 */
+#define ADDREMOVE_DEFAULT 1
 static int addremove = ADDREMOVE_DEFAULT;
 static int addremove_explicit = -1; /* unspecified */
 
@@ -476,20 +451,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (addremove && take_worktree_changes)
 		die(_("-A and -u are mutually incompatible"));
 
-	/*
-	 * Warn when "git add pathspec..." was given without "-u" or "-A"
-	 * and pathspec... covers a removed path.
-	 */
-	memset(&update_data, 0, sizeof(update_data));
-	if (!take_worktree_changes && addremove_explicit < 0)
-		update_data.warn_add_would_remove = 1;
-
 	if (!take_worktree_changes && addremove_explicit < 0 && argc)
-		/*
-		 * Turn "git add pathspec..." to "git add -A pathspec..."
-		 * in Git 2.0 but not yet
-		 */
-		; /* addremove = 1; */
+		/* Turn "git add pathspec..." to "git add -A pathspec..." */
+		addremove = 1;
 
 	if (!show_only && ignore_missing)
 		die(_("Option --ignore-missing can only be used together with --dry-run"));
@@ -579,6 +543,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	plug_bulk_checkin();
 
+	memset(&update_data, 0, sizeof(update_data));
 	if ((flags & ADD_CACHE_IMPLICIT_DOT) && prefix) {
 		/*
 		 * Check for modified files throughout the worktree so
-- 
1.8.2.1-740-ga5571a3

Re: [PATCH 3/2] git add <pathspec>... defaults to "-A"

[Eric Sunshine]

On Mon, Apr 22, 2013 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 48754cb..77ad391 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -53,8 +53,14 @@ OPTIONS
>         Files to add content from.  Fileglobs (e.g. `*.c`) can
>         be given to add all matching files.  Also a
>         leading directory name (e.g. `dir` to add `dir/file1`
> -       and `dir/file2`) can be given to add all files in the
> -       directory, recursively.
> +       and `dir/file2`) can be given to update the index to
> +       match the current state of the directory as a whole (e.g.
> +       specifying `dir` will record not just a file `dir/file1`
> +       modified in the working tree, a file `dir/file2` added to
> +       the working tree, but also a file `dir/file3` removed from
> +       the working tree.  Note that older versions of  Git used

s/of\s+Git/of Git/

> +       to ignore removed files; use `--no-all` option if you want
> +       to add modified or new files but ignore removed ones.
>
>  -n::
>  --dry-run::
> @@ -129,11 +135,9 @@ of Git, hence the form without <pathspec> should not be used.
>         files that have been removed from the working tree.  This
>         option is a no-op when no <pathspec> is used.
>  +
> -This option is primarily to help the current users of Git, whose
> -"git add <pathspec>..." ignores removed files.  In future versions
> -of Git, "git add <pathspec>..." will be a synonym to "git add -A
> -<pathspec>..." and "git add --ignore-removal <pathspec>..." will behave like
> -today's "git add <pathspec>...", ignoring removed files.
> +This option is primarily to help users who are used to older
> +versions of Git, whose "git add <pathspec>..." was a synonym
> +to "git add --no-all <pathspec>...", i.e. ignored removed files.

s/to/for/ [or] s/to/of/

>
>  -N::
>  --intent-to-add::

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Ramkumar Ramachandra]

[off-topic; what happened/happens to your series is entirely unrelated
to the issue]

Felipe Contreras wrote:
> Nobody knows how life began, and it doesn't matter now, what matters
> is how life evolves. It doesn't matter if the chicken was first, or
> the egg, what matters is that if all the chickens and eggs are gone,
> there won't be more.
>
> Plenty of projects have died because they stopped caring about their
> users, and without users there's no new developers, and the old
> developers eventually move on, and all the literary quality of commit
> messages have no eyes to see it.

I was a pure end-user of git until about Jan 2010.  I was initially
impressed with git because it behaved in a beautiful consistent
manner.  Then I dug in and found out that it had a beautiful codebase,
excellent mailing list (content and conventions), and large
development community.  I could literally read through the commit
messages and code with ease.  I do bounce between a few projects, but
always come back to git because nothing else fits the criterion.  What
I do not consider (as much as the other things) is the
number-of-end-users.

Then again, you would argue that I came across git only because of a
large enough user-base.  I agree with that, but you're practically
idolizing user-base as the most important thing.

My point is simple: yes, it's nice to have a big user base.  We
already do.  Now, what's the point of pitching to end-users who only
use the most basic functionality?  Their inputs are likely to be
useless (arising from misunderstandings) anyway.  They're not going to
be the next developers.  And they're not going to help create what our
next developer is looking for in us either (i.e. codebase, community).

Our primary customers are each other, because that's how we get a
tight community and great codebase.  And because the next potential
developer looks like one of us.

That does _not_ mean: live only within the community.  Everyone should
have a healthy interaction with the outside world, otherwise they risk
turning into researchers and suffering engineering myopia.  And
ofcourse not attract a large userbase.

Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Felipe Contreras]

On Tue, Apr 23, 2013 at 1:49 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:

> My point is simple: yes, it's nice to have a big user base.  We
> already do.  Now, what's the point of pitching to end-users who only
> use the most basic functionality?  Their inputs are likely to be
> useless (arising from misunderstandings) anyway.  They're not going to
> be the next developers.  And they're not going to help create what our
> next developer is looking for in us either (i.e. codebase, community).

That is your mistake right there. They *are* the next developers, you
yourself came from there. We all did.

In fact, this notion that there's a divide between users and
developers is a myth; it's a continuum that follows the Pareto
distribution. It happens in every healthy open source project.

And this is not an assumption, I've measured it:

http://felipec.wordpress.com/2011/11/21/no-project-is-more-important-than-its-users/

70% of the commits in git.git come from people that have provided less
than 6 patches. That 70% (maybe 80%, maybe 90%) would have never
happened, if git didn't have a large enough user-base. I'm not
idolizing the user-base, this project *is* the user-base, developers
are users, and without users there's no project.

Again, in the words of Linus: no project is more important than it's users.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

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

> Applying Jonathan's idea on top of the early part that has graduated
> to 'master', here is to add "--ignore-removal" (which is a more
> natural way to say "--no-all") and use it in the warning message.
>
> Junio C Hamano (2):
>   git add: --ignore-removal is a better named --no-all
>   git add: rephrase -A/--no-all warning
>
>  Documentation/git-add.txt | 10 ++++++----
>  builtin/add.c             | 23 +++++++++++++++++------
>  2 files changed, 23 insertions(+), 10 deletions(-)

I am planning to fast-track this to 'master' for 1.8.3, to
complement Jonathan's "add -u/-A without pathspec" warning.

It would be nice if people can eyeball the behaviour of tonight's
'next', find glitches (if any) and help polishing it before the
feature freeze.

One thing I noticed about Jonathan's warn_pathless_add() thing is
that even though it knows for which path we would behave differently
between the current version and Git 2.0, the warning message does
not say which path outside the current directory would be added if
not restricted with an explicit ".", and leaves the reader in
suspense.

We may want to fix it by tweaking the end of the message, perhaps?

 builtin/add.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index d4b40f2..24a2d6f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -36,7 +36,7 @@ struct update_callback_data {
 static const char *option_with_implicit_dot;
 static const char *short_option_with_implicit_dot;
 
-static void warn_pathless_add(void)
+static void warn_pathless_add(const char *path)
 {
 	static int shown;
 	assert(option_with_implicit_dot && short_option_with_implicit_dot);
@@ -67,12 +67,16 @@ static void warn_pathless_add(void)
 		  "  git add %s .\n"
 		  "  (or git add %s .)\n"
 		  "\n"
-		  "With the current Git version, the command is restricted to "
-		  "the current directory.\n"
+		  "With the current Git version, the command is limited to the current"
+		  "directory, and paths like '%s'\n"
+		  "that %s are not added.\n"
 		  ""),
 		option_with_implicit_dot, short_option_with_implicit_dot,
 		option_with_implicit_dot, short_option_with_implicit_dot,
-		option_with_implicit_dot, short_option_with_implicit_dot);
+		option_with_implicit_dot, short_option_with_implicit_dot,
+		path,
+		(!strcmp(short_option_with_implicit_dot, "-u")
+		 ? _("are modified") : _("are new")));
 }
 
 static int fix_unmerged_status(struct diff_filepair *p,
@@ -136,7 +140,7 @@ static void update_callback(struct diff_queue_struct *q,
 		 */
 		if (implicit_dot &&
 		    strncmp_icase(path, implicit_dot, implicit_dot_len)) {
-			warn_pathless_add();
+			warn_pathless_add(path);
 			continue;
 		}
 		switch (fix_unmerged_status(p, data)) {

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

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

> One thing I noticed about Jonathan's warn_pathless_add() thing is
> that even though it knows for which path we would behave differently
> between the current version and Git 2.0, the warning message does
> not say which path outside the current directory would be added if
> not restricted with an explicit ".", and leaves the reader in
> suspense.
>
> We may want to fix it by tweaking the end of the message, perhaps?

Hmph, bad idea.

At the point of calling warn_pathless_add(), it seems that we are
triggering this for paths that are not necessarily modified when run
with "add -n -u".

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Jonathan Nieder]

Junio C Hamano wrote:

> At the point of calling warn_pathless_add(), it seems that we are
> triggering this for paths that are not necessarily modified when run
> with "add -n -u".

Do you mean files that were touched but have no content change, or
something more subtle?

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> At the point of calling warn_pathless_add(), it seems that we are
>> triggering this for paths that are not necessarily modified when run
>> with "add -n -u".
>
> Do you mean files that were touched but have no content change, or
> something more subtle?

I had the change (which by the way needs a fix for the "found a
directory" codepath) on top of master, uncommitted, and no other
change (I also have some cruft that is not ignored).

    cd Documentation && ../git add -n -u

reported GIT-VERSION-GEN which was not touched.  It does not
reproduce, though...

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>
>>> At the point of calling warn_pathless_add(), it seems that we are
>>> triggering this for paths that are not necessarily modified when run
>>> with "add -n -u".
>>
>> Do you mean files that were touched but have no content change, or
>> something more subtle?
>
> I had the change (which by the way needs a fix for the "found a
> directory" codepath) on top of master, uncommitted, and no other
> change (I also have some cruft that is not ignored).
>
>     cd Documentation && ../git add -n -u
>
> reported GIT-VERSION-GEN which was not touched.  It does not
> reproduce, though...

Ahh, I haven't run anything under the debugger yet, but I think I
know what is going on.

Don't we limit our "update-index --refresh" equivalent to the
original pathspec, even though your "-u/-A sans pathspec" warning
detection relies on grabbing the changes from the entire tree?

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Jonathan Nieder]

Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:

>>> Do you mean files that were touched but have no content change, or
>>> something more subtle?
[...]
> Ahh, I haven't run anything under the debugger yet, but I think I
> know what is going on.
>
> Don't we limit our "update-index --refresh" equivalent to the
> original pathspec, even though your "-u/-A sans pathspec" warning
> detection relies on grabbing the changes from the entire tree?

I think it's more basic than that.  "git add" doesn't bother to
run an "update-index --refresh" equivalent before its main loop
unless you pass --refresh to it, since reading files to compare
them to the index would be duplicated work.  The files hit in
update_callback() are only potentially modified.

Maybe the warning should happen after add_file_to_index() has run,
letting git compare the old and new index entries for that path?

Jonathan

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Maybe the warning should happen after add_file_to_index() has run,
> letting git compare the old and new index entries for that path?

Yeah, new and deleted cases we do not have to worry about, so a
no-op add_file_to_index() is the only case we have to be careful.
There is a "if verbose, say 'add %s'" logic in the funciton, so it
should be possible to enhance the API without affecting existing
callers to extract that necessary information out of it.

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Junio C Hamano]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Maybe the warning should happen after add_file_to_index() has run,
>> letting git compare the old and new index entries for that path?
>
> Yeah, new and deleted cases we do not have to worry about, so a
> no-op add_file_to_index() is the only case we have to be careful.
> There is a "if verbose, say 'add %s'" logic in the funciton, so it
> should be possible to enhance the API without affecting existing
> callers to extract that necessary information out of it.

I've thought about this a bit more.

One possible solution would go like this:

 - Extend add_file_to_index() (the logic is add_to_index() in
   read-cache.c) so that it can return an extra boolean "I would add
   it, but that would be a no-op---the index already has that
   object" to the caller.

 - In update_callback(), when we are comparing _all_ paths due to
   "implicit-dot" logic, check if the path is outside the current
   directory, instead of unconditionally calling warn_pathless_add():

   * If fix_unmerged_status() tells us that we would go to the
     remove_file_from_index() codepath, instead of calling it, call
     warn_pathless_add() instead.

   * If we are going to call add_file_to_index(), call it with
     ADD_CACHE_PRETEND on using the extended interface to see if it
     is adding already up-to-date contents. If not, call
     warn_pathless_add().

But I think it is a much better solution to just refresh the index
like the attached patch when implicit_dot is active and we are not
at the top level directory.  The paths that are stat-dirty but have
the up-to-date contents need to be hashed at least once _anyway_ to
see if the current contents match with what is in the index.  If we
use the approach outlined above, the rehashing will be done in the
extended add_file_to_index(). If we simply refresh the entire cache,
the same check will be done there.  The only performance penalty
would be that we may end up running lstat() twice.

Incidentally, I noticed that we set implicit_dot=1 even when we are
already at the top-level directory.  I suspect the code may become
somewhat simpler if we set it only when (prefix != NULL), but it
probably would not matter.


 builtin/add.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index daf02c6..ec2359c 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -495,6 +495,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 		refresh(verbose, pathspec);
 		goto finish;
 	}
+	if (implicit_dot && !prefix)
+		refresh_cache(REFRESH_QUIET);
 
 	if (pathspec) {
 		int i;

Re: [PATCH 0/2] "git add -A/--no-all" finishing touches

[Jonathan Nieder]

Junio C Hamano wrote:

> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -495,6 +495,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  		refresh(verbose, pathspec);
>  		goto finish;
>  	}
> +	if (implicit_dot && !prefix)
> +		refresh_cache(REFRESH_QUIET);

I think you mean "if (implicit_dot && prefix)". :)

This strategy is much less invasive than the alternatives discussed,
so for what it's worth, with that correction,

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

I'll try to get time to work on those promised tests this weekend.