What's cooking in git.git (Aug 2011, #07; Wed, 24)

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' while commits prefixed with '+' are in 'next'.

I've sifted topics to the ones meant for 1.7.7 and the others that I'd prefer to cook a bit longer in "next" during the upcoming feature freeze, which should happen by the end of the week.

Please help us make the upcoming release as solid as we could. Thanks.

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

* jk/add-i-hunk-filter (2011-07-27) 5 commits
  (merged to 'next' on 2011-08-11 at 8ff9a56)
 + add--interactive: add option to autosplit hunks
 + add--interactive: allow negatation of hunk filters
 + add--interactive: allow hunk filtering on command line
 + add--interactive: factor out regex error handling
 + add--interactive: refactor patch mode argument processing

Needs documentation updates.

* jh/receive-count-limit (2011-05-23) 10 commits
 - receive-pack: Allow server to refuse pushes with too many objects
 - pack-objects: Estimate pack size; abort early if pack size limit is exceeded
 - send-pack/receive-pack: Allow server to refuse pushing too large packs
 - pack-objects: Allow --max-pack-size to be used together with --stdout
 - send-pack/receive-pack: Allow server to refuse pushes with too many commits
 - pack-objects: Teach new option --max-commit-count, limiting #commits in pack
 - receive-pack: Prepare for addition of the new 'limit-*' family of capabilities
 - Tighten rules for matching server capabilities in server_supports()
 - send-pack: Attempt to retrieve remote status even if pack-objects fails
 - Update technical docs to reflect side-band-64k capability in receive-pack

Would need another round to separate per-pack and per-session limits.

* jm/mergetool-pathspec (2011-06-22) 2 commits
 - mergetool: Don't assume paths are unmerged
 - mergetool: Add tests for filename with whitespace

I think this is a good idea, but it probably needs a re-roll. Cf. $gmane/176254, 176255, 166256

* jk/generation-numbers (2011-07-14) 7 commits
 - limit "contains" traversals based on commit generation
 - check commit generation cache validity against grafts
 - pretty: support %G to show the generation number of a commit
 - commit: add commit_generation function
 - add metadata-cache infrastructure
 - decorate: allow storing values instead of pointers
 - Merge branch 'jk/tag-contains-ab' (early part) into HEAD

The initial "tag --contains" de-pessimization without need for generation numbers is already in; backburnered.

* sr/transport-helper-fix-rfc (2011-07-19) 2 commits
 - t5800: point out that deleting branches does not work
 - t5800: document inability to push new branch with old content

* po/cygwin-backslash (2011-08-05) 2 commits
 - On Cygwin support both UNIX and DOS style path-names
 - git-compat-util: add generic find_last_dir_sep that respects is_dir_sep

I think a further refactoring (no, not my suggestion) was offered?

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

* jc/merge-reword (2011-05-25) 1 commit
  (merged to 'next' on 2011-08-24 at aa5cf7b)
 + merge: reword the final message

Will merge to "master".

* nk/branch-v-abbrev (2011-07-01) 1 commit
  (merged to 'next' on 2011-08-24 at e9152cf)
 + branch -v: honor core.abbrev

Will merge to "master".

* jk/pager-with-external-command (2011-08-19) 1 commit
  (merged to 'next' on 2011-08-24 at 083f5da)
 + support pager.* for external commands
 (this branch is used by jk/pager-with-alias and jk/pager-with-alias; uses jk/color-and-pager.)

Up to this part (but not "alias") the topic looked unconditionally a good thing to do.

Will aim to merge to "master" by -rc1.

* fk/use-kwset-pickaxe-grep-f (2011-08-20) 5 commits
  (merged to 'next' on 2011-08-23 at 93ba509)
 + Use kwset in grep
 + Use kwset in pickaxe
 + Adapt the kwset code to Git
 + Add string search routines from GNU grep
 + Add obstack.[ch] from EGLIBC 2.10

Will aim to merge to "master" by -rc1.

* jc/maint-autofix-tag-in-head (2011-08-19) 1 commit
  (merged to 'next' on 2011-08-23 at 18cee02)
 + commit: reduce use of redundant global variables

Will merge to "master".

* bw/doc-repo-layout (2011-08-23) 2 commits
  (merged to 'next' on 2011-08-24 at 605c730)
 + Mark http-fetch without -a as deprecated
 + Documentation: Grammar correction, wording fixes and cleanup

Will merge to "master".

* ci/forbid-unwanted-current-branch-update (2011-08-22) 2 commits
  (merged to 'next' on 2011-08-24 at 1e93b67)
 + Show interpreted branch name in error messages
 + Prevent force-updating of the current branch

Will aim to merge to "master" by -rc1.

* di/fast-import-blob-tweak (2011-08-22) 2 commits
  (merged to 'next' on 2011-08-24 at 52eef2a)
 + fast-import: treat cat-blob as a delta base hint for next blob
 + fast-import: count and report # of calls to diff_delta in stats

Will aim to merge to "master" by -rc1.

* di/fast-import-tagging (2011-08-23) 2 commits
  (merged to 'next' on 2011-08-24 at 67e0937)
 + fast-import: allow to tag newly created objects
 + fast-import: add tests for tagging blobs

Will aim to merge to "master" by -rc1.

* di/fast-import-deltified-tree (2011-08-14) 2 commits
  (merged to 'next' on 2011-08-23 at ee30265)
 + fast-import: prevent producing bad delta
 + fast-import: add a test for tree delta base corruption

Will aim to merge to "master" by -rc1.

* di/fast-import-ident (2011-08-11) 5 commits
  (merged to 'next' on 2011-08-23 at 9b86391)
 + fsck: improve committer/author check
 + fsck: add a few committer name tests
 + fast-import: check committer name more strictly
 + fast-import: don't fail on omitted committer name
 + fast-import: add input format tests

Will aim to merge to "master" by -rc1.

* di/fast-import-doc (2011-08-17) 1 commit
  (merged to 'next' on 2011-08-23 at dab4088)
 + doc/fast-import: document feature import-marks-if-exists

Will merge to "master".

* jc/combine-diff-callback (2011-08-20) 1 commit
  (merged to 'next' on 2011-08-24 at 9f9b42d)
 + combine-diff: support format_callback
 (this branch is used by fg/submodule-ff-check-before-push.)

Will merge to "master".

* jc/maint-clone-alternates (2011-08-23) 2 commits
  (merged to 'next' on 2011-08-23 at 7280deb)
 + clone: clone from a repository with relative alternates
 + clone: allow more than one --reference

Will aim to merge to "master" by -rc1.

* nd/maint-clone-gitdir (2011-08-22) 2 commits
  (merged to 'next' on 2011-08-24 at cbf052b)
 + clone: allow to clone from .git file
 + read_gitfile_gently(): rename misnamed function to read_gitfile()

Will aim to merge to "master" by -rc1.

* jc/traverse-commit-list (2011-08-22) 3 commits
  (merged to 'next' on 2011-08-24 at df50dd7)
 + revision.c: update show_object_with_name() without using malloc()
 + revision.c: add show_object_with_name() helper function
 + rev-list: fix finish_object() call

Not urgent; will not be in 1.7.7.

* rc/diff-cleanup-records (2011-08-17) 2 commits
  (merged to 'next' on 2011-08-23 at b8414f5)
 + Merge branch 'rc/histogram-diff' into HEAD
 + xdiff/xprepare: improve O(n*m) performance in xdl_cleanup_records()

Will aim to merge to "master" by -rc1.

* fk/make-auto-header-dependencies (2011-08-18) 1 commit
  (merged to 'next' on 2011-08-24 at 3da2c25)
 + Makefile: Use computed header dependencies if the compiler supports it

Not urgent; will not be in 1.7.7.

* jk/color-and-pager (2011-08-19) 10 commits
  (merged to 'next' on 2011-08-23 at cbb9495)
 + want_color: automatically fallback to color.ui
 + diff: don't load color config in plumbing
 + config: refactor get_colorbool function
 + color: delay auto-color decision until point of use
 + git_config_colorbool: refactor stdout_is_tty handling
 + diff: refactor COLOR_DIFF from a flag into an int
 + setup_pager: set GIT_PAGER_IN_USE
 + t7006: use test_config helpers
 + test-lib: add helper functions for config
 + t7006: modernize calls to unset
 (this branch is used by jk/pager-with-alias and jk/pager-with-external-command.)

Will aim to merge to "master" by -rc1.

* jk/pager-with-alias (2011-08-19) 1 commit
 - support pager.* for aliases
 (this branch uses jk/color-and-pager, jk/pager-with-external-command and jk/pager-with-external-command.)

Perhaps will drop.

* nd/decorate-grafts (2011-08-19) 5 commits
  (merged to 'next' on 2011-08-23 at 475d27e)
 + log: decorate "replaced" on to replaced commits
 + log: decorate grafted commits with "grafted"
 + Move write_shallow_commits to fetch-pack.c
 + Add for_each_commit_graft() to iterate all grafts
 + decoration: do not mis-decorate refs with same prefix

Will aim to merge to "master" by -rc1.

* va/p4-branch-import (2011-08-22) 4 commits
  (merged to 'next' on 2011-08-24 at f67f8af)
 + git-p4: Add simple test case for branch import
 + git-p4: Allow branch definition with git config
 + git-p4: Allow filtering Perforce branches by user
 + git-p4: Correct branch base depot path detection
 (this branch uses va/p4-rename-copy.)

Will merge to "master".

* va/p4-rename-copy (2011-08-22) 5 commits
  (merged to 'next' on 2011-08-24 at f1faa94)
 + git-p4: Process detectCopiesHarder with --bool
 + git-p4: Add test case for copy detection
 + git-p4: Add test case for rename detection
 + git-p4: Add description of rename/copy detection options
 + git-p4: Allow setting rename/copy detection threshold
 (this branch is used by va/p4-branch-import.)

Will merge to "master".

* da/difftool-mergtool-refactor (2011-08-19) 4 commits
  (merged to 'next' on 2011-08-23 at a1cc3be)
 + mergetools/meld: Use '--output' when available
 + mergetool--lib: Refactor tools into separate files
 + mergetool--lib: Make style consistent with git
 + difftool--helper: Make style consistent with git

Will merge to "master".

* mg/branch-set-upstream-previous (2011-08-19) 1 commit
  (merged to 'next' on 2011-08-23 at acef0b6)
 + branch.c: use the parsed branch name

Will merge to "master".

* di/parse-options-split (2011-08-11) 2 commits
  (merged to 'next' on 2011-08-23 at 6cd667f)
 + Reduce parse-options.o dependencies
 + parse-options: export opterr, optbug

Will merge to "master".

* mh/attr (2011-08-14) 7 commits
  (merged to 'next' on 2011-08-23 at 22faa6e)
 + Unroll the loop over passes
 + Change while loop into for loop
 + Determine the start of the states outside of the pass loop
 + Change parse_attr() to take a pointer to struct attr_state
 + Increment num_attr in parse_attr_line(), not parse_attr()
 + Document struct match_attr
 + Add a file comment

Will aim to merge to "master" by -rc1.

* mh/iterate-refs (2011-08-14) 6 commits
 - Retain caches of submodule refs
 - Store the submodule name in struct cached_refs
 - Allocate cached_refs objects dynamically
 - Change the signature of read_packed_refs()
 - Access reference caches only through new function get_cached_refs()
 - Extract a function clear_cached_refs()

I did not see anything fundamentally wrong with this series, but it was unclear what the benefit of these changes are. If the series were to read parts of the ref hierarchy (like refs/heads/) lazily, the story would have been different, though.

Not urgent; will not be in 1.7.7.

* jn/plug-empty-tree-leak (2011-08-16) 2 commits
  (merged to 'next' on 2011-08-23 at aee2184)
 + merge-recursive: take advantage of hardcoded empty tree
 + revert: plug memory leak in "cherry-pick root commit" codepath

Will merge to "master".

* ac/describe-dirty-refresh (2011-08-11) 1 commit
  (merged to 'next' on 2011-08-23 at b873611)
 + describe: Refresh the index when run with --dirty

Will merge to "master".

* en/merge-recursive-2 (2011-08-14) 57 commits
  (merged to 'next' on 2011-08-23 at ba6ad0d)
 + merge-recursive: Don't re-sort a list whose order we depend upon
 + merge-recursive: Fix virtual merge base for rename/rename(1to2)/add-dest
 + t6036: criss-cross + rename/rename(1to2)/add-dest + simple modify
 + merge-recursive: Avoid unnecessary file rewrites
 + t6022: Additional tests checking for unnecessary updates of files
 + merge-recursive: Fix spurious 'refusing to lose untracked file...' messages
 + t6022: Add testcase for spurious "refusing to lose untracked" messages
 + t3030: fix accidental success in symlink rename
 + merge-recursive: Fix working copy handling for rename/rename/add/add
 + merge-recursive: add handling for rename/rename/add-dest/add-dest
 + merge-recursive: Have conflict_rename_delete reuse modify/delete code
 + merge-recursive: Make modify/delete handling code reusable
 + merge-recursive: Consider modifications in rename/rename(2to1) conflicts
 + merge-recursive: Create function for merging with branchname:file markers
 + merge-recursive: Record more data needed for merging with dual renames
 + merge-recursive: Defer rename/rename(2to1) handling until process_entry
 + merge-recursive: Small cleanups for conflict_rename_rename_1to2
 + merge-recursive: Fix rename/rename(1to2) resolution for virtual merge base
 + merge-recursive: Introduce a merge_file convenience function
 + merge-recursive: Fix modify/delete resolution in the recursive case
 + merge-recursive: When we detect we can skip an update, actually skip it
 + merge-recursive: Provide more info in conflict markers with file renames
 + merge-recursive: Cleanup and consolidation of rename_conflict_info
 + merge-recursive: Consolidate process_entry() and process_df_entry()
 + merge-recursive: Improve handling of rename target vs. directory addition
 + merge-recursive: Add comments about handling rename/add-source cases
 + merge-recursive: Make dead code for rename/rename(2to1) conflicts undead
 + merge-recursive: Fix deletion of untracked file in rename/delete conflicts
 + merge-recursive: Split update_stages_and_entry; only update stages at end
 + merge-recursive: Allow make_room_for_path() to remove D/F entries
 + string-list: Add API to remove an item from an unsorted list
 + merge-recursive: Split was_tracked() out of would_lose_untracked()
 + merge-recursive: Save D/F conflict filenames instead of unlinking them
 + merge-recursive: Fix code checking for D/F conflicts still being present
 + merge-recursive: Fix sorting order and directory change assumptions
 + merge-recursive: Fix recursive case with D/F conflict via add/add conflict
 + merge-recursive: Avoid working directory changes during recursive case
 + merge-recursive: Remember to free generated unique path names
 + merge-recursive: Consolidate different update_stages functions
 + merge-recursive: Mark some diff_filespec struct arguments const
 + merge-recursive: Correct a comment
 + merge-recursive: Make BUG message more legible by adding a newline
 + t6022: Add testcase for merging a renamed file with a simple change
 + t6022: New tests checking for unnecessary updates of files
 + t6022: Remove unnecessary untracked files to make test cleaner
 + t6036: criss-cross + rename/rename(1to2)/add-source + modify/modify
 + t6036: criss-cross w/ rename/rename(1to2)/modify+rename/rename(2to1)/modify
 + t6036: tests for criss-cross merges with various directory/file conflicts
 + t6036: criss-cross with weird content can fool git into clean merge
 + t6036: Add differently resolved modify/delete conflict in criss-cross test
 + t6042: Add failing testcases for rename/rename/add-{source,dest} conflicts
 + t6042: Ensure rename/rename conflicts leave index and workdir in sane state
 + t6042: Add tests for content issues with modify/rename/directory conflicts
 + t6042: Add a testcase where undetected rename causes silent file deletion
 + t6042: Add a pair of cases where undetected renames cause issues
 + t6042: Add failing testcase for rename/modify/add-source conflict
 + t6042: Add a testcase where git deletes an untracked file

Will aim to merge to "master" by -rc1.

* fg/submodule-ff-check-before-push (2011-08-20) 2 commits
  (merged to 'next' on 2011-08-24 at 398e764)
 + push: teach --recurse-submodules the on-demand option
 + push: Don't push a repository with unpushed submodules
 (this branch uses jc/combine-diff-callback.)

Will aim to merge to "master" by -rc1.

* hv/submodule-update-none (2011-08-11) 2 commits
  (merged to 'next' on 2011-08-24 at 5302fc1)
 + add update 'none' flag to disable update of submodule by default
 + submodule: move update configuration variable further up

Not urgent; will not be in 1.7.7.

* jc/lookup-object-hash (2011-08-11) 6 commits
  (merged to 'next' on 2011-08-24 at 5825411)
 + object hash: replace linear probing with 4-way cuckoo hashing
 + object hash: we know the table size is a power of two
 + object hash: next_size() helper for readability
 + pack-objects --count-only
 + object.c: remove duplicated code for object hashing
 + object.c: code movement for readability

I do not think there is anything fundamentally wrong with this series, but the risk of breakage far outweighs observed performance gain in one particular workload. Will keep it in 'next' at least for one cycle.

Not urgent; will not be in 1.7.7.

* js/i18n-scripts (2011-08-08) 5 commits
  (merged to 'next' on 2011-08-23 at a1b5529)
 + submodule: take advantage of gettextln and eval_gettextln.
 + stash: take advantage of eval_gettextln
 + pull: take advantage of eval_gettextln
 + git-am: take advantage of gettextln and eval_gettextln.
 + gettext: add gettextln, eval_gettextln to encode common idiom

Will merge to "master".

* fg/submodule-git-file-git-dir (2011-08-22) 2 commits
  (merged to 'next' on 2011-08-23 at 762194e)
 + Move git-dir for submodules
 + rev-parse: add option --resolve-git-dir <path>

I do not think there is anything fundamentally wrong with this series, but the risk of breakage outweighs any benefit for having this new feature. Will keep it in 'next' at least for one cycle.

Not urgent; will not be in 1.7.7.

* jk/http-auth-keyring (2011-08-03) 13 commits
  (merged to 'next' on 2011-08-03 at b06e80e)
 + credentials: add "getpass" helper
 + credentials: add "store" helper
 + credentials: add "cache" helper
 + docs: end-user documentation for the credential subsystem
 + http: use hostname in credential description
 + allow the user to configure credential helpers
 + look for credentials in config before prompting
 + http: use credential API to get passwords
 + introduce credentials API
 + http: retry authentication failures for all http requests
 + remote-curl: don't retry auth failures with dumb protocol
 + improve httpd auth tests
 + url: decode buffers that are not NUL-terminated

Looked mostly reasonable except for the limitation that it is not clear how to deal with a site at which a user needs to use different passwords for different repositories. Will keep it in "next" at least for one cycle, until we start hearing real-world success reports on the list.

Not urgent; will not be in 1.7.7.

* rr/revert-cherry-pick-continue (2011-08-08) 18 commits
  (merged to 'next' on 2011-08-24 at 712c115)
 + revert: Propagate errors upwards from do_pick_commit
 + revert: Introduce --continue to continue the operation
 + revert: Don't implicitly stomp pending sequencer operation
 + revert: Remove sequencer state when no commits are pending
 + reset: Make reset remove the sequencer state
 + revert: Introduce --reset to remove sequencer state
 + revert: Make pick_commits functionally act on a commit list
 + revert: Save command-line options for continuing operation
 + revert: Save data for continuing after conflict resolution
 + revert: Don't create invalid replay_opts in parse_args
 + revert: Separate cmdline parsing from functional code
 + revert: Introduce struct to keep command-line options
 + revert: Eliminate global "commit" variable
 + revert: Rename no_replay to record_origin
 + revert: Don't check lone argument in get_encoding
 + revert: Simplify and inline add_message_to_msg
 + config: Introduce functions to write non-standard file
 + advice: Introduce error_resolve_conflict

Will keep it in 'next' at least for one cycle. Not urgent; will not be in 1.7.7.

On 08/25/2011 02:09 AM, Junio C Hamano wrote:

I am still working on lazy reading and hierarchical storage of reference caches. It's OK with me if you defer merging the above patch series until I have made more progress on the more significant improvements.

Michael

On Wed, Aug 24, 2011 at 05:09:09PM -0700, Junio C Hamano wrote:

I think Duy already mentioned this, but you may want to update your "what's cooking" note: it needs not just doc updates, but code to actually pass the options along from real git commands that use add--interactive, like add, checkout, reset, and stash.

So...what next? I don't really like leaving the contains traversal as-is. It's often much faster than the original version (because we do at most one to-the-roots traversal), but in some cases is slower (because the depth-first search doesn't cut itself off sanely, so an unlucky traversal means going to the roots instead of ending early).

Here are the options:

  1. Stick with timestamps as a proxy for generation numbers.

     a. Assume they're good enough. Possibly provide options to turn off
        this optimization (here, and possibly in name-rev) if you want
        to be more correct in the face of skew (at the cost of
        efficiency).

     b. Implement some kind of "this commit timestamp is bogus" cache.

     c. Make it easier to use and share replace objects with correct
        commit timestamps.

  2. Add generation numbers to headers, and try to calculate them on the
     fly as much as possible (i.e., Linus's approach).

  3. Add separate storage of generation numbers.

     a. As an on-the-fly cache (i.e., my patches).

     b. Combined with packfiles somehow (more efficient, less flaky, but
        may require a bump in index format version; alternatively, you
        could have pack-*.generation, which is a sparsely populated list
        of 32-bit ints, in the exact same order as the pack-*.idx file.
        No backwards compatibility issues, but slightly less efficient).

Thoughts?

I'm OK with holding this off for another round. I'd really like to get more feedback from third-party helper writers. OTOH, I think we have a little bit of a chicken-and-egg problem with testing. Most experienced git users aren't that interested in this, because they are just using ssh keys and ssh-agent, anyway. I wonder if we would do better to put it in a released version with a big "warning: this is ridiculously experimental and the interface to helpers is subject to change" warning. And that puts something in the hands of the people it was meant to help, and can get the attention of people to write helpers. I dunno.

-Peff

Jeff King <peff@peff.net> writes:

Thanks. Also tests are lacking, too. Although I do not necessarily see the lack of integration with anything but "add" a show-stopper (I consider "-p" to chekout, reset and stash are "nice to have"), you are correct that "add -i" and then choosing '[p]atch' gets very confused with

    Patch update>> 
    Use of uninitialized value in split at /git/git-pu/libexec/git-core/git-add--interactive line 742, <STDIN> line 3.
    Unknown option: --
    usage: git [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
               [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
               [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
               [-c name=value] [--help]
               <command> [<args>]
    Unknown option: --color
    usage: git [--version] [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
               [-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
               [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
               [-c name=value] [--help]
               <command> [<args>]

So it certainly seems not ready for the prime time.

Hmm, honestly speaking, I do not see much problem with it. My knee-jerk reaction is to go with 1.a and if we really want to do something 1.b perhaps but I suspect "these are bogus" cache wouldn't be so useful by itself and we may need a bit more information.

I actually do not think the lack of finer-than-host level granularity a problem we need to solve before moving forward. IIRC, when accessing "http://github.com/frotz" and "http://github.com/nitfol", you would key the authentication material with something like "http://github.com" (or was it "http:github.com"? the details do not matter for the purpose of this discussion). If another site that has multiple independent userbases within the same host, i.e. the user "xyzzy" at "http://gotheb.com/frotz" and the user "xyzzy" at "http://gotheb.com/nitfol" need to be treated as separate identities, it obviously is not enoug to use "gotheb.com" as the look-up key for the authentication material, but in such a case, the user knows an important piece of information that git can never figure out by itself, namely, where the authentication domain boundary lies.

We need to add "auth-domain" support, perhaps from the command line option and configuration, if we ever need to support such a site.

We can consider what you already have as the default case for a more general "we cut off at the hostname and take that as the auth-domain boundary unless told otherwise". We may not have the way to "tell otherwise" yet, but as long as we are reasonably confident that we know how to extend the system in a backward compatible way, it is not a show-stopper.

The primary reason why I wanted to hold this topic off was because of the frequency of bug report we saw this round to topics _after_ they hit the "master" branch, indicating that not many people are testing "next" during the development cycle as they used to in olden days.

On Thu, 25 Aug 2011 15:22:49 -0700 Junio C Hamano <gitster@pobox.com> wrote: 

JCH> We need to add "auth-domain" support, perhaps from the command line option JCH> and configuration, if we ever need to support such a site.

JCH> We can consider what you already have as the default case for a more JCH> general "we cut off at the hostname and take that as the auth-domain JCH> boundary unless told otherwise". We may not have the way to "tell JCH> otherwise" yet, but as long as we are reasonably confident that we know JCH> how to extend the system in a backward compatible way, it is not a JCH> show-stopper.

How about a config variable with regular expressions like

auth-domain.xyz.url = https://(.*@)?github.com/.*

so then accessing a remote with that URL with or without a username would pass "auth-domain=xyz" to the helper? If there's no defined auth-domain then it's not passed to the helper, so it has to just use the host name (if there is an auth-domain the helper gets it PLUS the hostname, of course).

I specify the "url" sub-key so we can add more auth-domain selection criteria or other functionality in the future.

That gives the user a way to do, for instance:

auth-domain.internalcompany.url = https://.*.mycompany.com/.*

I also wanted to suggest that the credential helper should be able to specify a SSL private user key and the passphrase for it for HTTPS connections to servers that require such keys.

JCH> The primary reason why I wanted to hold this topic off was because of the JCH> frequency of bug report we saw this round to topics _after_ they hit the JCH> "master" branch, indicating that not many people are testing "next" during JCH> the development cycle as they used to in olden days.

I'll be sure to test credential helpers next week.

Thank you Ted

On Thu, Aug 25, 2011 at 03:22:49PM -0700, Junio C Hamano wrote:

It is less ready than that. You cannot even use it from "git add" at this point. It is _only_ the perl bits, as I was just providing them to Duy, so he could write the C bits. So the patches as they are are useless. Hence no tests, since you can't even trigger the code without artifically calling add--interactive directly with the new options.

So it probably makes sense to just drop them (or just leave them in pu) for the next cycle until the other half materializes.

Hmm, that is a regression probably caused by my refactoring. Thanks for pointing it out. I'll take a look.

OK. I'll clean up and submit a patch for that, but I'll wait for post-1.7.7.

-Peff

On Thu, Aug 25, 2011 at 03:22:49PM -0700, Junio C Hamano wrote:

It's "http:github.com", which has always looked a bit ugly to me. I had hoped they would just be opaque blobs and nobody would need to look at them. But when you get into things like setting the username via the config, then users see them, and they need to look sane. Making them look more like a canonicalized URL is probably a good thing.

After seeing the helper that Lukas posted recently on the list, I am wondering if they should include the username, too. I had left it separate, because I wanted helpers to be able to index "foo@example.com" and "example.com" in the same slot. I.e., to realize that the latter could use the same credentials cached for the former. But it also complicates the helpers; instead of doing:

  credential = secure_storage_lookup(unique_token);
  return credential /* could be NULL */

they have to do:

  for credential in secure_storage_lookup(unique_token) {
    if (!username)
      return credential; /* take first one arbitrarily */
    else if (username == credential.username)
      return credential; /* ok, matched preferred username */
  }
  return NULL;

which implies that the secure storage can even store a list indexed under the token.

So perhaps a better model is to give the helper some canonicalized URL, like:

  https://foo@example.com

(where the canonicalization is important, because we want the helper to be able to just treat it like a string of bytes if it wants). And then we can naturally extend that to:

  https://foo@example.com/specific-repo.git

if the user wants a repo-specific authentication context.

I think in either case it gets tacked onto the auth token. But it probably makes sense now to choose a nice syntax for the token that will look good when we extend it later.

Ted wrote:

I like this. In fact, perhaps it makes sense for git to generate the maximal token, like:

  https://user@host.example.com/path/to/repo.git

and then provide the user with configuration like this to narrow it down as they see fit. Perhaps even do a substitution regexp to let them rewrite it arbitrarily. And then if we want to be more permissive by default, provide some backup regexps to be used when they don't provide their own, like cutting out the pathname portion.

-Peff

On Tue, Aug 30, 2011 at 10:38 PM, Jeff King <peff@peff.net> wrote: [snip]

A little feedback here: I do look into my keychain on Mac OS X. I tend to keep most of my credentials in a separate keychain that gets whenever my computer sleeps or the screen saver kicks on. So that blob ends up being user-visible to some degree. Could I munge it into something else? Sure. But I do wonder if it might be better to make it something closer to what the user expects to see.

Having the username separate is useful. At least under Mac OS X, the "account name" field is a separate search field. It does make it easier to fallback to just looking at the domain and path without having to parse the unique token. But as it is, we stand a chance at being able to reuse credentials already cached by the browser.

Or pass that the information via --domain and --path parameters. It'd be nice to keep most credential backends from having to parse urls. Not that its hard, just cumbersome. But the keychain implementation and the gnome-keyring implementation could both benefit from having the pieces broken out separately. Likewise, it's probably not difficult to parse it out of the token if we needed to.

[snip good discussion]

One thing that crossed my mind while looking at this: what happens when a command is meant to be non-interactive? Looking at the kdewallet implementation, it appears that not only is the credential helper intended to help do the lookup, but also ask the user for a password, if it doesn't find anything. That doesn't seem like it would play well in a non-interactive environment. Additionally, the act of looking up the entry could pop up a dialog in most keychain-like applications. Is there a need to be sensitive to the fact that we may be run non-interactively?

-John

On Fri, Sep 09, 2011 at 05:55:38AM -0400, John Szakmeister wrote:

Sure, I agree. I guess my question is: what does the user expect to see?

Perhaps it's worth providing the information in two forms: parsed and broken out by individual pieces, and as a more opaque blob. Then systems which care can use the pieces, and systems which are trying to be as simple as possible can use the blob.

That still leaves the question of how the user specifies policy about which parts of the blob are relevant. That is, how do they say that only the "domain" portion of the hostname is relevant? Or that the path is or is not relevant?

I was really hoping for the user to be able to specify this at the git level, to give each storage helper roughly the same feature set.

Maybe it would be enough to do something like:

  1. Assemble all of the parts (protocol, username (if any), hostname,
     path) into a canonicalized URL representing the authentication
     context.

  2. Look for git config matching the context URL, and allow
     transformations (e.g., match and replace the whole thing, or even
     regexp-style substitution).

  3. Break the resulting context URL back into constituent parts.

  4. Give the helper the context URL, and the broken down parts from
     (3). It chooses which to use.

I think this is somewhat outside the boundaries of what git can provide. We don't know whether we are interactive or not; we can only make guesses based on things like whether there is a terminal available. The helper should be able to make an even better guess, because it can ask for system-specific things (e.g., a Linux one might check whether $DISPLAY is set before trying to pop up a dialog). And helpers are free to simply return nothing. Even though most people will only configure a single helper, there is actually a stack, and git will try the next one, and so on until it gets an answer (or if it hits the end without an answer, will complain).

-Peff

On Sat, Sep 10, 2011 at 2:53 AM, Jeff King <peff@peff.net> wrote:

Originally I was going to use SecKeychainFindGenericPassword(), and the token would have been the "serviceName". However, SecKeychainFindInternetPassword() is clearly the better fit, which means busting out the individual bits.

[snip]

That would be useful.  Right now, it looks like I'll be parsing the token.

Ooh... yeah, I'm not sure. :-(

That seems burdensome.  There is a method in HTTP/HTTPS for saying
"use this set of credentials".  You'd do that via the security domain.
 That doesn't necessarily apply to other urls (ssh, for example), but
it would allow a storage backend to cache it for a specific path, but
fallback to looking up the credential using the security domain.

[snip]

Okay, I won't worry about it then.  Thanks Jeff!

-John

On Tue, 13 Sep 2011 04:29:30 -0400 John Szakmeister <john@szakmeister.net> wrote: 

JS> On Sat, Sep 10, 2011 at 2:53 AM, Jeff King <peff@peff.net> wrote:

JS> Originally I was going to use SecKeychainFindGenericPassword(), and JS> the token would have been the "serviceName". However, JS> SecKeychainFindInternetPassword() is clearly the better fit, which JS> means busting out the individual bits.

I would make it work like Safari, period.

JS> [snip]

JS> That would be useful.  Right now, it looks like I'll be parsing the token.

JS> Ooh... yeah, I'm not sure. :-(

I posted a simple proposal for auth domains, which would introduce an extra abstraction layer between the URL and the credential helper. I think that would help.

JS> That seems burdensome. There is a method in HTTP/HTTPS for saying JS> "use this set of credentials". You'd do that via the security domain. JS> That doesn't necessarily apply to other urls (ssh, for example), but JS> it would allow a storage backend to cache it for a specific path, but JS> fallback to looking up the credential using the security domain.

The HTTP security domain is set by the remote, not the Git user, and so is neither customizable nor reliable, unfortunately. Also as you mentioned it won't work for non-HTTP URLs, and it is determined late in the process, perhaps too late for some helpers.

Ted

Hi Junio,

On Wed, Aug 24, 2011 at 05:09:09PM -0700, Junio C Hamano wrote:

Have you seen my fixes to the tests of this here:

http://article.gmane.org/gmane.comp.version-control.git/179883

?

Cheers Heiko

Heiko Voigt <hvoigt@hvoigt.net> writes:

Not really. Please send it as a separate patch, as the new _failure case by itself is worth a comment in the "git log" output.

Thanks.