Re: [PATCH 1/5] refs: Introduce pseudoref and per-worktree ref concepts
On Mon, Jul 27, 2015 at 4:08 PM, David Turner wrote:
> refs: Introduce pseudoref and per-worktree ref concepts
>
> Add glossary entries for both concepts.
Based upon the above, I thought this was going to be a
documentation-only patch and was mildly surprised to find that it also
changed code. Perhaps:
Describe these concepts in the glossary and introduce
is_per_worktree_ref() to distinguish such files.
or something. Of course the "and" in there suggests that this might be
better off split into two patches...
More below.
> Pseudorefs and per-worktree refs do not yet have special handling,
> because the files refs backend already handles them correctly. Later,
> we will make the LMDB backend call out to the files backend to handle
> per-worktree refs.
>
> Signed-off-by: David Turner
> ---
> diff --git a/Documentation/glossary-content.txt
> b/Documentation/glossary-content.txt
> index ab18f4b..67952f3 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -411,6 +411,27 @@ exclude;;
> core Git. Porcelains expose more of a <>
> interface than the <>.
>
> +[[def_per_worktree_ref]]per-worktree ref::
> + Refs that are per-<>, rather than
> + global. This is presently only <>, but might
> + later include other unusual refs.
> +
> +[[def_pseudoref]]pseudoref::
> + Pseudorefs are a class of files under `$GIT_DIR` which behave
> + like refs for the purposes of rev-parse, but which are treated
> + specially by git. Psuedorefs both have names are all-caps,
s/names/& that/
> + and always start with a line consisting of a
> + <> followed by whitespace. So, HEAD is not a
> + pseudoref, because it is sometimes a symbolic ref. They might
> + optionally some additional data. `MERGE_HEAD` and
s/optionally/& contain/
> + `CHERRY_PICK_HEAD` are examples. Unlike
> + <>, these files cannot
> + be symbolic refs, and never have reflogs. They also cannot be
> + updated through the normal ref update machinery. Instead,
> + they are updated by directly writing to the files. However,
> + they can be read as if they were refs, so `git rev-parse
> + MERGE_HEAD` will work.
> +
> [[def_pull]]pull::
> Pulling a <> means to <> it and
> <> it. See also linkgit:git-pull[1].
> diff --git a/refs.c b/refs.c
> index 0b96ece..0d10b7b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2848,6 +2848,29 @@ static int delete_ref_loose(struct ref_lock *lock, int
> flag, struct strbuf *err)
> return 0;
> }
>
> +int is_per_worktree_ref(const char *refname)
> +{
> + return !strcmp(refname, "HEAD");
> +}
> +
> +static int is_pseudoref(const char *refname)
> +{
> + const char *c;
> +
> + if (strchr(refname, '/'))
> + return 0;
> +
> + if (is_per_worktree_ref(refname))
> + return 0;
> +
> + for (c = refname; *c; ++c) {
> + if (!isupper(*c) && *c != '-' && *c != '_')
> + return 0;
> + }
> +
> + return 1;
> +}
This static function doesn't seem to have any callers, thus seems out
of place in this patch.
> +
> int delete_ref(const char *refname, const unsigned char *old_sha1,
>unsigned int flags)
> {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] Documentation/config: mention "now" and "never" for 'expire' settings
[+cc:Paul Tan]
On Sun, Jul 26, 2015 at 9:41 PM, Michael Haggerty wrote:
> On 07/23/2015 09:00 PM, Eric Sunshine wrote:
>> In addition to approxidate-style values ("2.months.ago", "yesterday"),
>> consumers of 'gc.*expire*' configuration variables also accept and
>> respect 'now'/'all' ("do it immediately") and 'never'/'false' ("suppress
>> entirely").
>>
>> Suggested-by: Michael Haggerty
>> Signed-off-by: Eric Sunshine
>> ---
>> gc.pruneExpire::
>> When 'git gc' is run, it will call 'prune --expire 2.weeks.ago'.
>> Override the grace period with this config variable. The value
>> - "now" may be used to disable this grace period and always prune
>> - unreachable objects immediately.
>> + "now" may be used to disable this grace period and always prune
>> + unreachable objects immediately; or "never" to suppress pruning.
>
> A semicolon should be used without a conjunction, and the parts of a
> sentence joined by a semicolon should be independent clauses. So this
> should probably be
I was just getting ready to re-roll this series[1] to address
Michael's comments[2] and noticed that the add-on patch 7/6 which I
sent later[3] seems to have been botched when Junio applied it to
'pu'. It's currently at 36598db (Documentation/git-tools: drop
references to defunct tools, 2015-07-24) in
es/doc-clean-outdated-tools and it appears that the --scissors option
didn't cut off the leading cruft from the email conversation, thus the
commit has the wrong "subject" plus a bunch of email conversation gunk
in the commit message which doesn't belong. I understand that Junio
uses a relatively bleeding-edge version of Git for his day-to-day work
and was wondering if this is possible fallout from the git-am rewrite
in C?
[1]: http://thread.gmane.org/gmane.comp.version-control.git/274537
[2]: http://article.gmane.org/gmane.comp.version-control.git/274647
[3]: http://article.gmane.org/gmane.comp.version-control.git/274602
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/6] Documentation/git-tools: improve discoverability of Git wiki
These days, the best way to find Git-related tools is via a search engine. The Git wiki may be a distant second, and git-tools.txt falls in last place. Therefore, promote the Git wiki reference to the top of git-tools.txt so the reader will encounter it first, rather than hiding it away at the very bottom. Signed-off-by: Eric Sunshine --- Changes since v1: reword commit message slightly Documentation/git-tools.txt | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/Documentation/git-tools.txt b/Documentation/git-tools.txt index 78a0d95..129b8c0 100644 --- a/Documentation/git-tools.txt +++ b/Documentation/git-tools.txt @@ -6,10 +6,11 @@ Introduction Apart from Git contrib/ area there are some others third-party tools -you may want to look. - +you may want to look at. This document presents a brief summary of each tool and the corresponding link. +For a more comprehensive list, see: +http://git.or.cz/gitwiki/InterfacesFrontendsAndTools Alternative/Augmentative Porcelains @@ -112,7 +113,3 @@ Others This is an Emacs interface for Git. The user interface is modelled on pcl-cvs. It has been developed on Emacs 21 and will probably need some tweaking to work on XEmacs. - - -http://git.or.cz/gitwiki/InterfacesFrontendsAndTools has more -comprehensive list. -- 2.5.0.rc3.490.g8c70279 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/6] Documentation/git: drop outdated Cogito reference
Cogito hasn't been maintained since late 2006, so drop the reference to it. The warning that SCMS front-ends might override listed environment variables, however, may still be valuable, so keep it but generalize the wording. Suggested-by: Junio C Hamano Signed-off-by: Eric Sunshine --- No changes since v1. Documentation/git.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index ef76f95..21bc0a5 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -744,7 +744,7 @@ The Git Repository ~~ These environment variables apply to 'all' core Git commands. Nb: it is worth noting that they may be used/overridden by SCMS sitting above -Git so take care if using Cogito etc. +Git so take care if using a foreign front-end. 'GIT_INDEX_FILE':: This environment allows the specification of an alternate -- 2.5.0.rc3.490.g8c70279 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/6] Documentation/config: mention "now" and "never" for 'expire' settings
In addition to approxidate-style values ("2.months.ago", "yesterday"),
consumers of 'gc.*expire*' configuration variables also accept and
respect 'now' ("do it immediately") and 'never' ("suppress entirely").
Suggested-by: Michael Haggerty
Signed-off-by: Eric Sunshine
---
Changes since v1:
* grammatical corrections
* use "now"/"never" consistently; avoid "all"/"false"
Documentation/config.txt | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 583d24f..e09ee02 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1226,20 +1226,24 @@ gc.packrefs::
gc.pruneexpire::
When 'git gc' is run, it will call 'prune --expire 2.weeks.ago'.
Override the grace period with this config variable. The value
- "now" may be used to disable this grace period and always prune
- unreachable objects immediately.
+ "now" may be used to disable this grace period and always prune
+ unreachable objects immediately, or "never" may be used to
+ suppress pruning.
gc.worktreePruneExpire::
When 'git gc' is run, it calls
'git worktree prune --expire 3.months.ago'.
This config variable can be used to set a different grace
period. The value "now" may be used to disable the grace
- period and prune $GIT_DIR/worktrees immediately.
+ period and prune $GIT_DIR/worktrees immediately, or "never"
+ may be used to suppress pruning.
gc.reflogexpire::
gc..reflogexpire::
'git reflog expire' removes reflog entries older than
- this time; defaults to 90 days. With "" (e.g.
+ this time; defaults to 90 days. The value "now" expires all
+ entries immediately, and "never" suppresses expiration
+ altogether. With "" (e.g.
"refs/stash") in the middle the setting applies only to
the refs that match the .
@@ -1247,7 +1251,9 @@ gc.reflogexpireunreachable::
gc..reflogexpireunreachable::
'git reflog expire' removes reflog entries older than
this time and are not reachable from the current tip;
- defaults to 30 days. With "" (e.g. "refs/stash")
+ defaults to 30 days. The value "now" expires all entries
+ immediately, and "never" suppresses expiration altogether.
+ With "" (e.g. "refs/stash")
in the middle, the setting applies only to the refs that
match the .
--
2.5.0.rc3.490.g8c70279
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/6] Documentation/git-tools: drop references to defunct tools
Cogito -- unmaintained since late 2006[1] pg -- URL dead; web searches reveal no information quilt2git -- URL dead; web searches reveal no information (h)gct -- URL dead; no repository activity since 2007[2] [1]: http://git.or.cz/cogito/ [2]: http://repo.or.cz/w/hgct.git Signed-off-by: Eric Sunshine --- No changes since v1. Documentation/git-tools.txt | 31 --- 1 file changed, 31 deletions(-) diff --git a/Documentation/git-tools.txt b/Documentation/git-tools.txt index ab4aab9..48a3595 100644 --- a/Documentation/git-tools.txt +++ b/Documentation/git-tools.txt @@ -16,24 +16,6 @@ http://git.or.cz/gitwiki/InterfacesFrontendsAndTools Alternative/Augmentative Porcelains --- -- *Cogito* (http://www.kernel.org/pub/software/scm/cogito/) -+ -Cogito is a version control system layered on top of the Git tree history -storage system. It aims at seamless user interface and ease of use, -providing generally smoother user experience than the "raw" Core Git -itself and indeed many other version control systems. -+ -Cogito is no longer maintained as most of its functionality -is now in core Git. - - -- *pg* (http://www.spearce.org/category/projects/scm/pg/) -+ -pg is a shell script wrapper around Git to help the user manage a set of -patches to files. pg is somewhat like quilt or StGit, but it does have a -slightly different feature set. - - - *StGit* (http://www.procode.org/stgit/) + Stacked Git provides a quilt-like patch management functionality in the @@ -84,12 +66,6 @@ git-svn is a simple conduit for changesets between a single Subversion branch and Git. -- *quilt2git / git2quilt* (http://home-tj.org/wiki/index.php/Misc) -+ -These utilities convert patch series in a quilt repository and commit -series in Git back and forth. - - - *hg-to-git* (contrib/) + hg-to-git converts a Mercurial repository into a Git one, and @@ -101,13 +77,6 @@ in sync with the master Mercurial repository. Others -- -- *(h)gct* (http://www.cyd.liu.se/users/~freku045/gct/) -+ -Commit Tool or (h)gct is a GUI enabled commit tool for Git and -Mercurial (hg). It allows the user to view diffs, select which files -to committed (or ignored / reverted) write commit messages and -perform the commit itself. - - *git.el* (contrib/) + This is an Emacs interface for Git. The user interface is modelled on -- 2.5.0.rc3.490.g8c70279 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/6] minor documentation improvements
This is a re-roll of [1] which makes minor improvements to documentation based upon observations of Michael Haggerty, Junio, and myself. This version addresses review comments by Michael[2] and includes follow-on patch 7/6 which arose[3] from my observation that it may be time to retire the manually-maintained list of 3rd party tools altogether. This version also drops v1 patch 1/6 since that patch[4] has already migrated to 'next'[5]. This is built atop [5] in 'next'. A v1 to v2 interdiff is included below. [1]: http://thread.gmane.org/gmane.comp.version-control.git/274537 [2]: http://article.gmane.org/gmane.comp.version-control.git/274647 [3]: http://article.gmane.org/gmane.comp.version-control.git/274602 [4]: http://article.gmane.org/gmane.comp.version-control.git/274541 [5]: 5f5f553 (Documentation/git-worktree: fix broken 'linkgit' invocation, 2015-07-24) Eric Sunshine (6): Documentation/config: mention "now" and "never" for 'expire' settings Documentation/git: drop outdated Cogito reference Documentation/git-tools: improve discoverability of Git wiki Documentation/git-tools: fix item text formatting Documentation/git-tools: drop references to defunct tools Documentation/git-tools: retire manually-maintained list Documentation/config.txt| 16 -- Documentation/git-tools.txt | 124 +++- Documentation/git.txt | 2 +- 3 files changed, 20 insertions(+), 122 deletions(-) -- 2.5.0.rc3.490.g8c70279 --- 8< --- diff --git a/Documentation/config.txt b/Documentation/config.txt index ba37a36..e09ee02 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1227,21 +1227,23 @@ gc.pruneexpire:: When 'git gc' is run, it will call 'prune --expire 2.weeks.ago'. Override the grace period with this config variable. The value "now" may be used to disable this grace period and always prune - unreachable objects immediately; or "never" to suppress pruning. + unreachable objects immediately, or "never" may be used to + suppress pruning. gc.worktreePruneExpire:: When 'git gc' is run, it calls 'git worktree prune --expire 3.months.ago'. This config variable can be used to set a different grace period. The value "now" may be used to disable the grace - period and prune $GIT_DIR/worktrees immediately; or "never" to - suppress pruning. + period and prune $GIT_DIR/worktrees immediately, or "never" + may be used to suppress pruning. gc.reflogexpire:: gc..reflogexpire:: 'git reflog expire' removes reflog entries older than - this time; defaults to 90 days. The value "all" expires all - entries; and "false" disables expiration. With "" (e.g. + this time; defaults to 90 days. The value "now" expires all + entries immediately, and "never" suppresses expiration + altogether. With "" (e.g. "refs/stash") in the middle the setting applies only to the refs that match the . @@ -1249,8 +1251,9 @@ gc.reflogexpireunreachable:: gc..reflogexpireunreachable:: 'git reflog expire' removes reflog entries older than this time and are not reachable from the current tip; - defaults to 30 days. The value "all" expires all entries; and - "false" disables expiration. With "" (e.g. "refs/stash") + defaults to 30 days. The value "now" expires all entries + immediately, and "never" suppresses expiration altogether. + With "" (e.g. "refs/stash") in the middle, the setting applies only to the refs that match the . diff --git a/Documentation/git-tools.txt b/Documentation/git-tools.txt index 48a3595..2f4ff50 100644 --- a/Documentation/git-tools.txt +++ b/Documentation/git-tools.txt @@ -1,84 +1,10 @@ -A short Git tools survey - +Git Tools += +When Git was young, people looking for third-party Git-related tools came +to the Git project itself to find them, thus a list of such tools was +maintained here. These days, however, search engines fill that role much +more efficiently, so this manually-maintained list has been retired. -Introduction - - -Apart from Git contrib/ area there are some others third-party tools -you may want to look at. -This document presents a brief summary of each tool and the corresponding -link. -For a more comprehensive list, see: +See also the `contrib/` area, and the Git wiki: http://git.or.cz/gitwiki/InterfacesFrontendsAndTools - - -Alternative/Augmentative Porcelains - -- *StGit* (http://www.procode.org/stgit/
[PATCH v2 6/6] Documentation/git-tools: retire manually-maintained list
When Git was young, people looking for third-party Git-related tools came to the Git project itself to find them, so it made sense to maintain a list of tools here. These days, however, search engines fill that role much more efficiently, so retire the manually-maintained list. The list of front-ends and tools on the Git wiki rates perhaps a distant second to search engines, and may still have value, so retain a reference to it. Signed-off-by: Eric Sunshine --- New in v2. This is plopped at the end of the series, rather than being incorporated into it more fully, so that it can be dropped easily in case it is decided that there is still some merit to having a manually- maintained list. Documentation/git-tools.txt | 88 - 1 file changed, 7 insertions(+), 81 deletions(-) diff --git a/Documentation/git-tools.txt b/Documentation/git-tools.txt index 48a3595..2f4ff50 100644 --- a/Documentation/git-tools.txt +++ b/Documentation/git-tools.txt @@ -1,84 +1,10 @@ -A short Git tools survey - +Git Tools += +When Git was young, people looking for third-party Git-related tools came +to the Git project itself to find them, thus a list of such tools was +maintained here. These days, however, search engines fill that role much +more efficiently, so this manually-maintained list has been retired. -Introduction - - -Apart from Git contrib/ area there are some others third-party tools -you may want to look at. -This document presents a brief summary of each tool and the corresponding -link. -For a more comprehensive list, see: +See also the `contrib/` area, and the Git wiki: http://git.or.cz/gitwiki/InterfacesFrontendsAndTools - - -Alternative/Augmentative Porcelains - -- *StGit* (http://www.procode.org/stgit/) -+ -Stacked Git provides a quilt-like patch management functionality in the -Git environment. You can easily manage your patches in the scope of Git -until they get merged upstream. - - -History Viewers - -- *gitk* (shipped with git-core) -+ -gitk is a simple Tk GUI for browsing history of Git repositories easily. - - -- *gitview* (contrib/) -+ -gitview is a GTK based repository browser for Git - - -- *gitweb* (shipped with git-core) -+ -Gitweb provides full-fledged web interface for Git repositories. - - -- *qgit* (http://digilander.libero.it/mcostalba/) -+ -QGit is a git/StGit GUI viewer built on Qt/C++. QGit could be used -to browse history and directory tree, view annotated files, commit -changes cherry picking single files or applying patches. -Currently it is the fastest and most feature rich among the Git -viewers and commit tools. - -- *tig* (http://jonas.nitro.dk/tig/) -+ -tig by Jonas Fonseca is a simple Git repository browser -written using ncurses. Basically, it just acts as a front-end -for git-log and git-show/git-diff. Additionally, you can also -use it as a pager for Git commands. - - -Foreign SCM interface -- - -- *git-svn* (shipped with git-core) -+ -git-svn is a simple conduit for changesets between a single Subversion -branch and Git. - - -- *hg-to-git* (contrib/) -+ -hg-to-git converts a Mercurial repository into a Git one, and -preserves the full branch history in the process. hg-to-git can -also be used in an incremental way to keep the Git repository -in sync with the master Mercurial repository. - - -Others --- - -- *git.el* (contrib/) -+ -This is an Emacs interface for Git. The user interface is modelled on -pcl-cvs. It has been developed on Emacs 21 and will probably need some -tweaking to work on XEmacs. -- 2.5.0.rc3.490.g8c70279 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/6] Documentation/git-tools: fix item text formatting
Descriptive text for each tool item is incorrectly formatted using a fixed width font. Fix formatting to use a variable width font by unindenting the item text. Signed-off-by: Eric Sunshine --- No changes since v1. Documentation/git-tools.txt | 134 ++-- 1 file changed, 67 insertions(+), 67 deletions(-) diff --git a/Documentation/git-tools.txt b/Documentation/git-tools.txt index 129b8c0..ab4aab9 100644 --- a/Documentation/git-tools.txt +++ b/Documentation/git-tools.txt @@ -16,100 +16,100 @@ http://git.or.cz/gitwiki/InterfacesFrontendsAndTools Alternative/Augmentative Porcelains --- - - *Cogito* (http://www.kernel.org/pub/software/scm/cogito/) +- *Cogito* (http://www.kernel.org/pub/software/scm/cogito/) ++ +Cogito is a version control system layered on top of the Git tree history +storage system. It aims at seamless user interface and ease of use, +providing generally smoother user experience than the "raw" Core Git +itself and indeed many other version control systems. ++ +Cogito is no longer maintained as most of its functionality +is now in core Git. - Cogito is a version control system layered on top of the Git tree history - storage system. It aims at seamless user interface and ease of use, - providing generally smoother user experience than the "raw" Core Git - itself and indeed many other version control systems. - Cogito is no longer maintained as most of its functionality - is now in core Git. +- *pg* (http://www.spearce.org/category/projects/scm/pg/) ++ +pg is a shell script wrapper around Git to help the user manage a set of +patches to files. pg is somewhat like quilt or StGit, but it does have a +slightly different feature set. - - *pg* (http://www.spearce.org/category/projects/scm/pg/) - - pg is a shell script wrapper around Git to help the user manage a set of - patches to files. pg is somewhat like quilt or StGit, but it does have a - slightly different feature set. - - - - *StGit* (http://www.procode.org/stgit/) - - Stacked Git provides a quilt-like patch management functionality in the - Git environment. You can easily manage your patches in the scope of Git - until they get merged upstream. +- *StGit* (http://www.procode.org/stgit/) ++ +Stacked Git provides a quilt-like patch management functionality in the +Git environment. You can easily manage your patches in the scope of Git +until they get merged upstream. History Viewers --- - - *gitk* (shipped with git-core) - - gitk is a simple Tk GUI for browsing history of Git repositories easily. - - - - *gitview* (contrib/) - - gitview is a GTK based repository browser for Git +- *gitk* (shipped with git-core) ++ +gitk is a simple Tk GUI for browsing history of Git repositories easily. - - *gitweb* (shipped with git-core) +- *gitview* (contrib/) ++ +gitview is a GTK based repository browser for Git - Gitweb provides full-fledged web interface for Git repositories. +- *gitweb* (shipped with git-core) ++ +Gitweb provides full-fledged web interface for Git repositories. - - *qgit* (http://digilander.libero.it/mcostalba/) - QGit is a git/StGit GUI viewer built on Qt/C++. QGit could be used - to browse history and directory tree, view annotated files, commit - changes cherry picking single files or applying patches. - Currently it is the fastest and most feature rich among the Git - viewers and commit tools. +- *qgit* (http://digilander.libero.it/mcostalba/) ++ +QGit is a git/StGit GUI viewer built on Qt/C++. QGit could be used +to browse history and directory tree, view annotated files, commit +changes cherry picking single files or applying patches. +Currently it is the fastest and most feature rich among the Git +viewers and commit tools. - - *tig* (http://jonas.nitro.dk/tig/) - - tig by Jonas Fonseca is a simple Git repository browser - written using ncurses. Basically, it just acts as a front-end - for git-log and git-show/git-diff. Additionally, you can also - use it as a pager for Git commands. +- *tig* (http://jonas.nitro.dk/tig/) ++ +tig by Jonas Fonseca is a simple Git repository browser +written using ncurses. Basically, it just acts as a front-end +for git-log and git-show/git-diff. Additionally, you can also +use it as a pager for Git commands. Foreign SCM interface - - - *git-svn* (shipped with git-core) - - git-svn is a simple conduit for changesets between a single Subversion - branch and Git. - - - - *quilt2git / git2quilt* (http://home-tj.org/wiki/index.php/Misc) +- *git-svn* (shipped with git-core) ++ +git-svn is a simple conduit for changesets between a single Subversion +branch and Git. - These utilities convert patch series in a quilt repository and commit - series in Git back and forth. +- *quilt2git / git2quilt* (http://home-tj.org/wiki/index.php/Misc) ++ +These utilities
Re: [PATCH 2/6] Documentation/config: mention "now" and "never" for 'expire' settings
On Sun, Jul 26, 2015 at 9:41 PM, Michael Haggerty wrote:
> On 07/23/2015 09:00 PM, Eric Sunshine wrote:
>> In addition to approxidate-style values ("2.months.ago", "yesterday"),
>> consumers of 'gc.*expire*' configuration variables also accept and
>> respect 'now'/'all' ("do it immediately") and 'never'/'false' ("suppress
>> entirely").
>>
>> Suggested-by: Michael Haggerty
>> Signed-off-by: Eric Sunshine
>> ---
>> gc.pruneExpire::
>> When 'git gc' is run, it will call 'prune --expire 2.weeks.ago'.
>> Override the grace period with this config variable. The value
>> - "now" may be used to disable this grace period and always prune
>> - unreachable objects immediately.
>> + "now" may be used to disable this grace period and always prune
>> + unreachable objects immediately; or "never" to suppress pruning.
>
> A semicolon should be used without a conjunction, and the parts of a
> sentence joined by a semicolon should be independent clauses. So this
> should probably be
>
> [...] The value
> "now" may be used to disable this grace period and always prune
> unreachable objects immediately, or "never" may be used to
> suppress pruning.
I was absent from school that day...
>> @@ -1328,7 +1330,8 @@ gc.reflogExpireUnreachable::
>> gc..reflogExpireUnreachable::
>> 'git reflog expire' removes reflog entries older than
>> this time and are not reachable from the current tip;
>> - defaults to 30 days. With "" (e.g. "refs/stash")
>> + defaults to 30 days. The value "all" expires all entries; and
>> + "false" disables expiration. With "" (e.g. "refs/stash")
>> in the middle, the setting applies only to the refs that
>> match the .
>
> Also, I wonder why you suggest "now"/"never" for the first two settings,
> but "all"/"false" for the second two. Wouldn't it be less confusing to
> be consistent?
It was intentional due to the way I worded the sentence. It sounded
slightly strange to my ear to say:
The value "now" expires all entries; and "never"
disables expiration.
whereas:
The value "all" expires all entries; ...
sounded nice. But, upon reflection, with a slight re-wording[1], "all"
and "never" work, as well.
[1]: http://article.gmane.org/gmane.comp.version-control.git/274828
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/11] ref-filter: add option to pad atoms to the right
On Mon, Jul 27, 2015 at 3:27 AM, Karthik Nayak wrote: > Add a new atom "padright" and support %(padright:X) where X is a > number. This will align the succeeding atom value to the left > followed by spaces for a total length of X characters. If X is less > than the item size, the entire atom value is printed. > > Signed-off-by: Karthik Nayak > --- > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index e49d578..45dd7f8 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -127,6 +127,12 @@ color:: > Change output color. Followed by `:`, where names > are described in `color.branch.*`. > > +padright:: > + Pad succeeding atom to the right. Followed by `:`, > + where `value` states the total length of atom including the > + padding. If the `value` is greater than the atom length, then > + no padding is performed. Isn't this backward? Don't you mean ... If the atom length is greater than `value`, then no padding is performed. ? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 07/11] ref-filter: add option to match literal pattern
On Mon, Jul 27, 2015 at 3:27 AM, Karthik Nayak wrote:
> From: Karthik Nayak
>
> Since 'ref-filter' only has an option to match path names add an
> option for plain fnmatch pattern-matching.
>
> This is to support the pattern matching options which are used in `git
> tag -l` and `git branch -l` where we can match patterns like `git tag
> -l foo*` which would match all tags which has a "foo*" pattern.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 26eb26c..597b189 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -946,6 +946,32 @@ static int commit_contains(struct ref_filter *filter,
> struct commit *commit)
>
> /*
> * Return 1 if the refname matches one of the patterns, otherwise 0.
> + * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
> + * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
> + * matches "refs/heads/mas*", too).
> + */
> +static int match_pattern(const char **patterns, const char *refname)
> +{
> + /*
> +* When no '--format' option is given we need to skip the prefix
> +* for matching refs of tags and branches.
> +*/
> + if (skip_prefix(refname, "refs/tags/", &refname))
> + ;
> + else if (skip_prefix(refname, "refs/heads/", &refname))
> + ;
> + else if (skip_prefix(refname, "refs/remotes/", &refname))
> + ;
Or, more concisely:
skip_prefix(refname, "refs/tags/", &refname) ||
skip_prefix(refname, "refs/heads/", &refname) ||
skip_prefix(refname, "refs/remotes/", &refname);
since || short-circuits. No need for the 'if' or cascading 'else if's.
> + for (; *patterns; patterns++) {
> + if (!wildmatch(*patterns, refname, 0, NULL))
> + return 1;
> + }
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: handle multiple worktrees
On Tue, Jul 28, 2015 at 5:23 PM, David Turner wrote:
> Prevent merges to the same notes branch from different worktrees.
> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
> code we use to check that two HEADs in different worktrees don't point
> to the same branch. Modify that code, die_if_checked_out, to take a
> "head" ref to examine; previously, it just looked at HEAD.
die_if_checked_out() seems to be grossly misnamed following this
change since a branch (or detached head) can be "checked out", but
asking it if a HEAD or NOTES_MERGE_REF is "checked out" sounds weird.
Perhaps rename it to die_if_symref_shared() or something?
The new argument order, die_if_checked_out("HEAD", "branchname"), also
seems backward to me, at least with the current function name
(die_if_checked_out). With a better function name, maybe it wouldn't
be so bad.
I could also see die_if_checked_out() being refactored to move the
underlying logic to a new (better named) function for checking the
value of a particular file (HEAD, NOTES_MERGE_REF) in each linked
worktree. And then die_if_checked_out() would be a thin wrapper over
that new function. A reason for preferring this approach and keeping
die_if_checked_out() as a convenience wrapper is that it's likely that
it's going to be needed in more places in the future.
> Reported-by: Junio C Hamano
> Signed-off-by: David Turner
> ---
> diff --git a/branch.c b/branch.c
> index c85be07..60eadc6 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -311,21 +311,22 @@ void remove_branch_state(void)
> unlink(git_path("SQUASH_MSG"));
> }
>
> -static void check_linked_checkout(const char *branch, const char *id)
> +static void check_linked_checkout(const char *head, const char *branch,
> + const char *id)
> {
> struct strbuf sb = STRBUF_INIT;
> struct strbuf path = STRBUF_INIT;
> struct strbuf gitdir = STRBUF_INIT;
>
> /*
> -* $GIT_COMMON_DIR/HEAD is practically outside
> +* $GIT_COMMON_DIR/$head is practically outside
> * $GIT_DIR so resolve_ref_unsafe() won't work (it
> * uses git_path). Parse the ref ourselves.
> */
> if (id)
> - strbuf_addf(&path, "%s/worktrees/%s/HEAD",
> get_git_common_dir(), id);
> + strbuf_addf(&path, "%s/worktrees/%s/%s",
> get_git_common_dir(), id, head);
> else
> - strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> + strbuf_addf(&path, "%s/%s", get_git_common_dir(), head);
>
> if (!strbuf_readlink(&sb, path.buf, 0)) {
> if (!starts_with(sb.buf, "refs/") ||
> @@ -356,13 +357,13 @@ done:
> strbuf_release(&gitdir);
> }
>
> -void die_if_checked_out(const char *branch)
> +void die_if_checked_out(const char *head, const char *branch)
> {
> struct strbuf path = STRBUF_INIT;
> DIR *dir;
> struct dirent *d;
>
> - check_linked_checkout(branch, NULL);
> + check_linked_checkout(head, branch, NULL);
>
> strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
> dir = opendir(path.buf);
> @@ -373,7 +374,7 @@ void die_if_checked_out(const char *branch)
> while ((d = readdir(dir)) != NULL) {
> if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> continue;
> - check_linked_checkout(branch, d->d_name);
> + check_linked_checkout(head, branch, d->d_name);
> }
> closedir(dir);
> }
> diff --git a/branch.h b/branch.h
> index 58aa45f..3577fe5 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -57,6 +57,6 @@ extern int read_branch_desc(struct strbuf *, const char
> *branch_name);
> * worktree and die (with a message describing its checkout location) if
> * it is.
> */
> -extern void die_if_checked_out(const char *branch);
> +extern void die_if_checked_out(const char *head, const char *branch);
>
> #endif
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/6] refs: add ref_type function
On Tue, Jul 28, 2015 at 2:12 PM, David Turner wrote:
> Add a function ref_type, which categorizes refs as per-worktree,
> pseudoref, or normal ref.
>
> Later, we will use this in refs.c to treat pseudorefs specially.
> Alternate ref backends may use it to treat both pseudorefs and
> per-worktree refs differently.
>
> Signed-off-by: David Turner
> ---
> diff --git a/refs.c b/refs.c
> index 0b96ece..553ae8b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2848,6 +2848,35 @@ static int delete_ref_loose(struct ref_lock *lock, int
> flag, struct strbuf *err)
> return 0;
> }
>
> +static int is_per_worktree_ref(const char *refname)
> +{
> + return !strcmp(refname, "HEAD");
> +}
> +
> +static int is_pseudoref_syntax(const char *refname)
> +{
> + const char *c;
> +
> + if (strchr(refname, '/'))
> + return 0;
Isn't this redundant? Won't the below for-loop already return 0 if it
encounters a slash?
> + for (c = refname; *c; ++c) {
Style: c++
> + if (!isupper(*c) && *c != '-' && *c != '_')
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +enum ref_type ref_type(const char *refname)
> +{
> + if (is_per_worktree_ref(refname))
> + return REF_TYPE_PER_WORKTREE;
> + if (is_pseudoref_syntax(refname))
> + return REF_TYPE_PSEUDOREF;
> + return REF_TYPE_NORMAL;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/6] pseudorefs: create and use pseudoref update and delete functions
On Tue, Jul 28, 2015 at 2:12 PM, David Turner wrote:
> Pseudorefs should not be updated through the ref transaction
> API, because alternate ref backends still need to store pseudorefs
> in GIT_DIR (instead of wherever they store refs). Instead,
> change update_ref and delete_ref to call pseudoref-specific
> functions.
>
> Signed-off-by: David Turner
> ---
> diff --git a/refs.c b/refs.c
> index 553ae8b..2bd6aa6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2877,12 +2877,87 @@ enum ref_type ref_type(const char *refname)
> +static int delete_pseudoref(const char *pseudoref, const unsigned char
> *old_sha1)
> +{
> + static struct lock_file lock;
> + const char *filename;
> +
> + filename = git_path("%s", pseudoref);
> +
> + if (old_sha1 && !is_null_sha1(old_sha1)) {
> + int fd;
> + unsigned char actual_old_sha1[20];
> +
> + fd = hold_lock_file_for_update(&lock, filename,
> + LOCK_DIE_ON_ERROR);
> + if (fd < 0)
> + die_errno(_("Could not open '%s' for writing"),
> filename);
> + read_ref(pseudoref, actual_old_sha1);
> + if (hashcmp(actual_old_sha1, old_sha1)) {
> + warning("Unexpected sha1 when deleting %s",
> pseudoref);
> + return -1;
Does this need to release the lock file before returning?
> + }
> +
> + unlink(filename);
> + rollback_lock_file(&lock);
> + } else {
> + unlink(filename);
> + }
> +
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-add-interactive: edit current file in editor
Aside from whether or not this change is desirable, see a few pointers
below to improve the patch...
On Monday, July 27, 2015, Sina Siadat wrote:
> Adds a new option 'o' to the 'add -p' command that lets you open and edit the
> current file.
Imperative mood: "Add a new option..."
> The existing 'e' mode is used to manually edit the hunk. The new 'o' option
> allows you to open and edit the file without having to quit the loop. The
> hunks
> are updated when the editing is done, and the user will be able to review the
> updated hunks. Without this option you would have to quit the loop, edit the
> file, and execute 'add -p filename' again.
This descriptive material belongs in the commit message. Good.
> I would appreciate it if you could let me know what you think about this
> option. I will write more tests if there is any interest at all.
This, however, is commentary, which, while useful as part of the
submission process, does not belong in the commit message. Therefore,
it should be placed below the "---" line just before the diffstat.
> Thank you. :)
Missing sign-off. See Documentation/SubmittingPatches.
More below...
> ---
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index deae948..e5dd1c6 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -98,6 +98,12 @@ test_expect_success 'dummy edit works' '
> test_cmp expected diff
> '
>
> +test_expect_success 'dummy open works' '
> + (echo o; echo a) | git add -p &&
Some alternatives, which may or may not read better, but at least
avoid a process creation or two:
{ echo o; echo a; } | git add -p &&
printf "%s\n" o a | git add -p &&
printf "o\na\n" | git add -p &&
Those are just suggestions; not necessarily a request for change.
> + git diff > diff &&
Style: >diff
> + test_cmp expected diff
> +'
> +
> test_expect_success 'setup patch' '
> cat >patch < @@ -1,1 +1,4 @@
> --
> 2.5.0.rc3.2.g6f9504c.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 01/10] ref-filter: introduce 'ref_formatting_state'
On Tuesday, July 28, 2015, Karthik Nayak wrote:
> Introduce 'ref_formatting' structure to hold values of pseudo atoms
> which help only in formatting. This will eventually be used by atoms
> like `color` and the `padright` atom which will be introduced in a
> later patch.
Isn't this commit message outdated now that you no longer treat color
specially and since the terminology is changing from "pseudo" to
"modifier"? Also, isn't the structure now called
'ref_formatting_state' rather than 'ref_formatting'?
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 7561727..a919a14 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -620,7 +622,7 @@ static void populate_value(struct ref_array_item *ref)
> const char *name = used_atom[i];
> struct atom_value *v = &ref->value[i];
> int deref = 0;
> - const char *refname;
> + const char *refname = NULL;
What is this change about? It doesn't seem to be related to anything
else in the patch.
> const char *formatp;
> struct branch *branch = NULL;
>
> @@ -1190,30 +1192,47 @@ void ref_array_sort(struct ref_sorting *sorting,
> struct ref_array *array)
> +static void print_value(struct atom_value *v, struct ref_formatting_state
> *state)
> +{
> + struct strbuf value = STRBUF_INIT;
> + struct strbuf formatted = STRBUF_INIT;
> +
> + /*
> +* Some (pesudo) atoms have no immediate side effect, but only
> +* affect the next atom. Store the relevant information from
> +* these atoms in the 'state' variable for use when displaying
> +* the next atom.
> +*/
> + apply_formatting_state(state, v, &value);
The comment says that this is "storing" formatting state, however, the
code is actually "applying" the state. You could move this comment
down to show_ref_array_item() where formatting state actually gets
stored. Or you could fix it to talk about "applying" the state.
However, now that apply_formatting_state() has a meaningful name, you
could also drop the comment altogether since it doesn't say much
beyond what is said already by the function name.
> + switch (state->quote_style) {
> case QUOTE_NONE:
> - fputs(v->s, stdout);
> @@ -1254,9 +1273,26 @@ static void emit(const char *cp, const char *ep)
> +static void reset_formatting_state(struct ref_formatting_state *state)
> +{
> + int quote_style = state->quote_style;
> + memset(state, 0, sizeof(*state));
> + state->quote_style = quote_style;
I wonder if this sledge-hammer approach of saving one or two values
before clearing the entire 'ref_formatting_state' and then restoring
the saved values will scale well. Would it be better for this to just
individually reset the fields which need resetting and not touch those
that don't?
Also, the fact that quote_style has to be handled specially may be an
indication that it doesn't belong in this structure grouped with the
other modifiers or that you need better classification within the
structure. For instance:
struct ref_formatting_state {
struct global {
int quote_style;
};
struct local {
int pad_right;
};
where 'local' state gets reset by reset_formatting_state(), and
'global' is left alone.
That's just one idea, not necessarily a proposal, but is something to
think about since the current arrangement is kind of yucky.
> +}
> +
> void show_ref_array_item(struct ref_array_item *info, const char *format,
> int quote_style)
> {
> const char *cp, *sp, *ep;
> + struct ref_formatting_state state;
> +
> + memset(&state, 0, sizeof(state));
> + state.quote_style = quote_style;
It's a little bit ugly to use memset() here when you have
reset_formatting_state() available. You could set quote_style first,
and then call reset_formatting_state() rather than memset(). Or,
perhaps, change reset_formatting_state(), as described above, to stop
using the sledge-hammer approach.
> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
> struct atom_value *atomv;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/10] port tag.c to use ref-filter APIs
On Tuesday, July 28, 2015, Karthik Nayak wrote: > Changes in v6: > * Change the flow of commits, introduce rest_formatting_state. > * Rename > ref_formatting -> apply_formatting_state > apply_pseudo_state -> store_formatting_state > * Remove the patch for making color a pseudo atom, and rename > pseudo_atom to modifier_atom. > * Small grammatical changes. > > Side Note: --format="%(padright:X)" applies to the next available atom > and not to the next span. I find this more accurate as I don't see why > we'd want to pad something of known length. But its up for discussion This isn't supported by the current %(padright:) syntax, but an example would be if someone wants to pad a string composed of atoms and literal strings. For instance, the user might want to right-pad the composed string "%(refattribute1) glorked %(refattribute2)”. One possible syntax to support this sort of thing would be %(padright:n:content). For instance, using the example above: %(padright:20:%(refattribute1) glorked %(refattribute2)) Another would be %(padright:n)content%(end). The %(end) token could work with other formatting directives, such as left padding, alignment, etc. For instance: %(padright:20)%(refattribute1) glorked %(refattribute2)%(end) In fact, this sort of thing has come up multiple times, with the most general being actual embedding of a full-blown scripting language to support pretty much any desired formatting. Other less ambitious proposals have also been suggested. This isn't a suggestion that you should implement any of these, but just a heads-up that, when it comes to formatting, it's not at all uncommon for people to come up with needs not anticipated by the design. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 02/10] ref-filter: add option to pad atoms to the right
On Tuesday, July 28, 2015, Karthik Nayak wrote: > Add a new atom "padright" and support %(padright:X) where X is a > number. This will align the succeeding atom value to the left > followed by spaces for a total length of X characters. If X is less > than the item size, the entire atom value is printed. > > Add tests and documentation for the same. > > Signed-off-by: Karthik Nayak > --- > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index 505a360..19ac480 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -81,4 +81,20 @@ test_expect_success 'filtering with --contains' ' > test_cmp expect actual > ' > > +test_expect_success 'padding to the right using `padright`' ' > + cat >expect <<-\EOF && > + refs/heads/master|refs/heads/master|refs/heads/master| > + refs/heads/side|refs/heads/side |refs/heads/side| > + refs/odd/spot|refs/odd/spot|refs/odd/spot| > + refs/tags/double-tag|refs/tags/double-tag |refs/tags/double-tag| > + refs/tags/four|refs/tags/four |refs/tags/four| > + refs/tags/one|refs/tags/one|refs/tags/one| > + refs/tags/signed-tag|refs/tags/signed-tag |refs/tags/signed-tag| > + refs/tags/three|refs/tags/three |refs/tags/three| > + refs/tags/two|refs/tags/two|refs/tags/two| > + EOF > + git for-each-ref > --format="%(refname)%(padright:25)|%(refname)|%(refname)|" >actual && This fails to test the important case when the atom length is greater than the padright value (in which case no padding should be done, and the atom text should extend beyond the 'padright' column). > + test_cmp expect actual > +' > + > test_done > -- > 2.4.6 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 05/10] ref-filter: add support to sort by version
On Tuesday, July 28, 2015, Karthik Nayak wrote: > Add support to sort by version using the "v:refname" and > "version:refname" option. This is achieved by using the 'versioncmp()' > function as the comparing function for qsort. > > This option is included to support sorting by versions in `git tag -l` > which will eventaully be ported to use ref-filter APIs. > > Add documentation and tests for the same. > > Signed-off-by: Karthik Nayak > --- > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index 45dd7f8..2b60aee 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -151,6 +151,9 @@ For sorting purposes, fields with numeric values sort in > numeric > order (`objectsize`, `authordate`, `committerdate`, `taggerdate`). > All other fields are used to sort in their byte-value order. > > +There is also an option to sort by versions, this can be done by using > +the fieldname `version:refname` or in short `v:refname`. Nit: "or in short" reads a bit oddly. Perhaps: ...`version:refname` or its alias `v:refname`. or ...`version:refname` or the short-form `v:refname`. (I rather prefer the "alias" alternative.) > In any case, a field name that refers to a field inapplicable to > the object referred by the ref does not cause an error. It > returns an empty string instead. > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index 19ac480..68688a9 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -97,4 +97,40 @@ test_expect_success 'padding to the right using > `padright`' ' > +test_expect_success 'version sort' ' > + git for-each-ref --sort=version:refname --format="%(refname:short)" > refs/tags/ | grep "foo" >actual && > + cat >expect <<-\EOF && > + foo1.3 > + foo1.6 > + foo1.10 > + EOF > + test_cmp expect actual > +' > + > +test_expect_success 'version sort (shortened)' ' > + git for-each-ref --sort=v:refname --format="%(refname:short)" > refs/tags/ | grep "foo" >actual && > + cat >expect <<-\EOF && > + foo1.3 > + foo1.6 > + foo1.10 > + EOF > + test_cmp expect actual Nit: In the earlier review when I suggested using "v:refname" for one of the tests in order to exercise it (in addition to "version:refname"), I didn't mean that you had to add another (identical) test but rather that you could have one of the existing tests use "v:refname". (Not a big deal. You can leave this as is if you like. I just wanted to clarify.) > +' > + > +test_expect_success 'reverse version sort' ' > + git for-each-ref --sort=-version:refname --format="%(refname:short)" > refs/tags/ | grep "foo" >actual && > + cat >expect <<-\EOF && > + foo1.10 > + foo1.6 > + foo1.3 > + EOF > + test_cmp expect actual > +' > + > test_done > -- > 2.4.6 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom
On Tuesday, July 28, 2015, Karthik Nayak wrote:
> The 'colornext' atom allows us to color only the succeeding atom with
> a particular color. This resets any previous color preferences set. It
> is not compatible with `padright`. This is required for printing
> formats like `git branch -vv` where the upstream is colored in blue.
Can you explain here and in the commit message why %(colornext) is not
compatible with %(padright:)? Is it an implementation limitation, or a
syntax or semantic limitation, or what? Can the implementation be
fixed to make it compatible or does it require a redesign?
Also, please explain here and in the commit message why this highly
specialized colorizer ('colornext'), is needed even though a more
general purpose one ('color') is already available.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] connect: move error check to caller of parse_connect_url
On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:
> parse_connect_url() checks if the path component of the URL is
> empty and if so causes the program to die. As the function is to
> be used at other call sites which do not require this check, move
> up the error checking to the existing caller.
>
> Signed-off-by: Patrick Steinhardt
> ---
> diff --git a/connect.c b/connect.c
> index bdbcee4..e8b813d 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -613,9 +613,6 @@ enum protocol parse_connect_url(const char *url_orig,
> char **ret_host,
> else
> path = strchr(end, separator);
>
> - if (!path || !*path)
> - die("No path specified. See 'man git-pull' for valid url
> syntax");
> -
Given that there are several dereferences of 'path' following this bit
of code, won't this change lead to a crash when path==NULL?
> /*
> * null-terminate hostname and point path to ~ for URL's like this:
> *ssh://host.xz/~user/repo
> @@ -665,6 +662,9 @@ struct child_process *git_connect(int fd[2], const char
> *url,
> signal(SIGCHLD, SIG_DFL);
>
> protocol = parse_connect_url(url, &hostandport, &path);
> + if (!path || !*path)
> + die("No path specified. See 'man git-pull' for valid url
> syntax");
> +
> if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
> printf("Diag: url=%s\n", url ? url : "NULL");
> printf("Diag: protocol=%s\n", prot_name(protocol));
> --
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] worktree: Correct typo in documentation
On Wed, Jul 29, 2015 at 7:03 PM, Johan Sageryd wrote: > Signed-off-by: Johan Sageryd > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 3387e2f..d9d90b5 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -39,7 +39,7 @@ repository so that they do not get automatically pruned. > > If a linked working tree is stored on a portable device or network share > which is not always mounted, you can prevent its administrative files from > -being pruned by creating a file named 'lock' alongside the other > +being pruned by creating a file named 'locked' alongside the other > administrative files, optionally containing a plain text reason that > pruning should be suppressed. See section "DETAILS" for more information. Thanks for the patch. This is already fixed in the 'next' branch by 2e73ab6 (Documentation/git-worktree: fix incorrect reference to file "locked", 2015-07-20). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 03/11] ref-filter: add option to pad atoms to the right
On Thu, Jul 30, 2015 at 11:48 AM, Karthik Nayak wrote: > Add a new atom "padright" and support %(padright:X) where X is a > number. This will align the succeeding atom or string to the left > followed by spaces for a total length of X characters. If X is less > than the atom or string length then no padding is done. > > Add tests and documentation for the same. > > Signed-off-by: Karthik Nayak > --- > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index 505a360..48caac9 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -81,4 +81,36 @@ test_expect_success 'filtering with --contains' ' > test_cmp expect actual > ' > > +test_expect_success 'padding to the right using `padright`' ' > + cat >expect <<-\EOF && > + master|master| > + side|side | > + odd/spot|odd/spot | > + double-tag|double-tag| > + four|four | > + one|one | > + signed-tag|signed-tag| > + three|three | > + two|two | > + EOF > + git for-each-ref > --format="%(refname:short)%(padright:5)|%(padright:10)%(refname:short)|" > >actual && > + test_cmp expect actual > +' In an earlier review, Matthieu pointed out that this test failed to ensure that the 'padright' value did not leak into the next atom. In a subsequent version, you fixed the test to check that condition, but now you've somewhat lost it again, at least visually. That is, because whitespace is "invisible" and because 'padright' now also affects literal strings, someone reading this test can't tell if those trailing |'s in the expected output are padded or not. You could use a different format string to prove that the 'padright' value doesn't leak. For instance: %(padright:10)%(refname:short)|%(padright:5)|%(refname:short) This way, as long as the two |'s are side-by-side, then you've proved that the first one wasn't affected by the preceding 'padright:10'. You could also add back the %(refname:short) at the front of the pattern, as you currently have it, if you want to prove that padding is not enabled at the start of format. > +test_expect_success 'no padding when `padright` length is smaller than atom > length' ' > + cat >expect <<-\EOF && > + refs/heads/master| > + refs/heads/side| > + refs/odd/spot| > + refs/tags/double-tag| > + refs/tags/four| > + refs/tags/one| > + refs/tags/signed-tag| > + refs/tags/three| > + refs/tags/two| > + EOF > + git for-each-ref --format="%(padright:5)%(refname)|" >actual && > + test_cmp expect actual > +' > + > test_done > -- > 2.4.6 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/6] clone: add tests for cloning with empty path
On Wed, Jul 29, 2015 at 11:51 AM, Patrick Steinhardt wrote:
> Test behavior of `git clone` when working with an empty path
> component. This may be the case when cloning a file system's root
> directory or from a remote server's root.
>
> Signed-off-by: Patrick Steinhardt
> ---
> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
> index 553a3f6..acfa133 100755
> --- a/t/t1509-root-worktree.sh
> +++ b/t/t1509-root-worktree.sh
> @@ -237,6 +237,45 @@ test_foobar_foobar
>
> test_expect_success 'cleanup' 'rm -rf /.git'
>
> +say "clone .git at root without reponame"
> +
> +test_expect_success 'go to /' 'cd /'
> +test_expect_success 'setup' '
> + echo "Initialized empty Git repository in /.git/" > expected &&
> + git init > result &&
> + test_cmp expected result
> +'
I'd say something here about current style omitting the space after
the redirection operator (>), however, this script uniformly includes
the space, and being consistent with the existing style is important,
so I won't mention it. ;-)
> +test_clone_expect_dir() {
> + URL="$1"
> + DIR="$2"
> + echo "Cloning into '$DIR'..." >expected
> + echo "warning: You appear to have cloned an empty repository."
> >>expected
echo >expected <<-\EOF
Cloning into...
warning: You appear...
EOF
is more readable and maintainable.
> + git clone "$URL" 2>result >result
git clone "$URL" >result 2>&1
> + rm -r "$DIR"
> + test_cmp expected result
> +}
While not mandatory since it works as expected in its current form, it
would be nice to see a fully intact &&-chain in this function. That
way, if someone some day adds code which doesn't impact 'result' but
which might somehow fail, then the failure will be noticed.
> +
> +test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'
> +test_expect_success 'simple clone of /' '
> + echo "fatal: No directory name could be guessed." > expected &&
> + echo "Please specify a directory on the command line" >> expected &&
cat >expected <<-\EOF
fatal: No directory...
Please specify...
EOF
> + test_expect_code 128 git clone / 2>result >result &&
test_expect_code 128 git clone / >result 2>&1 &&
> + test_cmp expected result'
> +
> +test_expect_success 'clone with file://' '
> + test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user@' '
> + test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@' '
> + test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://:port' '
> + test_clone_expect_dir file://127.0.0.1:/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@:port' '
> + test_clone_expect_dir file://user:password@127.0.0.1:/ 127.0.0.1'
> +
> +test_expect_success 'cleanup' 'rm -rf /.git /clones'
> +
> say "auto bare gitdir"
>
> # DESTROY!
> --
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: support negative transfer.hideRefs
On Tue, Jul 28, 2015 at 3:59 PM, Jeff King wrote: > If you hide a hierarchy of refs using the transfer.hideRefs > config, there is no way to later override that config to > "unhide" it. This patch implements a "negative" hide which > causes matches to immediately be marked as unhidden, even if > another match would hide it. We take care to apply the > matches in reverse-order from how they are fed to us by the > config machinery, as that lets our usual "last one wins" > config precedence work (and entries in .git/config, for > example, will override /etc/gitconfig). > > Signed-off-by: Jeff King > --- > +test_expect_success 'set up some extra tags for ref hiding' ' > + git tag magic/one && > + git tag magic/two > +' > + > for configsection in transfer uploadpack > do > test_expect_success "Hide some refs with $configsection.hiderefs" ' > @@ -138,6 +143,16 @@ do > sed -e "/ refs\/tags\//d" >expect && > test_cmp expect actual > ' > + > + test_expect_success "Override hiding of $configsection.hiderefs" ' > + test_when_finished "test_unconfig $configsection.hiderefs" && > + git config --add $configsection.hiderefs refs/tags && > + git config --add $configsection.hiderefs "!refs/tags/magic" && > + git config --add $configsection.hiderefs refs/tags/magic/one > && > + git ls-remote . >actual && > + verbose grep refs/tags/magic/two actual For completeness, do you also want to add !grep refs/tags/magic/one actual to confirm that the third hideRefs did indeed override the second one? > + ' > + > done > > test_done > -- > 2.5.0.rc3.557.g17a1555 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref
On Fri, Jul 31, 2015 at 3:01 PM, David Turner wrote: > Add a new function, die_if_shared_symref, which works like > die_if_checked_out, but for all references. Refactor > die_if_checked_out to work in terms of die_if_shared_symref. > > Soon, we will use die_if_shared_symref to protect notes merges in > worktrees. I wonder if the diagnostic: 'blorp' is already checked out at '/path/name/' emitted by check_linked_checkout() needs to be adjusted for this change. It still makes sense for die_if_checked_out(), but sounds odd for die_if_shared_symref(). > Signed-off-by: David Turner > --- > diff --git a/branch.h b/branch.h > index 58aa45f..b2fca30 100644 > --- a/branch.h > +++ b/branch.h > @@ -59,4 +59,11 @@ extern int read_branch_desc(struct strbuf *, const char > *branch_name); > */ > extern void die_if_checked_out(const char *branch); > > +/* > + * Check if a symref points to a branch in the main worktree or any linked > + * worktree and die (with a message describing its checkout location) if > + * it is. Ditto here. This is still talking about "branch" and "checkout", whereas it's actually checking if a symref in some worktree already references the given referent. > + */ > +extern void die_if_shared_symref(const char *symref, const char *branch); s/branch/referent/ or something? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] notes: add notes.merge option to select default strategy
On Fri, Jul 31, 2015 at 3:21 PM, Jacob Keller wrote:
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for the configuration option. Ensure that command line
> --strategy option overrides the configured value. Ensure that -s can't
> be passed with --commit or --abort. Ensure that --abort and --commit
> can't be used together.
>
> Signed-off-by: Jacob Keller
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 88fdafb32bc0..7123311b563e 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,6 +744,36 @@ static int merge_commit(struct notes_merge_options *o)
> +static int parse_notes_strategy(const char *var, const char *arg,
> + enum notes_merge_strategy *strategy)
What is the 'var' argument for? It's not used by the function.
> +{
> + if (!strcmp(arg, "manual"))
> + *strategy = NOTES_MERGE_RESOLVE_MANUAL;
> + else if (!strcmp(arg, "ours"))
> + *strategy = NOTES_MERGE_RESOLVE_OURS;
> + else if (!strcmp(arg, "theirs"))
> + *strategy = NOTES_MERGE_RESOLVE_THEIRS;
> + else if (!strcmp(arg, "union"))
> + *strategy = NOTES_MERGE_RESOLVE_UNION;
> + else if (!strcmp(arg, "cat_sort_uniq"))
> + *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> + else {
> + return error("Unknown notes merge strategy: %s", arg);
Perhaps reporting of the error should be left to the callers so they
can tailor the message ("--strategy" vs. "notes.merge")? Doing so
would make it easier for the user to trace the source of a bogus
value.
> + }
> +
> + return 0;
> +}
> +
> +static int parse_opt_strategy(const struct option *opt, const char *arg, int
> unset)
> +{
> + enum notes_merge_strategy *strategy = opt->value;
> +
> + /* let merge() know we overrode the configured strategy */
> + override_strategy = 1;
> +
> + return parse_notes_strategy(NULL, arg, strategy);
> +}
> +
> static int merge(int argc, const char **argv, const char *prefix)
> {
> struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -749,14 +782,16 @@ static int merge(int argc, const char **argv, const
> char *prefix)
> - const char *strategy = NULL;
> struct option options[] = {
> OPT_GROUP(N_("General options")),
> OPT__VERBOSITY(&verbosity),
> OPT_GROUP(N_("Merge options")),
> - OPT_STRING('s', "strategy", &strategy, N_("strategy"),
> - N_("resolve notes conflicts using the given
> strategy "
> - "(manual/ours/theirs/union/cat_sort_uniq)")),
> + {
> + OPTION_CALLBACK, 's', "strategy", &strategy,
> N_("strategy"),
> + N_("resolve notes conflicts using the given strategy "
> + "(manual/ours/theirs/union/cat_sort_uniq)"),
> + PARSE_OPT_NONEG, parse_opt_strategy
> + },
Why change this from an OPT_STRING to an OPTION_CALLBACK? Couldn't you
just as easily have called parse_notes_strategy() after
parse_options()?
> OPT_GROUP(N_("Committing unmerged notes")),
> { OPTION_SET_INT, 0, "commit", &do_commit, NULL,
> N_("finalize notes merge by committing unmerged
> notes"),
> @@ -771,7 +806,7 @@ static int merge(int argc, const char **argv, const char
> *prefix)
> argc = parse_options(argc, argv, prefix, options,
> git_notes_merge_usage, 0);
>
> - if (strategy || do_commit + do_abort == 0)
> + if (override_strategy || do_commit + do_abort == 0)
> do_merge = 1;
> if (do_merge + do_commit + do_abort != 1) {
> error("cannot mix --commit, --abort or -s/--strategy");
> @@ -799,22 +834,7 @@ static int merge(int argc, const char **argv, const char
> *prefix)
> expand_notes_ref(&remote_ref);
> o.remote_ref = remote_ref.buf;
>
> - if (strategy) {
> - if (!strcmp(strategy, "manual"))
> - o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
> - else if (!strcmp(strategy, "ours"))
> - o.strategy = NOTES_MERGE_RESOLVE_OURS;
> - else if (!strcmp(strategy, "theirs"))
> - o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
> - else if (!strcmp(strategy, "union"))
> - o.strategy = NOTES_MERGE_RESOLVE_UNION;
> - else if (!strcmp(strategy, "cat_sort_uniq"))
> - o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> - else {
> - error("Unknown -s/--strategy: %s", strategy);
> - usage_with_options(git_notes_merge_usage, option
Re: [PATCH v3 1/2] worktrees: add die_if_shared_symref
On Fri, Jul 31, 2015 at 5:15 PM, David Turner wrote: > On Fri, 2015-07-31 at 15:35 -0400, Eric Sunshine wrote: >> On Fri, Jul 31, 2015 at 3:01 PM, David Turner >> wrote: >> > Add a new function, die_if_shared_symref, which works like >> > die_if_checked_out, but for all references. Refactor >> > die_if_checked_out to work in terms of die_if_shared_symref. >> > >> > Soon, we will use die_if_shared_symref to protect notes merges in >> > worktrees. >> >> I wonder if the diagnostic: >> >> 'blorp' is already checked out at '/path/name/' >> >> emitted by check_linked_checkout() needs to be adjusted for this >> change. It still makes sense for die_if_checked_out(), but sounds odd >> for die_if_shared_symref(). > > How about: > > 'refs/notes/y' is already referenced from 'NOTES_MERGE_REF' in > '/home/dturner/git/t/trash directory.t3320-notes-merge-worktrees/' > > Does that make sense? That might be the best we can do for the generic case of die_if_shared_symref(), but I wonder how easily the typical user would understand it when trying to checkout a branch already checked out elsewhere. One concern is that that message almost comes across as an internal Git error (thus inscrutable), whereas: % git checkout blorp 'blorp' is already checked out at '/some/path/' seems (to me) pretty clearly a user error, thus more easily understood. > It's not the only place in the error messages that mentions > NOTES_MERGE_REF, so I think we expect users to understand > NOTES_MERGE_REF. The alternative is to move the error handling to an > even higher level so we can give a notes-specific message. I don't > think that's necessary, but I'll do it if others do. Given such precedence, the generic error message might indeed be fine for the notes merge case, however, in general, you'd probably get better and more understandable error messages by tailoring them at the call sites. (That's a positive vote from me for for lifting error handling to a higher level.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] worktrees: add find_shared_symref
On Fri, Jul 31, 2015 at 6:11 PM, David Turner wrote:
> Add a new function, find_shared_symref, which contains the heart of
> die_if_checked_out, but works for all symrefs. Refactor
Slightly more explanatory:
..., but works for any symref, not just HEAD. Refactor
More below.
> die_if_checked_out to use the same infrastructure as
> find_shared_symref.
>
> Soon, we will use find_shared_symref to protect notes merges in
> worktrees.
>
> Signed-off-by: David Turner
> ---
> diff --git a/branch.c b/branch.c
> index c85be07..9b5e3b3 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -349,31 +350,47 @@ static void check_linked_checkout(const char *branch,
> const char *id)
> strbuf_addstr(&gitdir, get_git_common_dir());
> skip_prefix(branch, "refs/heads/", &branch);
> strbuf_strip_suffix(&gitdir, ".git");
> - die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
> + return strbuf_detach(&gitdir, NULL);
By returning here, you're now leaking 'path' and 'sb' (see the 'done'
label below).
> done:
> strbuf_release(&path);
> strbuf_release(&sb);
> strbuf_release(&gitdir);
> +
> + return NULL;
> }
>
> -void die_if_checked_out(const char *branch)
> +char *find_shared_symref(const char *symref, const char *target)
> {
> struct strbuf path = STRBUF_INIT;
> DIR *dir;
> struct dirent *d;
> + char *existing;
>
> - check_linked_checkout(branch, NULL);
> + if ((existing = find_linked_symref(symref, target, NULL)))
> + return existing;
>
> strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
> dir = opendir(path.buf);
> strbuf_release(&path);
> if (!dir)
> - return;
> + return NULL;
>
> while ((d = readdir(dir)) != NULL) {
> if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> continue;
> - check_linked_checkout(branch, d->d_name);
> + if ((existing = find_linked_symref(symref, target,
> d->d_name)))
> + return existing;
By returning here, you're leaking the open 'dir'.
> }
> closedir(dir);
> +
> + return NULL;
> +}
> +
> +void die_if_checked_out(const char *branch)
> +{
> + char *existing;
> +
> + existing = find_shared_symref("HEAD", branch);
> + if (existing)
> + die(_("'%s' is already checked out at '%s'"), branch,
> existing);
> }
> diff --git a/branch.h b/branch.h
> index 58aa45f..0f466c7 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -59,4 +59,12 @@ extern int read_branch_desc(struct strbuf *, const char
> *branch_name);
> */
> extern void die_if_checked_out(const char *branch);
>
> +/*
> + * Check if a per-worktree symref points to a ref in the main worktree
> + * or any linked worktree, and return the path to the exising worktree
> + * if it is. Returns NULL if there is no existing ref. The caller is
> + * responsible for freeing the returned path.
> + */
> +extern char *find_shared_symref(const char *symref, const char *branch);
I think you meant s/branch/target/.
> #endif
> --
> 2.0.4.315.gad8727a-twtrsrc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer
On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak wrote: > On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy > wrote: > Good point! I just was wondering if we need another atom just to print a star. > But your points certainly are valid. I'll implement this. Thanks :) > >>> This would remove the need of making the printing of the "*" to be >>> needed by ref-filter. As this is only needed in branch.c >>> >>> If going on what you're saying we could have a "%(starifcurrent)" atom or >>> something, but I don't see a general use for this. >> >> To have a really customizeable format, you would want to allow e.g. >> >> git branch --format '%(starifcurrent) %(objectname) %(refname)' >> >> if the user wants to get the sha1 before the refname, and still have the >> star in front. It's a bit frustrating to have a hardcoded format that >> the user can't reproduce with the --format option, since it means you >> can't easily make a small variation on it. > > Agreed. will have a "starifcurrent" atom :) Please don't. It's better to avoid these highly specialized solutions and instead seek general purpose ones. For instance, utilizing the %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch) which would allow a format like this: %(if:iscurrentbranch)*%(endif) Even more generic would be an %(ifeq:x:y) conditional and a %(currentbranch) atom: %(ifeq:refname:currentbranch)*%(endif) Those are just a couple ideas. Other variations are possible and likely preferable to the specialized %(starifcurrent). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms
On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin wrote:
> When sending an e-mail, the client and server must
> agree on an authentication mechanism. Some servers
> (due to misconfiguration or a bug) denies valid
s/denies/deny/
> credentials for certain mechanisms. In this patch,
> a new option --smtp-auth and configuration entry
> smtpauth are introduced.
>
> If smtp_auth is defined, it works as a whitelist
> of allowed mechanisms for authentication. There
> are four mechanisms supported: PLAIN, LOGIN,
> CRAM-MD5, DIGEST-MD5. However, their availability
> depends on the installed SASL library.
>
> Signed-off-by: Jan Viktorin
> ---
> git-send-email.perl | 31 ++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
At the very least, you will also want to update the documentation
(Documentation/git-send-email.txt) and, if possible, add new tests
(t/t9001-send-email.sh).
More below.
> diff --git a/git-send-email.perl b/git-send-email.perl
> index ae9f869..b00ed9d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
> return 1;
> }
>
> + # Do not allow arbitrary strings.
Can you explain why this restriction is needed. What are the
consequences of not limiting the input to this "approved" list?
> + my ($filtered_auth) = "";
Style: unnecessary parentheses
> + foreach ("PLAIN", "LOGIN", "CRAM-MD5", "DIGEST-MD5") {
This might read more nicely and be easier to maintain if written as:
foreach (qw/PLAIN LOGIN CRAM-MD5 DIGEST-MD5/) {
> + if($smtp_auth && $smtp_auth =~ /\b\Q$_\E\b/i) {
Style: space after 'if'
Also, why not lift the 'if ($smtp_auth)' check outside the loop since
its value never changes and there's no need to iterate over the list
if $smtp_auth is empty.
> + $filtered_auth .= $_ . " ";
Style question: Would this be more naturally expressed with
'filtered_auth' as an array onto which items are pushed, rather than
as a string? At the point of use, the string can be recreated via
join().
Not a big deal; just wondering.
> + }
> + }
> +
> + die "Invalid SMTP AUTH." if length $smtp_auth && !length
> $filtered_auth;
Style: drop capitalization: "invalid..."
Style: drop period at end
Style: add "\n" at end in order to suppress printing of the
perl line number and input line number which aren't
very meaningful for a user error
(Existing style in the script is not very consistent, but new code
probably should adhere the above suggestions.)
Also, don't you want to warn the user about tokens that don't match
one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than
dropping them silently?
> # Workaround AUTH PLAIN/LOGIN interaction defect
> # with Authen::SASL::Cyrus
> eval {
> @@ -1148,6 +1163,20 @@ sub smtp_auth_maybe {
> 'password' => $smtp_authpass
> }, sub {
> my $cred = shift;
> +
> + if($filtered_auth) {
Style: space after 'if'
> + my $sasl = Authen::SASL->new(
> + mechanism => $filtered_auth,
> + callback => {
> + user => $cred->{'username'},
> + pass => $cred->{'password'},
> + authname => $cred->{'username'},
> + }
> + );
> +
> + return !!$smtp->auth($sasl);
> + }
> +
> return !!$smtp->auth($cred->{'username'},
> $cred->{'password'});
> });
>
> --
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller wrote:
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for the configuration option. Ensure that command line
> --strategy option overrides the configured value. Ensure that -s can't
> be passed with --commit or --abort. Ensure that --abort and --commit
> can't be used together.
>
> Signed-off-by: Jacob Keller
> Cc: Johan Herland
> Cc: Michael Haggerty
> Cc: Eric Sunshine
Thanks, this looks better than the previous round. A few comments below...
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..d8944f5aec60 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,13 +101,13 @@ merge::
> any) into the current notes ref (called "local").
> +
> If conflicts arise and a strategy for automatically resolving
> -conflicting notes (see the -s/--strategy option) is not given,
> -the "manual" resolver is used. This resolver checks out the
> -conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
> -and instructs the user to manually resolve the conflicts there.
> -When done, the user can either finalize the merge with
> -'git notes merge --commit', or abort the merge with
> -'git notes merge --abort'.
> +conflicting notes (see the -s/--strategy option or notes.merge
> +config option) is not given, the "manual" resolver is used.
> +This resolver checks out the conflicting notes in a special
> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user
> +to manually resolve the conflicts there. When done, the user
> +can either finalize the merge with 'git notes merge --commit',
> +or abort the merge with 'git notes merge --abort'.
When you re-wrap the text like this, it's difficult to spot your one
little change in all the diff noise. For the sake of review, it's
often better to minimize the change, even if it leaves the text more
jagged than you'd like.
> remove::
> Remove the notes for given objects (defaults to HEAD). When
> @@ -181,10 +181,10 @@ OPTIONS
> -s ::
> --strategy=::
> When merging notes, resolve notes conflicts using the given
> - strategy. The following strategies are recognized: "manual"
> - (default), "ours", "theirs", "union" and "cat_sort_uniq".
> - See the "NOTES MERGE STRATEGIES" section below for more
> - information on each notes merge strategy.
> + strategy. Overrides "notes.merge". The following strategies are
> + recognized: `manual`, `ours`, `theirs`, `union` and
> + `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
> + for more information on each notes merge strategy.
Ditto.
> --commit::
> Finalize an in-progress 'git notes merge'. Use this option
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 88fdafb32bc0..728980bc79bf 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,6 +743,25 @@ static int merge_commit(struct notes_merge_options *o)
> return ret;
> }
>
> +static int parse_notes_strategy(const char *arg, enum notes_merge_strategy
> *strategy)
> +{
> + if (!strcmp(arg, "manual"))
> + *strategy = NOTES_MERGE_RESOLVE_MANUAL;
> + else if (!strcmp(arg, "ours"))
> + *strategy = NOTES_MERGE_RESOLVE_OURS;
> + else if (!strcmp(arg, "theirs"))
> + *strategy = NOTES_MERGE_RESOLVE_THEIRS;
> + else if (!strcmp(arg, "union"))
> + *strategy = NOTES_MERGE_RESOLVE_UNION;
> + else if (!strcmp(arg, "cat_sort_uniq"))
> + *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> + else {
> + return 1;
In this codebase, it's customary to return 0 on success and -1 on error
> + }
Style: drop unnecessary braces
> +
> + return 0;
> +}
> +
> static int merge(int argc, const char **argv, const char *prefix)
> {
> struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -950,6 +961,18 @@ static int get_ref(int argc, const char **argv, const
> char *prefix)
> return 0;
> }
>
> +static int git_notes_config(const char *var, const char *value, void *cb)
> +{
> + if (!strcmp(var, "notes.merge")) {
> + if (!value)
> + return config_error_nonbool(var);
> + if (parse_notes_st
Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy
On Sun, Aug 02, 2015 at 12:41:08AM -0700, Jacob Keller wrote: > On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine wrote: > >> If conflicts arise and a strategy for automatically resolving > >> -conflicting notes (see the -s/--strategy option) is not given, > >> -the "manual" resolver is used. This resolver checks out the > >> -conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`), > >> -and instructs the user to manually resolve the conflicts there. > >> -When done, the user can either finalize the merge with > >> -'git notes merge --commit', or abort the merge with > >> -'git notes merge --abort'. > >> +conflicting notes (see the -s/--strategy option or notes.merge > >> +config option) is not given, the "manual" resolver is used. > >> +This resolver checks out the conflicting notes in a special > >> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user > >> +to manually resolve the conflicts there. When done, the user > >> +can either finalize the merge with 'git notes merge --commit', > >> +or abort the merge with 'git notes merge --abort'. > > > > When you re-wrap the text like this, it's difficult to spot your one > > little change in all the diff noise. For the sake of review, it's > > often better to minimize the change, even if it leaves the text more > > jagged than you'd like. > > This results in something incredibly jagged. I can't find a good way > to split which minimizes the change. Would a 3rd patch which just > re-flows this be an acceptable alternative > > ie: add the words in one patch then re-flow afterwards in a second > patch with no changes? > > There is no good alternative as other re-flows I tried end up looking > way too jagged, as compared to surrounding documentation. Don't worry too much about it. Consider it something to keep in mind for future patches. I reviewed the change and it seemed okay. I mentioned it because one of the goals of patch submission, in addition to making an actual change, is to ease the review process. If Junio is okay with accepting it as is, then it's probably not worth spending more time trying to refine it. Having said that, I came up with the following for those two paragraphs, which gives a much less noisy diff and doesn't look too jagged: --- 8< --- If conflicts arise and a strategy for automatically resolving -conflicting notes (see the -s/--strategy option) is not given, -the "manual" resolver is used. This resolver checks out the +conflicting notes (see -s/--strategy or notes.merge configuration) is +not given, the "manual" resolver is used. This resolver checks out the conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user to manually resolve the conflicts there. When done, the user can either finalize the merge with --- 8< --- ...and... --- 8< --- When merging notes, resolve notes conflicts using the given - strategy. The following strategies are recognized: "manual" + strategy. Overrides the "notes.merge" configuration variable. + The following strategies are recognized: "manual" (default), "ours", "theirs", "union" and "cat_sort_uniq". See the "NOTES MERGE STRATEGIES" section below for more information on each notes merge strategy. --- 8< --- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] send-email: provide whitelist of SMTP AUTH mechanisms
On Sat, Aug 1, 2015 at 2:19 PM, Jan Viktorin wrote:
> On Sat, 1 Aug 2015 05:33:28 -0400 Eric Sunshine
> wrote:
>> On Fri, Jul 31, 2015 at 7:33 PM, Jan Viktorin
>> wrote:
>> At the very least, you will also want to update the documentation
>> (Documentation/git-send-email.txt) and, if possible, add new tests
>> (t/t9001-send-email.sh).
>
> I will update the documentation when it is clear, how the smtp-auth
> works.
>
> I have no idea, how to test the feature. I can see something like
> fake.sendmail in the file. How does it work? I can image a test whether
> user inserts valid values. What more?
That's what I was thinking. You could test if the die() is triggered
or if it emits warnings for bad values (assuming you implement that
feature). As for testing the actual authentication, I'm not sure you
can (and don't see any such testing in the script).
>> > diff --git a/git-send-email.perl b/git-send-email.perl
>> > index ae9f869..b00ed9d 100755
>> > --- a/git-send-email.perl
>> > +++ b/git-send-email.perl
>> > @@ -1129,6 +1134,16 @@ sub smtp_auth_maybe {
>> > return 1;
>> > }
>> >
>> > + # Do not allow arbitrary strings.
>>
>> Can you explain why this restriction is needed. What are the
>> consequences of not limiting the input to this "approved" list?
>
> This is more a check of an arbitrary user input then a check
> of an "approved list". It should be also used to inform user
> about invalid methods (however, I didn't implemented it yet).
What I was really asking was whether this sort of checking really
belongs in git-send-email or if it is better left to Net::SMTP (and
Authen::SASL) to do so since they are in better positions to know what
is valid and what is not. If the Perl module(s) generate suitable
diagnostics for bad input, then it makes sense to leave the checking
to them. If not, then I can understand your motivation for
git-send-email doing the checking instead in order to emit
user-friendly diagnostics.
So, that's what I meant when I asked 'What are the consequences of not
limiting the input to this "approved" list?'.
The other reason I asked was that it increases maintenance costs for
us to maintain a list of "approved" mechanisms, since the list needs
to be updated when new ones are implemented (and, as brian pointed
out, some may already exist which are not in your list).
>> > + $filtered_auth .= $_ . " ";
>>
>> Style question: Would this be more naturally expressed with
>> 'filtered_auth' as an array onto which items are pushed, rather than
>> as a string? At the point of use, the string can be recreated via
>> join().
>>
>> Not a big deal; just wondering.
>
> I am not a Perl programmer. Yesterday, I've discovered for the first
> time that Perl uses a dot for concatenation... I have no idea what
> happens when passing an array to Authen::SASL->new(). Moreover, the
> Perl arrays syntax rules scare me a bit ;).
You wouldn't pass the array to Authen::SASL, instead you would use
join() to transform the array back into a space-separated string. It's
probably moot (since you probably shouldn't be doing the filtering
manually), but the code would look something like this:
my @filtered_auth;
...
foreach (...) {
if (...) {
push @filtered_auth, $_;
}
}
...
if (@filtered_auth) {
my $sasl = Authen::SASL->new(
mechanism => join(' ', @filtered_auth),
...
>> Style: add "\n" at end in order to suppress printing of the
>> perl line number and input line number which aren't
>> very meaningful for a user error
>
> Another hidden Perl suprise, I guess...
Yes.
>> Also, don't you want to warn the user about tokens that don't match
>> one of the accepted (PLAIN, LOGIN, CRAM-MD5, DIGEST-MD5), rather than
>> dropping them silently?
>
> Yes, this would be great (as I've already mentioned). It's a question
> whether to include the check for the mechanisms or whether to leave
> the $smtp_auth variable as it is... Maybe just validate by a regex?
>
> The naming rules are defiend here:
> https://tools.ietf.org/html/rfc4422#page-8
> So, this looks to me as a better way.
Maybe. This leads back to my original question of whether it's really
git-send-email's responsibility to do validation or if that can be
left to Net::SMTP/Authen::SASL. If the Perl module(s) emit suitable
diagnostics for bad input, then validation can be omitted from
git-send-email.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin wrote:
> When sending an e-mail, the client and server must
> agree on an authentication mechanism. Some servers
> (due to misconfiguration or a bug) deny valid
> credentials for certain mechanisms. In this patch,
> a new option --smtp-auth and configuration entry
> smtpauth are introduced. If smtp_auth is defined,
> it works as a whitelist of allowed mechanisms for
> authentication selected from the ones supported by
> the installed SASL perl library.
Nit: This would read a bit more nicely if wrapped to 70-72 columns.
> Signed-off-by: Jan Viktorin
> ---
> diff --git a/Documentation/git-send-email.txt
> b/Documentation/git-send-email.txt
> index 7ae467b..c237c80 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -171,6 +171,14 @@ Sending
> +--smtp-auth=::
> + Specify allowed SMTP-AUTH mechanisms. This setting forces using only
> + the listed mechanisms. Separate allowed mechanisms by a whitespace.
Perhaps:
Whitespace-separated list of allowed SMTP-AUTH mechanisms.
> + Example: PLAIN LOGIN GSSAPI. If at least one of the specified
> mechanisms
> + matchs those advertised by the SMTP server and it is supported by the
> SASL
s/matchs/matches/
> + library we use, it is used for authentication. If neither of
> 'sendemail.smtpAuth'
> + or '--smtp-auth' is specified, all mechanisms supported on client can
> be used.
s/neither of/neither/
s/or/nor/
> diff --git a/git-send-email.perl b/git-send-email.perl
> index ae9f869..ebc1e90 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -75,6 +75,9 @@ git send-email [options] options >
> Pass an empty string to disable
> certificate
> verification.
> --smtp-domain * The domain name sent to HELO/EHLO
> handshake
> +--smtp-auth * Space separated list of allowed AUTH
> methods.
s/Space separated/Space-separated/
> + This setting forces to use one of the
> listed methods.
> + Supported: PLAIN LOGIN CRAM-MD5
> DIGEST-MD5.
Since you're no longer checking explicitly for these mechanisms, you
probably want to drop the "Supported:" line.
> --smtp-debug<0|1> * Disable, enable Net::SMTP debug.
>
>Automating:
> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {
> Authen::SASL->import(qw(Perl));
> };
>
> + if($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) {
> + die "invalid smtp auth: '${smtp_auth}'";
> + }
Style: space after 'if'
> # TODO: Authentication may fail not because credentials were
> # invalid but due to other reasons, in which we should not
> # reject credentials.
> @@ -1148,6 +1157,20 @@ sub smtp_auth_maybe {
> 'password' => $smtp_authpass
> }, sub {
> my $cred = shift;
> +
> + if($smtp_auth) {
Style: space after 'if'
> + my $sasl = Authen::SASL->new(
> + mechanism => $smtp_auth,
> + callback => {
> + user => $cred->{'username'},
> + pass => $cred->{'password'},
> + authname => $cred->{'username'},
> + }
> + );
> +
> + return !!$smtp->auth($sasl);
> + }
> +
> return !!$smtp->auth($cred->{'username'},
> $cred->{'password'});
> });
>
> --
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Aug 2015, #01; Mon, 3)
On Mon, Aug 3, 2015 at 6:18 PM, Junio C Hamano wrote: > I've kicked a few topics out of 'next' back to 'pu' for now. > - I think es/worktree-add and es/worktree-add-cleanup are good >shape overall, but we probably would want to make them into a >single topic. Is there anything I need to do to move this along, or is it something you will be handling locally? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] builtin/mv: Get rid of the last caller of get_pathspec
On Mon, Aug 3, 2015 at 1:53 PM, Stefan Beller wrote: > `get_pathspec` is deprecated and builtin/mv is its last caller, so getting > rid of `get_pathspec` is rather easy. By getting rid of `get_pathspec`, > the documentation such as 'technical/api-setup.txt' becomes easier to read > as the reader doesn't need to bear with the additional fact that > `get_pathspec` is deprecated. > > The code in 'builtin/mv' still requires some work to make it less ugly. Perhaps split this into two patches? One to free git-mv of get_pathspec() dependence, and the second to actually retire get_pathspec(). > Signed-off-by: Stefan Beller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] worktrees: add find_shared_symref
On Mon, Aug 3, 2015 at 2:48 PM, David Turner wrote:
> Add a new function, find_shared_symref, which contains the heart of
> die_if_checked_out, but works for any symref, not just HEAD. Refactor
> die_if_checked_out to use the same infrastructure as
> find_shared_symref.
>
> Soon, we will use find_shared_symref to protect notes merges in
> worktrees.
A couple comments below. The first may be worth a re-roll; the second
is more a taste thing. Neither is blocking if you don't happen to
re-roll.
> Signed-off-by: David Turner
> ---
> diff --git a/branch.c b/branch.c
> index c85be07..d2b3586 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -349,31 +350,53 @@ static void check_linked_checkout(const char *branch,
> const char *id)
> strbuf_addstr(&gitdir, get_git_common_dir());
> skip_prefix(branch, "refs/heads/", &branch);
> strbuf_strip_suffix(&gitdir, ".git");
> - die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
> +
> + strbuf_release(&path);
> + strbuf_release(&sb);
> + return strbuf_detach(&gitdir, NULL);
> done:
> strbuf_release(&path);
> strbuf_release(&sb);
> strbuf_release(&gitdir);
> +
> + return NULL;
> }
This would be cleaner and less redundant if you assign the existing
location to a variable and just fall through to the 'done' label:
char *existing = NULL;
...
skip_prefix(branch, "refs/heads/", &branch);
strbuf_strip_suffix(&gitdir, ".git");
existing = strbuf_detach(&gitdir, NULL);
done:
strbuf_release(&path);
strbuf_release(&sb);
strbuf_release(&gitdir);
return existing;
There's no worry that the "existing" path will be clobbered by
strbuf_release(&gitdir) since it's been detached already (and it's
safe to release the strbuf without affecting what has been detached
from it).
> -void die_if_checked_out(const char *branch)
> +char *find_shared_symref(const char *symref, const char *target)
> {
> struct strbuf path = STRBUF_INIT;
> DIR *dir;
> struct dirent *d;
> + char *existing;
>
> - check_linked_checkout(branch, NULL);
> + if ((existing = find_linked_symref(symref, target, NULL)))
> + return existing;
>
> strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
> dir = opendir(path.buf);
> strbuf_release(&path);
> if (!dir)
> - return;
> + return NULL;
>
> while ((d = readdir(dir)) != NULL) {
> if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> continue;
> - check_linked_checkout(branch, d->d_name);
> + existing = find_linked_symref(symref, target, d->d_name);
> + if (existing) {
> + closedir(dir);
> + return existing;
For consistency with code nearby, this could have been handled by
adding a 'done' label above the closedir() below and jumping to it
from here, and then 'return existing'.
> + }
> }
> closedir(dir);
> +
> + return NULL;
> +}
> +
> +void die_if_checked_out(const char *branch)
> +{
> + char *existing;
> +
> + existing = find_shared_symref("HEAD", branch);
> + if (existing)
> + die(_("'%s' is already checked out at '%s'"), branch,
> existing);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] remote: add get-url subcommand
On Mon, Aug 3, 2015 at 5:00 PM, Ben Boeckel wrote:
> Expanding `insteadOf` is a part of ls-remote --url and there is no way
> to expand `pushInsteadOf` as well. Add a get-url subcommand to be able
> to query both as well as a way to get all configured urls.
>
> Signed-off-by: Ben Boeckel
> ---
> diff --git a/builtin/remote.c b/builtin/remote.c
> index f4a6ec9..9278a83 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1497,6 +1503,53 @@ static int set_branches(int argc, const char **argv)
> +static int get_url(int argc, const char **argv)
> +{
> + int i, push_mode = 0, all_mode = 0;
> + const char *remotename = NULL;
> + struct remote *remote;
> + const char **url;
> + int url_nr;
> + struct option options[] = {
> + OPT_BOOL('\0', "push", &push_mode,
> +N_("query push URLs")),
A bit more explanatory:
"query push URLs rather than fetch URLs"
> + OPT_BOOL('\0', "all", &all_mode,
> +N_("return all URLs")),
> + OPT_END()
> + };
> + argc = parse_options(argc, argv, NULL, options,
> builtin_remote_geturl_usage,
> +PARSE_OPT_KEEP_ARGV0);
What is the reason for PARSE_OPT_KEEP_ARGV0 in this case?
> + if (argc < 1 || argc > 2)
> + usage_with_options(builtin_remote_geturl_usage, options);
So, 'argc' must be 1 or 2, which in 'argv' terms is argv[0] and argv[1]).
> + remotename = argv[1];
But here, argv[1] is accessed unconditionally, even though 'argc' may
have been 1, thus out of bounds.
> + if (!remote_is_configured(remotename))
> + die(_("No such remote '%s'"), remotename);
> + remote = remote_get(remotename);
> +
> + if (push_mode) {
> + url = remote->pushurl;
> + url_nr = remote->pushurl_nr;
> + } else {
> + url = remote->url;
> + url_nr = remote->url_nr;
> + }
> +
> + if (!url_nr)
> + die(_("no URLs configured for remote '%s'"), remotename);
> +
> + if (all_mode) {
> + for (i = 0; i < url_nr; i++)
> + printf_ln("%s", url[i]);
> + } else {
> + printf_ln("%s", *url);
> + }
> +
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] remote: add get-url subcommand
On Mon, Aug 3, 2015 at 8:16 PM, Ben Boeckel wrote: > On Mon, Aug 03, 2015 at 19:38:15 -0400, Eric Sunshine wrote: >> On Mon, Aug 3, 2015 at 5:00 PM, Ben Boeckel wrote: >> > + argc = parse_options(argc, argv, NULL, options, >> > builtin_remote_geturl_usage, >> > +PARSE_OPT_KEEP_ARGV0); >> >> What is the reason for PARSE_OPT_KEEP_ARGV0 in this case? > > Copied from get-url; I presume for more natural argv[] usage within the > function. > >> > + if (argc < 1 || argc > 2) >> > + usage_with_options(builtin_remote_geturl_usage, options); >> >> So, 'argc' must be 1 or 2, which in 'argv' terms is argv[0] and argv[1]). >> >> > + remotename = argv[1]; >> >> But here, argv[1] is accessed unconditionally, even though 'argc' may >> have been 1, thus out of bounds. > > Yep, should be (argc < 2 || argc > 2) (or 1 if PARSE_OPT_KEEP_ARGV0 is > removed). Off-by-one when converting from get-url. Or, expressed more naturally: if (argc != 1) usage_with_options(...); assuming the unnecessary PARSE_OPT_KEEP_ARGV0 is dropped. > I'll reroll tomorrow morning in case there are more comments until then > (particularly about PARSE_OPT_KEEP_ARGV0). This new code doesn't take advantage of it, and it's very rarely used in Git itself, thus its use here is a potential source of confusion, so it's probably best to drop it. (The same could be said for set_url(), but that's a separate topic.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Documentation/git-worktree: fix reference to 'locked' file
On Tue, Aug 4, 2015 at 8:27 AM, Patrick Steinhardt wrote: > The documentation of git-worktree refers to the 'locked' file as > 'lock'. Fix this to say 'locked' instead. Thanks for the patch. This is already fixed in 'next' by 2e73ab6 (Documentation/git-worktree: fix incorrect reference to file "locked", 2015-07-20) > Signed-off-by: Patrick Steinhardt > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 566ca92..3fedd9e 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -39,7 +39,7 @@ repository so that they do not get automatically pruned. > > If a linked working tree is stored on a portable device or network share > which is not always mounted, you can prevent its administrative files from > -being pruned by creating a file named 'lock' alongside the other > +being pruned by creating a file named 'locked' alongside the other > administrative files, optionally containing a plain text reason that > pruning should be suppressed. See section "DETAILS" for more information. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Documentation/git-worktree: fix duplicated 'from'
On Tue, Aug 4, 2015 at 8:27 AM, Patrick Steinhardt wrote: > Signed-off-by: Patrick Steinhardt > --- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 3387e2f..566ca92 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -124,7 +124,7 @@ thumb is do not make any assumption about whether a path > belongs to > $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something > inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path. > > -To prevent a $GIT_DIR/worktrees entry from from being pruned (which > +To prevent a $GIT_DIR/worktrees entry from being pruned (which Thanks. I vaguely recall spotting this repetition when preparing to move this chunk of text from git-checkout.txt to git-worktree.txt[1], and planned on fixing it in a follow-on patch (such as [2,3,4,5]), but forgot about it. For what it's worth (though certainly not necessary for such an obviously correct path): Acked-by: Eric Sunshine [1]: 93a3649 (Documentation: move linked worktree description from checkout to worktree, 2015-07-06) [2]: 6d3824c (Documentation/git-worktree: add BUGS section, 2015-07-06) [3]: af189b4 (Documentation/git-worktree: split technical info from general description, 2015-07-06) [4]: a8ba5dd (Documentation/git-worktree: add high-level 'lock' overview, 2015-07-06) [5]: 9645459 (Documentation/git-worktree: add EXAMPLES section, 2015-07-06) > can be useful in some situations, such as when the > entry's working tree is stored on a portable device), add a file named > 'locked' to the entry's directory. The file contains the reason in > -- > 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/6] clone: add tests for cloning with empty path
On Tue, Aug 4, 2015 at 7:29 AM, Patrick Steinhardt wrote:
> Test behavior of `git clone` when working with an empty path
> component. This may be the case when cloning a file system's root
> directory or from a remote server's root.
A few minor, mostly style-related, comments below...
> Signed-off-by: Patrick Steinhardt
> ---
> diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-worktree.sh
> index 553a3f6..d521ca3 100755
> --- a/t/t1509-root-worktree.sh
> +++ b/t/t1509-root-worktree.sh
> @@ -237,6 +237,49 @@ test_foobar_foobar
>
> test_expect_success 'cleanup' 'rm -rf /.git'
>
> +say "clone .git at root without reponame"
> +
> +test_expect_success 'go to /' 'cd /'
> +test_expect_success 'setup' '
> + echo "Initialized empty Git repository in /.git/" > expected &&
> + git init > result &&
> + test_cmp expected result
> +'
> +
> +test_clone_expect_dir() {
> + URL="$1"
> + DIR="$2"
It would be nice for the &&-chain to be intact for these two lines, as
well, since you never know where someone may insert code in the
future. If code is inserted above or between these lines, and the code
fails, its failure will go unnoticed due to the broken &&-chain.
> + cat <<-EOF >expected &&
In this codebase, it's customary to say:
cat >expected <<-EOF &&
> + Cloning into '$DIR'...
> + warning: You appear to have cloned an empty repository.
> + EOF
In this codebase, it's customary to place the content and EOF at the
same indentation level as the opening 'cat < + git clone "$URL" >result 2>&1 &&
> + rm -rf "$DIR" &&
> + test_cmp expected result
> +}
> +
> +test_expect_success 'go to /clones' 'mkdir /clones && cd /clones'
> +test_expect_success 'simple clone of /' '
> + cat <<-EOF >expected &&
Since you don't expect any variable interpolation in this case, you
can telegraph that intent more clearly via:
cat >expected <<-\EOF &&
> + fatal: No directory name could be guessed.
> + Please specify a directory on the command line
> + EOF
> + test_expect_code 128 git clone / >result 2>&1 &&
> + test_cmp expected result'
> +
> +test_expect_success 'clone with file://host/' '
> + test_clone_expect_dir file://127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user@host/' '
> + test_clone_expect_dir file://user@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@host/' '
> + test_clone_expect_dir file://user:password@127.0.0.1/ 127.0.0.1'
> +test_expect_success 'clone with file://host:port/' '
> + test_clone_expect_dir file://127.0.0.1:/ 127.0.0.1'
> +test_expect_success 'clone with file://user:password@host:port' '
> + test_clone_expect_dir file://user:password@127.0.0.1:/ 127.0.0.1'
> +
> +test_expect_success 'cleanup' 'rm -rf /.git /clones'
> +
> say "auto bare gitdir"
>
> # DESTROY!
> --
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] submodule: implement `module_name` as a builtin helper
On Tue, Aug 4, 2015 at 8:04 PM, Stefan Beller wrote:
> The goal of this series being rewriting `git submodule update`,
> we don't want to call out to the shell script for config lookups.
>
> So reimplement the lookup of the submodule name in C.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cb18ddf..dd5635f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -98,6 +100,48 @@ static int module_list(int argc, const char **argv, const
> char *prefix)
> +static int collect_module_names(const char *key, const char *value, void *cb)
> +{
> + size_t len;
> + struct string_list *sl = cb;
> +
> + if (starts_with(key, "submodule.")
> + && strip_suffix(key, ".path", &len)) {
> + struct strbuf sb = STRBUF_INIT;
> + strbuf_add(&sb, key + strlen("submodule."),
> + len - strlen("submodule."));
> + string_list_insert(sl, value)->util = strbuf_detach(&sb,
> NULL);
> + strbuf_release(&sb);
Why the complexity and overhead of a strbuf when the same could be
accomplished more easily and straightforwardly with xstrndup()?
> + }
> +
> + return 0;
> +}
> +
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> + struct string_list_item *item;
> + struct git_config_source config_source;
> + struct string_list values = STRING_LIST_INIT_DUP;
> +
> + if (!argc)
Do you mean?
if (argc != 1)
> + usage("git submodule--helper module_name \n");
> +
> + memset(&config_source, 0, sizeof(config_source));
> + config_source.file = ".gitmodules";
> +
> + if (git_config_with_options(collect_module_names, &values,
> + &config_source, 1) < 0)
> + die(_("unknown error occured while reading the git modules
> file"));
> +
> + item = string_list_lookup(&values, argv[0]);
> + if (item)
> + printf("%s\n", (char*)item->util);
> + else
> + die("No submodule mapping found in .gitmodules for path
> '%s'", argv[0]);
> + return 0;
> +}
> +
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> {
> if (argc < 2)
> @@ -106,6 +150,9 @@ int cmd_submodule__helper(int argc, const char **argv,
> const char *prefix)
> if (!strcmp(argv[1], "module_list"))
> return module_list(argc - 1, argv + 1, prefix);
>
> + if (!strcmp(argv[1], "module_name"))
> + return module_name(argc - 2, argv + 2, prefix);
> +
> usage:
> usage("git submodule--helper module_list\n");
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error pushing new branch: value too great for base (error token is...
On Wed, Aug 5, 2015 at 1:32 PM, Gaurav Chhabra wrote: > I had written the following code to check whether a push is for branch > deletion: > > #!/bin/bash > > NULL="" > if [[ "$new_sha" -eq "$NULL" ]]; then # Line 17 > remote: Stdin: [] > [9226289d2416af4cb7365d7aaa5e382bdb3d9a89] [refs/heads/rel-a] > remote: > remote: hooks/pre-receive: line 17: [[: > 9226289d2416af4cb7365d7aaa5e382bdb3d9a89: value too great for base > (error token is "922628 > 9d2416af4cb7365d7aaa5e382bdb3d9a89") > > Although the new branch gets pushed to remote but i'm not sure why i'm > getting this error and how can i fix it. I checked online and i get > few links where folks had similar issue but in each such case, the > error token was 08 or 09. I still tried the suggestion of using "10#" > in front of my $new_sha variable but to no avail. > > Any suggestions? Yes, try using the string comparison '=' operator rather than the numeric comparison operator '-eq'. if [[ "$new_sha" = "$NULL" ]]; then -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] contrib: teach completion about git-worktree options and arguments
On Thu, Jul 23, 2015 at 4:49 PM, Eric Sunshine wrote:
> Complete subcommands 'add' and 'prune', as well as their respective
> options --force, --detach, --dry-run, --verbose, and --expire. Also
> complete 'refname' in "git worktree add [-b ]
> ".
Ping[1]?
[1]: http://article.gmane.org/gmane.comp.version-control.git/274526
> Signed-off-by: Eric Sunshine
> ---
>
> This is RFC since this is my first foray into the Git completion script,
> and there are likely better and more idiomatic ways to accomplish this.
>
> contrib/completion/git-completion.bash | 32
> 1 file changed, 32 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index c97c648..07c34ef 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2564,6 +2564,38 @@ _git_whatchanged ()
> _git_log
> }
>
> +_git_worktree ()
> +{
> + local subcommands='add prune'
> + local subcommand="$(__git_find_on_cmdline "$subcommands")"
> + local c=2 n=0 refpos=2
> + if [ -z "$subcommand" ]; then
> + __gitcomp "$subcommands"
> + else
> + case "$subcommand,$cur" in
> + add,--*)
> + __gitcomp "--force --detach"
> + ;;
> + add,*)
> + while [ $c -lt $cword ]; do
> + case "${words[c]}" in
> + --*) ;;
> + -[bB]) ((refpos++)) ;;
> + *) ((n++)) ;;
> + esac
> + ((c++))
> + done
> + if [ $n -eq $refpos ]; then
> + __gitcomp_nl "$(__git_refs)"
> + fi
> + ;;
> + prune,--*)
> + __gitcomp "--dry-run --verbose --expire"
> + ;;
> + esac
> + fi
> +}
> +
> __git_main ()
> {
> local i c=1 command __git_dir
> --
> 2.5.0.rc3.407.g68aafd0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] argv_array: add argv_array_copy
On Thu, Aug 6, 2015 at 1:35 PM, Stefan Beller wrote:
> The copied argv array shall be an identical deep copy except for
> the internal allocation value.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/argv-array.c b/argv-array.c
> index 256741d..6d9c1dd 100644
> --- a/argv-array.c
> +++ b/argv-array.c
> @@ -68,3 +68,16 @@ void argv_array_clear(struct argv_array *array)
> +void argv_array_copy(struct argv_array *src, struct argv_array *dst)
'src' should be 'const'.
Typical Unix argument order has 'dst' first and 'src' second, i.e
strcpy(dst, src). Is it worth deviating from that precedent?
> +{
> + int i;
> +
> + dst->argv = xmalloc((src->argc + 1) * sizeof(*dst->argv));
What happens if 'dst' already has content? Isn't that being leaked
here? At the very least, don't you want to argv_array_clear(dst)?
> + dst->argc = src->argc;
> + dst->alloc = src->argc;
This is wrong, of course. The number allocated is actually argc+1, not argc.
> + for (i = 0; i < dst->argc ; i++)
While it's not wrong per se to use dst->argc as the terminating
condition, it is potentially misleading and confusing. Instead using
src->argc as the terminating condition will better telegraph that the
copy process is indeed predicated upon 'src'.
> + dst->argv[i] = xstrdup(src->argv[i]);
> + dst->argv[dst->argc] = NULL;
It's not clear why you want to hand-code the low-level functionality
again (such as array allocation and string duplication), risking (and
indeed making) errors in the process, when you could instead re-use
existing argv_array code. I would have expected to see
argv_array_copy() implemented as:
argv_array_clear(dst);
for (i = 0; i < src->argc; i++)
argv_array_push(dst, src->argv[i]);
which provides far fewer opportunities for errors to creep in.
Moreover, this function might be too special-purpose. That is, why
does it need to overwrite 'dst'? Can't you achieve the same
functionality by merely appending to 'dst', and leave it up to the
caller to decide whether 'dst' should be cleared beforehand or not? If
so, then you can drop the argv_array_clear(dst) from the above.
However, that begs the question: Why do you need argv_array_copy() at
all? Isn't the same functionality already provided by
argv_array_pushv()? To wit, a caller which wants to copy from 'src' to
'dst' can already do:
struct argv_array src = ...;
struct argv_array dst = ARGV_ARRAY_INIT;
argv_array_pushv(&dst, src->argv);
> +}
> +
> diff --git a/argv-array.h b/argv-array.h
> index c65e6e8..247627da 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -19,5 +19,6 @@ LAST_ARG_MUST_BE_NULL
> void argv_array_pushl(struct argv_array *, ...);
> void argv_array_pop(struct argv_array *);
> void argv_array_clear(struct argv_array *);
> +void argv_array_copy(struct argv_array *src, struct argv_array *dst);
>
> #endif /* ARGV_ARRAY_H */
> --
> 2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] builtin/mv: remove get_pathspec()
On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller wrote:
> builtin/mv: remove get_pathspec()
Misleading. Perhaps rephrase as:
mv: drop dependency upon deprecated get_pathspec
> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so
> reimplement `get_pathspec` literally in builtin/mv.c
Curious. Since this is just moving code around, rather than doing the
actual work to complete the final step as stated by the NEEDSWORK
comment, isn't it just moving the "problem" from one location to
another? Is it worth the code churn?
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/mv.c b/builtin/mv.c
> index d1d4316..99e9b3c 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -10,6 +10,7 @@
> #include "string-list.h"
> #include "parse-options.h"
> #include "submodule.h"
> +#include "pathspec.h"
>
> static const char * const builtin_mv_usage[] = {
> N_("git mv [] ... "),
> @@ -20,13 +21,16 @@ static const char * const builtin_mv_usage[] = {
> #define KEEP_TRAILING_SLASH 2
>
> static const char **internal_copy_pathspec(const char *prefix,
> - const char **pathspec,
> + const char **argv,
What is this change about? It doesn't seem to be related to anything
else in the patch or to its stated purpose, and makes the argument's
purpose less clear, so it's not obvious why it is a good change.
>int count, unsigned flags)
> {
> int i;
> + struct pathspec ps;
> const char **result = xmalloc((count + 1) * sizeof(const char *));
> - memcpy(result, pathspec, count * sizeof(const char *));
> + memcpy(result, argv, count * sizeof(const char *));
> result[count] = NULL;
> +
> + /* NEEDSWORK: Move these preprocessing steps into parse_pathspec */
> for (i = 0; i < count; i++) {
> int length = strlen(result[i]);
> int to_copy = length;
> @@ -42,7 +46,13 @@ static const char **internal_copy_pathspec(const char
> *prefix,
> result[i] = it;
> }
> }
> - return get_pathspec(prefix, result);
> +
> + parse_pathspec(&ps,
> + PATHSPEC_ALL_MAGIC &
> + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> + PATHSPEC_PREFER_CWD,
> + prefix, result);
> + return ps._raw;
> }
>
> static const char *add_slash(const char *path)
> --
> 2.5.0.239.g9728e1d.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] builtin/mv: remove get_pathspec()
On Thu, Aug 6, 2015 at 2:58 PM, Stefan Beller wrote: > On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine > wrote: >> On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller wrote: >>> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so >>> reimplement `get_pathspec` literally in builtin/mv.c >> >> Curious. Since this is just moving code around, rather than doing the >> actual work to complete the final step as stated by the NEEDSWORK >> comment, isn't it just moving the "problem" from one location to >> another? Is it worth the code churn? > > Yeah it is moving around the problem a bit. And the code churn is > unfortunate. Though when I was reading the documentation on > pathspecs, literally the first sentence was "Do not use get_pathspec, > it is out dated". And that was a sad taste for reading documentation. By loudly warning you about deprecation and, more importantly, pointing you at the accepted alternative, this documentation saves you from wasting time (both time spent reading and time spent going down a dead-end path). It would be a "sad taste" if it warned you quietly only at the end of the documentation or not at all. > It's ok to have such warnings in the docs, but as the first sentence as if > there was nothing more important than avoiding the out dated stuff? >From a documentation standpoint, there is nothing more important than warning you to avoid it since it is outdated and likely to go away in the future. > I mean I want to understand the actual code and how I can use it, right? No. It's deprecated and not meant for your use. It's a different matter if you want to understand what the function does because you've encountered a call in existing code, but that case is covered by the existing documentation still being intact (that is, it wasn't removed when the deprecation notice was added). > And there are different approaches to solving the problem. > I could have just reworded or even just rearranged the documentation. The documentation seems fine as-is. > The approach I take here includes a bit of code churn, but it moves the > problematic pieces all in one spot. Indeed, I had the "localizing the problem to one spot" argument in mind, and even wrote it as an answer to my own question, but deleted it before hitting "Send". The counterargument (aside from code churn) is, that by leaving it alone, it serves as a good reminder of the "problem" and is more likely to get noticed and (perhaps) fixed by someone than if it is hidden away in builtin/mv.c. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty
On Tue, Aug 4, 2015 at 10:08 AM, Paul Tan wrote: > When resuming, git-am detects if we are trying to feed it patches or not > by checking if stdin is a TTY. > > However, the test library redirects stdin to /dev/null. This makes it > difficult, for instance, to test the behavior of "git am -3" when > resuming, as git-am will think we are trying to feed it patches and > error out. > > Support this use case by extending test-terminal.perl to create a > pseudo-tty for the child process' standard input as well. An alternative would be to have git-am detect that it is being tested and pretend that isatty() returns true. There is some precedent for having core functionality recognize that it is being tested. See, for instance, environment variable TEST_DATE_NOW, and rev-list --test-bitmap. Doing so would allow the tests work on non-Unix platforms, as well. > Note that due to the way the code is structured, the child's stdin > pseudo-tty will be closed when we finish reading from our stdin. This > means that in the common case, where our stdin is attached to /dev/null, > the child's stdin pseudo-tty will be closed immediately. Some operations > like isatty(), which git-am uses, require the file descriptor to be > open, and hence if the success of the command depends on such functions, > test_terminal's stdin should be redirected to a source with large amount > of data to ensure that the child's stdin is not closed, e.g. > > test_terminal git am --3way > Signed-off-by: Paul Tan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 01/11] ref-filter: print output to strbuf for formatting
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
> Introduce a strbuf `output` which will act as a substitute rather than
> printing directly to stdout. This will be used for formatting
> eventually.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 46963a5..91482c9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1278,9 +1275,12 @@ void show_ref_array_item(struct ref_array_item *info,
> const char *format, int qu
> if (color_parse("reset", color) < 0)
> die("BUG: couldn't parse 'reset' as a color");
> resetv.s = color;
> - print_value(&resetv, quote_style);
> + print_value(&resetv, quote_style, &output);
> }
> + for (i = 0; i < output.len; i++)
> + printf("%c", output.buf[i]);
Everything up to this point seems straightforward, however, it's not
clear why you need to emit 'output' one character at a time. Is it
because it might contain a NUL '\0' character and therefore you can't
use the simpler printf("%s", output.buf)?
If that's the case, then why not just use fwrite() to emit it all at once?
fwrite(output.buf, output.len, 1, stdout);
> putchar('\n');
> + strbuf_release(&output);
> }
>
> /* If no sorting option is given, use refname to sort as default */
> --
> 2.5.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy
On Sun, Aug 2, 2015 at 6:10 AM, Jacob Keller wrote:
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for use of the configuration option. Include a test to ensure
> that --strategy correctly overrides the configured setting.
>
> Signed-off-by: Jacob Keller
> ---
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..9c4f8536182f 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,7 +101,7 @@ merge::
> If conflicts arise and a strategy for automatically resolving
> -conflicting notes (see the -s/--strategy option) is not given,
> +conflicting notes (see the "NOTES MERGE STRATEGIES" section) is not given,
> the "manual" resolver is used. This resolver checks out the
> conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
> and instructs the user to manually resolve the conflicts there.
> @@ -183,6 +183,7 @@ OPTIONS
> When merging notes, resolve notes conflicts using the given
> strategy. The following strategies are recognized: "manual"
> (default), "ours", "theirs", "union" and "cat_sort_uniq".
> + This option overrides the "notes.merge" configuration setting.
> See the "NOTES MERGE STRATEGIES" section below for more
> information on each notes merge strategy.
These two documentation updates are much easier to digest than the
noisy-diff versions of the previous attempt; and the patch overall is
a more pleasant read than v1.
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc55439..de0caa00df1b 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const
> char *prefix)
> return 0;
> }
>
> +static int git_notes_config(const char *var, const char *value, void *cb)
> +{
> + if (!strcmp(var, "notes.merge")) {
> + if (!value)
> + return config_error_nonbool(var);
> + if (parse_notes_strategy(value, &merge_strategy))
> + return error("Unknown notes merge strategy: %s",
> value);
> + else
> + return 0;
A purely subjective stylistic suggestion, which you can freely ignore
if your preference differs:
if (!value)
return ...;
if (parse_notes_strategy(...))
return ...;
return 0;
> + }
> +
> + return git_default_config(var, value, cb);
> +}
> +
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
> Introduce a ref_formatting_state which will eventually hold the values
> of modifier atoms. Implement this within ref-filter.
>
> Signed-off-by: Karthik Nayak
> ---
> +static void apply_formatting_state(struct ref_formatting_state *state,
> struct strbuf *final)
> +{
> + /* More formatting options to be evetually added */
> + strbuf_addbuf(final, state->output);
> + strbuf_release(state->output);
I guess the idea here is that you intend state->output to be re-used
and it is convenient to "clear" it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
[1]: http://article.gmane.org/gmane.comp.version-control.git/273094
> +}
> +
> void show_ref_array_item(struct ref_array_item *info, const char *format,
> int quote_style)
> {
> const char *cp, *sp, *ep;
> - struct strbuf output = STRBUF_INIT;
> + struct strbuf value = STRBUF_INIT;
> + struct strbuf final_buf = STRBUF_INIT;
> + struct ref_formatting_state state;
> int i;
>
> + memset(&state, 0, sizeof(state));
> + state.quote_style = quote_style;
> + state.output = &value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(&state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
This still allows re-use via strbuf_reset() mentioned above.
And, of course, you'd want to strbuf_release() it at the end of this
function where you're already releasing final_buf.
> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
> - struct atom_value *atomv;
> + struct atom_value *atomv = NULL;
What is this change about?
> ep = strchr(sp, ')');
> - if (cp < sp)
> - emit(cp, sp, &output);
> + if (cp < sp) {
> + emit(cp, sp, &state);
> + apply_formatting_state(&state, &final_buf);
> + }
> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
> &atomv);
> - print_value(atomv, quote_style, &output);
> + process_formatting_state(atomv, &state);
> + print_value(atomv, &state);
> + apply_formatting_state(&state, &final_buf);
> }
> if (*cp) {
> sp = cp + strlen(cp);
> - emit(cp, sp, &output);
> + emit(cp, sp, &state);
> + apply_formatting_state(&state, &final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
print_value() and emit() both imply outputting something, but neither
does so anymore.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:
> Implement an `align` atom which will act as a modifier atom and align
> any string with or without an %(atom) appearing before a %(end) atom
> to the right, left or middle.
For someone not familiar with the evolution of this patch series,
"align any string with or without an %(atom)" is confusing and
distracting and seems to imply that something extra mysterious is
going on behind the scenes. Keeping it simple makes it easier to
understand:
Add an `align` atom which left-, middle-, or right-aligns the
content between %(align:...) and %(end).
> It is followed by `:,`, where the `` is
'type' may not be the best name. Perhaps 'style' or 'position' or
something else would be better?
> either left, right or middle and `` is the total length
'paddinglength' is misleading. You're really describing the container
width in which alignment takes places, so perhaps call it 'width' or
'fieldwidth' or something.
> of the padding to be performed. If the atom length is more than the
> padding length then no padding is performed. e.g. to pad a succeeding
> atom to the middle with a total padding size of 40 we can do a
It's odd to have alignment described in terms of "padding" and
"padding length", especially in the case of "center" alignment. It
might be better to rephrase the discussion in terms of field width or
such.
> --format="%(align:middle,40).."
I may have missed the discussion, but why was comma chosen as a
separator here, rather than, say, colon? For instance:
%(align:middle:40)
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/Documentation/git-for-each-ref.txt
> b/Documentation/git-for-each-ref.txt
> index e49d578..d865f98 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,6 +127,14 @@ color::
> +align::
> + Align any string with or without %(atom) before the %(end)
> + atom to the right, left or middle. Followed by
Ditto regarding "any string with or without %(atom)" being confusing
to someone not familiar with the evolution of this patch series.
Instead, perhaps:
Left-, middle-, or right-align content between %(align:...)
and %(end).
> + `:,`, where the `` is either left,
> + right or middle and `` is the total length of
> + the padding to be performed. If the string length is more than
> + the padding length then no padding is performed.
Ditto regarding above observations about 'type', 'paddinglength', and "padding".
> diff --git a/ref-filter.c b/ref-filter.c
> index 2c074a1..d123299 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)
> const char *name = used_atom[i];
> struct atom_value *v = &ref->value[i];
> int deref = 0;
> - const char *refname;
> + const char *refname = NULL;
What is this change about?
> const char *formatp;
> struct branch *branch = NULL;
>
> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
> else
> v->s = " ";
> continue;
> + } else if (starts_with(name, "align:")) {
> + const char *valp = NULL;
Unnecessary NULL assignment.
> + struct align *align = xmalloc(sizeof(struct align));
> +
> + skip_prefix(name, "align:", &valp);
You could simplify the code by combining this skip_prefix() with the
earlier starts_with() in the conditional:
} else if (skip_prefix(name, "align:", &valp)) {
struct align *align = xmalloc(sizeof(struct align));
...
> + if (skip_prefix(valp, "left,", &valp))
> + align->align_type = ALIGN_LEFT;
> + else if (skip_prefix(valp, "right,", &valp))
> + align->align_type = ALIGN_RIGHT;
> + else if (skip_prefix(valp, "middle,", &valp))
> + align->align_type = ALIGN_MIDDLE;
> + else
> + die(_("align: improper format"));
You could do a better job of helping the user track down the offending
"improper format" by actually including it in the error message.
> + if (strtoul_ui(valp, 10, &align->align_value))
> + die(_("align: positive value expected"));
Ditto.
> + v->align = align;
> + v->modifier_atom = 1;
> + continue;
> + } else if (starts_with(name, "end")) {
> + v->end = 1;
> + v->modifier_atom = 1;
> + continue;
> } else
>
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
> Introduce a ref_formatting_state which will eventually hold the values
> of modifier atoms. Implement this within ref-filter.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index 91482c9..2c074a1 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep,
> struct strbuf *output)
> +static void apply_formatting_state(struct ref_formatting_state *state,
> struct strbuf *final)
> +{
> + /* More formatting options to be evetually added */
I forgot to mention this in my earlier review of this patch:
s/evetually/eventually/
> + strbuf_addbuf(final, state->output);
> + strbuf_release(state->output);
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote: > Implement an `align` atom which will act as a modifier atom and align > any string with or without an %(atom) appearing before a %(end) atom > to the right, left or middle. > > It is followed by `:,`, where the `` is > either left, right or middle and `` is the total length > of the padding to be performed. If the atom length is more than the > padding length then no padding is performed. e.g. to pad a succeeding > atom to the middle with a total padding size of 40 we can do a > --format="%(align:middle,40).." > > Add documentation and tests for the same. I forgot to mention in my earlier review of this patch that you should explain in the commit message, and probably the documentation, this this implementation (assuming I'm understanding the code) does not correctly support nested %(foo)...%(end) constructs, where %(foo) might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or some as yet unimagined modifier. Supporting nesting of these constructs will require pushing the formatting states onto a stack (or invoking the parser recursively). > Signed-off-by: Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:
> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine wrote:
>> On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak wrote:
>>> +static void apply_formatting_state(struct ref_formatting_state *state,
>>> struct strbuf *final)
>>> +{
>>> + /* More formatting options to be evetually added */
>>> + strbuf_addbuf(final, state->output);
>>> + strbuf_release(state->output);
>>
>> I guess the idea here is that you intend state->output to be re-used
>> and it is convenient to "clear" it here rather than making that the
>> responsibility of each caller. For re-use, it is more typical to use
>> strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
>
> it seems like a smarter way to around this without much overhead But it
> was more of to release it as its no longer required unless another modifier
> atom
> is encountered. Is it worth keeping hoping for another modifier atom
> eventually,
> and release it at the end like you suggested below?
If I understand your question correctly, it sounds like you're asking
about a memory micro-optimization. From an architectural standpoint,
it's cleaner for the entity which allocates a resource to also release
it. In this case, show_ref_array_item() allocates the strbuf, thus it
should be the one to release it.
And, although we shouldn't be worrying about micro-optimizations at
this point, if it were to be an issue, resetting the strbuf via
strbuf_reset(), which doesn't involve slow memory
deallocation/reallocation, is likely to be a winner over repeated
strbuf_release().
>>> + memset(&state, 0, sizeof(state));
>>> + state.quote_style = quote_style;
>>> + state.output = &value;
>>
>> It feels strange to assign a local variable reference to state.output,
>> and there's no obvious reason why you should need to do so. I would
>> have instead expected ref_format_state to be declared as:
>>
>> struct ref_formatting_state {
>>int quote_style;
>>struct strbuf output;
>> };
>>
>> and initialized as so:
>>
>> memset(&state, 0, sizeof(state));
>> state.quote_style = quote_style;
>> strbuf_init(&state.output, 0);
>
> This looks neater, thanks. It'll go along with the previous patch.
>
>> (In fact, the memset() isn't even necessary here since you're
>> initializing all fields explicitly, though perhaps you want the
>> memset() because a future patch adds more fields which are not
>> initialized explicitly?)
>
> Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
>>> for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>>> - struct atom_value *atomv;
>>> + struct atom_value *atomv = NULL;
>>
>> What is this change about?
>
> To remove the warning about atomv being unassigned before usage.
Hmm, where were you seeing that warning? The first use of 'atomv'
following its declaration is in the get_ref_atom_value() below, and
(as far as the compiler knows) that should be setting its value.
>>> ep = strchr(sp, ')');
>>> - if (cp < sp)
>>> - emit(cp, sp, &output);
>>> + if (cp < sp) {
>>> + emit(cp, sp, &state);
>>> + apply_formatting_state(&state, &final_buf);
>>> + }
>>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
>>> &atomv);
>>> - print_value(atomv, quote_style, &output);
>>> + process_formatting_state(atomv, &state);
>>> + print_value(atomv, &state);
>>> + apply_formatting_state(&state, &final_buf);
>>> }
>>> if (*cp) {
>>> sp = cp + strlen(cp);
>>> - emit(cp, sp, &output);
>>> + emit(cp, sp, &state);
>>> + apply_formatting_state(&state, &final_buf);
>>
>> I'm getting the feeling that these functions
>> (process_formatting_state, print_value, emit, apply_formatting_state)
>> are becoming misnamed (again) with the latest structural changes (but
>>
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak wrote:
> On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine
> wrote:
>> On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak wrote:
>>> On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine
>>> wrote:
>>>> It feels strange to assign a local variable reference to state.output,
>>>> and there's no obvious reason why you should need to do so. I would
>>>> have instead expected ref_format_state to be declared as:
>>>>
>>>> struct ref_formatting_state {
>>>>int quote_style;
>>>>struct strbuf output;
>>>> };
>>>>
>>>> and initialized as so:
>>>>
>>>> memset(&state, 0, sizeof(state));
>>>> state.quote_style = quote_style;
>>>> strbuf_init(&state.output, 0);
>>>>
>>>> (In fact, the memset() isn't even necessary here since you're
>>>> initializing all fields explicitly, though perhaps you want the
>>>> memset() because a future patch adds more fields which are not
>>>> initialized explicitly?)
>>>
>>> Yea the memset is needed for bit fields evnetually added in the future.
>>
>> Perhaps move the memset() to the first patch which actually requires
>> it, where it won't be (effectively) dead code, as it becomes here once
>> you make the above change.
>
> But why would I need it there, we need to only memset() the
> ref_formatting_state
> which is introduced here. Also here it helps in setting the strbuf
> within ref_formatting_state to {0, 0, 0}.
If you declare ref_formatting_state as shown above, and then
initialize it like so:
state.quote_style = quote_style;
strbuf_init(&state.output, 0);
then (as of this patch) the structure is fully initialized because
you've initialized each member individually. Adding a memset() above
those two lines would be do-nothing -- it would be wasted code -- and
would likely confuse someone reading the code, specifically because
the code is do-nothing and has no value (in this patch). Making each
patch understandable is one of your goals when organizing the patch
series; if a patch confuses a reader (say, by doing unnecessary work),
then it isn't satisfying that goal.
As for the strbuf member, it's initialized explicitly via
strbuf_init(), so there's no value in having memset() first initialize
it to {0, 0, 0}. Again, that's wasted code.
In a later patch, when you add another ref_formatting_state member or
two, then you will need to initialize those members too. That
initialization may be in the form of explicit assignment to each
member, or it may be the memset() sledgehammer approach, but the
initialization for those members should be added in the patch which
introduces them.
It's true that the end result is the same. By the end of the series,
you'll have memset() above the 'state.quote' and 'state.output'
initialization lines to ensure that your various boolean fields and
whatnot are initialized to zero, but each patch should be
self-contained and make sense on its own, doing just the work that it
needs to do, and not doing unrelated work. For this patch, the
memset() is unrelated work. For the later patch in which you add more
fields to ref_formatting_state(), the memset() is work necessary to
satisfy that patch's objective, thus belongs there.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] worktrees: add find_shared_symref
On Mon, Aug 3, 2015 at 7:06 PM, Eric Sunshine wrote:
> On Mon, Aug 3, 2015 at 2:48 PM, David Turner wrote:
>> Add a new function, find_shared_symref, which contains the heart of
>> die_if_checked_out, but works for any symref, not just HEAD. Refactor
>> die_if_checked_out to use the same infrastructure as
>> find_shared_symref.
>>
>> Soon, we will use find_shared_symref to protect notes merges in
>> worktrees.
>>
>> Signed-off-by: David Turner
>> ---
>> diff --git a/branch.c b/branch.c
>> index c85be07..d2b3586 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -349,31 +350,53 @@ static void check_linked_checkout(const char *branch,
>> const char *id)
>> strbuf_addstr(&gitdir, get_git_common_dir());
>> skip_prefix(branch, "refs/heads/", &branch);
>> strbuf_strip_suffix(&gitdir, ".git");
>> - die(_("'%s' is already checked out at '%s'"), branch, gitdir.buf);
In addition to the other issues already mentioned, this one is a bit
more serious (though still rather superficial):
With this change, the skip_prefix(branch, "refs/heads/", &branch)
becomes meaningless. It was used by the die() to provide a nicer
looking error message, however, 'branch' now is never used after
skip_prefix(). More below...
>> +
>> + strbuf_release(&path);
>> + strbuf_release(&sb);
>> + return strbuf_detach(&gitdir, NULL);
>> done:
>> strbuf_release(&path);
>> strbuf_release(&sb);
>> strbuf_release(&gitdir);
>> +
>> + return NULL;
>> }
>
> This would be cleaner and less redundant if you assign the existing
> location to a variable and just fall through to the 'done' label:
>
> char *existing = NULL;
> ...
> skip_prefix(branch, "refs/heads/", &branch);
> strbuf_strip_suffix(&gitdir, ".git");
> existing = strbuf_detach(&gitdir, NULL);
> done:
> strbuf_release(&path);
> strbuf_release(&sb);
> strbuf_release(&gitdir);
> return existing;
>
> There's no worry that the "existing" path will be clobbered by
> strbuf_release(&gitdir) since it's been detached already (and it's
> safe to release the strbuf without affecting what has been detached
> from it).
>
>> -void die_if_checked_out(const char *branch)
>> +char *find_shared_symref(const char *symref, const char *target)
>> {
>> struct strbuf path = STRBUF_INIT;
>> DIR *dir;
>> struct dirent *d;
>> + char *existing;
>>
>> - check_linked_checkout(branch, NULL);
>> + if ((existing = find_linked_symref(symref, target, NULL)))
>> + return existing;
>>
>> strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
>> dir = opendir(path.buf);
>> strbuf_release(&path);
>> if (!dir)
>> - return;
>> + return NULL;
>>
>> while ((d = readdir(dir)) != NULL) {
>> if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
>> continue;
>> - check_linked_checkout(branch, d->d_name);
>> + existing = find_linked_symref(symref, target, d->d_name);
>> + if (existing) {
>> + closedir(dir);
>> + return existing;
>
> For consistency with code nearby, this could have been handled by
> adding a 'done' label above the closedir() below and jumping to it
> from here, and then 'return existing'.
>
>> + }
>> }
>> closedir(dir);
>> +
>> + return NULL;
>> +}
>> +
>> +void die_if_checked_out(const char *branch)
>> +{
>> + char *existing;
>> +
>> + existing = find_shared_symref("HEAD", branch);
>> + if (existing)
>> + die(_("'%s' is already checked out at '%s'"), branch,
>> existing);
Unlike the original die() in check_linked_checkout() which printed the
branch named shortened by skip_prefix(), this one still prints the
long-form branch name. An easy fix would be to move the skip_prefix()
invocation down to here to skip the "refs/heads/" prefix just before
die().
>> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak wrote:
> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine wrote:
>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:
>>> of the padding to be performed. If the atom length is more than the
>>> padding length then no padding is performed. e.g. to pad a succeeding
>>> atom to the middle with a total padding size of 40 we can do a
>>
>> It's odd to have alignment described in terms of "padding" and
>> "padding length", especially in the case of "center" alignment. It
>> might be better to rephrase the discussion in terms of field width or
>> such.
>>
>>> --format="%(align:middle,40).."
>
> Ok this makes sense,
> I'll rephrase as :
>
> `` is the total length of the content with alignment.
This doesn't really make sense. isn't the content length; it's
the size of the area into which the content will be placed.
> If the atom length is more than the width then no alignment is performed.
What "atom"? I think you mean the content between %(align:) and %(end)
rather than "atom". The description might be clearer if you actually
say "content between %(align:) and %(end)" to indicate specifically
what is being aligned.
> e.g. to align a succeeding atom to the middle with a total width of 40
> we can do:
> --format="%(align:middle,40).."
>>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
>>> else
>>> v->s = " ";
>>> continue;
>>> + } else if (starts_with(name, "align:")) {
>>> + const char *valp = NULL;
>>
>> Unnecessary NULL assignment.
>
> Thats required for the second skip_prefix and so on.
> Else we get: "warning: ‘valp’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]"
Okay, so that's because skip_prefix() is inline, and it doesn't touch
its "out" argument unless it actually skips the prefix. Ugly, but
makes sense, although I think this issue would go away if you combined
the starts_with() and skips_prefix() as suggested earlier.
>>> + struct align *align = xmalloc(sizeof(struct align));
>>> +
>>> + skip_prefix(name, "align:", &valp);
>>
>> You could simplify the code by combining this skip_prefix() with the
>> earlier starts_with() in the conditional:
>>
>> } else if (skip_prefix(name, "align:", &valp)) {
>> struct align *align = xmalloc(sizeof(struct align));
>> ...
>
> That would require valp to be previously defined. Hence the split.
Yes, it would require declaring 'valp' earlier, but that seems a
reasonable tradeoff for cleaner, simpler, less redundant code.
>>> static void apply_formatting_state(struct ref_formatting_state *state,
>>> struct strbuf *final)
>>> {
>>> - /* More formatting options to be evetually added */
>>> + if (state->align && state->end) {
>>> + struct strbuf *value = state->output;
>>> + int len = 0, buf_len = value->len;
>>> + struct align *align = state->align;
>>> +
>>> + if (!value->buf)
>>> + return;
>>> + if (!is_utf8(value->buf)) {
>>> + len = value->len - utf8_strwidth(value->buf);
>>
>> What is this doing, exactly? If the string is *not* utf-8, then you're
>> asking it for its utf-8 width. Am I reading that correctly?
>>
>> Also, what is 'len' supposed to represent? I guess you want it to be
>> the difference between the byte length and the display length, but the
>> name 'len' doesn't convey that at all, nor does it help the reader
>> understand the code below where you do the actual formatting.
>>
>> In fact, if I'm reading this correctly, then 'len' is always zero in
>> your tests (because the tests never trigger this conditional), so this
>> functionality is never being exercised.
>
> There shouldn't be a "!" there, will change.
> I guess 'utf8_compensation' would be a better name.
Definitely better than 'len'.
>>> + else if (align->align_type == ALIGN_MIDDLE) {
>>> + int right = (align->align_value - buf_len)/2;
>>> +
Re: [PATCH v2] worktree: list operation
Thanks for the patch. Comments below...
On Sat, Aug 8, 2015 at 8:19 PM, Michael Rappazzo wrote:
> worktree: list operation
Imperative mood:
worktree: add 'list' command
> 'git worktree list' will list the main worktree followed by any linked
> worktrees which were created using 'git worktree add'. The option
> '--main-only' will restrict the list to only the main worktree.
Missing sign-off.
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 3387e2f..2b6b543 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -59,6 +60,10 @@ prune::
> +list::
> +
> +List the main worktree followed by all of the linked worktrees.
> +
> OPTIONS
> ---
> @@ -86,6 +91,9 @@ OPTIONS
> +--main-only::
> + With `list`, only list the main worktree.
Considering that the main worktree is always listed first, I wonder
how much value the --main-only option has. That is (Windows aside),
the same can easily be achieved via:
git worktree list | head -1
The more options we have, the more we have to document, test, and
support, so I'm feeling skeptical about --main-only since it can be
easily handled externally (for instance, via "head").
> -v::
> --verbose::
> With `prune`, report all removals.
> @@ -36,7 +37,7 @@ static int prune_worktree(const char *id, struct strbuf
> *reason)
> fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
> if (fd < 0) {
> strbuf_addf(reason, _("Removing worktrees/%s: unable to read
> gitdir file (%s)"),
> - id, strerror(errno));
> + id, strerror(errno));
Unintended whitespace change here (and 7 places below)? The
indentation looks fine as-is, so I think you don't want to include
these changes.
> return 1;
> }
> len = st.st_size;
> @@ -316,6 +317,71 @@ static int add(int ac, const char **av, const char
> *prefix)
> +static int list(int ac, const char **av, const char *prefix)
> +{
> + int main_only = 0;
> + struct option options[] = {
> + OPT_BOOL(0, "main-only", &main_only, N_("only list the main
> worktree")),
> + OPT_END()
> + };
> +
> + ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> + if (ac)
> + usage_with_options(worktree_usage, options);
> +
> + const char *work_tree;
> + work_tree = get_git_work_tree();
> + if (!work_tree)
> + die("This operation must be run in a work tree");
Why this restriction? Other git-worktree operations support a bare
repository, so this one ought to also.
(In fact, when dealing with a bare repository, the "main worktree" is
really the bare repository, so the term "worktree" is a bit of a
misnomer.)
> + struct strbuf worktree_git_path = STRBUF_INIT;
> + strbuf_addf(&worktree_git_path, _("%s/.git"), work_tree);
> +
> + struct strbuf main_work_tree = STRBUF_INIT;
> + if (is_directory(worktree_git_path.buf)) {
This is probably not a valid test. Even in the main worktree, ".git"
may not be a directory; it could be a symref to the actual repository.
> + /* This is the main tree */
> + strbuf_addstr(&main_work_tree, work_tree);
> + } else {
> + const char *git_dir = get_git_dir();
> + strbuf_addf(&main_work_tree, "%.*s", (int)(strstr(git_dir,
> "/.git/") - git_dir), git_dir);
> + }
> + printf("%s\n", main_work_tree.buf);
This can probably all be done more simply by taking a hint from the
code which tests if a branch is already checked out in the main or a
linked worktree. For the main worktree, it just uses
get_git_common_dir() and strips the trailing ".git" and prints that.
For instance:
strbuf_addstr(&sb, get_git_common_dir());
strbuf_strip_suffix(&sb, ".git");
printf(...);
should probably suffice in place of all the above code.
> + if (!main_only) {
> + chdir( main_work_tree.buf );
Style: Drop spaces inside parentheses.
I realize that the program exits after printing the list of worktrees,
but this chdir() makes me uncomfortable, partly because it's not
necessary, and partly because it could negatively impact code which
someone later adds to extend "list", if that new code expects the
current working directory to be the top of the worktree (as most git
code assumes).
> + if ( is_directory(git_path("worktrees")) ) {
> + DIR *dir = opendir( git_path("worktrees") );
Style: Drop spaces inside parentheses (both lines).
The opendir() will fail if the directory doesn't exist anyhow, so the
'if (is_directory(...))' is superfluous and can be dropped altogether.
Taking a hint from the code which tests if a branch is already checked
out elsewhere:
strbuf_addf(&sb, "%s/worktrees", get_git_common_dir());
dir = opendir(sb.buf);
> +
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak wrote:
> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine wrote:
>> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak wrote:
>>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine
>>> wrote:
>>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak
>>>> wrote:
>>>>> + else if (align->align_type == ALIGN_MIDDLE) {
>>>>> + int right = (align->align_value - buf_len)/2;
>>>>> + strbuf_addf(final, "%*s%-*s", align->align_value
>>>>> - right + len,
>>>>> + value->buf, right, "");
>>>>
>>>> An aesthetic aside: When (align_value - buf_len) is an odd number,
>>>> this implementation favors placing more whitespace to the left of the
>>>> string, and less to the right. In practice, this often tends to look a
>>>> bit more awkward than the inverse of placing more whitespace to the
>>>> right, and less to the left (but that again is subjective).
>>>
>>> I know that, maybe we could add an additional padding to even out the value
>>> given?
>>
>> I don't understand your question. I was merely suggesting (purely
>> subjectively), for the "odd length" case, putting the extra space
>> after the centered text rather than before it. For instance:
>>
>> int left = (align->align_value - buf_len) / 2;
>> strbuf_addf(final, "%*s%-*s", left, "",
>> align->align_value - left + len, value->buf);
>>
>> or any similar variation which would give the same result.
>
> I get this could be done, what I was asking was, Consider given a alignment
> width of 25 would be better to make that 26 so that we have even padding on
> both sides. But I don't like the adding of manipulating user given data.
I thought you might be asking that, but wasn't certain. I do agree
with your conclusion that second-guessing the user is a bad idea, and
that you should give the user exactly what was requested.
>> That raises another question. Why are 'struct ref_formatting_state',
>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
>> all? Aren't those private implementation details of ref-filter.c, or
>> do you expect other code to be using them?
>
> I guess struct ref_formatting_state and struct align could be moved to
> ref-filter.c. About struct atom_value its referenced by ref_array_item()
> so any reader reading about this, would find it easier if atom_value()
> is at the same place.
Do you expect callers ever to be manipulating or otherwise accessing
the atom_value of ref_array_item? If callers have no business mucking
with atom_value, then one option would be to simply forward declare
atom_value in the header:
struct atom_value;
struct ref_array_item {
...
struct atom_value *value;
...
};
which makes atom_value opaque to clients of ref-filter. The actual
declaration of atom_value would then be moved to ref-filter.c, thus
kept private.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 03/11] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak wrote:
> On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine wrote:
>> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak wrote:
>>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine
>>> wrote:
>>>> That raises another question. Why are 'struct ref_formatting_state',
>>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
>>>> all? Aren't those private implementation details of ref-filter.c, or
>>>> do you expect other code to be using them?
>>>
>>> I guess struct ref_formatting_state and struct align could be moved to
>>> ref-filter.c. About struct atom_value its referenced by ref_array_item()
>>> so any reader reading about this, would find it easier if atom_value()
>>> is at the same place.
>>
>> Do you expect callers ever to be manipulating or otherwise accessing
>> the atom_value of ref_array_item? If callers have no business mucking
>> with atom_value, then one option would be to simply forward declare
>> atom_value in the header:
>>
>> struct atom_value;
>>
>> struct ref_array_item {
>> ...
>> struct atom_value *value;
>> ...
>> };
>>
>> which makes atom_value opaque to clients of ref-filter. The actual
>> declaration of atom_value would then be moved to ref-filter.c, thus
>> kept private.
>
> Also the code that this was done in has been excepted into `next`
> so either I send a new series for the same, or write a patch just to
> move this from ref-filter.h to ref-filter.c. So what would you suggest?
To my eye, atom_value seems to encapsulate a bunch of state local to
and only meaningful to ref-filter's internal workings, so it doesn't
really belong in the public header. Assuming that you don't foresee
any callers ever needing to access the properties of atom_value, then
it might indeed be reasonable to introduce a patch which moves it from
the .h file to the .c file (while leaving only a forward declaration
in the .h file).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Sun, Aug 2, 2015 at 2:57 PM, Eric Sunshine wrote:
> On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin wrote:
>> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {
>> Authen::SASL->import(qw(Perl));
>> };
>>
>> + if($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) {
>> + die "invalid smtp auth: '${smtp_auth}'";
>> + }
>
> Style: space after 'if'
By the way, I notice that Authen::SASL::Perl implementation itself
normalizes the incoming mechanism to uppercase, if necessary:
$mechanism =~ s/^\s*\b(.*)\b\s*$/$1/g;
$mechanism =~ s/-/_/g;
$mechanism = uc $mechanism;
Since it doesn't require uppercase, it's not clear how much benefit
there is to adding a strict regex check to git-send-email.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Sun, Aug 9, 2015 at 1:19 PM, Eric Sunshine wrote:
> On Sun, Aug 2, 2015 at 2:57 PM, Eric Sunshine wrote:
>> On Sun, Aug 2, 2015 at 12:42 PM, Jan Viktorin
>> wrote:
>>> @@ -1136,6 +1141,10 @@ sub smtp_auth_maybe {
>>> Authen::SASL->import(qw(Perl));
>>> };
>>>
>>> + if($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) {
>>> + die "invalid smtp auth: '${smtp_auth}'";
>>> + }
>>
>> Style: space after 'if'
>
> By the way, I notice that Authen::SASL::Perl implementation itself
> normalizes the incoming mechanism to uppercase, if necessary:
>
> $mechanism =~ s/^\s*\b(.*)\b\s*$/$1/g;
> $mechanism =~ s/-/_/g;
> $mechanism = uc $mechanism;
>
> Since it doesn't require uppercase, it's not clear how much benefit
> there is to adding a strict regex check to git-send-email.
Hmm, perhaps I was looking at the wrong chunk of code. You had already
referenced the real code here[1], and it doesn't appear to do any case
transformation (it only replaces "-" with "_").
[1]: http://article.gmane.org/gmane.comp.version-control.git/275161
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Wed, Aug 5, 2015 at 3:17 AM, Jan Viktorin wrote:
> Do I understand well that you are complaining about too
> narrow commmit message?
Yes, I'm a complainer. ;-) It's minor, though, not a big deal, and
certainly not worth a re-roll if that was the only issue. In fact,
other than the undesirable "Supported:" line in the documentation, all
comments on v2 were minor and not demanding of a re-roll.
> I am trying to figure out how to write a test. It is
> not very clear to me, what the testing suite does. My
> attempt looks this way at the moment:
>
> 1657 do_smtp_auth_test() {
> 1658 git send-email \
> 1659 --from="Example " \
> 1660 --to=some...@example.com \
> 1661 --smtp-server="$(pwd)/fake.sendmail" \
> 1662 --smtp-auth="$1" \
> 1663 -v \
> 1664 0001-*.patch \
> 1665 2>errors >out
> 1666 }
> 1667
> 1668 test_expect_success $PREREQ 'accepts SMTP AUTH mechanisms (see RFC-4422,
> p. 8)' '
> 1669 do_smtp_auth_test "PLAIN LOGIN CRAM-MD5 DIGEST-MD5 GSSAPI
> EXTERNAL ANONYMOUS" &&
> 1670 do_smtp_auth_test "ABCDEFGHIKLMNOPQRSTUVWXYZ 0123456789_-"
Wouldn't this one fail the regex check you added which limits the
length to 20 characters?
> 1671 '
> 1672
> 1673 test_expect_success $PREREQ 'does not accept non-RFC-4422 strings for
> SMTP AUTH' '
> 1674 test_must_fail do_smtp_auth_test "../ATTACK" &&
> 1675 test_must_fail do_smtp_auth_test "TOO-LONG-BUT-VALID-STRING" &&
> 1676 test_must_fail do_smtp_auth_test "no-lower-case-sorry"
> 1677 '
>
> * I do not know yet, what to check after each do_smtp_auth_test call.
If you were able somehow to capture the interaction with
Auth::SASL::Perl, then you'd probably want to test if it received the
whitelisted mechanisms specified via --smtp-auth, however... (see
below)
> * Perhaps, each case should have its own test_expect_success call?
The grouping seems okay as-is.
> * Why send-email -v does not generate any output?
As far as I know, git-send-email doesn't accept a -v flag.
> (I found a directory 'trash directory.t9001-send-email', however, the
> errors file is always empty.)
Was it empty even for the cases which should have triggered the
validation regex to invoke die()?
> * Is there any other place where the files out, errors are placed?
No.
> * I have no idea what the fake.sendmail does (I could see its contents
> but still...). Is it suitable for my tests?
It dumps its command-line arguments to one file ("commandline") and
its stdin to another ("msgtxt"), but otherwise does no work. This is
useful for tests which need to make sure that the command-line and/or
message content gets augmented in some way, but won't help your case
since it can't capture the script's interaction with
Authen::SASL::Perl.
> * Should I check the behaviour '--smtp-auth overrides
> sendemail.smtpAuth'?
That would be nice, but there doesn't seem to be a good way to do it
via an existing testing mechanism since you can't check the
git-sendemail's interaction with Auth::SASL::Perl. The same holds for
your question above about what to check after each do_smtp_auth_test()
call.
One possibility which comes to mind is to create a fake
Authen::SASL::Perl which merely dumps its input mechanisms to a file,
and arrange for the Perl search path to find the fake one instead. You
could then check the output file to see if it reflects your
expectations. However, this may be overkill and perhaps not worth the
effort (especially if you're not a Perl programmer).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] worktrees: add find_shared_symref
On Mon, Aug 10, 2015 at 1:52 PM, David Turner wrote: > worktrees: add find_shared_symref s/worktrees/branch/ perhaps? > Add a new function, find_shared_symref, which contains the heart of > die_if_checked_out, but works for any symref, not just HEAD. Refactor > die_if_checked_out to use the same infrastructure as > find_shared_symref. > > Soon, we will use find_shared_symref to protect notes merges in > worktrees. > > Signed-off-by: David Turner > --- > This version addresses Eric Sunshine's comments on v5. It fixes an error > message and cleans up the code. All issues identified in previous versions seem to have been addressed, and nothing else pops out at me. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error pushing new branch: value too great for base (error token is...
On Mon, Aug 10, 2015 at 6:29 PM, Jacob Keller wrote:
> On Mon, Aug 10, 2015 at 2:54 AM, Gaurav Chhabra
> wrote:
>> Apologies for the delay in reply! I tried your suggestion and it
>> works. Thanks! :)
>>
>> I'm curious why integer comparison is throwing error. Shouldn't i be
>> comparing numbers with numeric operator?
>
> Yes, but shell doesn't treat hex numbers as numbers. So it will work
> only if the string is a decimal number.
This particular case deserves a bit more explanation. The expression
in question was this:
if [[ "$new_sha" -eq "$NULL" ]]; then
where 'new_sha' was 9226289d2416af4cb7365d7aaa5e382bdb3d9a89.
In Bash, inside the [[ .. ]], it did attempt evaluating the SHA1 as a
*decimal* number, however, when it encountered the "d", it complained
that it was outside the allowed range of decimal digits ("0"..."9").
Had the SHA1 been prefixed by a "0x", the [[...]] context would have
dealt with it just fine.
Outside the [[...]] context, arguments to -eq do need to be base-10 integers.
Nevertheless, a SHA1 is effective an opaque value. There's little, if
anything, to be gained by treating it as a numeric quantity, hence
string '=' makes more sense than numeric '-eq'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/2] worktrees: add find_shared_symref
On Mon, Aug 10, 2015 at 6:42 PM, David Turner wrote: > On Mon, 2015-08-10 at 18:30 -0400, Eric Sunshine wrote: >> On Mon, Aug 10, 2015 at 1:52 PM, David Turner >> wrote: >> > worktrees: add find_shared_symref >> >> s/worktrees/branch/ perhaps? > > Do you mean "this is in branch.c, so should be labeled with branch"? > > Because this change is mostly about non-branch refs, so I think saying > "branch" is confusing. That's why I labelled it "worktrees"; that's the > broad topic that's being addressed. Okay. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] send-email: provide whitelist of SMTP AUTH mechanisms
On Mon, Aug 10, 2015 at 6:06 AM, Jan Viktorin wrote: > On Sun, 9 Aug 2015 14:13:33 -0400 > Eric Sunshine wrote: >> One possibility which comes to mind is to create a fake >> Authen::SASL::Perl which merely dumps its input mechanisms to a file, >> and arrange for the Perl search path to find the fake one instead. You >> could then check the output file to see if it reflects your >> expectations. However, this may be overkill and perhaps not worth the >> effort (especially if you're not a Perl programmer). > > I think that Authen::SASL::Perl mock would not help. I wanted to create > some fake sendmail (but this is impossible as stated above because > then the perl modules are not used). So the only way would be to > provide some fake socket with a static content on the other side. This > is really an overkill to just test the few lines of code. Agreed. > So, what more can I do for this feature? I don't have any further suggestions. Other than the unwanted "Supported:" line in the documentation and the couple style issues[1], the patch seems sufficiently complete, as-is. The validation regex gets a "meh" from me merely because it's not clear how beneficial it will be in practice, but that's not an outright objection; I don't feel strongly about it either way. [1]: http://article.gmane.org/gmane.comp.version-control.git/275150 > I think that the basic regex test is OK. It can accept lowercase > letters and do an explicit uppercase call. I do not like to rely on > internals of the SASL library. As you could see, the SASL::Perl does > not check its inputs in a very good way and its code is quite unclear > (strange for a library providing security features). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] branch: refactor width computation
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak wrote:
> Remove unnecessary variables from ref_list and ref_item which were
> used for width computation. This is to make ref_item similar to
> ref-filter's ref_array_item. This will ensure a smooth port of
> branch.c to use ref-filter APIs in further patches.
>
> Previously the maxwidth was computed when inserting the refs into the
> ref_list. Now, we obtain the entire ref_list and then compute
> maxwidth.
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc8beb..b058b74 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> -static int calc_maxwidth(struct ref_list *refs)
> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
> {
> - int i, w = 0;
> + int i, max = 0;
> for (i = 0; i < refs->index; i++) {
> + struct ref_item *it = &refs->list[i];
> + int w = utf8_strwidth(it->name);
> +
> if (refs->list[i].ignore)
> continue;
Nit: Unnecessary work. You compute the width and then immediately
throw it away when 'ignore' is true.
Also, you use 'it' elsewhere rather than 'refs->list[i]' but not this
line, which makes it seem out of place. I would have expected to see:
if (it->ignore)
continue;
(Despite the noisier diff, the end result will be more consistent.)
> - if (refs->list[i].width > w)
> - w = refs->list[i].width;
> + if (it->kind == REF_REMOTE_BRANCH)
> + w += remote_bonus;
> + if (w > max)
> + max = w;
> }
> - return w;
> + return max;
> }
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak wrote:
> From: Karthik Nayak
>
> Remove unnecessary variables from ref_list and ref_item which were
> used for width computation. This is to make ref_item similar to
> ref-filter's ref_array_item. This will ensure a smooth port of
> branch.c to use ref-filter APIs in further patches.
>
> Previously the maxwidth was computed when inserting the refs into the
> ref_list. Now, we obtain the entire ref_list and then compute
> maxwidth.
>
> Based-on-patch-by: Jeff King
> Mentored-by: Christian Couder
> Mentored-by: Matthieu Moy
> Signed-off-by: Karthik Nayak
> ---
> builtin/branch.c | 61
> +---
> 1 file changed, 32 insertions(+), 29 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc8beb..b058b74 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv,
> int force, int kinds,
> struct ref_item {
> char *name;
> char *dest;
> - unsigned int kind, width;
> + unsigned int kind;
> struct commit *commit;
> int ignore;
> };
>
> struct ref_list {
> struct rev_info revs;
> - int index, alloc, maxwidth, verbose, abbrev;
> + int index, alloc, verbose, abbrev;
> struct ref_item *list;
> struct commit_list *with_commit;
> int kinds;
> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct
> object_id *oid, int flag
> newitem->name = xstrdup(refname);
> newitem->kind = kind;
> newitem->commit = commit;
> - newitem->width = utf8_strwidth(refname);
> newitem->dest = resolve_symref(orig_refname, prefix);
> newitem->ignore = 0;
> - /* adjust for "remotes/" */
> - if (newitem->kind == REF_REMOTE_BRANCH &&
> - ref_list->kinds != REF_REMOTE_BRANCH)
> - newitem->width += 8;
> - if (newitem->width > ref_list->maxwidth)
> - ref_list->maxwidth = newitem->width;
>
> return 0;
> }
> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct
> ref_item *item,
> }
>
> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> - int abbrev, int current, char *prefix)
> + int abbrev, int current, const char *remote_prefix)
> {
> char c;
> int color;
> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> + const char *prefix = "";
>
> if (item->ignore)
> return;
> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> break;
> case REF_REMOTE_BRANCH:
> color = BRANCH_COLOR_REMOTE;
> + prefix = remote_prefix;
> break;
> default:
> color = BRANCH_COLOR_PLAIN;
> @@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> strbuf_release(&out);
> }
>
> -static int calc_maxwidth(struct ref_lis
Re: [PATCH 03/10] branch: bump get_head_description() to the top
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak wrote: > This is a preperatory patch for 'roll show_detached HEAD into regular > ref_list'. This patch moves get_head_descrition() to the top so that s/descrition/description/ > it can be used in print_ref_item(). > > Signed-off-by: Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] branch: roll show_detached HEAD into regular ref_list
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak wrote:
> Remove show_detached() and make detached HEAD to be rolled into
> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
> supporting the same in append_ref(). This eliminates the need for an
> extra function and helps in easier porting of branch.c to use
> ref-filter APIs.
>
> Before show_detached() used to check if the HEAD branch satisfies the
> '--contains' option, now that is taken care by append_ref().
>
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 65f6d0d..81815c9 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -535,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> int color;
> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> const char *prefix = "";
> + const char *desc = item->name;
>
> if (item->ignore)
> return;
> @@ -547,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> color = BRANCH_COLOR_REMOTE;
> prefix = remote_prefix;
> break;
> + case REF_DETACHED_HEAD:
> + color = BRANCH_COLOR_CURRENT;
> + desc = get_head_description();
> + break;
> default:
> color = BRANCH_COLOR_PLAIN;
> break;
> @@ -558,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> color = BRANCH_COLOR_CURRENT;
> }
>
> - strbuf_addf(&name, "%s%s", prefix, item->name);
> + strbuf_addf(&name, "%s%s", prefix, desc);
> if (verbose) {
> int utf8_compensation = strlen(name.buf) -
> utf8_strwidth(name.buf);
> strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
> @@ -581,6 +591,8 @@ static void print_ref_item(struct ref_item *item, int
> maxwidth, int verbose,
> }
> strbuf_release(&name);
> strbuf_release(&out);
> + if (item->kind == REF_DETACHED_HEAD)
> + free((void *)desc);
This would be cleaner, and more easily extended to other cases if you
used a 'to_free' variable. For instance, something like this:
const char *desc = item->name;
char *to_free = NULL;
...
case REF_DETACHED_HEAD:
...
desc = to_free = get_head_description();
break;
...
strbuf_addf(&name, "%s%s", prefix, desc);
...
free(to_free);
Note that it's safe to free 'to_free' when it's NULL, so you don't
need to protect the free() with that ugly specialized 'if' which
checks for REF_DETACHED_HEAD. You can also do away with the "cast to
non-const" when freeing.
> }
> @@ -642,7 +638,14 @@ static int print_ref_list(int kinds, int detached, int
> verbose, int abbrev, stru
> cb.ref_list = &ref_list;
> cb.pattern = pattern;
> cb.ret = 0;
> + /*
> +* First we obtain all regular branch refs and then if the
s/then//
> +* HEAD is detached then we insert that ref to the end of the
> +* ref_fist so that it can be printed first.
> +*/
> for_each_rawref(append_ref, &cb);
> + if (detached)
> + head_ref(append_ref, &cb);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Tue, Aug 11, 2015 at 4:51 PM, Johannes Sixt wrote: > Invoking plink requires special treatment, and we have support and even > test cases for the commands 'plink' and 'tortoiseplink'. We also support > .exe variants for these two and there is a test for 'plink.exe'. > > On Windows, however, where support for plink.exe would be relevant, the > test case fails because it is not possible to execute a file with a .exe > extension that is actually not a binary executable---it is a shell > script in our test. We have to disable the test case on Windows. > > Considering, that 'plink.exe' is irrelevant on non-Windows, let's just > remove the test and assume that the code "just works". putty and plink are used on Unix as well. A quick check of Mac OS X, Linux, and FreeBSD reveals that package managers on each platform have putty and plink packages available. > Signed-off-by: Johannes Sixt > --- > t/t5601-clone.sh | 6 -- > 1 file changed, 6 deletions(-) > > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh > index 9b34f3c..df69bf6 100755 > --- a/t/t5601-clone.sh > +++ b/t/t5601-clone.sh > @@ -353,12 +353,6 @@ test_expect_success 'plink is treated specially (as > putty)' ' > expect_ssh "-P 123" myhost src > ' > > -test_expect_success 'plink.exe is treated specially (as putty)' ' > - copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink.exe" && > - git clone "[myhost:123]:src" ssh-bracket-clone-plink-1 && > - expect_ssh "-P 123" myhost src > -' > - > test_expect_success 'tortoiseplink is like putty, with extra arguments' ' > copy_ssh_wrapper_as "$TRASH_DIRECTORY/tortoiseplink" && > git clone "[myhost:123]:src" ssh-bracket-clone-plink-2 && > -- > 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Tue, Aug 11, 2015 at 6:56 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> On Tue, Aug 11, 2015 at 4:51 PM, Johannes Sixt wrote: >>> Invoking plink requires special treatment, and we have support and even >>> test cases for the commands 'plink' and 'tortoiseplink'. We also support >>> .exe variants for these two and there is a test for 'plink.exe'. >>> >>> On Windows, however, where support for plink.exe would be relevant, the >>> test case fails because it is not possible to execute a file with a .exe >>> extension that is actually not a binary executable---it is a shell >>> script in our test. We have to disable the test case on Windows. >>> >>> Considering, that 'plink.exe' is irrelevant on non-Windows, let's just >>> remove the test and assume that the code "just works". >> >> putty and plink are used on Unix as well. A quick check of Mac OS X, >> Linux, and FreeBSD reveals that package managers on each platform have >> putty and plink packages available. > > But they do not force their users to say "plink.exe", but instead > let them invoke "plink", no? > > The test before the one that was removed is about "plink" (sans .exe), > and what was removed is with ".exe", so I think J6t's patch is OK. Ah, you're correct. I overlooked the extra emphasis j6t's commit message placed on ".exe". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] send-email: provide whitelist of SMTP AUTH mechanisms
On Wed, Aug 12, 2015 at 01:39:44AM +0200, Jan Viktorin wrote: > When sending an e-mail, the client and server must agree on an > authentication mechanism. Some servers (due to misconfiguration > or a bug) deny valid credentials for certain mechanisms. In this > patch, a new option --smtp-auth and configuration entry smtpAuth > are introduced. If smtp_auth is defined, it works as a whitelist > of allowed mechanisms for authentication selected from the ones > supported by the installed SASL perl library. > > Signed-off-by: Jan Viktorin > --- > diff --git a/Documentation/git-send-email.txt > b/Documentation/git-send-email.txt > index f14705e..82c6ae8 100644 > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -171,6 +171,17 @@ Sending > to determine your FQDN automatically. Default is the value of > 'sendemail.smtpDomain'. > > +--smtp-auth=:: > + Whitespace-separated list of allowed SMTP-AUTH mechanisms. This setting > + forces using only the listed mechanisms. Example: > + > + $ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ... > + > + If at least one of the specified mechanisms matches the ones advertised > by the > + SMTP server and if it is supported by the utilized SASL library, the > mechanism > + is used for authentication. If neither 'sendemail.smtpAuth' nor > '--smtp-auth' > + is specified, all mechanisms supported by the SASL library can be used. Unfortuantely, this won't format correctly in Asciidoc. The following squash-in fixes it... 8< Subject: [PATCH] fixup! send-email: provide whitelist of SMTP AUTH mechanisms --- Documentation/git-send-email.txt | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 82c6ae8..9e4f130 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -174,13 +174,15 @@ Sending --smtp-auth=:: Whitespace-separated list of allowed SMTP-AUTH mechanisms. This setting forces using only the listed mechanisms. Example: - - $ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ... - - If at least one of the specified mechanisms matches the ones advertised by the - SMTP server and if it is supported by the utilized SASL library, the mechanism - is used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth' - is specified, all mechanisms supported by the SASL library can be used. ++ +-- +$ git send-email --smtp-auth="PLAIN LOGIN GSSAPI" ... +-- ++ +If at least one of the specified mechanisms matches the ones advertised by the +SMTP server and if it is supported by the utilized SASL library, the mechanism +is used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth' +is specified, all mechanisms supported by the SASL library can be used. --smtp-pass[=]:: Password for SMTP-AUTH. The argument is optional: If no -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option
On Tue, Aug 11, 2015 at 4:57 PM, Jacob Keller wrote: > Add new option "notes..merge" option which specifies the merge > strategy for merging into a given notes ref. This option enables > selection of merge strategy for particular notes refs, rather than all > notes ref merges, as user may not want cat_sort_uniq for all refs, but > only some. Note that the is the local reference we are merging > into, not the remote ref we merged from. The assumption is that users > will mostly want to configure separate local ref merge strategies rather > than strategies depending on which remote ref they merge from. Also, > notes..merge overrides the general behavior as it is more specific. > > Signed-off-by: Jacob Keller > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 488c2e8eec1b..2c283ebc309e 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1912,6 +1912,12 @@ notes.merge:: > STRATEGIES" section of linkgit:git-notes[1] for more information > on each strategy. > > +notes..merge:: > + Which merge strategy to choose if the local ref for a notes merge > + matches , overriding "notes.merge". just be a s/just/must/ > + fully qualified refname. See "NOTES MERGE STRATEGIES" section in > + linkgit:git-notes[1] for more information on the available strategies. > + -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] http: add support for specifying the SSL version
On Wed, Aug 12, 2015 at 04:24:51PM +0200, Elia Pinto wrote: > Teach git about a new option, "http.sslVersion", which permits one to > specify the SSL version to use when negotiating SSL connections. The > setting can be overridden by the GIT_SSL_VERSION environment > variable. > > Signed-off-by: Elia Pinto > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 315f271..76a4f2b 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1595,6 +1595,27 @@ http.saveCookies:: > +http.sslVersion:: > + The SSL version to use when negotiating an SSL connection, if you > + want to force the default. The available and default version depend on > + whether libcurl was built against NSS or OpenSSL and the particular > configuration > + of the crypto library in use. Internally this sets the > 'CURLOPT_SSL_VERSION' > + option; see the libcurl documentation for more details on the format > + of this option and for the ssl version supported. Actually the possible > values > + of this option are: > + > + - sslv2 > + - sslv3 > + - tlsv1 > + - tlsv1.0 > + - tlsv1.1 > + - tlsv1.2 > ++ > +Can be overridden by the 'GIT_SSL_VERSION' environment variable. > +To force git to use libcurl's default ssl version and ignore any > +explicit http.sslversion option, set 'GIT_SSL_VERSION' to the > +empty string. Unfortunately, this won't format properly in Asciidoc; the final paragraph will be indented as part of the itemized list supported SSL versions. Here's a squash-in to fix it: 8< Subject: [PATCH] fixup! http: add support for specifying the SSL version --- Documentation/config.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 76a4f2b..b23b01a 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1610,6 +1610,7 @@ http.sslVersion:: - tlsv1.0 - tlsv1.1 - tlsv1.2 + + Can be overridden by the 'GIT_SSL_VERSION' environment variable. To force git to use libcurl's default ssl version and ignore any -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cleanup submodule_config a bit.
On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:
> In the first hunk, `submodule` is NULL all the time, so we can make it clearer
> by directly returning NULL.
>
> In the second hunk, we can directly return the lookup values as it also makes
> the coder clearer.
>
> Signed-off-by: Stefan Beller
> ---
> submodule-config.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/submodule-config.c b/submodule-config.c
> index 199692b..08e93cc 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -387,7 +387,7 @@ static const struct submodule *config_from(struct
> submodule_cache *cache,
> }
>
> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
> - return submodule;
> + return NULL;
There are a couple other places which return 'submodule' when it is
known to be NULL. One could rightly expect that they deserve the same
treatment, otherwise, the code becomes more confusing since it's not
obvious why 'return NULL' is used some places but not others.
> switch (lookup_type) {
> case lookup_name:
> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct
> submodule_cache *cache,
>
> switch (lookup_type) {
> case lookup_name:
> - submodule = cache_lookup_name(cache, sha1, key);
> - break;
> + return cache_lookup_name(cache, sha1, key);
> case lookup_path:
> - submodule = cache_lookup_path(cache, sha1, key);
> - break;
> + return cache_lookup_path(cache, sha1, key);
> + default:
> + return NULL;
> }
> -
> - return submodule;
Earlier in the function, there's effectively a clone of this logic to
which you could apply the same transformation. Changing it here, while
ignoring the clone, makes the code more confusing (or at least
inconsistent) rather than less.
> }
>
> static const struct submodule *config_from_path(struct submodule_cache
> *cache,
> --
> 2.5.0.234.gefc8a62
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cleanup submodule_config a bit.
On Wed, Aug 12, 2015 at 5:34 PM, Stefan Beller wrote:
> On Wed, Aug 12, 2015 at 2:13 PM, Eric Sunshine
> wrote:
>> On Wed, Aug 12, 2015 at 3:13 PM, Stefan Beller wrote:
>>> if (!gitmodule_sha1_from_commit(commit_sha1, sha1))
>>> - return submodule;
>>> + return NULL;
>>
>> There are a couple other places which return 'submodule' when it is
>> known to be NULL. One could rightly expect that they deserve the same
>> treatment, otherwise, the code becomes more confusing since it's not
>> obvious why 'return NULL' is used some places but not others.
>
> They were slightly less obvious to me, fixed now as well!
>
>>> @@ -420,14 +420,12 @@ static const struct submodule *config_from(struct
>>> submodule_cache *cache,
>>> switch (lookup_type) {
>>> case lookup_name:
>>> - submodule = cache_lookup_name(cache, sha1, key);
>>> - break;
>>> + return cache_lookup_name(cache, sha1, key);
>>> case lookup_path:
>>> - submodule = cache_lookup_path(cache, sha1, key);
>>> - break;
>>> + return cache_lookup_path(cache, sha1, key);
>>> + default:
>>> + return NULL;
>>> }
>>> -
>>> - return submodule;
>>
>> Earlier in the function, there's effectively a clone of this logic to
>> which you could apply the same transformation. Changing it here, while
>> ignoring the clone, makes the code more confusing (or at least
>> inconsistent) rather than less.
>
> Not quite. Note the `if (submodule)` in the earlier version, so in case
> cache_lookup_{name, path} return NULL, we keep going. The change I
> propose is at the end of the function and we definitely return no matter
> if it is NULL or not.
Okay, cache_lookup_{name, path}() can indeed return NULL, so the same
transformation won't work. Sorry for the noise.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: add support for specifying the SSL version
On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto wrote:
> Teach git about a new option, "http.sslVersion", which permits one to
> specify the SSL version to use when negotiating SSL connections. The
> setting can be overridden by the GIT_SSL_VERSION environment
> variable.
>
> Signed-off-by: Elia Pinto
> ---
> This is the third version of the patch. The changes compared to the previous
> version are:
Looks better. A few comments below...
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index c97c648..6e9359c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -364,9 +381,22 @@ static CURL *get_curl_handle(void)
> if (http_proactive_auth)
> init_curl_http_auth(result);
>
> + if (getenv("GIT_SSL_VERSION"))
> + ssl_version = getenv("GIT_SSL_VERSION");
> + if (ssl_version != NULL && *ssl_version) {
> + int i;
> + for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
> + if (sslversions[i].name != NULL &&
> *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
This sort of loop is normally either handled by indexing up to a limit
(ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
(NULL, in this case). It is redundant to use both, as this code does.
The former (using ARRAY_SIZE) is typically employed when you know the
number of items upfront, such as when the item list is local and
compiled in; the latter (NULL sentinel) is typically used when
receiving an item list as an argument to a function where you don't
know the item count upfront (and the item count is not passed to the
function as a separate argument).
In this case, the item list is local and its size is known to the
compiler, so that suggests using ARRAY_SIZE, and dropping the NULL
sentinel.
Style aside: This 'if' statement is very wide and likely should be
wrapped over multiple lines (trying to keep the code within an
80-column limit).
> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
> + sslversions[i].ssl_version);
> + break;
> + }
> + if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl
> version %s: using default",
> + ssl_version);
Style:
Drop spaces inside 'if' parentheses.
Place warning() on its own line.
> + }
> +
> if (getenv("GIT_SSL_CIPHER_LIST"))
> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> -
> if (ssl_cipherlist != NULL && *ssl_cipherlist)
> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> ssl_cipherlist);
> --
> 2.5.0.234.gefc8a62.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: add support for specifying the SSL version
On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto wrote:
> 2015-08-13 17:47 GMT+02:00 Eric Sunshine :
>> On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto wrote:
>>> Teach git about a new option, "http.sslVersion", which permits one to
>>> specify the SSL version to use when negotiating SSL connections. The
>>> setting can be overridden by the GIT_SSL_VERSION environment
>>> variable.
>>>
>>> Signed-off-by: Elia Pinto
>>> ---
>>> This is the third version of the patch. The changes compared to the
>>> previous version are:
>>
>> Looks better. A few comments below...
>>
>>> diff --git a/contrib/completion/git-completion.bash
>>> b/contrib/completion/git-completion.bash
>>> index c97c648..6e9359c 100644
>>> --- a/contrib/completion/git-completion.bash
>>> +++ b/contrib/completion/git-completion.bash
>>> @@ -364,9 +381,22 @@ static CURL *get_curl_handle(void)
>>> if (http_proactive_auth)
>>> init_curl_http_auth(result);
>>>
>>> + if (getenv("GIT_SSL_VERSION"))
>>> + ssl_version = getenv("GIT_SSL_VERSION");
>>> + if (ssl_version != NULL && *ssl_version) {
>>> + int i;
>>> + for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>>> + if (sslversions[i].name != NULL &&
>>> *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>>
>> This sort of loop is normally either handled by indexing up to a limit
>> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
>> (NULL, in this case). It is redundant to use both, as this code does.
> I do not think. sslversions[i].name can be null, see how the structure
> is initialized. No ?
The initialization:
static struct {
const char *name;
long ssl_version;
} sslversions[] = {
{ "sslv2", CURL_SSLVERSION_SSLv2 },
...
{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
{ NULL }
};
terminates the list with a NULL sentinel entry, which does indeed set
sslversions[i].name to NULL. When you know the item count ahead of
time (as you do in this case), this sort of end-of-list sentinel is
redundant, and complicates the code unnecessarily. For instance, the
'sslversions[i].name != NULL' expression in the 'if':
if (sslversions[i].name != NULL && *sslversions[i].name ...
is an unwanted complication. In fact, the '*sslversions[i].name'
expression is also unnecessary.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: add support for specifying the SSL version
On Thu, Aug 13, 2015 at 12:15 PM, Elia Pinto wrote:
> 2015-08-13 18:11 GMT+02:00 Eric Sunshine :
>> On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto wrote:
>>> 2015-08-13 17:47 GMT+02:00 Eric Sunshine :
>>>>> + if (ssl_version != NULL && *ssl_version) {
>>>>> + int i;
>>>>> + for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>>>>> + if (sslversions[i].name != NULL &&
>>>>> *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>>>>
>>>> This sort of loop is normally either handled by indexing up to a limit
>>>> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
>>>> (NULL, in this case). It is redundant to use both, as this code does.
>>> I do not think. sslversions[i].name can be null, see how the structure
>>> is initialized. No ?
>>
>> The initialization:
>>
>> static struct {
>>const char *name;
>>long ssl_version;
>>} sslversions[] = {
>>{ "sslv2", CURL_SSLVERSION_SSLv2 },
>>...
>>{ "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>>{ NULL }
>> };
>>
>> terminates the list with a NULL sentinel entry, which does indeed set
>> sslversions[i].name to NULL. When you know the item count ahead of
>> time (as you do in this case), this sort of end-of-list sentinel is
>> redundant, and complicates the code unnecessarily. For instance, the
>> 'sslversions[i].name != NULL' expression in the 'if':
>>
>> if (sslversions[i].name != NULL && *sslversions[i].name ...
>>
>> is an unwanted complication. In fact, the '*sslversions[i].name'
>> expression is also unnecessary.
> I agree. But this is what suggested me Junio: =). What do I have to do ?
> It becomes difficult to keep everyone happy: =)
You're referring to [1] in which Junio's example table initialization
had the NULL sentinel. That approach is fine, and my earlier comment:
This sort of loop is normally either handled by indexing up to a
limit (ARRAY_SIZE, in this case) or by iterating until hitting a
sentinel (NULL, in this case). It is redundant to use both...
wasn't saying that you shouldn't use the NULL sentinel. It said only
that you should choose one approach rather than complicating the code
unnecessarily by mixing the two.
So, your loop can either look like this, if you use the NULL sentinel:
struct ssl_map *p = sslversions;
while (p->name) {
if (!strcmp(ssl_version, p->name))
...
}
or like this, if you use ARRAY_SIZE:
for (i = 0; i < ARRAY_SIZE(sslversions); i++) {
if (!strcmp(ssl_version, sslversions[i].name))
...
}
Each loop form is valid, and (other than the fact that the compiler
knows the array size, thus slightly favoring the ARRAY_SIZE form) the
choice of which of the above two forms to use isn't that important,
and you can choose whichever you like, but please do choose one of the
above two. If you feel that Junio would be happier with the
NULL-sentinel form, then go with that.
[1]: http://article.gmane.org/gmane.comp.version-control.git/275773
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] http: add support for specifying the SSL version
On Thu, Aug 13, 2015 at 12:37 PM, Eric Sunshine wrote:
> So, your loop can either look like this, if you use the NULL sentinel:
>
> struct ssl_map *p = sslversions;
> while (p->name) {
> if (!strcmp(ssl_version, p->name))
> ...
> }
That's not quite correct. 'p' needs to be incremented, of course, so:
struct ssl_map *p;
for (p = sslversions; p->name; p++) {
if (!strcmp(ssl_version, p->name))
...
}
would be nicely idiomatic.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] refs: make refs/worktree/* per-worktree
On Wed, Aug 12, 2015 at 5:57 PM, David Turner wrote: > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh > index 93605f4..28e6dff 100755 > --- a/t/t0060-path-utils.sh > +++ b/t/t0060-path-utils.sh > +test_expect_success 'handle per-worktree refs in refs/worktree' ' > + git commit --allow-empty -m "initial commit" && > + git worktree add -b branch worktree && > + ( > + cd worktree && > + git commit --allow-empty -m "test commit" && > + git for-each-ref | test_must_fail grep refs/worktree && s/test_must_fail/!/ >From t/README: On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. We are not in the business of verifying that the world given to us sanely works. > + git update-ref refs/worktree/something HEAD && > + git rev-parse refs/worktree/something >../worktree-head && > + git for-each-ref | grep refs/worktree/something > + ) && > + test_path_is_missing .git/refs/worktree && > + test_must_fail git rev-parse refs/worktree/something && > + git update-ref refs/worktree/something HEAD && > + git rev-parse refs/worktree/something >main-head && > + ! test_cmp main-head worktree-head > +' -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 05/13] ref-filter: implement an `align` atom
On Sun, Aug 9, 2015 at 10:11 AM, Karthik Nayak wrote: > Implement an `align` atom which left-, middle-, or right-aligns the > content between %(align:..) and %(end). > > Signed-off-by: Karthik Nayak > --- > diff --git a/Documentation/git-for-each-ref.txt > b/Documentation/git-for-each-ref.txt > index e49d578..d949812 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -127,6 +127,15 @@ color:: > Change output color. Followed by `:`, where names > are described in `color.branch.*`. > > +align:: > + Implement an `align` atom which left-, middle-, or > + right-aligns the content between %(align:..) and This documentation seems to be copied a bit too literally from the commit message. Saying "Implement an `align` atom" makes sense for the commit message, but not for the documentation of the 'align' atom. Instead, Left-, middle-, or right-align the content between %(align:...) and %(end). would sound better. > + %(end). Followed by `:,`, where the > + `` is either left, right or middle and `` is > + the total length of the content with alignment. If the > + contents length is more than the width then no alignment is > + performed. Currently nested alignment is not supported. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory
On Thu, Aug 13, 2015 at 2:02 PM, Doug Kelly wrote:
> From: Junio C Hamano
>
> The hook to report "garbage" files in $GIT_OBJECT_DIRECTORY/pack/
> could be generic but is too specific to count-object's needs.
>
> Move the part to produce human-readable messages to count-objects,
> and refine the interface to callback with the "bits" with values
> defined in the cache.h header file, so that other callers (e.g.
> prune) can later use the same mechanism to enumerate different
> kinds of garbage files and do something intelligent about them,
> other than reporting in textual messages.
>
> Signed-off-by: Junio C Hamano
Since you're forwarding Junio's patch, you'd also want to sign-off
(following his).
> ---
> diff --git a/builtin/count-objects.c b/builtin/count-objects.c
> index ad0c799..4c3198e 100644
> --- a/builtin/count-objects.c
> +++ b/builtin/count-objects.c
> @@ -15,9 +15,31 @@ static int verbose;
> static unsigned long loose, packed, packed_loose;
> static off_t loose_size;
>
> -static void real_report_garbage(const char *desc, const char *path)
> +const char *bits_to_msg(unsigned seen_bits)
If you don't expect other callers outside this file, then this should
be declared 'static'. If you do expect future external callers, then
this should be declared in a public header file (but renamed to be
more meaningful).
> +{
> + switch (seen_bits) {
> + case 0:
> + return "no corresponding .idx or .pack";
> + case PACKDIR_FILE_GARBAGE:
> + return "garbage found";
> + case PACKDIR_FILE_PACK:
> + return "no corresponding .idx";
> + case PACKDIR_FILE_IDX:
> + return "no corresponding .pack";
> + case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
> + default:
> + return NULL;
> + }
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf
On Sun, Aug 9, 2015 at 10:11 AM, Karthik Nayak wrote:
> Add strbuf_utf8_align() which will align a given string into a strbuf
> as per given align_type and width. If the width is greater than the
> string length then no alignment is performed.
In addition to Junio's valuable comments...
> Signed-off-by: Karthik Nayak
> ---
> diff --git a/utf8.h b/utf8.h
> index 5a9e94b..db8ca63 100644
> --- a/utf8.h
> +++ b/utf8.h
> @@ -55,4 +55,17 @@ int mbs_chrlen(const char **text, size_t *remainder_p,
> const char *encoding);
> */
> int is_hfs_dotgit(const char *path);
>
> +typedef enum {
> + ALIGN_LEFT,
> + ALIGN_MIDDLE,
> + ALIGN_RIGHT
> +} align_type;
> +
> +/*
> + * Align the string given and store it into a strbuf as per the type
> + * and width.
> + */
Please extend this documentation to state explicitly that this
function preserves (does not truncate) the input string if it is wider
than 'width'. That's quite important information for the caller to
know.
(Aside: I could easily see this function being extended to support
optional truncation, but that's a separate topic, and something that
can be done by someone else when the feature is actually needed; it's
not your responsibility.)
> +void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int
> width,
> + const char *s);
> +
> #endif
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv4] http: add support for specifying the SSL version
On Fri, Aug 14, 2015 at 12:35 PM, Elia Pinto wrote:
> Teach git about a new option, "http.sslVersion", which permits one to
> specify the SSL version to use when negotiating SSL connections. The
> setting can be overridden by the GIT_SSL_VERSION environment
> variable.
>
> Signed-off-by: Elia Pinto
> ---
> This is the fourth revision of the patch. Changes from previous
>
> - Used only ARRAY_SIZE for walking sslversions (Eric, Junio)
> - Fixed some problems of style: spurious blanks in if stm, wrapped warning
Thanks, this is looking better. A few very minor style-related
comments below (most or all of which were mentioned previously, I
think)...
> diff --git a/http.c b/http.c
> index e9c6fdd..83118b5 100644
> --- a/http.c
> +++ b/http.c
> @@ -364,9 +380,22 @@ static CURL *get_curl_handle(void)
> if (http_proactive_auth)
> init_curl_http_auth(result);
>
> + if (getenv("GIT_SSL_VERSION"))
> + ssl_version = getenv("GIT_SSL_VERSION");
> + if (ssl_version != NULL && *ssl_version) {
In this codebase, it is customary to omit the "!= NULL", so:
if (ssl_version && *ssl_version) {
> + int i;
> + for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
Style: Drop space after open '(' and before close ')' in 'for' loop:
for (i = 0; i < ARRAY_SIZE(sslversions); i++) {
> + if (!strcmp(ssl_version,sslversions[i].name)) {
Style: Insert space after comma.
> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
> + sslversions[i].ssl_version);
> + break;
> + }
> + }
> + if (i == ARRAY_SIZE(sslversions)) warning("unsupported ssl
> version %s: using default",ssl_version);
warning() call should be on its own line.
Style: Insert space after comma.
> + }
> +
> if (getenv("GIT_SSL_CIPHER_LIST"))
> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> -
> if (ssl_cipherlist != NULL && *ssl_cipherlist)
> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> ssl_cipherlist);
> --
> 2.5.0.235.gb9bd8dc
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/4] notes: add notes.mergestrategy option to select default strategy
On Fri, Aug 14, 2015 at 1:50 PM, Jacob Keller wrote:
> Teach git-notes about "notes.mergestrategy" to select a general strategy
> for all notes merges. This enables a user to always get expected merge
> strategy such as "cat_sort_uniq" without having to pass the "-s" option
> manually.
>
> Signed-off-by: Jacob Keller
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 042348082709..97109f8d419c 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -796,21 +814,16 @@ static int merge(int argc, const char **argv, const
> char *prefix)
> expand_notes_ref(&remote_ref);
> o.remote_ref = remote_ref.buf;
>
> + git_config_get_string_const("notes.mergestrategy",
> &configured_strategy);
> +
> if (strategy) {
> - if (!strcmp(strategy, "manual"))
> - o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
> - else if (!strcmp(strategy, "ours"))
> - o.strategy = NOTES_MERGE_RESOLVE_OURS;
> - else if (!strcmp(strategy, "theirs"))
> - o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
> - else if (!strcmp(strategy, "union"))
> - o.strategy = NOTES_MERGE_RESOLVE_UNION;
> - else if (!strcmp(strategy, "cat_sort_uniq"))
> - o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> - else {
> + if (parse_notes_strategy(strategy, &o.strategy)) {
> error("Unknown -s/--strategy: %s", strategy);
This error message points the user at the source of the problem
("--strategy"). Good.
> usage_with_options(git_notes_merge_usage, options);
> }
> + } else if (configured_strategy) {
> + if (parse_notes_strategy(configured_strategy, &o.strategy))
> + die("Unknown notes merge strategy: %s",
> configured_strategy);
This error message doesn't. Perhaps it should mention the config
variable "notes.mergestrategy" to help the user track down the problem
more quickly?
> }
>
> t = init_notes_check("merge");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Close config file handle if the entry to unset is not found
On Fri, Aug 14, 2015 at 3:18 PM, Sven Strickroth wrote:
> Without this patch there might be open handle leaks.
Thanks for the patch. A question below...
> Signed-off-by: Sup Yut Sum
> Signed-off-by: Sven Strickroth
> ---
> diff --git a/config.c b/config.c
> index 9fd275f..89b49e3 100644
> --- a/config.c
> +++ b/config.c
> @@ -2048,6 +2048,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> if ((store.seen == 0 && value == NULL) ||
> (store.seen > 1 && multi_replace == 0)) {
> ret = CONFIG_NOTHING_SET;
> + close(in_fd);
> goto out_free;
>From a cursory read of the code, it appears that there are several
other places where the open 'in_fd' gets leaked which would deserve
the same treatment. So, it's not clear why this patch handles only
this one case. Am I missing something?
> }
>
> --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Correctly close config file handle in case of error
On Fri, Aug 14, 2015 at 3:44 PM, Sven Strickroth wrote:
> Without this patch there might be open file handle leaks.
Thanks, this looks better. One comment below...
> Signed-off-by: Sven Strickroth
> Signed-off-by: Sup Yut Sum
> ---
> diff --git a/config.c b/config.c
> index 9fd275f..c06dc2f 100644
> --- a/config.c
> +++ b/config.c
> @@ -2010,6 +2010,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> error("invalid pattern: %s", value_regex);
> free(store.value_regex);
> ret = CONFIG_INVALID_PATTERN;
> + close(in_fd);
> goto out_free;
> }
> }
> @@ -2034,6 +2035,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> free(store.value_regex);
> }
> ret = CONFIG_INVALID_FILE;
> + close(in_fd);
> goto out_free;
> }
>
> @@ -2048,6 +2050,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> if ((store.seen == 0 && value == NULL) ||
> (store.seen > 1 && multi_replace == 0)) {
> ret = CONFIG_NOTHING_SET;
> + close(in_fd);
> goto out_free;
> }
>
> @@ -2062,6 +2065,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> config_filename, strerror(errno));
> ret = CONFIG_INVALID_FILE;
> contents = NULL;
> + close(in_fd);
> goto out_free;
Each of these cases flows through 'out_free', so an alternate approach
would be to close 'in_fd' there instead. Doing so has the benefit that
it is less likely for future code changes to make the same mistake of
failing to close the file descriptor.
Of course, you'd need to initialize 'in_fd' to some "invalid" value
(such as -1) which 'out_free' can check, as well as setting 'in_fd' to
that invalid value after the legitimate existing close().
int in_fd = -1;
...
if (whatever_error)
goto out_free;
...
close(in_fd);
in_fd = -1;
...
out_free:
if (in_fd >= 0)
close(in_fd);
...
or something...
> }
> close(in_fd);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5] http: add support for specifying the SSL version
On Fri, Aug 14, 2015 at 3:37 PM, Elia Pinto wrote:
> Teach git about a new option, "http.sslVersion", which permits one to
> specify the SSL version to use when negotiating SSL connections. The
> setting can be overridden by the GIT_SSL_VERSION environment
> variable.
>
> Signed-off-by: Elia Pinto
> ---
> This is the fifth version of the patch. Changes from the previous version:
>
> - Minor style changes (Eric)
Looks better. Thanks.
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 315f271..b23b01a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1595,6 +1595,28 @@ http.saveCookies::
> If set, store cookies received during requests to the file specified
> by
> http.cookieFile. Has no effect if http.cookieFile is unset.
>
> +http.sslVersion::
> + The SSL version to use when negotiating an SSL connection, if you
> + want to force the default. The available and default version depend
> on
> + whether libcurl was built against NSS or OpenSSL and the particular
> configuration
> + of the crypto library in use. Internally this sets the
> 'CURLOPT_SSL_VERSION'
> + option; see the libcurl documentation for more details on the format
> + of this option and for the ssl version supported. Actually the
> possible values
> + of this option are:
> +
> + - sslv2
> + - sslv3
> + - tlsv1
> + - tlsv1.0
> + - tlsv1.1
> + - tlsv1.2
> +
> ++
> +Can be overridden by the 'GIT_SSL_VERSION' environment variable.
> +To force git to use libcurl's default ssl version and ignore any
> +explicit http.sslversion option, set 'GIT_SSL_VERSION' to the
> +empty string.
> +
> http.sslCipherList::
>A list of SSL ciphers to use when negotiating an SSL connection.
>The available ciphers depend on whether libcurl was built against
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index c97c648..6e9359c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2118,6 +2118,7 @@ _git_config ()
> http.postBuffer
> http.proxy
> http.sslCipherList
> + http.sslVersion
> http.sslCAInfo
> http.sslCAPath
> http.sslCert
> diff --git a/http.c b/http.c
> index e9c6fdd..4c5a2e0 100644
> --- a/http.c
> +++ b/http.c
> @@ -37,6 +37,20 @@ static int curl_ssl_verify = -1;
> static int curl_ssl_try;
> static const char *ssl_cert;
> static const char *ssl_cipherlist;
> +static const char *ssl_version;
> +static struct {
> + const char *name;
> + long ssl_version;
> + } sslversions[] = {
> + { "sslv2", CURL_SSLVERSION_SSLv2 },
> + { "sslv3", CURL_SSLVERSION_TLSv1 },
> + { "tlsv1", CURL_SSLVERSION_TLSv1 },
> +#if LIBCURL_VERSION_NUM >= 0x072200
> + { "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
> + { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
> + { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 }
> +#endif
> +};
> #if LIBCURL_VERSION_NUM >= 0x070903
> static const char *ssl_key;
> #endif
> @@ -190,6 +204,8 @@ static int http_options(const char *var, const char
> *value, void *cb)
> }
> if (!strcmp("http.sslcipherlist", var))
> return git_config_string(&ssl_cipherlist, var, value);
> + if (!strcmp("http.sslversion", var))
> + return git_config_string(&ssl_version, var, value);
> if (!strcmp("http.sslcert", var))
> return git_config_string(&ssl_cert, var, value);
> #if LIBCURL_VERSION_NUM >= 0x070903
> @@ -364,9 +380,23 @@ static CURL *get_curl_handle(void)
> if (http_proactive_auth)
> init_curl_http_auth(result);
>
> + if (getenv("GIT_SSL_VERSION"))
> + ssl_version = getenv("GIT_SSL_VERSION");
> + if (ssl_version && *ssl_version) {
> + int i;
> + for (i = 0; i < ARRAY_SIZE(sslversions); i++) {
> + if (!strcmp(ssl_version, sslversions[i].name)) {
> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
> + sslversions[i].ssl_version);
> + break;
> + }
> + }
> + if (i == ARRAY_SIZE(sslversions))
> + warning("unsupported ssl version %s: using default",
> ssl_version);
> + }
> +
> if (getenv("GIT_SSL_CIPHER_LIST"))
> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
> -
> if (ssl_cipherlist != NULL && *ssl_cipherlist)
> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
> ssl_cipherlist);
> --
> 2.5.0.235.gb9bd8dc
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a mess
Re: [PATCH] Correctly close config file handle in case of error
On Fri, Aug 14, 2015 at 4:03 PM, Sven Strickroth wrote:
> Without this patch there might be open file handle leaks.
>
> Signed-off-by: Sven Strickroth
> Signed-off-by: Sup Yut Sum
> ---
> diff --git a/config.c b/config.c
> index 9fd275f..8138d5d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> goto out_free;
> }
> close(in_fd);
> + in_fd = -1;
You also need to initialize 'in_fd' to -1 at its point of declaration
since there are goto's to 'out_free' which occur before the `in_fd =
open(...)'.
> if (chmod(lock->filename.buf, st.st_mode & 0) < 0) {
> error("chmod on %s failed: %s",
> @@ -2148,6 +2149,8 @@ out_free:
> free(filename_buf);
> if (contents)
> munmap(contents, contents_sz);
> + if (in_fd >= 0)
> + close(in_fd);
> return ret;
>
> write_err_out:
> --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Correctly close config file handle in case of error
On Fri, Aug 14, 2015 at 4:21 PM, Sven Strickroth wrote:
> Without this patch there might be open file handle leaks.
>
> Signed-off-by: Sven Strickroth
> Signed-off-by: Sup Yut Sum
Better. Thanks.
Reviewed-by: Eric Sunshine
> ---
> diff --git a/config.c b/config.c
> index 9fd275f..83caa25 100644
> --- a/config.c
> +++ b/config.c
> @@ -1935,7 +1935,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> const char *key, const char *value,
> const char *value_regex, int multi_replace)
> {
> - int fd = -1, in_fd;
> + int fd = -1, in_fd = -1;
> int ret;
> struct lock_file *lock = NULL;
> char *filename_buf = NULL;
> @@ -2065,6 +2065,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> goto out_free;
> }
> close(in_fd);
> + in_fd = -1;
>
> if (chmod(lock->filename.buf, st.st_mode & 0) < 0) {
> error("chmod on %s failed: %s",
> @@ -2148,6 +2149,8 @@ out_free:
> free(filename_buf);
> if (contents)
> munmap(contents, contents_sz);
> + if (in_fd >= 0)
> + close(in_fd);
> return ret;
>
> write_err_out:
> --
> Best regards,
> Sven Strickroth
> PGP key id F5A9D4C4 @ any key-server
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] 2.5.0 build with NO_PERL is broken
On Fri, Aug 14, 2015 at 3:59 PM, Junio C Hamano wrote: > Renato Botelho writes: >> I also found that some commands require perl when NO_PERL is set: > > NO_PERL merely means "I want to build a subset of Git that is meant > to be usable on a system without a working Perl installed". These > scripts that do require Perl installed are indeed not expected to > work under NO_PERL (if you think about it, that would be natural and > the only sensible expectation---otherwise we would have coded them > without using Perl at all, in which case there will be no need for > NO_PERL in the first place). Nevertheless, there's still the problem, due to 527ec39 (generate-cmdlist: parse common group commands, 2015-05-21), that git doesn't build at all anymore when Perl is unavailable. One option would be to go with the awk version of 'generate-cmdlist'[1], which restricts itself to POSIX and was tested on Linux, FreeBSD, and Mac OS X[2], though I'm not sure we really want to go there, particularly if you're uncomfortable[3] about introducing awk into the toolchain. Another option would be to rewrite the (more complex) generate-cmdlist in shell, which I think should be possible, though it will be uglier and more verbose. [1]: http://article.gmane.org/gmane.comp.version-control.git/269307/ [2]: http://article.gmane.org/gmane.comp.version-control.git/269336/ [3]: http://article.gmane.org/gmane.comp.version-control.git/269324/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/4] notes: add notes.mergestrategy option to select default strategy
On Fri, Aug 14, 2015 at 4:48 PM, Jacob Keller wrote:
> From: Jacob Keller
>
> Teach git-notes about "notes.mergestrategy" to select a general strategy
> for all notes merges. This enables a user to always get expected merge
> strategy such as "cat_sort_uniq" without having to pass the "-s" option
> manually.
>
> Signed-off-by: Jacob Keller
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 042348082709..d65134f89b40 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -738,6 +738,41 @@ static int merge_commit(struct notes_merge_options *o)
> +static int git_config_get_notes_strategy(const char *key,
> +enum notes_merge_strategy *strategy)
> +{
> + const char *value = NULL;
> +
> + git_config_get_string_const(key, &value);
> +
> + if (value) {
> + if (parse_notes_strategy(value, strategy))
> + git_die_config(key, "unknown notes merge strategy
> '%s'", value);
> + else
> + return 0;
> + }
> +
> + return 1;
> +}
Nice. This seems like a better (and more user-helpful) approach.
Instead of initializing value to NULL and ignoring the return value of
git_config_get_string_const(), would it instead make sense to respect
the return of git_config_get_string_const(), like this?
const char *value;
if (!git_config_get_string_const(key, &value)) {
if (parse_notes_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy '%s'", value);
return 0;
}
return 1;
Or, the equivalent, but less indented:
if (git_config_get_string_const(key, &value))
return 1;
if (parse_notes_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy '%s'", value);
return 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] 2.5.0 build with NO_PERL is broken
On Fri, Aug 14, 2015 at 5:02 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> Nevertheless, there's still the problem, due to 527ec39 >> (generate-cmdlist: parse common group commands, 2015-05-21), that git >> doesn't build at all anymore when Perl is unavailable. > > I do not think that is anything new. We always have assumed "some" > version of Perl available in order to run t/ scripts. True, but prior to 527ec39, without Perl available, git itself could at least be built and used (with some commands unavailable), even if it couldn't be fully tested. As of 527ec39, however, git won't even build because common-cmds.h can't be generated. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option
On Fri, Aug 14, 2015 at 6:01 PM, Junio C Hamano wrote:
> Jacob Keller writes:
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 12a42b583f98..bdfd9c7d29b4 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> + strbuf_addf(&merge_key, "notes.%s.mergestrategy", short_ref);
>> +
>> + if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
>> + git_config_get_notes_strategy("notes.mergestrategy",
>> &o.strategy);
>> }
>
> I think you are leaking merge_key after you are done using it.
In addition to fixing the leak, since 'merge_key' is only used within
this block, it might also make sense to declare it in this block
rather than at the top of the function.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
