Re: [PATCH v1 00/19] Enable options --signoff, --reset-author for pick, reword
Hi Peff, Jeff King writes: On Tue, Jul 29, 2014 at 01:18:00AM +0200, Fabian Ruch wrote: this is a reroll of the patch series that enables rudimentary support of line options for git-rebase's to-do list commands and reimplements the well-known commands `reword` and `squash` in terms of a parameterised `do_pick`. I just finished reading over the whole series (which is my first real exposure to it). With the exception of the comments I already sent, it looks pretty reasonable to me. Thanks for splitting it and writing good commit messages; that made it relatively easy to follow what was going on. Thanks a lot for taking the time. Your review revealed more shortcomings of the patch series. Now that the replay of root commits is logged, the log is dumped on the console even in non-verbose mode. And, I must admit that the fact that `--no-edit` hasn't been a (documented) feature of git-commit for all time didn't struck me at all as a possible reason for using `-C`, which is a little embarrassing. I will include more details in separate replies to your comments later. Fabian -- 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
[L10N] Startup of Git 2.1.0 l10n round 1
Hi, Git v2.1.0-rc0 has been released for one week, and I'm sorry it's a bit late to announce the startup of new round of l10n. This time there are 38 new messages need to be translated since last update for v2.0.0: l10n: git.pot: v2.1.0 round 1 (38 new, 9 removed) Generate po/git.pot from v2.1.0-rc0 for git v2.1.0 l10n round 1. Signed-off-by: Jiang Xin worldhello@gmail.com You can get it from the usual place: https://github.com/git-l10n/git-po/ As how to update your XX.po and help to translate Git, please see Updating a XX.po file and other sections in “po/README file. -- Jiang Xin -- 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/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)
On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen tbo...@web.de wrote:
On 08/01/2014 07:55 PM, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Somewhat underexplained, given that it seems to add some new
semantics.
+static void clear_filename(struct lock_file *lk)
+{
+ free(lk-filename);
+ lk-filename = NULL;
+}
It is good to abstract out lk-filename[0] = '\0', which used to be
the way we say that we are done with the lock. But I am somewhat
surprised to see that there aren't so many locations that used to
check !!lk-filename[0] to see if we are done with the lock to require
a corresponding wrapper.
static void remove_lock_file(void)
{
pid_t me = getpid();
while (lock_file_list) {
if (lock_file_list-owner == me
- lock_file_list-filename[0]) {
+ lock_file_list-filename) {
... and this seems to be the only location?
While looking at possible fallout of merging this topic to any
branch, I am starting to suspect that it is probably a bad idea for
clear-filename to free lk-filename. I am wondering if it would be
safer to do:
- in lock_file(), free lk-filename if it already exists before
what you do in that function with your series;
- update is this lock already held? check !!lk-filename[0] to
check for (lk-filename !!lk-filename[0]);
- in clear_filename(), clear lk-filename[0] = '\0', but do not
free lk-filename itself.
Then existing callers that never suspected that lk-filename can be
NULL and thought that it does not need freeing can keep doing the
same thing as before without leaking nor breaking.
If we want to adopt the new world order at once, alternatively, you
can keep the code in this series but then lk-filename needs to be
renamed to something that the current code base has not heard of to
force breakage at the link time for us to notice.
I grepped for 'lk-filename' and checked if the ones in read-cache.c
and refs.c are OK (they seem to be), but that is not a very robust
check.
I dunno.
My first impression reading this patch was to rename
clear_filename() into free_and_clear_filename() or better free_filename(),
but I never pressed the send button ;-)
Reading the discussion above makes me wonder if lk-filename may be replaced
by a strbuf
some day, and in this case clear_filename() will become reset_filenmae() ?
I didn't realize Mike is making a lot more changes in lockfile.c, part
of that is converting lk-filename to use strbuf [1]. Perhaps I should
just withdraw this series, wait until Mike's series is merged, then
redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.
[1] http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232
--
Duy
--
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 report about symlinks
On Mon, Aug 4, 2014 at 12:19 AM, Junio C Hamano gits...@pobox.com wrote: I think you, who dug to find out where to add the check, already know this, and I am writing this mainly for myself and for the list archive, but when the knee-jerk has-syjmlink-leading-path missing? reaction came to me, two obvious optimizations also came to my mind: - When checking a cache entry a/b/c/d/e and we find out a/b/c is a symbolic link, we mark it as ~CE_VALID, but at the same time, we learn a/b/c/any/thing are also ~CE_VALID with that check, so we _could_, after the has_symlink_leading_path once returns true, so there may be an opportunity to fast-forward the scan, marking all paths under a/b/c as ~CE_VALID. - After finding out a/b/c is a symbolic link to cause anything underneath marked as ~CE_VALID, we also know a/ and a/b/ exists as real directories, so a later check for a/b/some/thing can start checking at a/b/some/ without checking a/ and a/b/. Just checking, you meant CE_UPTODATE, not CE_VALID, right? CE_VALID is only used with --assume-unchanged The latter is more important as it is a much more common case that the shape of the working tree not to change. But I think the implementation of has_symlink_leading_path() already has these optimizations built inside around the (unfortunately named) struct cache_def, so it probably would not give us much boost to implement such a fast-forwarding of the scan. Yeah my first thought is another flood of lstat(). But it looks like has_symlink_leading_path() tries hard to reduce lstat() already. We could disable this call in filesytems that do not support symlinks (e.g. vfat) but I guess that's just a minority. -- Duy -- 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 2/8] config.c: fix accuracy of line number in errors
Tanay Abhra tanay...@gmail.com writes: From: Matthieu Moy matthieu@grenoble-inp.fr I usually use my @imag.fr, not @grenoble-inp.fr address as Git author (even though my mailer has @grenoble-inp.fr as From: field). Both addresses are equivalent so it's no big deal. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 0/8] Rewrite `git_config()` using config-set API
Tanay Abhra tanay...@gmail.com writes: [Patch v7]: style nit corrected. (1/8) is Matthieu's translation patch. git_die_config_linenr() helper function added. Diff between v6 and v7 appended for review. This series is now Reviewed-by: Matthieu Moy matthieu@imag.fr Thanks, -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
`git_pretty_formats_config()` continues without checking git_config_string's
return value which can lead to a SEGFAULT. Instead return -1 when
git_config_string fails signalling `git_config()` to die printing the location
of the erroneous variable.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pretty.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/pretty.c b/pretty.c
index 3a1da6f..72dbf55 100644
--- a/pretty.c
+++ b/pretty.c
@@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const
char *value, void *c
commit_format-name = xstrdup(name);
commit_format-format = CMIT_FMT_USERFORMAT;
- git_config_string(fmt, var, value);
+ if (git_config_string(fmt, var, value))
+ return -1;
+
if (starts_with(fmt, format:) || starts_with(fmt, tformat:)) {
commit_format-is_tformat = fmt[0] == 't';
fmt = strchr(fmt, ':') + 1;
--
1.9.0.GIT
--
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 RFC v2 05/19] rebase -i: Implement reword in terms of do_pick
Fabian Ruch baf...@gmail.com writes:
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -555,20 +555,7 @@ do_next () {
comment_for_reflog reword
mark_action_done
- do_pick $sha1 $rest
- # TODO: Work around the fact that git-commit lets us
- # disable either both the pre-commit and the commit-msg
- # hook or none. Disable the pre-commit hook because the
- # tree is left unchanged but run the commit-msg hook
- # from here because the log message is altered.
- git commit --allow-empty --amend --no-post-rewrite -n
${gpg_sign_opt:+$gpg_sign_opt}
- if test -x $GIT_DIR/hooks/commit-msg
- then
- $GIT_DIR/hooks/commit-msg
$GIT_DIR/COMMIT_EDITMSG
- fi || {
- warn Could not amend commit after successfully
picking $sha1... $rest
- exit_with_patch $sha1 1
- }
+ do_pick --edit $sha1 $rest
I would have found this easier to review if squashed into the previous
patch. My reaction reading the previous patch was Uh, why duplicate
code?, and reading this one Ah, that's OK. A single patch doing both
would have avoided the confusion.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
Tanay Abhra tanay...@gmail.com writes: `git_pretty_formats_config()` continues without checking git_config_string's return value which can lead to a SEGFAULT. Indeed, without the patch: $ git -c pretty.my= log --pretty=my error: Missing value for 'pretty.my' zsh: segmentation fault git -c pretty.my= log --pretty=my diff --git a/pretty.c b/pretty.c index 3a1da6f..72dbf55 100644 --- a/pretty.c +++ b/pretty.c @@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c commit_format-name = xstrdup(name); commit_format-format = CMIT_FMT_USERFORMAT; - git_config_string(fmt, var, value); + if (git_config_string(fmt, var, value)) + return -1; + Ack-ed-by: Matthieu Moy matthieu@imag.fr My first thought reading this was why not rewrite using non-callback API?, but this particular call to git_config needs to iterate over config keys anyway. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 RFC v2 05/19] rebase -i: Implement reword in terms of do_pick
Hi Matthieu,
thanks for taking a look at this patch series. I might have caused some
confusion by starting with version v1 again after removing the RFC tag.
The current reroll[1] is tagged with PATCH v1, not PATCH RFC v2.
I'm sorry if this is the reason why your reply appears on this sub-thread.
Your concerns below are of course noted.
Fabian
[1] http://article.gmane.org/gmane.comp.version-control.git/254361
Matthieu Moy writes:
Fabian Ruch baf...@gmail.com writes:
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -555,20 +555,7 @@ do_next () {
comment_for_reflog reword
mark_action_done
-do_pick $sha1 $rest
-# TODO: Work around the fact that git-commit lets us
-# disable either both the pre-commit and the commit-msg
-# hook or none. Disable the pre-commit hook because the
-# tree is left unchanged but run the commit-msg hook
-# from here because the log message is altered.
-git commit --allow-empty --amend --no-post-rewrite -n
${gpg_sign_opt:+$gpg_sign_opt}
-if test -x $GIT_DIR/hooks/commit-msg
-then
-$GIT_DIR/hooks/commit-msg
$GIT_DIR/COMMIT_EDITMSG
-fi || {
-warn Could not amend commit after successfully
picking $sha1... $rest
-exit_with_patch $sha1 1
-}
+do_pick --edit $sha1 $rest
I would have found this easier to review if squashed into the previous
patch. My reaction reading the previous patch was Uh, why duplicate
code?, and reading this one Ah, that's OK. A single patch doing both
would have avoided the confusion.
--
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 report about symlinks
Duy Nguyen pclo...@gmail.com writes: Just checking, you meant CE_UPTODATE, not CE_VALID, right? CE_VALID is only used with --assume-unchanged Yup. 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: Bug report about symlinks
René Scharfe l@web.de writes: Am 03.08.2014 um 19:19 schrieb Junio C Hamano: And do we need to use the threaded_ variant of the function here? Hmmm, this is a tangent, but you comment made me wonder if we also need to adjust preload_thread() in preload-index.c somehow, but we do not touch CE_UPTODATE there, so it probably is not necessary. The function calls ce_mark_uptodate(), which does set CE_UPTODATE. It calls threaded_has_symlink_leading_path() before lstat() already, however. (Since f62ce3de: Make index preloading check the whole path to the file.) Yeah, by we do not touch, I meant for paths that is beyond a symlink, we do not touch (i.e. we have that continue before lstat-match-then-mark sequence). The caller of refresh_cache_ent() is walking an array of sorted pathnames aka istate-cache[] in a single-threaded fashion, possibly with a pathspec to limit the scan. There are two direct callers (refresh_index(), refresh_cache_entry()) and several indirect ones. Do we have a way to detect unsynchronized parallel access to the has_symlink_leading_path()-cache? Checking the full callers-of-callers tree manually looks a bit scary to me. The threaded variant is not used anybody outside preload-index, so currently we should be OK, I would think. -- 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/1] doc: format-patch: don't use origin as a branch name
Philip Oakley philipoak...@iee.org writes: Historically (5 Nov 2005 v0.99.9-46-g28ffb89) the git-format-patch used 'origin' as the upstream branch name. That name is now used as the nominal name for the upstream remote. While 'origin' would be DWIMmed (do what I mean) to be that remote's primary branch, do not assume the reader is ready for such magic. Good thinking. Likewise, do not use 'origin/master' which may not be up to date with the remote, nor reflect the reader's master branch. The patch series should be relative to the reader's view of 'git show-branch HEAD master'. This however is backwards, no? The history on 'origin/master' may not be up-to-date in the sense that if you run 'git fetch' you might get more, but it absolutely is up-to-date in the sense that it shows what the origin has to the best of your repository's current knowledge. Compared to that, what the user's local 'master' has is much less relevant. For one thing, if a more recent commit that is on the remote repository is missing on 'origin/master' because you haven't fetched recently, by definition that commit will not be on your 'master' either, so you have the same staleness issue to the exact degree. Even worse, when you are developing a topic to upstream, it is a good practice to merge your topic to your own 'master' to check it with the wider project codebase that is more recent than where your topic earlier forked from, and it makes little sense to tell 'exclude what I have on my master' to format-patch when extracting changes to upstream out of such a topic. You send what the other side has, not what you do not have on your local 'master' branch. Use the more modern 'master' as the reference branch name. There is nothing 'modern' in 'master'. I think the original description was written before we switched to the separate remote layout. What is in 'refs/remote/origin/master' these days was stored and updated at 'refs/heads/origin' and no other branch from the remote repository was tracked back then. The changes to be upstreamed are output by grabbing what are not in 'origin', whose modern equivalent is 'origin/master'. In short, if your patch were s|origin|origin/master|, instead of s|origin|master|, that would be an adjustment to the more modern world that is still faithful to the intent of the original. Signed-off-by: Philip Oakley philipoak...@iee.org --- Documentation/git-format-patch.txt | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index c0fd470..b0f041f 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -523,25 +523,25 @@ $ git format-patch -k --stdout R1..R2 | git am -3 -k * Extract all commits which are in the current branch but not in the -origin branch: +master branch: + -$ git format-patch origin +$ git format-patch master + For each commit a separate file is created in the current directory. -* Extract all commits that lead to 'origin' since the inception of the +* Extract all commits that lead to 'master' since the inception of the project: + -$ git format-patch --root origin +$ git format-patch --root master * The same as the previous one: + -$ git format-patch -M -B origin +$ git format-patch -M -B master + Additionally, it detects and handles renames and complete rewrites -- 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: Everday contents
Philip Oakley philipoak...@iee.org writes: From: Junio C Hamano gits...@pobox.com Philip Oakley philipoak...@iee.org writes: From: Junio C Hamano gits...@pobox.com Sent: Friday, July 25, 2014 11:08 PM ... | Individual Developer (Participant)[[Individual Developer (Participant)]] | ... | $ git pull git://git.kernel.org/pub/.../jgarzik/libata-dev.git ALL 5 Would I be right that ALL can simply be dropped as something from back then' (13 Dec 2005 v0.99.9-516-g44db136) that I'm ignorant of? The answer depends on the reason why you want to drop it from the example. That ALL is merely a branch name, like master, etc. That was the bit I was missing. I had it in my head that it was possibly some historic option as it was in caps. Yeah, I suspected as much ;-) Thanks for a careful reading. -- 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/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)
Duy Nguyen pclo...@gmail.com writes: I didn't realize Mike is making a lot more changes in lockfile.c, part of that is converting lk-filename to use strbuf [1]. Perhaps I should just withdraw this series, wait until Mike's series is merged, then redo 3/3 on top. Or Mike could just take 3/3 in as part of his series. During the pre-release freeze I would like to see new topics be calmer ;-) Serializing or not, inter-developer coordination is always very much appreciated. 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: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API
Tanay Abhra tanay...@gmail.com writes:
Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.
It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,
if (!git_config_get_value(key, value)) {
/* NULL values not allowed */
if (!value)
git_config_die(key);
else
/* do work */
}
It feels a bit unnatural at the API level that this does not take
'value'; I do understand that it is not a big deal in the error code
path to locate again the value from the configuration using the key,
but still.
It feels even more unnatural that the caller cannot say _how_ it
finds the value offending by not taking any message. For one
particular callchain, e.g. git_config_get_string() that eventually
calls git_config_string() which will show an error message via
config_error_nonbool(), you may not want any extra message, but for
new callers that wants to make sure value falls within a supported
range, this forces it to write
if (!git_config_get_int(key, num)) {
if (!(0 num num 4)) {
error('%s' must be between 1 and 3);
git_config_die(key);
}
/* otherwise work */
}
and then the error message would say something like:
error: 'core.frotz' must be between 1 and 3
fatal: bad config variable 'core.frotz' at file line 15 in .git/config
which sounds somewhat backwards, at least to me.
+NORETURN
+void git_die_config_linenr(const char *key, const char *filename, int linenr)
+{
+ if (!linenr)
+ die(_(unable to parse '%s' from command-line config), key);
Do we have existing code that says we signal that it is from the
command line by setting linenr to zero already? Otherwise I would
have thought filename == NULL would be a more sensible convention.
Otherwise OK.
+ else
+ die(_(bad config variable '%s' at file line %d in %s),
At least, quote the last '%s'.
+ key,
+ linenr,
+ filename);
Don't waste vertical real-estate line this.
Perhaps
die(_(bad config variable '%s' in file '%s' at line %d),
key, linenr, filename);
--
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 7/8] add a test for semantic errors in config files
Tanay Abhra tanay...@gmail.com writes: + grep fatal: bad config variable '\''alias.br'\'' at file line 2 in .git/config result This test is too tight (the full string), and also needs to know about i18n, I think. -- 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/1] doc: format-patch: don't use origin as a branch name
Junio C Hamano gits...@pobox.com writes: Compared to that, what the user's local 'master' has is much less relevant. For one thing, if a more recent commit that is on the remote repository is missing on 'origin/master' because you haven't fetched recently, by definition that commit will not be on your 'master' either, so you have the same staleness issue to the exact degree. Even worse, when you are developing a topic to upstream, it clarification. I used to upstream as a verb to mean sending the work you did to be applied. is a good practice to merge your topic to your own 'master' to check it with the wider project codebase that is more recent than where your topic earlier forked from, and it makes little sense to tell 'exclude what I have on my master' to format-patch when extracting changes to upstream out of such a topic. You send what the other side has, not what you do not have on your local 'master' branch. and I have a stupid typo here; obviously I should have typed: You send what the other side does not have. -- 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] Documentation: avoid dangling modifier for imap-send
brian m. carlson sand...@crustytoothpaste.net writes: Avoid a nonsensical misreading by moving the modifier closer to the word it modifies. Signed-off-by: brian m. carlson sand...@crustytoothpaste.net --- Documentation/git-imap-send.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..23231e1 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -43,7 +43,7 @@ imap.folder:: imap.tunnel:: Command used to setup a tunnel to the IMAP server through which commands will be piped instead of using a direct network connection - to the server. Required when imap.host is not set to use imap-send. + to the server. Required to use imap-send when imap.host is not set. To be honest, I find both versions are equally confusing. How about dropping the three words to use imap-send? -- 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 00/11] git_config callers rewritten with the new config-set API
The ta/config-set API is more or less solidified. This series builds on the top of 4c715ebb in pu (ta/config-set). On top of it, it also requires series [1] (Rewrite `git_config()` using config-set API) for proper error checking. This series is the first batch of patches which rewrites the existing callers using a non-callback approach. This series aims to, * rewrite the existing callers, as you can see from the diff stat the bew API provides a much concise and clear control flow. * stress test the new API, see if any corner cases or deficiencies arise or not. The series passes all the tests, only thing to watch is that the config variables that have been rewritten are single valued only. Though I have tried my best to ascertain it, still mistakes may arise. p.s: I haven't decided yet about whether to introduce a new helper set, for example git_config_get_value_fmt() which would behave like strbuf_addf(). It will probably come in a later series. [1]: http://thread.gmane.org/gmane.comp.version-control.git/254633/ Tanay Abhra (11): alias.c| 25 ++-- archive.c | 12 +++- branch.c | 27 +++--- builtin/gc.c | 51 +++- daemon.c | 27 +- fetch-pack.c | 35 - http-backend.c | 31 - imap-send.c| 61 ++ pager.c| 40 +- read-cache.c | 14 +++--- rerere.c | 43 - 11 files changed, 116 insertions(+), 250 deletions(-) -- 1.9.0.GIT -- 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 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
http-backend.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 80790bb..106ca6b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -219,29 +219,22 @@ static void get_idx_file(char *name)
send_local_file(application/x-git-packed-objects-toc, name);
}
-static int http_config(const char *var, const char *value, void *cb)
+static void http_config(void)
{
- const char *p;
+ int i, value = 0;
+ struct strbuf var = STRBUF_INIT;
- if (!strcmp(var, http.getanyfile)) {
- getanyfile = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool(http.getanyfile, getanyfile);
- if (skip_prefix(var, http., p)) {
- int i;
-
- for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
- struct rpc_service *svc = rpc_service[i];
- if (!strcmp(p, svc-config_name)) {
- svc-enabled = git_config_bool(var, value);
- return 0;
- }
- }
+ for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
+ struct rpc_service *svc = rpc_service[i];
+ strbuf_addf(var, http.%s, svc-config_name);
+ if (!git_config_get_bool(var.buf, value))
+ svc-enabled = value;
+ strbuf_reset(var);
}
- /* we are not interested in parsing any other configuration here */
- return 0;
+ strbuf_release(var);
}
static struct rpc_service *select_service(const char *name)
@@ -627,7 +620,7 @@ int main(int argc, char **argv)
access(git-daemon-export-ok, F_OK) )
not_found(Repository not exported: '%s', dir);
- git_config(http_config, NULL);
+ http_config();
cmd-imp(cmd_arg);
return 0;
}
--
1.9.0.GIT
--
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 06/11] rerere.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
rerere.c | 43 ---
1 file changed, 12 insertions(+), 31 deletions(-)
diff --git a/rerere.c b/rerere.c
index d84b495..20b18ad 100644
--- a/rerere.c
+++ b/rerere.c
@@ -573,15 +573,11 @@ static int do_plain_rerere(struct string_list *rr, int fd)
return write_rr(rr, fd);
}
-static int git_rerere_config(const char *var, const char *value, void *cb)
+static void git_rerere_config(void)
{
- if (!strcmp(var, rerere.enabled))
- rerere_enabled = git_config_bool(var, value);
- else if (!strcmp(var, rerere.autoupdate))
- rerere_autoupdate = git_config_bool(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
+ git_config_get_bool(rerere.enabled, rerere_enabled);
+ git_config_get_bool(rerere.autoupdate, rerere_autoupdate);
+ git_config(git_default_config, NULL);
}
static int is_rerere_enabled(void)
@@ -606,7 +602,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)
{
int fd;
- git_config(git_rerere_config, NULL);
+ git_rerere_config();
if (!is_rerere_enabled())
return -1;
@@ -699,24 +695,6 @@ static void unlink_rr_item(const char *name)
rmdir(git_path(rr-cache/%s, name));
}
-struct rerere_gc_config_cb {
- int cutoff_noresolve;
- int cutoff_resolve;
-};
-
-static int git_rerere_gc_config(const char *var, const char *value, void *cb)
-{
- struct rerere_gc_config_cb *cf = cb;
-
- if (!strcmp(var, gc.rerereresolved))
- cf-cutoff_resolve = git_config_int(var, value);
- else if (!strcmp(var, gc.rerereunresolved))
- cf-cutoff_noresolve = git_config_int(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
-}
-
void rerere_gc(struct string_list *rr)
{
struct string_list to_remove = STRING_LIST_INIT_DUP;
@@ -724,9 +702,12 @@ void rerere_gc(struct string_list *rr)
struct dirent *e;
int i, cutoff;
time_t now = time(NULL), then;
- struct rerere_gc_config_cb cf = { 15, 60 };
+ int cutoff_noresolve = 15;
+ int cutoff_resolve = 60;
- git_config(git_rerere_gc_config, cf);
+ git_config_get_int(gc.rerereresolved, cutoff_resolve);
+ git_config_get_int(gc.rerereunresolved, cutoff_noresolve);
+ git_config(git_default_config, NULL);
dir = opendir(git_path(rr-cache));
if (!dir)
die_errno(unable to open rr-cache directory);
@@ -736,12 +717,12 @@ void rerere_gc(struct string_list *rr)
then = rerere_last_used_at(e-d_name);
if (then) {
- cutoff = cf.cutoff_resolve;
+ cutoff = cutoff_resolve;
} else {
then = rerere_created_at(e-d_name);
if (!then)
continue;
- cutoff = cf.cutoff_noresolve;
+ cutoff = cutoff_noresolve;
}
if (then now - cutoff * 86400)
string_list_append(to_remove, e-d_name);
--
1.9.0.GIT
--
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 05/11] fetchpack.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
fetch-pack.c | 35 ---
1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index b8a58fa..a13e9db 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -869,34 +869,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
*args,
return ref;
}
-static int fetch_pack_config(const char *var, const char *value, void *cb)
+static void fetch_pack_config(void)
{
- if (strcmp(var, fetch.unpacklimit) == 0) {
- fetch_unpack_limit = git_config_int(var, value);
- return 0;
- }
-
- if (strcmp(var, transfer.unpacklimit) == 0) {
- transfer_unpack_limit = git_config_int(var, value);
- return 0;
- }
-
- if (strcmp(var, repack.usedeltabaseoffset) == 0) {
- prefer_ofs_delta = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, fetch.fsckobjects)) {
- fetch_fsck_objects = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, transfer.fsckobjects)) {
- transfer_fsck_objects = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_int(fetch.unpacklimit, fetch_unpack_limit);
+ git_config_get_int(transfer.unpacklimit, transfer_unpack_limit);
+ git_config_get_bool(repack.usedeltabaseoffset, prefer_ofs_delta);
+ git_config_get_bool(fetch.fsckobjects, fetch_fsck_objects);
+ git_config_get_bool(transfer.fsckobjects, transfer_fsck_objects);
- return git_default_config(var, value, cb);
+ git_config(git_default_config, NULL);
}
static void fetch_pack_setup(void)
@@ -904,7 +885,7 @@ static void fetch_pack_setup(void)
static int did_setup;
if (did_setup)
return;
- git_config(fetch_pack_config, NULL);
+ fetch_pack_config();
if (0 = transfer_unpack_limit)
unpack_limit = transfer_unpack_limit;
else if (0 = fetch_unpack_limit)
--
1.9.0.GIT
--
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 04/11] archive.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
archive.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/archive.c b/archive.c
index 3fc0fb2..952a659 100644
--- a/archive.c
+++ b/archive.c
@@ -402,14 +402,6 @@ static int parse_archive_args(int argc, const char **argv,
return argc;
}
-static int git_default_archive_config(const char *var, const char *value,
- void *cb)
-{
- if (!strcmp(var, uploadarchive.allowunreachable))
- remote_allow_unreachable = git_config_bool(var, value);
- return git_default_config(var, value, cb);
-}
-
int write_archive(int argc, const char **argv, const char *prefix,
int setup_prefix, const char *name_hint, int remote)
{
@@ -420,7 +412,9 @@ int write_archive(int argc, const char **argv, const char
*prefix,
if (setup_prefix prefix == NULL)
prefix = setup_git_directory_gently(nongit);
- git_config(git_default_archive_config, NULL);
+ git_config_get_bool(uploadarchive.allowunreachable,
remote_allow_unreachable);
+ git_config(git_default_config, NULL);
+
init_tar_archiver();
init_zip_archiver();
--
1.9.0.GIT
--
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 03/11] read-cache.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.
Use an intermediate value, as `version` can not be used directly in
git_config_get_int() due to incompatible type.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
read-cache.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..acb132d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1238,24 +1238,16 @@ static struct cache_entry *refresh_cache_entry(struct
cache_entry *ce,
#define INDEX_FORMAT_DEFAULT 3
-static int index_format_config(const char *var, const char *value, void *cb)
-{
- unsigned int *version = cb;
- if (!strcmp(var, index.version)) {
- *version = git_config_int(var, value);
- return 0;
- }
- return 1;
-}
-
static unsigned int get_index_format_default(void)
{
char *envversion = getenv(GIT_INDEX_VERSION);
char *endp;
+ int value;
unsigned int version = INDEX_FORMAT_DEFAULT;
if (!envversion) {
- git_config(index_format_config, version);
+ if (!git_config_get_int(index.version, value))
+ version = value;
if (version INDEX_FORMAT_LB || INDEX_FORMAT_UB version) {
warning(_(index.version set, but the value is
invalid.\n
Using version %i), INDEX_FORMAT_DEFAULT);
--
1.9.0.GIT
--
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 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
daemon.c | 27 +--
1 file changed, 5 insertions(+), 22 deletions(-)
diff --git a/daemon.c b/daemon.c
index e6b51ed..fb16664 100644
--- a/daemon.c
+++ b/daemon.c
@@ -230,23 +230,6 @@ struct daemon_service {
int overridable;
};
-static struct daemon_service *service_looking_at;
-static int service_enabled;
-
-static int git_daemon_config(const char *var, const char *value, void *cb)
-{
- const char *service;
-
- if (skip_prefix(var, daemon., service)
- !strcmp(service, service_looking_at-config_name)) {
- service_enabled = git_config_bool(var, value);
- return 0;
- }
-
- /* we are not interested in parsing any other configuration here */
- return 0;
-}
-
static int daemon_error(const char *dir, const char *msg)
{
if (!informative_errors)
@@ -324,6 +307,7 @@ static int run_service(const char *dir, struct
daemon_service *service)
{
const char *path;
int enabled = service-enabled;
+ struct strbuf var = STRBUF_INIT;
loginfo(Request %s for '%s', service-name, dir);
@@ -354,12 +338,11 @@ static int run_service(const char *dir, struct
daemon_service *service)
}
if (service-overridable) {
- service_looking_at = service;
- service_enabled = -1;
- git_config(git_daemon_config, NULL);
- if (0 = service_enabled)
- enabled = service_enabled;
+ strbuf_addf(var, daemon.%s, service-config_name);
+ git_config_get_bool(var.buf, enabled);
+ strbuf_release(var);
}
+
if (!enabled) {
logerror('%s': service not enabled for '%s',
service-name, path);
--
1.9.0.GIT
--
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 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
builtin/gc.c | 51 ---
1 file changed, 20 insertions(+), 31 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..4612ef5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -55,44 +55,33 @@ static void remove_pidfile_on_signal(int signo)
raise(signo);
}
-static int gc_config(const char *var, const char *value, void *cb)
+static void gc_config(void)
{
- if (!strcmp(var, gc.packrefs)) {
+ const char *value;
+
+ if (!git_config_get_value(gc.packrefs, value)) {
if (value !strcmp(value, notbare))
pack_refs = -1;
else
- pack_refs = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, gc.aggressivewindow)) {
- aggressive_window = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.aggressivedepth)) {
- aggressive_depth = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.auto)) {
- gc_auto_threshold = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.autopacklimit)) {
- gc_auto_pack_limit = git_config_int(var, value);
- return 0;
+ pack_refs = git_config_bool(gc.packrefs, value);
}
- if (!strcmp(var, gc.autodetach)) {
- detach_auto = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, gc.pruneexpire)) {
- if (value strcmp(value, now)) {
+
+ git_config_get_int(gc.aggressivewindow, aggressive_window);
+ git_config_get_int(gc.aggressivedepth, aggressive_depth);
+ git_config_get_int(gc.auto, gc_auto_threshold);
+ git_config_get_int(gc.autopacklimit, gc_auto_pack_limit);
+ git_config_get_bool(gc.autodetach, detach_auto);
+
+ if (!git_config_get_string_const(gc.pruneexpire, prune_expire)) {
+ if (strcmp(prune_expire, now)) {
unsigned long now = approxidate(now);
- if (approxidate(value) = now)
- return error(_(Invalid %s: '%s'), var, value);
+ if (approxidate(prune_expire) = now) {
+ error(_(Invalid %s: '%s'), gc.pruneexpire,
prune_expire);
+ git_die_config(gc.pruneexpire);
+ }
}
- return git_config_string(prune_expire, var, value);
}
- return git_default_config(var, value, cb);
+ git_config(git_default_config, NULL);
}
static int too_many_loose_objects(void)
@@ -301,7 +290,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_pushl(prune, prune, --expire, NULL );
argv_array_pushl(rerere, rerere, gc, NULL);
- git_config(gc_config, NULL);
+ gc_config();
if (pack_refs 0)
pack_refs = !is_bare_repository();
--
1.9.0.GIT
--
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 08/11] pager.c: replace `git_config()` with `git_config_get_value()`
Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pager.c | 40 +---
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
#define DEFAULT_PAGER less
#endif
-struct pager_config {
- const char *cmd;
- int want;
- char *value;
-};
-
/*
* This is split up from the rest of git so that we can do
* something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
return width;
}
-static int pager_command_config(const char *var, const char *value, void *data)
+/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
+int check_pager_config(const char *cmd)
{
- struct pager_config *c = data;
- if (starts_with(var, pager.) !strcmp(var + 6, c-cmd)) {
- int b = git_config_maybe_bool(var, value);
+ int want = -1;
+ struct strbuf key = STRBUF_INIT;
+ const char *value = NULL;
+ strbuf_addf(key, pager.%s, cmd);
+ if (!git_config_get_value(key.buf, value)) {
+ int b = git_config_maybe_bool(key.buf, value);
if (b = 0)
- c-want = b;
+ want = b;
else {
- c-want = 1;
- c-value = xstrdup(value);
+ want = 1;
+ pager_program = xstrdup(value);
}
}
- return 0;
-}
-
-/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
-int check_pager_config(const char *cmd)
-{
- struct pager_config c;
- c.cmd = cmd;
- c.want = -1;
- c.value = NULL;
- git_config(pager_command_config, c);
- if (c.value)
- pager_program = c.value;
- return c.want;
+ strbuf_release(key);
+ return want;
}
--
1.9.0.GIT
--
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 11/11] branch.c: replace `git_config()` with `git_config_get_string()
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 27 +++
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/branch.c b/branch.c
index 735767d..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,30 +140,17 @@ static int setup_tracking(const char *new_ref, const char
*orig_ref,
return 0;
}
-struct branch_desc_cb {
- const char *config_name;
- const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
- struct branch_desc_cb *desc = cb;
- if (strcmp(desc-config_name, var))
- return 0;
- free((char *)desc-value);
- return git_config_string(desc-value, var, value);
-}
-
int read_branch_desc(struct strbuf *buf, const char *branch_name)
{
- struct branch_desc_cb cb;
+ char *v = NULL;
struct strbuf name = STRBUF_INIT;
strbuf_addf(name, branch.%s.description, branch_name);
- cb.config_name = name.buf;
- cb.value = NULL;
- git_config(read_branch_desc_cb, cb);
- if (cb.value)
- strbuf_addstr(buf, cb.value);
+ if (git_config_get_string(name.buf, v)) {
+ strbuf_release(name);
+ return -1;
+ }
+ strbuf_addstr(buf, v);
+ free(v);
strbuf_release(name);
return 0;
}
--
1.9.0.GIT
--
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 10/11] alias.c: replace `git_config()` with `git_config_get_string()`
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
alias.c | 25 ++---
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/alias.c b/alias.c
index 758c867..6aa164a 100644
--- a/alias.c
+++ b/alias.c
@@ -1,26 +1,13 @@
#include cache.h
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
- const char *name;
- if (skip_prefix(k, alias., name) !strcmp(name, alias_key)) {
- if (!v)
- return config_error_nonbool(k);
- alias_val = xstrdup(v);
- return 0;
- }
- return 0;
-}
-
char *alias_lookup(const char *alias)
{
- alias_key = alias;
- alias_val = NULL;
- git_config(alias_lookup_cb, NULL);
- return alias_val;
+ char *v = NULL;
+ struct strbuf key = STRBUF_INIT;
+ strbuf_addf(key, alias.%s, alias);
+ git_config_get_string(key.buf, v);
+ strbuf_release(key);
+ return v;
}
#define SPLIT_CMDLINE_BAD_ENDING 1
--
1.9.0.GIT
--
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 09/11] imap-send.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
imap-send.c | 61 +++--
1 file changed, 27 insertions(+), 34 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 524fbab..586bdd8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,36 @@ static int split_msg(struct strbuf *all_msgs, struct
strbuf *msg, int *ofs)
static char *imap_folder;
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
{
- if (!skip_prefix(key, imap., key))
- return 0;
+ const char *val = NULL;
+
+ git_config_get_bool(imap.sslverify, server.ssl_verify);
+ git_config_get_bool(imap.preformattedhtml, server.use_html);
+ git_config_get_string(imap.folder, imap_folder);
- /* check booleans first, and barf on others */
- if (!strcmp(sslverify, key))
- server.ssl_verify = git_config_bool(key, val);
- else if (!strcmp(preformattedhtml, key))
- server.use_html = git_config_bool(key, val);
- else if (!val)
- return config_error_nonbool(key);
-
- if (!strcmp(folder, key)) {
- imap_folder = xstrdup(val);
- } else if (!strcmp(host, key)) {
- if (starts_with(val, imap:))
- val += 5;
- else if (starts_with(val, imaps:)) {
- val += 6;
- server.use_ssl = 1;
+ if (!git_config_get_value(imap.host, val)) {
+ if (!val) {
+ config_error_nonbool(imap.host);
+ git_die_config(imap.host);
+ } else {
+ if (starts_with(val, imap:))
+ val += 5;
+ else if (starts_with(val, imaps:)) {
+ val += 6;
+ server.use_ssl = 1;
+ }
+ if (starts_with(val, //))
+ val += 2;
+ server.host = xstrdup(val);
}
- if (starts_with(val, //))
- val += 2;
- server.host = xstrdup(val);
- } else if (!strcmp(user, key))
- server.user = xstrdup(val);
- else if (!strcmp(pass, key))
- server.pass = xstrdup(val);
- else if (!strcmp(port, key))
- server.port = git_config_int(key, val);
- else if (!strcmp(tunnel, key))
- server.tunnel = xstrdup(val);
- else if (!strcmp(authmethod, key))
- server.auth_method = xstrdup(val);
+ }
- return 0;
+ git_config_get_string(imap.user, server.user);
+ git_config_get_string(imap.pass, server.pass);
+ git_config_get_int(imap.port, server.port);
+ git_config_get_string(imap.tunnel, server.tunnel);
+ git_config_get_string(imap.authmethod, server.auth_method);
}
int main(int argc, char **argv)
@@ -1383,7 +1376,7 @@ int main(int argc, char **argv)
usage(imap_send_usage);
setup_git_directory_gently(nongit_ok);
- git_config(git_imap_config, NULL);
+ git_imap_config();
if (!server.port)
server.port = server.use_ssl ? 993 : 143;
--
1.9.0.GIT
--
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 03/19] rebase -i: reword executes pre-commit hook on interim commit
Hi,
Jeff King writes:
On Tue, Jul 29, 2014 at 01:18:03AM +0200, Fabian Ruch wrote:
Specify the git-commit option `--no-verify` to disable the pre-commit
hook when editing the log message. Because `--no-verify` also skips
the commit-msg hook, execute the hook from within
git-rebase--interactive after the commit is created. Fortunately, the
commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
git-commit terminates. Caveat: In case the commit-msg hook finds the
new log message ill-formatted, the user is only notified of the
failed commit-msg hook but the log message is used for the commit
anyway. git-commit ought to offer more fine-grained control over
which hooks are executed.
Thanks for a nice explanation of the tradeoff. Have you looked at adding
an option to git-commit? We already have --no-post-rewrite. I think you
would just need:
diff --git a/builtin/commit.c b/builtin/commit.c
index 5ed6036..f7af220 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -102,6 +102,7 @@ static int quiet, verbose, no_verify, allow_empty,
dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
static char *sign_commit;
+static int no_pre_commit;
/*
* The default commit message cleanup mode will remove the lines
@@ -661,7 +662,8 @@ static int prepare_to_commit(const char *index_file,
const char *prefix,
/* This checks and barfs if author is badly specified */
determine_author_info(author_ident);
- if (!no_verify run_commit_hook(use_editor, index_file, pre-commit,
NULL))
+ if (!no_verify !no_pre_commit
+ run_commit_hook(use_editor, index_file, pre-commit, NULL))
return 0;
if (squash_message) {
@@ -1604,6 +1606,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
N_(terminate entries with NUL)),
OPT_BOOL(0, amend, amend, N_(amend previous commit)),
OPT_BOOL(0, no-post-rewrite, no_post_rewrite, N_(bypass
post-rewrite hook)),
+ OPT_BOOL(0, no-pre-commit, no_pre_commit, N_(bypass
pre-commit hook)),
{ OPTION_STRING, 'u', untracked-files, untracked_files_arg,
N_(mode), N_(show untracked files, optional modes: all, normal, no.
(Default: all)), PARSE_OPT_OPTARG, NULL, (intptr_t)all },
/* end commit contents options */
though I would also not be opposed to some more uniform hook selection
mechanism (e.g., --no-verify=pre-commit or something).
While the --no-verify= mechanism doesn't add a new option to the
git-commit interface but lets one refine the --no-verify option, users
might find it weird to have an argument to name disabled hooks but then
not be able to disable all hooks that way.
To have that uniform hook selection without duplicating code, we might
want to have something like --bypass-hook= right away. This would cover
--no-post-rewrite as well and add support for disabling
prepare-commit-msg and post-commit, whose execution cannot be controlled
via the git-commit interface at the moment.
Since the hook selection wouldn't have to change, the options parsing
code seems to be simpler (--bypass-hook= would have to support several
occurrences with different arguments which could be implemented as an
OPT_CALLBACK?) and git-commit decided to have --no- options for hook
selection so far, I would lean towards your patch from above.
I know you didn't say anything otherwise, I just wanted to expand a
little. You find an amended version of your patch below that documents
--no-verify as a synonym for --no-pre-commit and --no-commit-msg, and
adds tests.
diff --git a/builtin/commit.c b/builtin/commit.c
index 5e2221c..813aa78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -98,12 +98,27 @@ static char *edit_message, *use_message;
static char *fixup_message, *squash_message;
static int all, also, interactive, patch_interactive, only, amend, signoff;
static int edit_flag = -1; /* unspecified */
-static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
+static int quiet, verbose, allow_empty, dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
static char *sign_commit;
/*
+ * The verify variable is interpreted as a bitmap of enabled commit
+ * verification hooks according to the legend below.
+ *
+ * By default, the pre-commit and commit-msg hooks are enabled. This
+ * is represented by both the PRE_COMMIT and COMMIT_MSG bits being
+ * set.
+ *
+ * The bitmap is changed through the command line options
+ * --no-verify, --no-pre-commit and --no-commit-msg.
+ */
+#define PRE_COMMIT (10)
+#define COMMIT_MSG (11)
+static int verify = PRE_COMMIT | COMMIT_MSG;
+
+/*
* The default commit message cleanup mode will remove the lines
*
Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API
On 8/4/2014 11:37 PM, Junio C Hamano wrote:
Tanay Abhra tanay...@gmail.com writes:
Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.
It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,
if (!git_config_get_value(key, value)) {
/* NULL values not allowed */
if (!value)
git_config_die(key);
else
/* do work */
}
It feels a bit unnatural at the API level that this does not take
'value'; I do understand that it is not a big deal in the error code
path to locate again the value from the configuration using the key,
but still.
But, we don't have a use for value as it is not denoted in the error
string, that is why I left it out.
It feels even more unnatural that the caller cannot say _how_ it
finds the value offending by not taking any message. For one
particular callchain, e.g. git_config_get_string() that eventually
calls git_config_string() which will show an error message via
config_error_nonbool(), you may not want any extra message, but for
new callers that wants to make sure value falls within a supported
range, this forces it to write
if (!git_config_get_int(key, num)) {
if (!(0 num num 4)) {
error('%s' must be between 1 and 3);
git_config_die(key);
}
/* otherwise work */
}
and then the error message would say something like:
error: 'core.frotz' must be between 1 and 3
fatal: bad config variable 'core.frotz' at file line 15 in .git/config
which sounds somewhat backwards, at least to me.
I was aping the old git_config() system, it also does exactly what you described
above. for example, builtin/gc.c line 91,
if (!strcmp(var, gc.pruneexpire)) {
if (value strcmp(value, now)) {
unsigned long now = approxidate(now);
if (approxidate(value) = now)
return error(_(Invalid %s: '%s'), var, value);
}
would print,
error: Invalid gc.pruneexpire: 'value'
fatal: bad config variable 'gc.pruneexpire' at file line 15 in
.git/config
or imap-send.c line 1340,
if (!strcmp(sslverify, key))
server.ssl_verify = git_config_bool(key, val);
else if (!strcmp(preformattedhtml, key))
server.use_html = git_config_bool(key, val);
else if (!val)
return config_error_nonbool(key);
again would cause a error die, message combo as above. There are many
examples like that.
We can easily take a custom error message but again I was just aping the old
system.
+NORETURN
+void git_die_config_linenr(const char *key, const char *filename, int
linenr)
+{
+if (!linenr)
+die(_(unable to parse '%s' from command-line config), key);
Do we have existing code that says we signal that it is from the
command line by setting linenr to zero already? Otherwise I would
have thought filename == NULL would be a more sensible convention.
Otherwise OK.
Noted. Next reroll will have filename as the convention.
+else
+die(_(bad config variable '%s' at file line %d in %s),
At least, quote the last '%s'.
Noted. 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: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
On Mon, Aug 4, 2014 at 11:45 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Tanay Abhra tanay...@gmail.com writes: `git_pretty_formats_config()` continues without checking git_config_string's return value which can lead to a SEGFAULT. Indeed, without the patch: $ git -c pretty.my= log --pretty=my error: Missing value for 'pretty.my' zsh: segmentation fault git -c pretty.my= log --pretty=my This probably should be formalized as a proper test and included with Tanay's patch. diff --git a/pretty.c b/pretty.c index 3a1da6f..72dbf55 100644 --- a/pretty.c +++ b/pretty.c @@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c commit_format-name = xstrdup(name); commit_format-format = CMIT_FMT_USERFORMAT; - git_config_string(fmt, var, value); + if (git_config_string(fmt, var, value)) + return -1; + Ack-ed-by: Matthieu Moy matthieu@imag.fr My first thought reading this was why not rewrite using non-callback API?, but this particular call to git_config needs to iterate over config keys anyway. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra tanay...@gmail.com wrote:
Use `git_config_get_bool()` family instead of `git_config()` to take
advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
http-backend.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 80790bb..106ca6b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -219,29 +219,22 @@ static void get_idx_file(char *name)
send_local_file(application/x-git-packed-objects-toc, name);
}
-static int http_config(const char *var, const char *value, void *cb)
+static void http_config(void)
{
- const char *p;
+ int i, value = 0;
+ struct strbuf var = STRBUF_INIT;
- if (!strcmp(var, http.getanyfile)) {
- getanyfile = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool(http.getanyfile, getanyfile);
- if (skip_prefix(var, http., p)) {
- int i;
-
- for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
- struct rpc_service *svc = rpc_service[i];
- if (!strcmp(p, svc-config_name)) {
- svc-enabled = git_config_bool(var, value);
- return 0;
- }
- }
+ for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
+ struct rpc_service *svc = rpc_service[i];
+ strbuf_addf(var, http.%s, svc-config_name);
+ if (!git_config_get_bool(var.buf, value))
+ svc-enabled = value;
+ strbuf_reset(var);
}
There is a behavior change here. The original code set svc-enabled to
the first matching rpc_service[] entry, whereas the new code sets it
to the last matching entry. Is this change intentional?
- /* we are not interested in parsing any other configuration here */
- return 0;
+ strbuf_release(var);
}
static struct rpc_service *select_service(const char *name)
@@ -627,7 +620,7 @@ int main(int argc, char **argv)
access(git-daemon-export-ok, F_OK) )
not_found(Repository not exported: '%s', dir);
- git_config(http_config, NULL);
+ http_config();
cmd-imp(cmd_arg);
return 0;
}
--
1.9.0.GIT
--
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] convert: Stream from fd to required clean filter instead of mmap
Steffen Prohaska proha...@zib.de writes:
The data is streamed to the filter process anyway. Better avoid mapping
the file if possible. This is especially useful if a clean filter
reduces the size, for example if it computes a sha1 for binary data,
like git media. The file size that the previous implementation could
handle was limited by the available address space; large files for
example could not be handled with (32-bit) msysgit. The new
implementation can filter files of any size as long as the filter output
is small enough.
The new code path is only taken if the filter is required. The filter
consumes data directly from the fd. The original data is not available
to git, so it must fail if the filter fails.
Yay ;-)
The test that exercises required filters is modified to verify that the
data actually has been modified on its way from the file system to the
object store.
The expectation on the process size is tested using /usr/bin/time. An
alternative would have been tcsh, which could be used to print memory
information as follows:
tcsh -c 'set time=(0 %M); cmd'
Although the logic could perhaps be simplified with tcsh, I chose to use
'time' to avoid a dependency on tcsh.
Signed-off-by: Steffen Prohaska proha...@zib.de
---
convert.c | 58 ++-
convert.h | 10 +---
sha1_file.c | 29 --
t/t0021-conversion.sh | 68
+++
4 files changed, 149 insertions(+), 16 deletions(-)
diff --git a/convert.c b/convert.c
index cb5fbb4..58a516a 100644
--- a/convert.c
+++ b/convert.c
@@ -312,11 +312,12 @@ static int crlf_to_worktree(const char *path, const
char *src, size_t len,
struct filter_params {
const char *src;
unsigned long size;
+ int fd;
const char *cmd;
const char *path;
};
-static int filter_buffer(int in, int out, void *data)
+static int filter_buffer_or_fd(int in, int out, void *data)
{
/*
* Spawn cmd and feed the buffer contents through its stdin.
@@ -325,6 +326,7 @@ static int filter_buffer(int in, int out, void *data)
struct filter_params *params = (struct filter_params *)data;
int write_err, status;
const char *argv[] = { NULL, NULL };
+ int fd;
/* apply % substitution to cmd */
struct strbuf cmd = STRBUF_INIT;
@@ -355,7 +357,17 @@ static int filter_buffer(int in, int out, void *data)
sigchain_push(SIGPIPE, SIG_IGN);
- write_err = (write_in_full(child_process.in, params-src, params-size)
0);
+ if (params-src) {
+ write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
+ } else {
+ /* dup(), because copy_fd() closes the input fd. */
+ fd = dup(params-fd);
+ if (fd 0)
+ write_err = error(failed to dup file descriptor.);
+ else
+ write_err = copy_fd(fd, child_process.in);
+ }
+
if (close(child_process.in))
write_err = 1;
if (write_err)
@@ -371,7 +383,7 @@ static int filter_buffer(int in, int out, void *data)
return (write_err || status);
}
-static int apply_filter(const char *path, const char *src, size_t len,
+static int apply_filter(const char *path, const char *src, size_t len, int
fd,
struct strbuf *dst, const char *cmd)
{
/*
@@ -392,11 +404,12 @@ static int apply_filter(const char *path, const char
*src, size_t len,
return 1;
memset(async, 0, sizeof(async));
- async.proc = filter_buffer;
+ async.proc = filter_buffer_or_fd;
async.data = params;
async.out = -1;
params.src = src;
params.size = len;
+ params.fd = fd;
params.cmd = cmd;
params.path = path;
@@ -747,6 +760,22 @@ static void convert_attrs(struct conv_attrs *ca, const
char *path)
}
}
+int would_convert_to_git_filter_fd(const char *path) {
Style; opening brace on its own line:
int would_convert_to_git_filter_fd(const char *path)
{
+ struct conv_attrs ca;
+ convert_attrs(ca, path);
+
+ if (!ca.drv)
As we do not allow decl-after-stmt, it is easier to read if the
first blank line in the function was between the local variable
definition and the first statement. I.e.
int would_convert_to_git_filter_fd(const char *path)
{
struct ...;
convert_attrs(...);
if (!ca.drv)
+ return 0;
+
+ /* Apply a filter to an fd only if the filter is required to succeed.
+ * We must die if the filter fails, because the original data before
+ * filtering is not available. */
/*
* multi-line
* comment style.
*/
+ if (!ca.drv-required)
+ return 0;
+
+ return apply_filter(path, 0, 0, -1, 0, ca.drv-clean);
What's
Re: [PATCH] pack-bitmap: do not use gcc packed attribute
Am 02.08.2014 01:10, schrieb Jeff King:
On Fri, Aug 01, 2014 at 06:37:39PM -0400, Jeff King wrote:
Btw.: Using struct-packing on 'struct bitmap_disk_entry' means that the
binary format of .bitmap files is incompatible between GCC and other
builds, correct?
The on-disk format is defined by JGit; if there are differences between
the builds, that's a bug (and I would not be too surprised if there is
one, as bitmaps have gotten very extensive testing on 32- and 64-bit
gcc, but probably not much elsewhere).
We do use structs to represent disk structures in other bits of the
packfile code (e.g., struct pack_idx_header), but the struct is vanilla
enough that we assume every compiler gives us two tightly-packed 32-bit
integers without having to bother with the packed attribute (and it
seems to have worked in practice).
We should probably be more careful with that bitmap code. It looks like
it wouldn't be too bad to drop it. I'll see if I can come up with a
patch.
I confirmed that this does break horribly without the packed attribute
(as you'd expect; it's asking for 48-bit alignment!). p5310 notices it,
_if_ you have jgit installed to check against.
Here's a fix.
Subject: pack-bitmap: do not use gcc packed attribute
The __attribute__ flag may be a noop on some compilers.
That's OK as long as the code is correct without the
attribute, but in this case it is not. We would typically
end up with a struct that is 2 bytes too long due to struct
padding, breaking both reading and writing of bitmaps.
We can work around this by using an array of unsigned char
to represent the data, and relying on get/put_be32 to handle
alignment issues as we interact with the array.
Signed-off-by: Jeff King p...@peff.net
---
The accessors may be overkill; each function is called only a single
time in the whole codebase. But doing it this way rather than accessing
entry[4] inline at least puts the magic constants all in one place.
[...]
Hmm, I wonder if it wouldn't be simpler to read / write the desired on-disk
structure directly, without copying to a uchar[6] first.
When writing, sha1write already buffers the data, so calling this with 4/1/1
bytes of payload shouldn't affect performance.
Similarly for reading - we already have a function to read a bitmap and
advance the 'file' position, why not have similar functions to read uint8/32
in a stream-based fashion?
This raises the question why we read via mmap at all (without munmap()ing the
file when done...). We copy all data into internal data structures anyway. Is
an fopen/fread-based solution (with fread_u8/_u32 helpers) that much slower?
Here's what I came up with (just a sketch, commit message is lacky and the
helper functions deserve a better place / name):
8-
Subject: [PATCH] pack-bitmap: do not use packed structs to read / write bitmap
files
Signed-off-by: Karsten Blees bl...@dcon.de
---
pack-bitmap-write.c | 18 +-
pack-bitmap.c | 21 ++---
pack-bitmap.h | 6 --
3 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 5f1791a..01995cb 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -465,6 +465,16 @@ static const unsigned char *sha1_access(size_t pos, void
*table)
return index[pos]-sha1;
}
+static inline void sha1write_u8(struct sha1file *f, uint8_t data)
+{
+ sha1write(f, data, sizeof(data));
+}
+static inline void sha1write_u32(struct sha1file *f, uint32_t data)
+{
+ data = htonl(data);
+ sha1write(f, data, sizeof(data));
+}
+
static void write_selected_commits_v1(struct sha1file *f,
struct pack_idx_entry **index,
uint32_t index_nr)
@@ -473,7 +483,6 @@ static void write_selected_commits_v1(struct sha1file *f,
for (i = 0; i writer.selected_nr; ++i) {
struct bitmapped_commit *stored = writer.selected[i];
- struct bitmap_disk_entry on_disk;
int commit_pos =
sha1_pos(stored-commit-object.sha1, index, index_nr,
sha1_access);
@@ -481,11 +490,10 @@ static void write_selected_commits_v1(struct sha1file *f,
if (commit_pos 0)
die(BUG: trying to write commit not in index);
- on_disk.object_pos = htonl(commit_pos);
- on_disk.xor_offset = stored-xor_offset;
- on_disk.flags = stored-flags;
+ sha1write_u32(f, commit_pos);
+ sha1write_u8(f, stored-xor_offset);
+ sha1write_u8(f, stored-flags);
- sha1write(f, on_disk, sizeof(on_disk));
dump_bitmap(f, stored-write_as);
}
}
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 91e4101..cb1b2dd 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -197,13 +197,23 @@ static struct stored_bitmap *store_bitmap(struct
Re: struct hashmap_entry packing
Am 02.08.2014 00:37, schrieb Jeff King:
On Tue, Jul 29, 2014 at 10:40:12PM +0200, Karsten Blees wrote:
The sizeof() has to be the same regardless of whether the hashmap_entry
is standalone or in another struct, and therefore must be padded up to
16 bytes. If we stored x in that padding in the combined struct, it
would be overwritten by our memset.
The struct-packing patch was ultimately dropped because there was no way
to reliably make it work on all platforms. See [1] for discussion, [2] for
the final, 'most compatible' version.
Thanks for the pointers; I should have guessed there was more to it and
searched the archive myself.
Hmmm. Now that we have __attribute__((packed)) in pack-bitmap.h, perhaps
we should do the same for stuct hashmap_entry? (Which was the original
proposal anyway...). Only works for GCC, but that should cover most builds
/ platforms.
I don't see any reason to avoid the packed attribute, if it helps us. As
you noted, anything using __attribute__ probably supports it, and if
not, we can conditionally #define PACKED_STRUCT or something, like we do
for NORETURN. Since it's purely an optimization, if another compiler
doesn't use it, no big deal.
That being said, I don't know if those padding bytes are actually
causing a measurable slowdown. It may not even be worth the trouble.
Its not about performance (or correctness, in case of platforms that don't
support unaligned read), just about saving memory (e.g. mapping int to int
requires 24 bytes per entry, vs. 16 with packed structs).
The padding at the end of a structure is only needed for proper alignment in
arrays. Struct hashmap_entry is always used as prefix of some other structure,
never as an array, so there are no alignment issues here.
Typical memory layouts on 64-bit platforms are as follows (note that even in
the 'followed by int64' case, all members are properly aligned):
Unpacked struct followed by int32 - wastes 1/3 of memory:
struct {
struct hashmap_entry {
00-07 struct hashmap_entry *next;
08-11 int hash;
12-15 // padding
} ent;
16-19 int32_t value;
20-23 // padding
}
Packed struct followed by int32:
struct {
struct hashmap_entry {
00-07 struct hashmap_entry *next;
08-11 int hash;
} ent;
12-15 int32_t value;
}
Packed struct followed by int64:
struct {
struct hashmap_entry {
00-07 struct hashmap_entry *next;
08-11 int hash;
} ent;
12-15 // padding
16-23 int64_t value;
}
Array of packed struct (not used):
struct hashmap_entry {
00-07 struct hashmap_entry *next;
08-11 int hash;
}; // [0]
struct hashmap_entry {
12-19 struct hashmap_entry *next; // !!!unaligned!!!
20-23 int hash;
}; // [1]
--
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] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Aug 4, 2014 at 11:45 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Tanay Abhra tanay...@gmail.com writes: `git_pretty_formats_config()` continues without checking git_config_string's return value which can lead to a SEGFAULT. Indeed, without the patch: $ git -c pretty.my= log --pretty=my error: Missing value for 'pretty.my' zsh: segmentation fault git -c pretty.my= log --pretty=my This probably should be formalized as a proper test and included with Tanay's patch. Not sure it's worth the trouble: the bug corresponds to a mis-application of a pattern used in tens of places in Git's code (basically, each call to git_config_string, 50 callsites). Testing this particular case does not ensure non-regression, and testing all occurences of the pattern would be overkill IMHO. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 5/8] config: add `git_die_config()` to the config-set API
Tanay Abhra tanay...@gmail.com writes:
I was aping the old git_config() system, it also does exactly what you
described
above. for example, builtin/gc.c line 91,
if (!strcmp(var, gc.pruneexpire)) {
if (value strcmp(value, now)) {
unsigned long now = approxidate(now);
if (approxidate(value) = now)
return error(_(Invalid %s: '%s'), var, value);
}
would print,
error: Invalid gc.pruneexpire: 'value'
fatal: bad config variable 'gc.pruneexpire' at file line 15 in
.git/config
It's good to do at least as well as the old system, but I agree with
Junio that it's suboptimal.
Having a single API call to replace
error('%s' must be between 1 and 3);
git_config_die(key);
with stg like:
git_config_die(key, '%s' must be between 1 and 3);
in Junio's example would allow git_config_die to format the error
message the way it likes (i.e. it can be the same as before when you
introduce it, and improve afterwards).
I've never been disturbed by the quality of Git's error messages wrt
config files (it's not a compiler!), so having good quality messages is
not a high priority IMHO, but having a good API that as a side effect
can produce good error messages is important. If changing the error
messages requires rewritting all callers later, then we've missed the
point doing the refactoring now.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] t5704: Fix the test that checks for excluded tags
Lukas Fleischer g...@cryptocrack.de writes: In c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), we added a test to check whether annotated tags, which fall outside the specified date range, are excluded from bundles. However, when initializing the repository, a command to create a lightweight tag was used. Fix this by replacing `git tag` by `git tag -a`. Furthermore, explicitly mention in the test message that an annotated tag is created and also test whether tags within the specified date range are included properly. Note that this fix reveals that the annotated tag exclusion actually does not work. Therefore, the test is marked expect-failure for now. Signed-off-by: Lukas Fleischer g...@cryptocrack.de --- t/t5704-bundle.sh | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index a45c316..2f063ea 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -6,7 +6,7 @@ test_description='some bundle related tests' test_expect_success 'setup' ' test_commit initial test_tick - git tag -m tag tag + git tag -am tag tag I'd prefer to see this spelled as -a -m tag, but anyway, this suggests to me that a request to create a light-weight tag should be made to error out when -m is given, or automatically promote itself to create an annotated tag, perhaps? That is in line with what happens when you do git tag -F file tagname. Oh, wait. $ git tag -d foo $ git rev-parse refs/tags/foo -- fatal: bad revision 'refs/tags/foo' $ git tag -m msg foo $ git cat-file -t refs/tags/foo tag $ git cat-file tag refs/tags/foo object d84843c... type commit tag foo tagger Junio msg $ git version git version 2.1.0-rc0-247-g66c8a75 The output from git blame -L'/^int cmd_tag/,/^}/' builtin/tag.c seems to indicate that we automatically turned annotate on when a message is given via -m or -F since the very first version of git tag that was re-implemented in C, i.e. 62e09ce9 (Make git tag a builtin., 2007-07-20). Your analysis starts to sound fishy. What version of Git are you talking about? test_commit second test_commit third git tag -d initial @@ -14,7 +14,10 @@ test_expect_success 'setup' ' git tag -d third ' -test_expect_success 'tags can be excluded by rev-list options' ' +test_expect_failure 'annotated tags can be excluded by rev-list options' ' + git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 + git ls-remote bundle output + grep tag output git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 git ls-remote bundle output ! grep tag 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 2/2] bundle: Fix exclusion of annotated tags
Lukas Fleischer g...@cryptocrack.de writes: In commit c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), support for excluding annotated tags outside the specified date range was added. However, the wrong order of parameters was chosen when calling memchr(). Fix this by swapping the character to search for with the maximum length parameter. Signed-off-by: Lukas Fleischer g...@cryptocrack.de --- bundle.c | 4 ++-- t/t5704-bundle.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bundle.c b/bundle.c index 71a21a6..b708906 100644 --- a/bundle.c +++ b/bundle.c @@ -221,8 +221,8 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) line = memmem(buf, size, \ntagger , 8); if (!line++) return 1; - lineend = memchr(line, buf + size - line, '\n'); - line = memchr(line, lineend ? lineend - line : buf + size - line, ''); + lineend = memchr(line, '\n', buf + size - line); + line = memchr(line, '', lineend ? lineend - line : buf + size - line); Good spotting; thanks. if (!line++) return 1; date = strtoul(line, NULL, 10); diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index 2f063ea..8a4d299 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -14,7 +14,7 @@ test_expect_success 'setup' ' git tag -d third ' -test_expect_failure 'annotated tags can be excluded by rev-list options' ' +test_expect_success 'annotated tags can be excluded by rev-list options' ' git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 git ls-remote bundle output grep tag 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 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family
Tanay Abhra tanay...@gmail.com writes:
@@ -354,12 +338,11 @@ static int run_service(const char *dir, struct
daemon_service *service)
[...]
}
+
if (!enabled) {
logerror('%s': service not enabled for '%s',
service-name, path);
Avoid whitespace-only change like this one.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] t5704: Fix the test that checks for excluded tags
Junio C Hamano gits...@pobox.com writes: diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index a45c316..2f063ea 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -6,7 +6,7 @@ test_description='some bundle related tests' test_expect_success 'setup' ' test_commit initial test_tick -git tag -m tag tag +git tag -am tag tag ... Oh, wait. In any case, the fix in 2/2 is real, and applying both and then reverting the above hunk passes the test. Also, applying both, reverting the above hunk *and* reverting the fix to bundle.c of course makes the rest fail. So I would be tempted to squash these two patches into one using the log message from the latter one, while excluding the change in the above hunk. Thanks. @@ -14,7 +14,10 @@ test_expect_success 'setup' ' git tag -d third ' -test_expect_success 'tags can be excluded by rev-list options' ' +test_expect_failure 'annotated tags can be excluded by rev-list options' ' +git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 +git ls-remote bundle output +grep tag output git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 git ls-remote bundle output ! grep tag 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] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
On Mon, Aug 04, 2014 at 05:45:44PM +0200, Matthieu Moy wrote: Tanay Abhra tanay...@gmail.com writes: `git_pretty_formats_config()` continues without checking git_config_string's return value which can lead to a SEGFAULT. Indeed, without the patch: $ git -c pretty.my= log --pretty=my error: Missing value for 'pretty.my' zsh: segmentation fault git -c pretty.my= log --pretty=my Hmm. Not related to the original patch, but that really looks like a bug. Shouldn't git -c pretty.my= ... set pretty.my to the empty string? I'd expect git -c pretty.my ... to set it to NULL (i.e., the implicit true you get from omitting the = in the config files themselves). -Peff -- 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 08/11] pager.c: replace `git_config()` with `git_config_get_value()`
Tanay Abhra tanay...@gmail.com writes:
Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pager.c | 40 +---
I find the patch more readable with --histogram, cut-and-pasted below in
case other reviewers find it better too.
diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
#define DEFAULT_PAGER less
#endif
-struct pager_config {
- const char *cmd;
- int want;
- char *value;
-};
-
/*
* This is split up from the rest of git so that we can do
* something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
return width;
}
-static int pager_command_config(const char *var, const char *value, void *data)
-{
- struct pager_config *c = data;
- if (starts_with(var, pager.) !strcmp(var + 6, c-cmd)) {
- int b = git_config_maybe_bool(var, value);
- if (b = 0)
- c-want = b;
- else {
- c-want = 1;
- c-value = xstrdup(value);
- }
- }
- return 0;
-}
-
/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
int check_pager_config(const char *cmd)
{
- struct pager_config c;
- c.cmd = cmd;
- c.want = -1;
- c.value = NULL;
- git_config(pager_command_config, c);
- if (c.value)
- pager_program = c.value;
- return c.want;
+ int want = -1;
+ struct strbuf key = STRBUF_INIT;
+ const char *value = NULL;
+ strbuf_addf(key, pager.%s, cmd);
+ if (!git_config_get_value(key.buf, value)) {
+ int b = git_config_maybe_bool(key.buf, value);
+ if (b = 0)
+ want = b;
+ else {
+ want = 1;
+ pager_program = xstrdup(value);
+ }
+ }
+ strbuf_release(key);
+ return want;
}
I first wondered why you were not using git_config_get_maybe_bool(), but
you want to interpret non-boolean values as command-names, so you do
need this two-steps get_value - maybe_bool.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family
Tanay Abhra tanay...@gmail.com writes:
+ if (approxidate(prune_expire) = now) {
+ error(_(Invalid %s: '%s'), gc.pruneexpire,
prune_expire);
+ git_die_config(gc.pruneexpire);
+ }
That is a case where the API looks suboptimal, see Junio's remark in the
other thread.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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 00/11] git_config callers rewritten with the new config-set API
Tanay Abhra tanay...@gmail.com writes: This series is the first batch of patches which rewrites the existing callers using a non-callback approach. This series aims to, * rewrite the existing callers, as you can see from the diff stat the bew API provides a much concise and clear control flow. * stress test the new API, see if any corner cases or deficiencies arise or not. I went through the series. I agree with Eric that there's a slight behavior change in http-backend.c, and I don't think it's a good thing. Other than that, the series look good to me, although I would probably need a second look to double check. Tanay: I suggest you keep this as one 11-patches series. If you rewrite more callsites, I'd find it easier to review as a second, independant series rather than an ever-growing single series. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 5/8] config: add `git_die_config()` to the config-set API
Tanay Abhra tanay...@gmail.com writes:
On 8/4/2014 11:37 PM, Junio C Hamano wrote:
Tanay Abhra tanay...@gmail.com writes:
Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.
It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,
if (!git_config_get_value(key, value)) {
/* NULL values not allowed */
if (!value)
git_config_die(key);
else
/* do work */
}
It feels a bit unnatural at the API level that this does not take
'value'; I do understand that it is not a big deal in the error code
path to locate again the value from the configuration using the key,
but still.
But, we don't have a use for value as it is not denoted in the error
string, that is why I left it out.
That is my point. Why doesn't the error message talk about what
value the caller found was offensive, and in what way?
+ else
+ die(_(bad config variable '%s' at file line %d in %s),
At least, quote the last '%s'.
Noted. Thanks.
Actually, at file line sounded very strange, at least to me, hence
the suggested reword in the part you did not quote.
--
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 2/2] bundle: Fix exclusion of annotated tags
In commit c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), support for excluding annotated tags outside the specified date range was added. However, the wrong order of parameters was chosen when calling memchr(). Fix this by swapping the character to search for with the maximum length parameter. Signed-off-by: Lukas Fleischer g...@cryptocrack.de --- bundle.c | 4 ++-- t/t5704-bundle.sh | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bundle.c b/bundle.c index 71a21a6..b708906 100644 --- a/bundle.c +++ b/bundle.c @@ -221,8 +221,8 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) line = memmem(buf, size, \ntagger , 8); if (!line++) return 1; - lineend = memchr(line, buf + size - line, '\n'); - line = memchr(line, lineend ? lineend - line : buf + size - line, ''); + lineend = memchr(line, '\n', buf + size - line); + line = memchr(line, '', lineend ? lineend - line : buf + size - line); if (!line++) return 1; date = strtoul(line, NULL, 10); diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index 2d53388..a828c71 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -14,7 +14,7 @@ test_expect_success 'setup' ' git tag -d third ' -test_expect_failure 'tags can be excluded by rev-list options' ' +test_expect_success 'tags can be excluded by rev-list options' ' git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 git ls-remote bundle output grep tag output -- 2.0.4 -- 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 1/2] t5704: Complement annotated tag exclusion test
In c9a42c4 (bundle: allow rev-list options to exclude annotated tags, 2009-01-02), we added a test to check whether annotated tags, which fall outside the specified date range, are excluded from bundles. Complement this test by also checking whether tags inside the date range are included. Since this addition reveals that the annotated tag exclusion is flawed, mark the test expect-failure for now. Signed-off-by: Lukas Fleischer g...@cryptocrack.de --- I decided that it is still worthwhile to have this in a separate patch. Feel free to squash 1/2 and 2/2 or let me know that they should be merged if you prefer that. t/t5704-bundle.sh | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh index a45c316..2d53388 100755 --- a/t/t5704-bundle.sh +++ b/t/t5704-bundle.sh @@ -14,7 +14,10 @@ test_expect_success 'setup' ' git tag -d third ' -test_expect_success 'tags can be excluded by rev-list options' ' +test_expect_failure 'tags can be excluded by rev-list options' ' + git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 + git ls-remote bundle output + grep tag output git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 git ls-remote bundle output ! grep tag output -- 2.0.4 -- 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] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
Jeff King p...@peff.net writes: On Mon, Aug 04, 2014 at 05:45:44PM +0200, Matthieu Moy wrote: Tanay Abhra tanay...@gmail.com writes: `git_pretty_formats_config()` continues without checking git_config_string's return value which can lead to a SEGFAULT. Indeed, without the patch: $ git -c pretty.my= log --pretty=my error: Missing value for 'pretty.my' zsh: segmentation fault git -c pretty.my= log --pretty=my Hmm. Not related to the original patch, but that really looks like a bug. Shouldn't git -c pretty.my= ... set pretty.my to the empty string? I'd expect git -c pretty.my ... to set it to NULL (i.e., the implicit true you get from omitting the = in the config files themselves). Indeed. strbuf_split_buf() does not seem to distinguish between x= and x. No time to debug this further, sorry. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 07/19] rebase -i: log the replay of root commits
Hi, Jeff King writes: On Tue, Jul 29, 2014 at 01:18:07AM +0200, Fabian Ruch wrote: The command line used to recreate root commits specifies the option `-q` which suppresses the commit summary message. However, git-rebase--interactive tends to tell the user about the commits it creates in the final history, if she wishes (cf. command line option `--verbose`). The code parts handling non-root commits and squash commits all output commit summary messages. Do not make the replay of root commits an exception. Remove the option to make the report of the rebased history complete. It is OK that the commit summary is still suppressed when git-commit is used to initialize the authorship of the sentinel commit because this additional commit is an implementation detail hidden from the final history. The removed `-q` option was probably introduced as a copy-and-paste error stemming from that part of the root commit handling code. I'm confused. This implies that we should be seeing summaries for other commits, but not root commits, and this patch is bring them into harmony. But if I have a repo like this: git init -q repo cd repo for i in one two; do echo $i file git add file git commit -q -m $i done then using stock git gives me this: $ GIT_EDITOR=true git rebase -i --root 21 | perl -pe 's/\r/\\r\n/g' Rebasing (1/2)\r Rebasing (2/2)\r Successfully rebased and updated refs/heads/master. but with your patch, I get: $ GIT_EDITOR=true git.compile rebase -i --root 21 | perl -pe 's/\r/\\r\n/g' Rebasing (1/2)\r [detached HEAD 60834b3] one Date: Fri Aug 1 20:00:05 2014 -0400 1 file changed, 1 insertion(+) create mode 100644 file Rebasing (2/2)\r Successfully rebased and updated refs/heads/master. Am I misunderstanding the purpose of the patch? Thanks for laying out the differences in the user visible output. With stock git we are seeing summaries for other commits, but not root commits, _with the --verbose option_. It's the fault of my patch to show the summary even in non-verbose mode. This is now fixed by wrapping the relevant command in 'output', a shell function defined in git-rebase.sh as follows: output=$($@ 21 ) status=$? test $status != 0 printf %s\n $output return $status The problem that git-rebase--interactive has is that the redirection of stdin to a variable (or a file) does not work reliably with commands that invoke the log message editor, that is 'reword' and 'squash' produce their output both in verbose and non-verbose mode. I wouldn't know how to fix this but 1) invoking $GIT_EDITOR from git-rebase--interactive.sh, but to make this right there should be a built-in command for shell scripts and an interface for git-commit that prepare the editor contents like git-commit does now, or 2) forcing $GIT_EDITOR and git-commit to print on distinct file descriptors, which would involve temporarily wrapping the $GIT_EDITOR call in a shell script that redirects stdin to some other file descriptor similar to what t/test-lib.sh does, or cat $state_dir/editor.sh EOF #!/bin/sh $(git var GIT_EDITOR) \$* 3 EOF chmod +x $state_dir/editor.sh ( export GIT_EDITOR=$state_dir/editor.sh $@ 31 $state_dir/output 21 ) 3) passing the --quiet option in non-verbose mode and omitting it in verbose mode, which would cover the '$status != 0' above for if a command fails, it should indicate its error status despite being asked to be silent. Options 2) and 3) seem attainable within this patch series and 3) sounds like the cleanest option but I'm uncertain if I'm missing something here. The only command line that is wrapped in 'output' and that doesn't support a --quiet option seems to be a 'warn' line which could simply be skipped in non-verbose mode. (Johannes Schindelin is cc'd as the original author of git-rebase--interactive.sh and 'output' in particular). Fabian -- 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 08/19] rebase -i: root commits are replayed with an unnecessary option
Hi Jeff, Jeff King writes: On Tue, Jul 29, 2014 at 01:18:08AM +0200, Fabian Ruch wrote: The command line used to recreate root commits specifies the effectless option `-C`. It makes git-commit reuse commit message and authorship of the named commit. However, the commit being amended here, which is the sentinel commit, already carries the authorship and log message of the commit being replayed. Remove the option. Since `-C` (in contrast to `-c`) does not invoke the editor and the `--amend` option invokes it by default, disable editor invocation again by specifying `--no-edit`. I found this description a little backwards. The -C does have an effect, as you noticed in the second paragraph. I think the reasoning is more like: The command line used to recreate root commits uses -C to suppress the commit editor. This is unnecessarily confusing, though, because that suppression is a secondary effect of the option. The main purpose of -C is to pull the metadata from another commit, but here we know that this is a noop, since we are amending a commit just created from the same data. At the time, commit did not yet know --no-edit, and this was a reasonable way to get the desired behavior. We can switch it to use --no-edit to make the intended effect more obvious. Thanks again, I shamelessly copied your formulation but squeezed in an undocumented because --no-edit had just been implemented (commit ca1ba2010), though was then still missing from the git-commit manpage. Fabian -- 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/1] doc: format-patch: don't use origin as a branch name
From: Junio C Hamano gits...@pobox.com Philip Oakley philipoak...@iee.org writes: Historically (5 Nov 2005 v0.99.9-46-g28ffb89) the git-format-patch used 'origin' as the upstream branch name. That name is now used as the nominal name for the upstream remote. While 'origin' would be DWIMmed (do what I mean) to be that remote's primary branch, do not assume the reader is ready for such magic. Good thinking. Likewise, do not use 'origin/master' which may not be up to date with the remote, nor reflect the reader's master branch. The patch series should be relative to the reader's view of 'git show-branch HEAD master'. This however is backwards, no? The history on 'origin/master' may not be up-to-date in the sense that if you run 'git fetch' you might get more, but it absolutely is up-to-date in the sense that it shows what the origin has to the best of your repository's current knowledge. I still think that the user/reader shouldn't be creating patches based on wherever someone else had got to, rather it should just be patches from their own feature branch. However the rest of your argument still stands with regard to accidental/unexpected conflicts with other upstream work, and the reader should ensure they are already up to date - maybe it needs a comment line to state that. Compared to that, what the user's local 'master' has is much less relevant. For one thing, if a more recent commit that is on the remote repository is missing on 'origin/master' because you haven't fetched recently, by definition that commit will not be on your 'master' either, so you have the same staleness issue to the exact degree. Even worse, when you are developing a topic to upstream, it is a good practice to merge your topic to your own 'master' to check it with the wider project codebase that is more recent than where your topic earlier forked from, and it makes little sense to tell 'exclude what I have on my master' to format-patch when extracting changes to upstream out of such a topic. You send what the other side has, not what you do not have on your local 'master' branch. Use the more modern 'master' as the reference branch name. There is nothing 'modern' in 'master'. Noted. I think the original description was written before we switched to the separate remote layout. What is in 'refs/remote/origin/master' these days was stored and updated at 'refs/heads/origin' and no other branch from the remote repository was tracked back then. The changes to be upstreamed are output by grabbing what are not in 'origin', whose modern equivalent is 'origin/master'. In short, if your patch were s|origin|origin/master|, instead of s|origin|master|, that would be an adjustment to the more modern world that is still faithful to the intent of the original. I think we would need to clarify that (the intent) for the reader. I'll see what I can do. (suggestion below) Signed-off-by: Philip Oakley philipoak...@iee.org --- Documentation/git-format-patch.txt | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index c0fd470..b0f041f 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -523,25 +523,25 @@ $ git format-patch -k --stdout R1..R2 | git am -3 -k * Extract all commits which are in the current branch but not in the -origin branch: +master branch: + -$ git format-patch origin +$ git format-patch master + For each commit a separate file is created in the current directory. Perhaps insert Note: Your 'master' should be up to date with respect to 'origin/master' before creating and sending patches upstream to avoid unexpected conflicts. ? -* Extract all commits that lead to 'origin' since the inception of the +* Extract all commits that lead to 'master' since the inception of the project: + -$ git format-patch --root origin +$ git format-patch --root master * The same as the previous one: + -$ git format-patch -M -B origin +$ git format-patch -M -B master + Additionally, it detects and handles renames and complete rewrites -- Philip -- 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] config: teach git -c to recognize an empty string
On Mon, Aug 04, 2014 at 11:06:03PM +0200, Matthieu Moy wrote:
Hmm. Not related to the original patch, but that really looks like a
bug. Shouldn't git -c pretty.my= ... set pretty.my to the empty string?
I'd expect git -c pretty.my ... to set it to NULL (i.e., the implicit
true you get from omitting the = in the config files themselves).
Indeed.
strbuf_split_buf() does not seem to distinguish between x= and x. No
time to debug this further, sorry.
Oh, I didn't expect you to work on it. The bug is totally my fault. :)
Your email just made me realize it was there.
Here's a patch to fix it.
-- 8 --
Subject: config: teach git -c to recognize an empty string
In a config file, you can do:
[foo]
bar
to turn the foo.bar boolean flag on, and you can do:
[foo]
bar=
to set foo.bar to the empty string. However, git's -c
parameter treats both:
git -c foo.bar
and
git -c foo.bar=
as the boolean flag, and there is no way to set a variable
to the empty string. This patch enables the latter form to
do that.
Signed-off-by: Jeff King p...@peff.net
---
This is technically a backwards incompatibility, but I'd consider it a
simple bugfix. The existing behavior was unintentional, made no sense,
and was never documented.
Looking over strbuf_split's interface, I think it's rather
counter-intuitive, and I was tempted to change it. But there are several
other callers that rely on it, and the chance for introducing a subtle
bug is high. This is the least invasive fix (and it really is not any
less readable than what was already there :) ).
Documentation/git.txt | 5 +
config.c | 12 ++--
t/t1300-repo-config.sh | 11 +++
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index b1c4f7a..e7783f0 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -447,6 +447,11 @@ example the following invocations are equivalent:
given will override values from configuration files.
The name is expected in the same format as listed by
'git config' (subkeys separated by dots).
++
+Note that omitting the `=` in `git -c foo.bar ...` is allowed and sets
+`foo.bar` to the boolean true value (just like `[foo]bar` would in a
+config file). Including the equals but with an empty value (like `git -c
+foo.bar= ...`) sets `foo.bar` to the empty string.
--exec-path[=path]::
Path to wherever your core Git programs are installed.
diff --git a/config.c b/config.c
index 058505c..fe6216f 100644
--- a/config.c
+++ b/config.c
@@ -162,19 +162,27 @@ void git_config_push_parameter(const char *text)
int git_config_parse_parameter(const char *text,
config_fn_t fn, void *data)
{
+ const char *value;
struct strbuf **pair;
+
pair = strbuf_split_str(text, '=', 2);
if (!pair[0])
return error(bogus config parameter: %s, text);
- if (pair[0]-len pair[0]-buf[pair[0]-len - 1] == '=')
+
+ if (pair[0]-len pair[0]-buf[pair[0]-len - 1] == '=') {
strbuf_setlen(pair[0], pair[0]-len - 1);
+ value = pair[1] ? pair[1]-buf : ;
+ } else
+ value = NULL;
+
strbuf_trim(pair[0]);
if (!pair[0]-len) {
strbuf_list_free(pair);
return error(bogus config parameter: %s, text);
}
+
strbuf_tolower(pair[0]);
- if (fn(pair[0]-buf, pair[1] ? pair[1]-buf : NULL, data) 0) {
+ if (fn(pair[0]-buf, value, data) 0) {
strbuf_list_free(pair);
return -1;
}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3f80ff0..46f6ae2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1010,6 +1010,17 @@ test_expect_success 'git -c key=value support' '
test_must_fail git -c name=value config core.name
'
+# We just need a type-specifier here that cares about the
+# distinction internally between a NULL boolean and a real
+# string (because most of git's internal parsers do care).
+# Using --path works, but we do not otherwise care about
+# its semantics.
+test_expect_success 'git -c can represent empty string' '
+ echo expect
+ git -c foo.empty= config --path foo.empty actual
+ test_cmp expect actual
+'
+
test_expect_success 'key sanity-checking' '
test_must_fail git config foo=bar
test_must_fail git config foo=.bar
--
2.1.0.rc0.286.g5c67d74
--
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/1] doc: format-patch: don't use origin as a branch name
Philip Oakley philipoak...@iee.org writes: This however is backwards, no? The history on 'origin/master' may not be up-to-date in the sense that if you run 'git fetch' you might get more, but it absolutely is up-to-date in the sense that it shows what the origin has to the best of your repository's current knowledge. I still think that the user/reader shouldn't be creating patches based on wherever someone else had got to, rather it should just be patches from their own feature branch. You forked your topic branch off of the shared project history aka origin/master and built some. You may have sent some patches off of your previous work to the upstream, and origin/master may or may not have applied some of them since your topic forked from it. The patches you are sending out is from your own topic branch. You may be cooking multiple topics, and your local 'master' branch, which you never push back to 'origin/master', may contain any of these branches. You do not fork off a new topic out of there. Best case, you would fork from 'origin/master'; a bit worse case, you have to fork from another of your topic branch that your new topic has to depend on. Nowhere I am assuming that the reader is creating paches based on wherever someone else had got to. Sorry, but I have no idea what you are complaining about. However the rest of your argument still stands with regard to accidental/unexpected conflicts with other upstream work, and the reader should ensure they are already up to date - maybe it needs a comment line to state that. Sorry, but I am not sure how much you understood what I wrote. The primary reason why 'origin' in the example should be replaced with 'origin/master' is because that is the literal adjustment from the pre-separate-remote world order to today's world order. The local branch 'origin' (more specifically, 'refs/heads/origin') used to be what we used to keep track of 'master' of the upstream, which we use 'refs/remotes/origin/master' these days. Side note: DWIMming origin to remotes/origin/HEAD to remotes/origin/master was invented to keep supporting this 'origin' keeps track of the default upstream convention when we transitioned from the old world order to separate-remote layout. And the reason why 'origin' should not be replaced with 'master' is because your 'master' may already have patches from the topic you are working on, i.e. in your current branch, that the upstream does not yet have. Running git format-patch origin/master will show what needs to be accepted by the upstream from you to reproduce your work in full; if you run git format-patch master, it may miss some parts that you already have in your local 'master' but not yet in the upstream. I never talked about conflicts, and I still think that it is completely outside the scope of these examples. Avoidance of conflicts with the work that is already commited to your upstream since you forked is the job for rebase, not format-patch. The reason why it is wrong to replace 'origin' in that text with 'master' does not have anything to do with conflict avoidance. Puzzled... -- 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] config: teach git -c to recognize an empty string
Jeff King p...@peff.net writes: This is technically a backwards incompatibility, but I'd consider it a simple bugfix. The existing behavior was unintentional, made no sense, and was never documented. Yeah, I tend to agree. I actually would not shed any tears if the breakage were that it was impossible to pass NULL is true boolean via git -c interface, but it is the other way around. It is much more grave a problem that we cannot pass an empty string as a value, and we should fix it. Looking over strbuf_split's interface, I think it's rather counter-intuitive, and I was tempted to change it. But there are several other callers that rely on it, and the chance for introducing a subtle bug is high. This is the least invasive fix (and it really is not any less readable than what was already there :) ). ;-) +# We just need a type-specifier here that cares about the +# distinction internally between a NULL boolean and a real +# string (because most of git's internal parsers do care). +# Using --path works, but we do not otherwise care about +# its semantics. +test_expect_success 'git -c can represent empty string' ' + echo expect + git -c foo.empty= config --path foo.empty actual + test_cmp expect actual +' Another way may be git config -l and see if we see a = on the entry for foo.empty, but I think the way you did this is nicer. test_expect_success 'key sanity-checking' ' test_must_fail git config foo=bar test_must_fail git config foo=.bar -- 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
[ANNOUNCE] Git v2.1.0-rc1
v2.1.0-rc1, the first release candidate Git for v2.1, is now available for testing at the usual places. The tarballs are found at: https://www.kernel.org/pub/software/scm/git/testing/ The following public repositories all have a copy of the 'v2.1.0-rc1' tag and the 'master' branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = https://code.google.com/p/git-core/ url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git Git v2.1 Release Notes (draft) == Backward compatibility notes * The default value we give to the environment variable LESS has been changed from FRSX to FRX, losing S (chop long lines instead of wrapping). Existing users who prefer not to see line-wrapped output may want to set $ git config core.pager less -S to restore the traditional behaviour. It is expected that people find output from the most subcommands easier to read with the new default, except for blame which tends to produce really long lines. To override the new default only for git blame, you can do this: $ git config pager.blame less -S * A few disused directories in contrib/ have been retired. Updates since v2.0 -- UI, Workflows Features * Since the very beginning of Git, we gave the LESS environment a default value FRSX when we spawn less as the pager. S (chop long lines instead of wrapping) has been removed from this default set of options, because it is more or less a personal taste thing, as opposed to others that have good justifications (i.e. R is very much justified because many kinds of output we produce are colored and FX is justified because output we produce is often shorter than a page). * The logic and data used to compute the display width needed for UTF-8 strings have been updated to match Unicode 7.0 better. * HTTP-based transports learned to propagate the error messages from the webserver better to the client coming over the HTTP transport. * The completion script for bash (in contrib/) has been updated to handle aliases that define complex sequence of commands better. * The core.preloadindex configuration variable is by default enabled, allowing modern platforms to take advantage of the multiple cores they have. * git clone applies the if cloning from a local disk, physically copy repository using hardlinks, unless otherwise told not to with --no-local optimization when url.*.insteadOf mechanism rewrites a git clone $URL that refers to a repository over the network to a clone from a local disk. * git commit --date=date option learned to read from more timestamp formats, including --date=now. * The `core.commentChar` configuration variable is used to specify a custom comment character other than the default # to be used in the commit log editor. This can be set to `auto` to attempt to choose a different character that does not conflict with what already starts a line in the message being edited for cases like git commit --amend. * git format-patch learned --signature-file=file to take the mail signature from. * git grep learned grep.fullname configuration variable to force --full-name to be default. This may cause regressions on scripted users that do not expect this new behaviour. * git imap-send learned to ask the credential helper for auth material. * git log and friends now understand the value auto set to the log.decorate configuration variable to enable the --decorate option automatically when the output is sent to tty. * git merge without argument, even when there is an upstream defined for the current branch, refused to run until merge.defaultToUpstream is set to true. Flip the default of that configuration variable to true. * git mergetool learned to drive the vimdiff3 backend. * mergetool.prompt used to default to 'true', always asking do you really want to run the tool on this path?. Among the two purposes this prompt serves, ignore the use case to confirm that the user wants to view particular path with the named tool, and redefine the meaning of the prompt only to confirm the choice of the tool made by the autodetection (for those who configured the tool explicitly, the prompt shown for the latter purpose is simply annoying). Strictly speaking, this is a backward incompatible change and the users need to explicitly set the variable to 'true' if they want to resurrect the now-ignored use case. * git replace learned the --edit subcommand to create a replacement by editing an existing object. * git replace learned a --graft option to rewrite parents of a commit. * git send-email learned --to-cover and --cc-cover
Webadmin Email felhasználói;
-- Ez a web-mail rendszergazda. Kérjük, tájékoztatni kell, hogy az e-mail szerver most lett frissítve, és az e-mail kell frissíteni immediately.This folyamat az, hogy a cég e-mail szerver frissítik, és a védett, mint mindig. Kérjük kattintson az alábbi linkre és kövesse az utasításokat, hogy frissítse a számla http://mailupdattw23.jigsy.com/ Üdvözlettel, Admin. webmail -- 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/1] doc: format-patch: don't use origin as a branch name
From: Junio C Hamano gits...@pobox.com Philip Oakley philipoak...@iee.org writes: This however is backwards, no? The history on 'origin/master' may not be up-to-date in the sense that if you run 'git fetch' you might get more, but it absolutely is up-to-date in the sense that it shows what the origin has to the best of your repository's current knowledge. I still think that the user/reader shouldn't be creating patches based on wherever someone else had got to, rather it should just be patches from their own feature branch. You forked your topic branch off of the shared project history aka origin/master and built some. You may have sent some patches off of your previous work to the upstream, and origin/master may or may not have applied some of them since your topic forked from it. The patches you are sending out is from your own topic branch. You may be cooking multiple topics, and your local 'master' branch, which you never push back to 'origin/master', may contain any of these branches. You do not fork off a new topic out of there. Best case, you would fork from 'origin/master'; a bit worse case, you have to fork from another of your topic branch that your new topic has to depend on. Nowhere I am assuming that the reader is creating paches based on wherever someone else had got to. Sorry, but I have no idea what you are complaining about. I think we are talking at cross purposes. My starting point is that (the examples says that) the reader wants to create a patch series for a local branch, relative to their some name branch which they branched from (e.g. the example, relative to Git, could have been from a 'pu' picked up a couple of days earlier, when they'd have said 'git format-patch pu' ;-). Having generated their short patch series, and they probably want to expose the series to some collaborator or other for comment, help and guidance (or whatever). They may just want to review it themselves. But that choice of what to do with it is surely not part of the format-patch documentation. So I'm trying to avoid defining a workflow (then or now). In the case when the patch series is meant to be ready for being applied upstream then all the other considerations apply, but the example doesn't, at least in my eyes, claim to be that. IIRC I once did make the mistake of using format-patch on a branch (off pu) I hadn't rebased since fetching and updating the local pu, so it produced a lot of extra patches, but I digress. However the rest of your argument still stands with regard to accidental/unexpected conflicts with other upstream work, and the reader should ensure they are already up to date - maybe it needs a comment line to state that. Sorry, but I am not sure how much you understood what I wrote. That may be true. I taken your not be up-to-date to be relative to the real upstream. The primary reason why 'origin' in the example should be replaced with 'origin/master' is because that is the literal adjustment from the pre-separate-remote world order to today's world order. I was trying to avoid a literal adjustment to what I'd perceived as a presumed workflow. The local branch 'origin' (more specifically, 'refs/heads/origin') used to be what we used to keep track of 'master' of the upstream, which we use 'refs/remotes/origin/master' these days. Side note: DWIMming origin to remotes/origin/HEAD to remotes/origin/master was invented to keep supporting this 'origin' keeps track of the default upstream convention when we transitioned from the old world order to separate-remote layout. And the reason why 'origin' should not be replaced with 'master' is because your 'master' may already have patches from the topic you are working on, i.e. in your current branch, that the upstream does not yet have. So this a 'develop on master' view, rather than a 'develop on feature branches' approach? Which could explain the misunderstanding. Running git format-patch origin/master will show what needs to be accepted by the upstream from you to reproduce your work in full; if you run git format-patch master, it may miss some parts that you already have in your local 'master' but not yet in the upstream. I never talked about conflicts, and I still think that it is completely outside the scope of these examples. Avoidance of conflicts with the work that is already commited to your upstream since you forked is the job for rebase, not format-patch. The reason why it is wrong to replace 'origin' in that text with 'master' does not have anything to do with conflict avoidance. Puzzled... -- Did any of that help? -- Philip -- 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: [ANNOUNCE] Git v2.1.0-rc1
On 04/08/14 23:31, Junio C Hamano wrote: v2.1.0-rc1, the first release candidate Git for v2.1, is now [snip] * The leaf function to check validity of a refname format has been micro-optimized, using SSE2 instructions when available. A few breakages during its development have been caught and fixed already but there might remain some more still; please test and report if you find any. This has been removed. ATB, Ramsay Jones -- 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] Documentation: avoid dangling modifier for imap-send
On Mon, Aug 04, 2014 at 11:29:33AM -0700, Junio C Hamano wrote: brian m. carlson sand...@crustytoothpaste.net writes: Avoid a nonsensical misreading by moving the modifier closer to the word it modifies. Signed-off-by: brian m. carlson sand...@crustytoothpaste.net --- Documentation/git-imap-send.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..23231e1 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -43,7 +43,7 @@ imap.folder:: imap.tunnel:: Command used to setup a tunnel to the IMAP server through which commands will be piped instead of using a direct network connection - to the server. Required when imap.host is not set to use imap-send. + to the server. Required to use imap-send when imap.host is not set. To be honest, I find both versions are equally confusing. How about dropping the three words to use imap-send? Fine by me. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
[PATCH v2] Documentation: avoid dangling modifier for imap-send
Avoid a nonsensical misreading by removing needless text. Signed-off-by: brian m. carlson sand...@crustytoothpaste.net --- Documentation/git-imap-send.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..e50b1c4 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -43,7 +43,7 @@ imap.folder:: imap.tunnel:: Command used to setup a tunnel to the IMAP server through which commands will be piped instead of using a direct network connection - to the server. Required when imap.host is not set to use imap-send. + to the server. Required when imap.host is not set. imap.host:: A URL identifying the server. Use a `imap://` prefix for non-secure -- 2.0.1 -- 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] Documentation: avoid dangling modifier for imap-send
brian m. carlson wrote: --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -43,7 +43,7 @@ imap.folder:: imap.tunnel:: Command used to setup a tunnel to the IMAP server through which commands will be piped instead of using a direct network connection - to the server. Required when imap.host is not set to use imap-send. + to the server. Required when imap.host is not set. Should the neighboring instances of '[Rr]equired to use imap-send be changed to plain Required, too? (I suspect yes.) Thanks, Jonathan -- 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] Documentation: avoid dangling modifier for imap-send
Jonathan Nieder wrote: Should the neighboring instances of '[Rr]equired to use imap-send be changed to plain Required, too? (I suspect yes.) Here's what that would look like. -- 8 -- From: brian m. carlson sand...@crustytoothpaste.net Subject: imap-send doc: omit confusing to use imap-send modifier It wouldn't make sense for these configuration variables to be required for Git in general to function. 'Required' in this context means required for git imap-send to work. Noticed while trying to figure out what the sentence describing imap.tunnel meant. [jn: expanded to also simplify explanation of imap.folder and imap.host in the same way] Signed-off-by: brian m. carlson sand...@crustytoothpaste.net Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/git-imap-send.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..d3b465d 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -38,17 +38,17 @@ Variables imap.folder:: The folder to drop the mails into, which is typically the Drafts folder. For example: INBOX.Drafts, INBOX/Drafts or - [Gmail]/Drafts. Required to use imap-send. + [Gmail]/Drafts. Required. imap.tunnel:: Command used to setup a tunnel to the IMAP server through which commands will be piped instead of using a direct network connection - to the server. Required when imap.host is not set to use imap-send. + to the server. Required when imap.host is not set. imap.host:: A URL identifying the server. Use a `imap://` prefix for non-secure connections and a `imaps://` prefix for secure connections. - Ignored when imap.tunnel is set, but required to use imap-send + Ignored when imap.tunnel is set, but required. otherwise. imap.user:: -- -- 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] Documentation: avoid dangling modifier for imap-send
On Mon, Aug 04, 2014 at 07:51:08PM -0700, Jonathan Nieder wrote: Jonathan Nieder wrote: Should the neighboring instances of '[Rr]equired to use imap-send be changed to plain Required, too? (I suspect yes.) Here's what that would look like. -- 8 -- From: brian m. carlson sand...@crustytoothpaste.net Subject: imap-send doc: omit confusing to use imap-send modifier It wouldn't make sense for these configuration variables to be required for Git in general to function. 'Required' in this context means required for git imap-send to work. Noticed while trying to figure out what the sentence describing imap.tunnel meant. [jn: expanded to also simplify explanation of imap.folder and imap.host in the same way] Signed-off-by: brian m. carlson sand...@crustytoothpaste.net Signed-off-by: Jonathan Nieder jrnie...@gmail.com --- Documentation/git-imap-send.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index 875d283..d3b465d 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -38,17 +38,17 @@ Variables imap.folder:: The folder to drop the mails into, which is typically the Drafts folder. For example: INBOX.Drafts, INBOX/Drafts or - [Gmail]/Drafts. Required to use imap-send. + [Gmail]/Drafts. Required. imap.tunnel:: Command used to setup a tunnel to the IMAP server through which commands will be piped instead of using a direct network connection - to the server. Required when imap.host is not set to use imap-send. + to the server. Required when imap.host is not set. imap.host:: A URL identifying the server. Use a `imap://` prefix for non-secure connections and a `imaps://` prefix for secure connections. - Ignored when imap.tunnel is set, but required to use imap-send + Ignored when imap.tunnel is set, but required. This has an extra period at the end of the line. otherwise. imap.user:: -- -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH v2] Documentation: avoid dangling modifier for imap-send
brian m. carlson wrote: This has an extra period at the end of the line. Good catch. -- 8 -- Subject: fixup! imap-send doc: omit confusing to use imap-send modifier --- Thanks, Jonathan Documentation/git-imap-send.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt index d3b465d..eabcaf0 100644 --- a/Documentation/git-imap-send.txt +++ b/Documentation/git-imap-send.txt @@ -48,8 +48,7 @@ imap.tunnel:: imap.host:: A URL identifying the server. Use a `imap://` prefix for non-secure connections and a `imaps://` prefix for secure connections. - Ignored when imap.tunnel is set, but required. - otherwise. + Ignored when imap.tunnel is set, but required otherwise. imap.user:: The username to use when logging in to the 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
Charity Foundation
I,Mrs Birgit authenticate this email, you can read about me on: http://en.wikipedia.org/wiki/Birgit_Rausing I have funds for you to manage and disburse to various charities of your choice. If you are sure you can handle this, it will be of help to you and others. Please reply if you are interested for more details.Email:mrsli...@cablespeed.com With love, Mrs Birgit Rausing -- 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
