* Re: Question about commit message wrapping
From: Sidney San Martín @ 2011-12-09 14:10 UTC (permalink / raw)
To: Frans Klaver, git
In-Reply-To: <op.v57na7120aolir@keputer>
On Dec 9, 2011, at 2:05 AM, Frans Klaver wrote:
> On Fri, 09 Dec 2011 02:59:06 +0100, Sidney San Martín <s@sidneysm.com> wrote:
>
>> Hey, I want to ask about the practice of wrapping commit messages to 70-something charaters.
>>
>> The webpage most cited about it, which I otherwise really like, is
>>
>> http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
>>
>> *Nothing else* in my everyday life works this way anymore. Line wrapping gets done on the display end in my email client, my web browser, my ebook reader entirely automatically, and it adapts to the size of the window.
>
> Actually, opera-mail autowraps at 72 characters but sets the text format to flowed. It also wraps the quoted text when you reply. But there's a reasonable chance that you don't use opera in your daily life. On the other hand I would not be surprised if most decent e-mail clients worked that way.
>
Interesting… either way, the end result is that the receiving mail client can wrap the lines to whatever length it (or you, as its operator) desires, which I think we can agree is a good thing, right?
> Nobody's forcing you to use the same practice in your own projects anyway.
>
>
>>
>> That article gives two reasons why commits should be wrapped to 72 columns. First:
>>
>>> git log doesn’t do any special special wrapping of the commit messages. With the default pager of less -S, this means your paragraphs flow far off the edge of the screen, making them difficult to read. On an 80 column terminal, if we subtract 4 columns for the indent on the left and 4 more for symmetry on the right, we’re left with 72 columns.
>>
>> Here, I put a patch at the bottom of this email that wraps commit messages to, right now, 80 columns when they're displayed. (It’s a quick one, probably needs configurability. Also, beware, I don’t program in C too much.)
>
> Hm. Saying "that's how the tool works" is not a good reason in my opinion. There might be tons of other reasons for wrapping at 80 characters. Readability is one that comes to mind for me.
>
That's my basic point. I hope it didn't seem like I was arguing against reading commit messages wrapped to 80 columns, by default. I only wanted to discuss whether it makes more sense to handle it on the display end instead of asking committers to do it in advance.
- My phone shows text most comfortably at about 40 characters per line. I do look at terminals at 80 columns most of the time, but not always, and I sometimes browse projects in GUI tools that use a proportional font in a window may be narrower or wider than that.
- Right now, when I *am* in an 80-col terminal I have to trust everyone else to wrap their commit messages. Not everyone does. I feel like it would be more effective to give git the ability to wrap them automatically when I read them.
>>
>> Second:
>>
>>> git format-patch --stdout converts a series of commits to a series of emails, using the messages for the message body. Good email netiquette dictates we wrap our plain text emails such that there’s room for a few levels of nested reply indicators without overflow in an 80 column terminal. (The current rails.git workflow doesn’t include email, but who knows what the future will bring.)
>>
>> There's been a standard for flowed plain text emails (which don't have to wrap at 80 columns) for well over ten years, RFC-2646 and is widely supported. Besides, code in diffs is often longer than 7x characters, and wrapping, like `git log`, could be done inside git. FWIW, there are a bunch of merge commits with lines longer than 80 characters in the git repo itself.
>
> Yes, that standard allows e-mail clients to display the text more fluidly, even if the source text is word-wrapped. While git uses e-mail format, it isn't an e-mail client. I always interpreted this whole thing as git basically creating plain-text e-mails. You're actually writing the source of the e-mail in your commit message. If you care about actual use in e-mail (like we do here on the list) you might want to add the relevant header to the mails. That said, Apple Mail (the client you used to send your mail) doesn't even use the RFC you quote in the sent message. That mail is going to be a pain in the butt to read in mutt from work ;).
>
Sorry, I'm not sure what you mean by, “If you care about actual use in e-mail (like we do here on the list) you might want to add the relevant header to the mails”.
Interesting, I didn't realize that Mail didn't use it. It does, however, use quoted-printable which, as far as I can tell, has a similar effect on line wrapping. What happens when you view this email in mutt?
>
>>
>> Finally, people read commits these days in many other places than `git log` (and make commits in many other places than a text editor configured to wrap). Most every GUI and already word wraps commit messages just fine. As a result, there are commits in popular repos much longer than the 72-column standard and no one notices. Instead, properly-formatted commit messages end up looking cramped when you see them in anywhere wider than 80 columns.
>
> Cramped? I think it's compact and actually I prefer it over long lines.
>
>> Am I crazy?
>
> Probably not. Don't take my word for it. I'm not a psychiatrist.
>
>
>> If this makes sense to anyone else, I'd be happy to help massage this into something git-worthy, with some help (never worked on Git before).
>>
>> - - -
>>
>> From a93b390d1506652d4ad41d1cbd987ba98a8deca0 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
>> Date: Thu, 8 Dec 2011 20:26:23 -0500
>> Subject: [PATCH] Wrap commit messages on display
>>
>> - Wrap to 80 characters minus the indent
>> - Use a hanging indent for lines which begin with "- "
>> - Do not wrap lines which begin with whitespace
>> ---
>> pretty.c | 10 ++++++++--
>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/pretty.c b/pretty.c
>> index 230fe1c..15804ce 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1243,8 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
>> memset(sb->buf + sb->len, ' ', indent);
>> strbuf_setlen(sb, sb->len + indent);
>> }
>> - strbuf_add(sb, line, linelen);
>> - strbuf_addch(sb, '\n');
>> + if (line[0] == ' ' || line[0] == '\t') {
>> + strbuf_add(sb, line, linelen);
>> + } else {
>> + struct strbuf wrapped = STRBUF_INIT;
>> + strbuf_add(&wrapped, line, linelen);
>> + strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + (line[0] == '-' && line[1] == ' ' ? 2 : 0), 80 - indent);
>
> While on the subject, In my mail view, the new line started with the [1] from line[1], in the quote the line looks entirely different. Now this is code we're talking about, so it makes slightly more sense to have a proper wrapping hard-coded. Compare the above with the following:
>
> + int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0);
> [...]
> + strbuf_add_wrapped_text(sb, wrapped.buf, 0,
> + indent + hanging_indent,
> + 80 - indent);
>
> Much clearer, no? I personally usually have two or three terminals tucked next to each other, so I can look at two or three things at the same time. 80 characters limit is a nice feature then.
Good point, that makes it clearer either way. I put an updated patch at the bottom of this email (also fixed forgetting the newline after lines with leading whitespace). I hope it's OK to include patches this way, I understand that they're supposed to represent whole emails but want to include them with this discussion.
>
>
>> + strbuf_addch(sb, '\n');
>> + }
>> }
>> }
>>
>
> Cheers,
> Frans
From 53fd7deedaf5ac522c9d752e79cf71561cc57f07 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sidney=20San=20Marti=CC=81n?= <s@sidneysm.com>
Date: Thu, 8 Dec 2011 20:26:23 -0500
Subject: [PATCH] Wrap commit messages on display
- Wrap to 80 characters, minus the indent
- Use a hanging indent for lines which begin with "- "
- Do not wrap lines which begin with whitespace
---
pretty.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/pretty.c b/pretty.c
index 230fe1c..841ccd1 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1243,7 +1243,14 @@ void pp_remainder(const struct pretty_print_context *pp,
memset(sb->buf + sb->len, ' ', indent);
strbuf_setlen(sb, sb->len + indent);
}
- strbuf_add(sb, line, linelen);
+ if (line[0] == ' ' || line[0] == '\t') {
+ strbuf_add(sb, line, linelen);
+ } else {
+ struct strbuf wrapped = STRBUF_INIT;
+ strbuf_add(&wrapped, line, linelen);
+ int hanging_indent = ((line[0] == '-' && line[1] == ' ') ? 2 : 0);
+ strbuf_add_wrapped_text(sb, wrapped.buf, 0, indent + hanging_indent, 80 - indent);
+ }
strbuf_addch(sb, '\n');
}
}
--
1.7.8
^ permalink raw reply related
* What's cooking in git.git (Dec 2011, #03; Fri, 9)
From: Junio C Hamano @ 2011-12-10 0:09 UTC (permalink / raw)
To: git
Here are the topics that have been cooking. Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in
'next'.
Two of the more important topics slated for 1.7.9 have been merged to
'master'. Let's hope the other ones also will stabilize soonish, so that
we can smoothly close this cycle early.
Here are the repositories that have my integration branches:
With maint, master, next, pu, todo:
git://git.kernel.org/pub/scm/git/git.git
git://repo.or.cz/alt-git.git
https://code.google.com/p/git-core/
https://github.com/git/git
With only maint and master:
git://git.sourceforge.jp/gitroot/git-core/git.git
git://git-core.git.sourceforge.net/gitroot/git-core/git-core
With all the topics and integration branches:
https://github.com/gitster/git
The preformatted documentation in HTML and man format are found in:
git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
git://repo.or.cz/git-{htmldocs,manpages}.git/
https://code.google.com/p/git-{htmldocs,manpages}.git/
https://github.com/gitster/git-{htmldocs,manpages}.git/
--------------------------------------------------
[Graduated to "master"]
* ab/pull-rebase-config (2011-11-07) 1 commit
(merged to 'next' on 2011-12-05 at 6d235a4)
+ pull: introduce a pull.rebase option to enable --rebase
* jc/pull-signed-tag (2011-11-12) 15 commits
(merged to 'next' on 2011-12-05 at c949bd1)
+ commit-tree: teach -m/-F options to read logs from elsewhere
+ commit-tree: update the command line parsing
+ commit: teach --amend to carry forward extra headers
+ merge: force edit and no-ff mode when merging a tag object
+ commit: copy merged signed tags to headers of merge commit
+ merge: record tag objects without peeling in MERGE_HEAD
+ merge: make usage of commit->util more extensible
+ fmt-merge-msg: Add contents of merged tag in the merge message
+ fmt-merge-msg: package options into a structure
+ fmt-merge-msg: avoid early returns
+ refs DWIMmery: use the same rule for both "git fetch" and others
+ fetch: allow "git fetch $there v1.0" to fetch a tag
+ merge: notice local merging of tags and keep it unwrapped
+ fetch: do not store peeled tag object names in FETCH_HEAD
+ Split GPG interface into its own helper library
(this branch is used by jc/signed-commit.)
Allow pulling/merging a signed tag instead of a branch tip, and record
the GPG signature in the merge commit for later audit.
* jc/request-pull-show-head-4 (2011-11-09) 12 commits
(merged to 'next' on 2011-12-05 at 8f05b45)
+ request-pull: use the annotated tag contents
+ fmt-merge-msg.c: Fix an "dubious one-bit signed bitfield" sparse error
+ environment.c: Fix an sparse "symbol not declared" warning
+ builtin/log.c: Fix an "Using plain integer as NULL pointer" warning
+ fmt-merge-msg: use branch.$name.description
+ request-pull: use the branch description
+ request-pull: state what commit to expect
+ request-pull: modernize style
+ branch: teach --edit-description option
+ format-patch: use branch description in cover letter
+ branch: add read_branch_desc() helper function
+ Merge branch 'bk/ancestry-path' into jc/branch-desc
Allow setting "description" for branches and use it to help communications
between humans in various workflow elements. It also allows requesting for
a signed tag to be pulled and shows the tag message in the generated message.
* nd/resolve-ref (2011-12-05) 2 commits
(merged to 'next' on 2011-12-05 at d55637f)
+ Copy resolve_ref() return value for longer use
+ Convert many resolve_ref() calls to read_ref*() and ref_exists()
* rs/allocate-cache-entry-individually (2011-10-26) 2 commits
(merged to 'next' on 2011-12-05 at 241e711)
+ cache.h: put single NUL at end of struct cache_entry
+ read-cache.c: allocate index entries individually
* sg/complete-refs (2011-10-21) 9 commits
(merged to 'next' on 2011-12-05 at 03e5527)
+ completion: remove broken dead code from __git_heads() and __git_tags()
+ completion: fast initial completion for config 'remote.*.fetch' value
+ completion: improve ls-remote output filtering in __git_refs_remotes()
+ completion: query only refs/heads/ in __git_refs_remotes()
+ completion: support full refs from remote repositories
+ completion: improve ls-remote output filtering in __git_refs()
+ completion: make refs completion consistent for local and remote repos
+ completion: optimize refs completion
+ completion: document __gitcomp()
--------------------------------------------------
[New Topics]
* rr/revert-cherry-pick (2011-12-09) 9 commits
- revert: simplify communicating command-line arguments
- revert: report fine-grained error messages from insn parser
- revert: allow mixed pick and revert instructions
- t3510 (cherry-pick-sequencer): remove malformed sheet 2
- t3510 (cherry-pick-sequencer): use exit status
- revert: simplify getting commit subject in format_todo()
- revert: tolerate extra spaces, tabs in insn sheet
- revert: make commit subjects in insn sheet optional
- revert: free msg in format_todo()
This is not the latest one but the last one posted to the list.
* ew/keepalive (2011-12-05) 1 commit
- enable SO_KEEPALIVE for connected TCP sockets
Comments?
* jc/checkout-m-twoway (2011-12-06) 1 commit
(merged to 'next' on 2011-12-09 at c946009)
+ checkout -m: no need to insist on having all 3 stages
* tr/cache-tree (2011-12-06) 5 commits
- reset: update cache-tree data when appropriate
- commit: write cache-tree data when writing index anyway
- Refactor cache_tree_update idiom from commit
- Test the current state of the cache-tree optimization
- Add test-scrap-cache-tree
Will merge to 'next' after taking another look.
* tr/userdiff-c-returns-pointer (2011-12-06) 1 commit
(merged to 'next' on 2011-12-09 at 0b6a092)
+ userdiff: allow * between cpp funcname words
* jc/commit-amend-no-edit (2011-12-08) 5 commits
(merged to 'next' on 2011-12-09 at b9cfa4e)
+ test: commit --amend should honor --no-edit
+ commit: honour --no-edit
+ t7501 (commit): modernize style
+ test: remove a porcelain test that hard-codes commit names
+ test: add missing "&&" after echo command
* jl/submodule-status-failure-report (2011-12-08) 1 commit
(merged to 'next' on 2011-12-09 at 53eb3b3)
+ diff/status: print submodule path when looking for changes fails
* ks/tag-cleanup (2011-12-09) 1 commit
(merged to 'next' on 2011-12-09 at cbea045)
+ git-tag: introduce --cleanup option
* rr/test-chaining (2011-12-09) 5 commits
- t3040 (subprojects-basic): fix '&&' chaining, modernize style
- t1510 (worktree): fix '&&' chaining
- t3030 (merge-recursive): use test_expect_code
- test: fix '&&' chaining
- t3200 (branch): fix '&&' chaining
I think Martin's patches to t3401 should also be queued here.
--------------------------------------------------
[Cooking]
* bc/maint-apply-check-no-patch (2011-12-05) 2 commits
(merged to 'next' on 2011-12-09 at fc780cd)
+ builtin/apply.c: report error on failure to recognize input
+ t/t4131-apply-fake-ancestor.sh: fix broken test
* aw/rebase-i-stop-on-failure-to-amend (2011-11-30) 1 commit
(merged to 'next' on 2011-12-09 at a117e83)
+ rebase -i: interrupt rebase when "commit --amend" failed during "reword"
* jc/split-blob (2011-12-01) 6 commits
. WIP (streaming chunked)
- chunked-object: fallback checkout codepaths
- bulk-checkin: support chunked-object encoding
- bulk-checkin: allow the same data to be multiply hashed
- new representation types in the packstream
- varint-in-pack: refactor varint encoding/decoding
(this branch uses jc/stream-to-pack.)
Not ready. At least pack-objects and fsck need to learn the new encoding
for the series to be usable locally, and then index-pack/unpack-objects
needs to learn it to be used remotely.
* jh/fast-import-notes (2011-11-28) 3 commits
(merged to 'next' on 2011-12-09 at 2b01132)
+ fast-import: Fix incorrect fanout level when modifying existing notes refs
+ t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling
+ t9301: Fix testcase covering up a bug in fast-import's notes fanout handling
Comments?
* tj/maint-imap-send-remove-unused (2011-11-23) 2 commits
(merged to 'next' on 2011-12-09 at 877cc11)
+ Merge branch 'maint' into tj/imap-send-remove-unused
+ imap-send: Remove unused 'use_namespace' variable
* cn/maint-lf-to-crlf-filter (2011-11-28) 1 commit
(merged to 'next' on 2011-12-09 at c374d14)
+ convert: track state in LF-to-CRLF filter
* jn/branch-move-to-self (2011-11-28) 2 commits
(merged to 'next' on 2011-12-09 at 7d27260)
+ Allow checkout -B <current-branch> to update the current branch
+ branch: allow a no-op "branch -M <current-branch> HEAD"
* jk/credentials (2011-12-09) 20 commits
- credential: use git_prompt instead of git_getpass
- prompt: use git_terminal_prompt
- add generic terminal prompt function
- refactor git_getpass into generic prompt function
- move git_getpass to its own source file
- imap-send: don't check return value of git_getpass
- imap-send: avoid buffer overflow
- t: add test harness for external credential helpers
- credentials: add "store" helper
- strbuf: add strbuf_add*_urlencode
- credentials: add "cache" helper
- docs: end-user documentation for the credential subsystem
- credential: make relevance of http path configurable
- credential: add credential.*.username
- credential: apply helper config
- http: use credential API to get passwords
- credential: add function for parsing url components
- introduce credentials API
- t5550: fix typo
- test-lib: add test_config_global variant
Looking good.
* nd/ignore-might-be-precious (2011-11-28) 2 commits
(merged to 'next' on 2011-12-09 at 1a94553)
+ checkout,merge: disallow overwriting ignored files with --no-overwrite-ignore
+ Merge branch 'nd/maint-ignore-exclude' into nd/ignore-might-be-precious
* jk/upload-archive-use-start-command (2011-11-21) 1 commit
(merged to 'next' on 2011-12-09 at 88cb83a)
+ upload-archive: use start_command instead of fork
* jk/maint-1.6.2-upload-archive (2011-11-21) 1 commit
+ archive: don't let remote clients get unreachable commits
(this branch is used by jk/maint-upload-archive.)
* jk/maint-upload-archive (2011-11-21) 1 commit
(merged to 'next' on 2011-12-09 at 03deb16)
+ Merge branch 'jk/maint-1.6.2-upload-archive' into jk/maint-upload-archive
(this branch uses jk/maint-1.6.2-upload-archive.)
* ab/enable-i18n (2011-12-05) 1 commit
- i18n: add infrastructure for translating Git with gettext
Will merge to 'next'.
* jc/signed-commit (2011-11-29) 5 commits
- gpg-interface: allow use of a custom GPG binary
- pretty: %G[?GS] placeholders
- test "commit -S" and "log --show-signature"
- log: --show-signature
- commit: teach --gpg-sign option
Not exactly urgent.
* jc/stream-to-pack (2011-12-01) 5 commits
(merged to 'next' on 2011-12-09 at d0fd605)
+ bulk-checkin: replace fast-import based implementation
+ csum-file: introduce sha1file_checkpoint
+ finish_tmp_packfile(): a helper function
+ create_tmp_packfile(): a helper function
+ write_pack_header(): a helper function
(this branch is used by jc/split-blob.)
Teaches "git add" to send large-ish blob data straight to a packfile.
This is a continuation to the "large file support" topic. The codepath to
move data from worktree to repository is made aware of streaming, just
like the checkout codepath that goes the other way, which was done in the
previous "large file support" topic in the 1.7.7 cycle.
* jn/gitweb-side-by-side-diff (2011-10-31) 8 commits
- gitweb: Add navigation to select side-by-side diff
- gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"
- t9500: Add basic sanity tests for side-by-side diff in gitweb
- t9500: Add test for handling incomplete lines in diff by gitweb
- gitweb: Give side-by-side diff extra CSS styling
- gitweb: Add a feature to show side-by-side diff
- gitweb: Extract formatting of diff chunk header
- gitweb: Refactor diff body line classification
Replaces a series from Kato Kazuyoshi on the same topic.
Is this ready for 'next'?
--------------------------------------------------
[Discarded]
* hv/submodule-merge-search (2011-10-13) 4 commits
. submodule.c: make two functions static
. allow multiple calls to submodule merge search for the same path
. push: Don't push a repository with unpushed submodules
. push: teach --recurse-submodules the on-demand option
* 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
* sr/fix-fast-export-tips (2011-11-05) 3 commits
. fast-export: output reset command for commandline revs
. fast-export: do not refer to non-existing marks
. t9350: point out that refs are not updated correctly
* jc/lookup-object-hash (2011-08-11) 6 commits
. 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
* jc/verbose-checkout (2011-10-16) 2 commits
. checkout -v: give full status output after switching branches
. checkout: move the local changes report to the end
* eh/grep-scale-to-cpunum (2011-11-05) 1 commit
. grep: detect number of CPUs for thread spawning
* jc/commit-tree-extra (2011-11-12) 2 commits
. commit-tree: teach -C <extra-commit>
. commit-tree: teach -x <extra>
(this branch uses jc/pull-signed-tag; is tangled with jc/signed-commit.)
* cb/daemon-permission-errors (2011-10-17) 2 commits
. daemon: report permission denied error to clients
. daemon: add tests
* ld/p4-labels-branches (2011-11-30) 4 commits
. git-p4: importing labels should cope with missing owner
. git-p4: add test for p4 labels
. git-p4: cope with labels with empty descriptions
. git-p4: handle p4 branches and labels containing shell chars
* mh/ref-api-2 (2011-11-16) 15 commits
. refs: loosen over-strict "format" check
. resolve_gitlink_ref_recursive(): change to work with struct ref_cache
. Pass a (ref_cache *) to the resolve_gitlink_*() helper functions
. resolve_gitlink_ref(): improve docstring
. get_ref_dir(): change signature
. refs: change signatures of get_packed_refs() and get_loose_refs()
. is_dup_ref(): extract function from sort_ref_array()
. add_ref(): add docstring
. parse_ref_line(): add docstring
. is_refname_available(): remove the "quiet" argument
. clear_ref_array(): rename from free_ref_array()
. refs: rename parameters result -> sha1
. refs: rename "refname" variables
. struct ref_entry: document name member
. cache.h: add comments for git_path() and git_path_submodule()
(this branch is tangled with mh/ref-api-3.)
* mh/ref-api-3 (2011-11-16) 26 commits
. refs: loosen over-strict "format" check
. is_refname_available(): reimplement using do_for_each_ref_in_array()
. names_conflict(): simplify implementation
. names_conflict(): new function, extracted from is_refname_available()
. repack_without_ref(): reimplement using do_for_each_ref_in_array()
. do_for_each_ref_in_array(): new function
. do_for_each_ref(): correctly terminate while processesing extra_refs
. add_ref(): take a (struct ref_entry *) parameter
. create_ref_entry(): extract function from add_ref()
. parse_ref_line(): add a check that the refname is properly formatted
. repack_without_ref(): remove temporary
. Rename another local variable name -> refname
. resolve_gitlink_ref_recursive(): change to work with struct ref_cache
. Pass a (ref_cache *) to the resolve_gitlink_*() helper functions
. resolve_gitlink_ref(): improve docstring
. get_ref_dir(): change signature
. refs: change signatures of get_packed_refs() and get_loose_refs()
. is_dup_ref(): extract function from sort_ref_array()
. add_ref(): add docstring
. parse_ref_line(): add docstring
. is_refname_available(): remove the "quiet" argument
. clear_ref_array(): rename from free_ref_array()
. refs: rename parameters result -> sha1
. refs: rename "refname" variables
. struct ref_entry: document name member
. cache.h: add comments for git_path() and git_path_submodule()
(this branch is tangled with mh/ref-api-2.)
^ permalink raw reply
* Bug in filter-branch example for moving into a subdirectory
From: Jesse Keating @ 2011-12-10 0:07 UTC (permalink / raw)
To: git
I ran into a bug with the example in the man page for filter-branch, for moving everything into a subdir:
git filter-branch --index-filter \
'git ls-files -s | sed "s-\t\"*-&newsubdir/-" |
GIT_INDEX_FILE=$GIT_INDEX_FILE.new \
git update-index --index-info &&
mv "$GIT_INDEX_FILE.new" "$GIT_INDEX_FILE"' HEAD
The problem seems to be when a commit happens that is just a file removal with no added file. In those cases, git ls-files doesn't output anything. Trying to throw a -d option in doesn't seem to improve matters.
So the problem is that if a commit is just file removals, this example will actually crash. Unfortunately I don't have a suggested solution at the moment.
--
Jesse Keating
Fedora -- Freedom² is a feature!
identi.ca: http://identi.ca/jkeating
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Junio C Hamano @ 2011-12-10 0:02 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8AZg3DgZzmPyXhCH9bGBqo9UN7-zLt_feTtpyajf5U1tw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> That's interesting. First of all xmalloc() is controlled by us while
> index-pack code might lead to stack overflow exploit (never done it,
> not sure if it's really pratical to do in this case).
What do you exactly mean by "stack overflow exploit"?
If your callee has prepares a stackframe that is not sufficiently big but
carelessly tries to store more than it has space for, such a write can
overflow the stack (without hardware traps) and overwrite return address,
and instead of coming back to you, the control can be transferred to
random places.
But I do not think that is what we are talking about here.
You attempt to write parameters and return address to the area of memory
pointed by your stack pointer, advance the stack pointer to create a stack
frame and the callee attempts to write to its local variables in the newly
allocated stack frame. These memory accesses eventually attempt to touch
memory beyond the address range the kernel gave you page table entries to
be used as your stack space, and hardware traps. If you haven't run out of
the stack, a new page is lazily added to the page table and your attempted
access will succeed. If you are recursing too deeply, you won't be given a
new page and you will be killed by the kernel. That is a rather controlled
death of the process, unlike smashing the contents of the stack to jump to
a randomly chosen place, isn't it?
Of course, some platforms do not have an unwritable gap between the stack
segment that grow downwards and the heap that grow upwards, and also your
stackframe could be larger than such a gap (in this particular callchain I
do not think that is the case), so the above discussion does not apply
universally, though.
^ permalink raw reply
* Re: [PATCH 4/7] refactor git_getpass into generic prompt function
From: Junio C Hamano @ 2011-12-09 23:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, Erik Faye-Lund
In-Reply-To: <20111208083133.GD26409@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 2. The first series had a special "name" parameter just for generating
> error messages. This drops it in the name of simplicity, so error
> messages have gone from (assuming you don't have a tty):
>
> Could not read password: No such device or address
>
> to:
>
> Could not read 'Username for 'https://example.com': ': No such
> device or address
>
> which is verbose, yes, but contains a little more useful
> information. The formatting is rather unfortunate,...
It also would be unpleasant to i18n it, I suspect.
> + r = getpass(prompt);
> + if (!r)
> + die_errno("could not read '%s'", prompt);
Taking advantage of the "prompt-string"-ness of the message, this might be
a cuter workaround:
fatal: Password: <<could not be read>>
But I do not think it matters that much. Let's queue what you have, and
work out these details in-tree.
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Phil Hord @ 2011-12-09 23:57 UTC (permalink / raw)
To: andreas.t.auer_gtml_37453; +Cc: Jens Lehmann, git
In-Reply-To: <4EE07FCD.8090702@ursus.ath.cx>
On Thu, Dec 8, 2011 at 4:13 AM, <andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
>
> On 07.12.2011 23:23 Jens Lehmann wrote:
>> > If you have tracking branches, the supermodule can just update the
>> > corresponding branch. If this branch is currently checkedout and
>> > the work area is clean, then the work area is updated, too. If
>> > there is currently a local branch or a diffent super-branch
>> > checked out then the working area should be considered "detached"
>> > from the superproject and not updated.
>>
>> This sounds a lot like the "follow branch tip" model we discussed
>> recently (which could be configured via .gitmodules), but I'm not
>> sure you really are in the same boat here.
>
> When I understood that correctly it was just a configuration to what branch
> should be automatically checked out in the submodule. This seems to be too
> complicated IMO, because when you have different branches in the
> superproject then you may want to have different branches in the submodules,
> too, but you would need to configure that submodule branch in .gitmodules
> for each branch separately. I.e. in the master branch the .gitmodule may
> contain "master", in the maint branch the .gitmodules may have "maint" as
> the branch to follow.
Yes, but maybe you can update this information in the .gitmodules file
easily with a command. Maybe it could be something simpler than "git
sync-gitmodules-branches", but that is essentially what it would do:
it would save the current branch in each submodule as the "tracked"
branch in the .gitmodules file.
The advantages to this, I think, are that
1. Your "submodule A follows branch X" information is explicit in the
.gitmodules file and so it is not hidden when I examine your patch.
(It sounds to me like the refs/super/* branches would necessarily be
hard to find since the refs/ hierarchy is usually meta data about
local and remote branches. Maybe I should think about tags and notes
more, though.)
2. When you change to "submodule A now follows branch Y", this
information can be unambiguously saved in the commit where it occurred
rather than tucked away, again, in refs/super/*.
The disadvantage, maybe, are that you must now use 'git submodule
sync' or something like that to put any .gitmodules changes into
effect.
Or maybe that is an advantage. How often will this branch tracking change?
I like where you are going, though. But I have trouble following your
meaning when you toss around words like "ref" and "HEAD" and "branch"
and "super-branch". Maybe we can set up a strawman repo (virtually or
on github) where you can explain what happens now and what you wish
would happen instead.
For example, I have some repos like this:
super
+--subA
+--subB
I wish I could do this:
cd super && checkout master
to get this:
super (master)
+--subA (master)
+--subB (master)
Or, if I have SubB on super/'master' tracking 'foo', I could get this:
super (master)
+--subA (master)
+--subB (foo)
And I wish these commands would work as if this was all one repository:
cd super && git diff master maint
cd super && git grep foo
cd subA && git grep foo -- :/
cd super && git status
cd subA && git status
But I wonder what this would do:
cd super && git remote -v &&
cd subA && git remote -v
> I do want to follow the tip of the branch, if the superproject has that
> currently checked out. If the superproject checks out a tagged version for a
> rebuild, then the submodule should not follow the tip, but should get a
> detached HEAD of the corresponding commit, just as the superproject. When
> the superproject goes back to the branch, the submodule should go back to
> its tracking branch.
Now this makes sense. I want the same thing. I want to preserve
history on "old" commits, but I want to "advance to tip" on "new"
commits.
The trouble, I think, is in telling the difference between "old" and
"new". I think it means there is a switch, like --auto-follow (or
--no-auto-follow for the alternate if core.auto_follow is set). But
having a config option as the default is likely to break lots of
scripts.
So maybe I need a new command that does this:
git checkout master && git submodule foreach 'git checkout master'
Maybe it's called "git checkout master --recurse-submodules". But I
seem to recall this is already a non-starter for some reason, and
anyway it doesn't solve the "variant branches in some submodules"
problem.
Which brings us back to .gitmodules.
>> > With this concept you could even switch branches in the
>> > superproject and the attached submodules follow - still having no
>> > detached HEAD. When you want to do some local work on the
>> > submodule you checkout a local branch and merge back into the super
>> > branch later.
>>
>> You lost me here. How can you merge a submodule branch into one of
>> the superproject?
>
> It wouldn't work, if the super/* branch would contain a superproject's
> SHA-1, that is right. But as explained above, it points to a commit of the
> submodule.
>>
>>
>> But we would want to have a deterministic update procedure, no? (And
>> what has more freedom than a detached HEAD? ;-)__
>
> I think my proposal would be deterministic.
> And everything where you can commit to has more freedom than a detached HEAD
You can commit to a detached HEAD. I do it all the time. The trick
is in not switching away from a detached HEAD with your local commits
still on it. :-)
>> > Even though it will raise a lot of detailed questions like "should
>> > the refs/super/* be pushed/pulled when syncing the submodule
>> > repositories".
>>
>> I doubt that is a good idea, as that might conflict with the same
>> submodule sitting in a different superproject. But I'm interested to
>> hear how you want to solve that.
>
> The first answer to my question was "yes, you need to transfer the refs or
> you get unreferenced objects" and "no, you can't transfer the refs, because
> they are owned by the superproject, not the submodule."
> But binding a submodule to a superproject makes perfect sense if it is _one_
> project that is split into submodules. In that case you only have one
> superproject for a submodule and for that purpose it would be good workflow.
This is not useful to me, though. Sorry.
> It is even nice to see which commits in the submodule belong to what
> branches in the superproject or to what release version (so tracking
> superproject tags would make sense, too). If you have a submodule that has
> more than one superproject but these are well-defined, it could be solved
> using refspecs (e.g. refs/super/foo/* for one and refs/super/bar/* for the
> other superproject), but currently I can't think of a context where this
> makes sense.
I can, but this does put the cart before the horse. The submodule is
subservient to the super project in all my setups. The submodule is
not aware who owns him. He is a bit like the DAG in reverse. He
knows one direction only (children), not the other (parents).
Phil
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-09 23:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111209233957.GC10560@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Speaking of which, I hackishly ported Jay's osxkeychain helper to the
> new format last night. I'll try to clean that up and post it tonight.
;-).
^ permalink raw reply
* [PATCH 4/4] git-p4: test for absolute PWD problem
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9808-chdir.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 49 insertions(+), 0 deletions(-)
create mode 100755 t/t9808-chdir.sh
diff --git a/t/t9808-chdir.sh b/t/t9808-chdir.sh
new file mode 100755
index 0000000..e6fd681
--- /dev/null
+++ b/t/t9808-chdir.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='git-p4 relative chdir'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "change 1"
+ )
+'
+
+# P4 reads from P4CONFIG file to find its server params, if the
+# environment variable is set
+test_expect_success 'P4CONFIG and absolute dir clone' '
+ printf "P4PORT=$P4PORT\nP4CLIENT=$P4CLIENT\n" >p4config &&
+ test_when_finished "rm \"$TRASH_DIRECTORY/p4config\"" &&
+ test_when_finished cleanup_git &&
+ (
+ P4CONFIG=p4config && export P4CONFIG &&
+ unset P4PORT P4CLIENT &&
+ "$GITP4" clone --verbose --dest="$git" //depot
+ )
+'
+
+# same thing, but with relative directory name, note missing $ on --dest
+test_expect_success 'P4CONFIG and relative dir clone' '
+ printf "P4PORT=$P4PORT\nP4CLIENT=$P4CLIENT\n" >p4config &&
+ test_when_finished "rm \"$TRASH_DIRECTORY/p4config\"" &&
+ test_when_finished cleanup_git &&
+ (
+ P4CONFIG=p4config && export P4CONFIG &&
+ unset P4PORT P4CLIENT &&
+ "$GITP4" clone --verbose --dest="git" //depot
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 3/4] git-p4: use absolute directory for PWD env var
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
From: Gary Gibbons <ggibbons@perforce.com>
P4 only looks at the environment variable $PWD to figure out
where it is, so chdir() has code to set that every time. But
when the clone --destination is not an absolute path, PWD will
not be absolute and P4 won't be able to find any files expected
to be in the current directory. Fix this by expanding PWD to
an absolute path.
One place this crops up is when using a P4CONFIG environment
variable to specify P4 parameters, such as P4USER or P4PORT.
Setting P4CONFIG=.p4config works for p4 invocations from the
current directory. But if the value of PWD is not absolute, it
fails.
[ update description --pw ]
Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
contrib/fast-import/git-p4 | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 99d3abe..0083f86 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -53,9 +53,10 @@ def p4_build_cmd(cmd):
def chdir(dir):
# P4 uses the PWD environment variable rather than getcwd(). Since we're
- # not using the shell, we have to set it ourselves.
- os.environ['PWD']=dir
+ # not using the shell, we have to set it ourselves. This path could
+ # be relative, so go there first, then figure out where we ended up.
os.chdir(dir)
+ os.environ['PWD'] = os.getcwd()
def die(msg):
if verbose:
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 2/4] git-p4: submit test for auto-creating clientPath
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9807-submit.sh | 38 ++++++++++++++++++++++++++++++++++++++
1 files changed, 38 insertions(+), 0 deletions(-)
create mode 100755 t/t9807-submit.sh
diff --git a/t/t9807-submit.sh b/t/t9807-submit.sh
new file mode 100755
index 0000000..85ab0d0
--- /dev/null
+++ b/t/t9807-submit.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='git-p4 submit'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "change 1"
+ )
+'
+
+test_expect_success 'submit with no client dir' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ echo file2 >file2 &&
+ git add file2 &&
+ git commit -m "git commit 2" &&
+ rm -rf "$cli" &&
+ git config git-p4.skipSubmitEdit true &&
+ "$GITP4" submit
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 1/4] git-p4: ensure submit clientPath exists before chdir
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
From: Gary Gibbons <ggibbons@perforce.com>
Submitting patches back to p4 requires a p4 "client". This
is a mapping from server depot paths into a local directory.
The directory need not exist or be populated with files; only
the mapping on the server is required. When there is no
directory, make git-p4 automatically create it.
[ reword description --pw ]
Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
contrib/fast-import/git-p4 | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index b975d67..99d3abe 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1099,6 +1099,10 @@ class P4Submit(Command, P4UserMap):
print "Perforce checkout for depot path %s located at %s" % (self.depotPath, self.clientPath)
self.oldWorkingDirectory = os.getcwd()
+ # ensure the clientPath exists
+ if not os.path.exists(self.clientPath):
+ os.makedirs(self.clientPath)
+
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
p4_sync("...")
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 0/4] git-p4: paths for p4
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
There are two problems associated with how we set up paths for
use by p4, fixed in this series. Each fix is accompanied by
another patch that is the unit test.
There is a break in the sequence in the t98* patches here, but
that is on purpose to avoid collision with new tests from other
in-flight patches.
1-2:
in submit, create clientPath if it does not exist
3-4:
in clone, make sure p4 sees an absolute path
Gary Gibbons (2):
git-p4: ensure submit clientPath exists before chdir
git-p4: use absolute directory for PWD env var
Pete Wyckoff (2):
git-p4: submit test for auto-creating clientPath
git-p4: test for absolute PWD problem
contrib/fast-import/git-p4 | 9 ++++++-
t/t9807-submit.sh | 38 ++++++++++++++++++++++++++++++++++
t/t9808-chdir.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 94 insertions(+), 2 deletions(-)
create mode 100755 t/t9807-submit.sh
create mode 100755 t/t9808-chdir.sh
--
1.7.8.rc4.42.g8317d
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehwdcob3.fsf@alter.siamese.dyndns.org>
On Fri, Dec 09, 2011 at 03:34:08PM -0800, Junio C Hamano wrote:
> > We _could_ modify credential_match() to automatically reject such a
> > pattern at that level,...
>
> I do not think that is such a good idea to modify "match()" function
> either, as I agree match with empty has its uses, but that does not stop
> "rewrite_credential_file()" from being safe by default, no? After all, the
> one that makes the decision to drop things that match the pattern is that
> function (it chooses to give NULL to match_cb).
Yeah, you could move it down to that level, but there isn't much point.
rewrite_credential_file is unique to credential-store, and the only two
callers are store_credential (which has its own, stricter rules already)
and remove_credential, which we are modifying here.
Note that I didn't bother with the same safety valve for
credential-cache. It is, after all, a cache that will go away eventually
anyway, so safety is less interesting.
Third-party helpers will have to do their own checks anyway, as in
general I don't plan on them linking directly against git code.
Speaking of which, I hackishly ported Jay's osxkeychain helper to the
new format last night. I'll try to clean that up and post it tonight.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-09 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111209231800.GA14376@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I think that's a reasonable compromise. Instead of the patch I
> posted earlier, how about this:
>
> diff --git a/credential-store.c b/credential-store.c
> index a2c2cd0..26f7589 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
>
> static void remove_credential(const char *fn, struct credential *c)
> {
> - rewrite_credential_file(fn, c, NULL);
> + /*
> + * Sanity check that we actually have something to match
> + * against. The input we get is a restrictive pattern,
> + * so technically a blank credential means "erase everything".
> + * But it is too easy to accidentally send this, since it is equivalent
> + * to empty input. So explicitly disallow it, and require that the
> + * pattern have some actual content to match.
> + */
> + if (c->protocol || c->host || c->path || c->username)
> + rewrite_credential_file(fn, c, NULL);
> }
Looks very sane.
> We _could_ modify credential_match() to automatically reject such a
> pattern at that level,...
I do not think that is such a good idea to modify "match()" function
either, as I agree match with empty has its uses, but that does not stop
"rewrite_credential_file()" from being safe by default, no? After all, the
one that makes the decision to drop things that match the pattern is that
function (it chooses to give NULL to match_cb).
> So the "empty pattern" does actually have a use, from the end-users's
> point of view. It's just that with removal, it's a little more dangerous
> and a little less likely to be useful (as compared to lookup).
>
> -Peff
^ permalink raw reply
* Re: [PATCHv2 12/13] credentials: add "store" helper
From: Jeff King @ 2011-12-09 23:19 UTC (permalink / raw)
To: git
In-Reply-To: <20111206062305.GL29233@sigill.intra.peff.net>
On Tue, Dec 06, 2011 at 01:23:05AM -0500, Jeff King wrote:
> +int main(int argc, const char **argv)
> +{
> + const char * const usage[] = {
> + "git credential-store [options] <action>",
> + NULL
> + };
> + const char *op;
> + struct credential c = CREDENTIAL_INIT;
> + char *file = NULL;
> + struct option options[] = {
> + OPT_STRING_LIST(0, "file", &file, "path",
> + "fetch and store credentials in <path>"),
> + OPT_END()
> + };
Eek, this should be OPT_STRING, of course. I'll fix it in my next
re-roll, but I wanted to mention it in case anybody is reviewing v2.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkf1fwvn.fsf@alter.siamese.dyndns.org>
On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > It works, and it detects truncated output both ways properly (I know
> > because I had to update every test, since the old output was missing the
> > end-of-credential marker).
> >
> > It makes me a little sad, because the original format (relying on EOF)
> > was so Unix-y.
>
> It saddens me, too. A reasonable middle ground would be to stop treating
> an empty input as "no restriction" but "never matches".
>
> I suspect that it is far more likely for a helper to fail (due to
> configuration errors, for example) before it produces any output than
> after it gives some but not all output lines.
Yeah, I think that's a reasonable compromise. Instead of the patch I
posted earlier, how about this:
diff --git a/credential-store.c b/credential-store.c
index a2c2cd0..26f7589 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
static void remove_credential(const char *fn, struct credential *c)
{
- rewrite_credential_file(fn, c, NULL);
+ /*
+ * Sanity check that we actually have something to match
+ * against. The input we get is a restrictive pattern,
+ * so technically a blank credential means "erase everything".
+ * But it is too easy to accidentally send this, since it is equivalent
+ * to empty input. So explicitly disallow it, and require that the
+ * pattern have some actual content to match.
+ */
+ if (c->protocol || c->host || c->path || c->username)
+ rewrite_credential_file(fn, c, NULL);
}
static int lookup_credential(const char *fn, struct credential *c)
We _could_ modify credential_match() to automatically reject such a
pattern at that level, but it does actually have a use on the lookup
side. In config, a context like "https://example.com/foo.git" would
match each of:
[credential "https://example.com/foo.git"]
helper = ...
[credential "https://example.com"]
helper = ...
[credential "https://"]
helper = ...
[credential]
helper = ...
The final one is an just an extension of the others to the empty pattern
(you could also spell it [credential ""], and it would have the same
effect).
So the "empty pattern" does actually have a use, from the end-users's
point of view. It's just that with removal, it's a little more dangerous
and a little less likely to be useful (as compared to lookup).
-Peff
^ permalink raw reply related
* Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2
From: Ramkumar Ramachandra @ 2011-12-09 23:03 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1323445326-24637-1-git-send-email-artagnon@gmail.com>
Hi,
I'm off on a short vacation next week. I've already fixed up the
series in response to most (if not all) reviews. If anyone's
interested in putting in some finishing touches, it's available here:
https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 195e68e
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 22:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111209195042.GG20913@elie.hsd1.il.comcast.net>
Hey,
Jonathan Nieder wrote:
> You can make git misbehave before applying the commit.
Right, it's all in the discussion surrounding $gmane/184031. I
couldn't recall because it was so long ago :\
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-09 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vaa71hd5l.fsf@alter.siamese.dyndns.org>
On Fri, 09 Dec 2011 18:23:50 +0100, Junio C Hamano <gitster@pobox.com>
wrote:
> "Frans Klaver" <fransklaver@gmail.com> writes:
>
>>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want
>>> without
>>> this much trouble?
>>
>> I just had a good look through the man page of access(2), and I think
>> it depends. access works for the real uid, which is what I attempted
>> to implement in the above check as well. However, do we actually need
>> to use the real uid or do we need the set uid (geteuid(2))?
>
> Does it matter? We do not use seteuid or setegid ourselves and we do not
> expect to be installed as owned by root with u+s bit set.
That's what I thought, but needed to know for sure that this was the case.
> access(2) checks with real uid exactly because it would not make a
> difference to normal user level programs _and_ it makes it easier for a
> suid programs to check with the real identity, and our use case falls
> into the former, no?
>
Certainly looks like. Thanks. I'll reroll somewhere next week.
Frans
^ permalink raw reply
* Re: [PATCH 1/2] index_pack: indent find_unresolved_detals one level
From: Junio C Hamano @ 2011-12-09 21:27 UTC (permalink / raw)
To: pclouds; +Cc: git, Shawn Pearce
In-Reply-To: <4ee0be67.05c1e70a.1956.ffff800b@mx.google.com>
pclouds@gmail.com writes:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> The next patch puts most of the code in one level deeper. By indenting
> separately, it'd be easier to see the actual changes.
Yuck.
Isn't it a sign that "the next patch" should perhaps be helped by a small
helper function that does whatever the part you are indenting here?
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-09 21:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0m2veE8FmFVTPEqNAmbtvm1sWVHtFt0QOWU=huQFafeBw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> a
> positive exit status can be interpreted as a conflict, but this is
> clearly not the case here. How do we fix this problem? By creating
> an API for "git commit", not by shelling out like this and letting it
> take over the exit status.
That might be a nice thing to do anyway, but I don't see how it would
solve anything. The new "git commit" API would presumably return an
integer or enum value to indicate the result of trying to commit.
Tests in the testsuite for the "git commit" API would use the "git
commit" command, which would expose the newly fine-grained values
somehow. And other people scripting but wanting the porcelain to take
care of basic UI would benefit, too. Right?
Actually, I think cherry-pick returning a positive exit status for
"nothing left to commit after resolving conflicts" would be sensible.
It is "I did what you asked but need your help to determine the final
content of the commit or decide to skip it", rather than "you asked
for something unsensible and I am bailing out".
^ permalink raw reply
* Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status
From: Jonathan Nieder @ 2011-12-09 21:00 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=a=-4N4aZHuu=5zkNtwmfLsog5WkBVbuJAbYpvaLUsAg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> - test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>> + test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>> test_path_is_dir .git/sequencer &&
>>
>> Encountered conflicts, preserving options, but the exit is with status
>> 128? Smells like a bug.
>
> No bug.
Ok. I'm fuzzy on the details, but is it possible to make this change
in such a way as to make that obvious? For example, perhaps this
should be split into several tests: one to check that such mistaken
use of "-m 1" with non-merge commits correctly interrupts the
cherry-pick and pleads to the user for advice (should it?), another to
check that doing so produces an exit status of 128 (if it should), and
another to make sure that doing so, fixing things up somehow, and
resuming the sequence allows the effect of "-m 1" to carry over to
later commits.
^ permalink raw reply
* Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()
From: Ramkumar Ramachandra @ 2011-12-09 20:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209194727.GF20913@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
> Can cur->item->buffer be NULL?
As Junio correctly points out in $gmane/186365, no. To quote him:
parse_insn_line() uses lookup_commit_reference() which
calls parse_object() on the object name (and if it is a tag, deref_tag()
will parse the tagged object until we see something that is not a tag), so
we know cur->item is parsed before we see it
>> Also, remove the unnecessary check on cur->item: the previous patch
>> makes sure that instruction sheets with missing commit subjects are
>> parsable.
>
> But now I am confused
> [...]
This part of the commit message is simply awful. Replaced with:
Also remove the unnecessary check on cur->item->buffer:
the lookup_commit_reference() call in parse_insn_line() has already
made sure of this.
-- Ram
^ permalink raw reply
* Re: [PATCH 8/9] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2011-12-09 20:47 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-9-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -719,8 +719,10 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
> return 0;
> }
>
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> + struct replay_insn_list *item, int lineno)
> {
> + const char *todo_file = git_path(SEQ_TODO_FILE);
> unsigned char commit_sha1[20];
I know that this function does not call git_path() again before the
value is used, so this is safe today, but I do not trust people in the
future to preserve that property (for example, maybe someone will want
to call get_sha1() earlier). Why not wait to call git_path() when it
is time to use the value it returns?
^ permalink raw reply
* Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Jonathan Nieder @ 2011-12-09 20:37 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0mEP5nDgdosOiquQ_FWbNRZesi38NeCD_yGPvJ8JQxkGg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> I've noticed that
> the diffing algorithm performs especially badly for t/*.sh -- rebasing
> tests is generally a huge pain.
No clue about this particular situation, but I suspect the general
cause for such rebasing trouble is adding tests at the end of the file
(or some other contended place). Better to figure out a logical place
for each test and put it there from the start.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox