Re: [PATCH 41/68] init: use strbufs to store paths
On 2015-10-04 05.37, Jeff King wrote:
> On Sat, Oct 03, 2015 at 11:12:13PM +0200, Torsten Bögershausen wrote:
>
>>> Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly
>>> to probe_utf8_pathname_composition() without changing its signature.
>> True, ( I was thinking that the test did only work on case insensitive FS).
>> We can skip that change.
>>
>> Beside that, I later realized, that a better signature could be:
>> +void probe_utf8_pathname_composition(const char *path, size_t len)
>>
>> I can send a proper patch the next days.
> That is the original signature, before my sprintf series. I do not mind
> leaving that as-is, and simply cleaning up probe_utf8_pathname_composition
> by using a strbuf internally there. Though I have to wonder if it even
> needs us to pass _anything_ at that point. It could just call
> git_path_buf("config%s", auml_nfd) itself. The whole reason to pass
> anything was to let it reuse the buffer the caller had.
>
> -Peff
Makes sense, here is V2:
git diff 07690109b6a252ac7cbede
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 89f2c05..4892579 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -276,7 +276,7 @@ static int create_default_files(const char *template_path)
path = git_path_buf(&buf, "CoNfIg");
if (!access(path, F_OK))
git_config_set("core.ignorecase", "true");
-probe_utf8_pathname_composition(path);
+probe_utf8_pathname_composition();
}
strbuf_release(&buf);
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index b4dd3c7..64b85f2 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -8,6 +8,7 @@
#include "cache.h"
#include "utf8.h"
#include "precompose_utf8.h"
+#include "strbuf.h"
typedef char *iconv_ibp;
static const char *repo_encoding = "UTF-8";
@@ -36,28 +37,27 @@ static size_t has_non_ascii(const char *s, size_t maxlen,
size_t *strlen_c)
}
-void probe_utf8_pathname_composition(struct strbuf *path)
+void probe_utf8_pathname_composition(void)
{
+struct strbuf sbuf = STRBUF_INIT;
static const char *auml_nfc = "\xc3\xa4";
static const char *auml_nfd = "\x61\xcc\x88";
-size_t baselen = path->len;
+const char *path;
int output_fd;
if (precomposed_unicode != -1)
return; /* We found it defined in the global config, respect it */
-strbuf_addstr(path, auml_nfc);
+path = git_path_buf(&sbuf, "%s", auml_nfc);
output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600);
if (output_fd >= 0) {
close(output_fd);
-strbuf_setlen(path, baselen);
-strbuf_addstr(path, auml_nfd);
+path = git_path_buf(&sbuf, "%s", auml_nfd);
precomposed_unicode = access(path, R_OK) ? 0 : 1;
git_config_set("core.precomposeunicode", precomposed_unicode ? "true" :
"false");
-strbuf_setlen(path, baselen);
-strbuf_addstr(path, auml_nfc);
+path = git_path_buf(&sbuf, "%s", auml_nfc);
if (unlink(path))
die_errno(_("failed to unlink '%s'"), path);
}
-strbuf_setlen(path, baselen);
+strbuf_release(&sbuf);
}
diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
index 7fc7be5..a94e7c4 100644
--- a/compat/precompose_utf8.h
+++ b/compat/precompose_utf8.h
@@ -27,7 +27,7 @@ typedef struct {
} PREC_DIR;
void precompose_argv(int argc, const char **argv);
-void probe_utf8_pathname_composition(struct strbuf *path);
+void probe_utf8_pathname_composition(void);
PREC_DIR *precompose_utf8_opendir(const char *dirname);
struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *dirp);
And this is fix for David:
diff --git a/refs.h b/refs.h
index f499093..7dee497 100644
--- a/refs.h
+++ b/refs.h
@@ -670,7 +670,6 @@ typedef int (*ref_transaction_verify_fn)(struct
ref_transaction *transaction,
unsigned int flags, struct strbuf *err);
typedef int (*ref_transaction_commit_fn)(struct ref_transaction *transaction,
struct strbuf *err);
-typedef void (*ref_transaction_free_fn)(struct ref_transaction *transaction);
/* reflog functions */
typedef int (*for_each_reflog_ent_fn)(const char *refname,
--
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 01/43] refs.c: create a public version of verify_refname_available
On 2015-10-03 18.50, David Turner wrote: > On Sat, 2015-10-03 at 07:02 +0200, Torsten Bögershausen wrote: >> On 29.09.15 00:01, David Turner wrote: >> (Not sure if this is the right thread to report on) >> >> In file included from builtin/commit.c:20: >> ./refs.h:695:16: warning: redefinition of typedef 'ref_transaction_free_fn' >> is a C11 feature >> [-Wtypedef-redefinition] >> typedef void (*ref_transaction_free_fn)(struct ref_transaction *transaction); >>^ > > Fixed, thanks. > > What compiler flag did you turn on to see that warning? This compiler reported it as an error, not a warning: gcc --version i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5666) (dot 3) Copyright (C) 2007 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. -- 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: Git feature request: mark a commit as minor
On Fri, Oct 2, 2015 at 11:44 PM, Felipe Micaroni Lalli wrote: > Actually we already use the keyword MINOR for that, exactly as you said. > > The suggestion was made because I think it is a common behavior and it > would be nice to be a meta info to standardize this (today each team > adopt a different pattern for that - you used "TRIVIAL" e.g.). Nice > things could be done with this meta-info. It could be totally ignored > (current git operation) or it could be used to filter, to sort, to group > commits, to show the log pretty etc. > >> The issue is that not everyone considers these changes as "minor". > > I understand this issue, I know it is subjective. But if someone don't > want to make the distinction just don't use the argument --hide-minor > for example. > > I think use of git-notes is probably the best way. Not sure how/if you can implement filtering on that, but I don't personally think something like this belongs in core git. Regards, Jake -- 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 41/68] init: use strbufs to store paths
On Sat, Oct 03, 2015 at 11:12:13PM +0200, Torsten Bögershausen wrote:
> > Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly
> > to probe_utf8_pathname_composition() without changing its signature.
> True, ( I was thinking that the test did only work on case insensitive FS).
> We can skip that change.
>
> Beside that, I later realized, that a better signature could be:
> +void probe_utf8_pathname_composition(const char *path, size_t len)
>
> I can send a proper patch the next days.
That is the original signature, before my sprintf series. I do not mind
leaving that as-is, and simply cleaning up probe_utf8_pathname_composition
by using a strbuf internally there. Though I have to wonder if it even
needs us to pass _anything_ at that point. It could just call
git_path_buf("config%s", auml_nfd) itself. The whole reason to pass
anything was to let it reuse the buffer the caller had.
-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: [RFC/PATCH v1] Add Travis CI support
On Sat, Oct 03, 2015 at 11:23:52PM +0100, Roberto Tyley wrote: > On 28 September 2015 at 19:47, Junio C Hamano wrote: > > I won't enable it on github.com:gitster/git anyway, so I do not > > think that is a concern. I thought what people are talking about > > was to add it on github.com:git/git, but have I been misreading the > > thread? I do not even own the latter repository (I only can push > > into it). > > I was momentarily surprised to hear that Junio doesn't own github.com/git/git > but I had a quick look at the github.com/git organisation, and it turns > out that Peff and Scott Chacon are the current owners - so at the > moment I think they're the only ones who could switch on the GitHub > webhook to hit Travis. There is a @git/git team on GitHub that should have full access to the git/git repository, and Junio is on that (but I also do not _expect_ Junio to spend time managing it; he has plenty of other things to do). I am on vacation at the moment, but am happy to look at it when I get back in a few weeks. -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: [RFC/PATCH v1] Add Travis CI support
Junio C Hamano writes: > On Sat, Oct 3, 2015 at 3:23 PM, Roberto Tyley wrote: >> >> Given this, enabling Travis CI for git/git seems pretty low risk, >> are there any strong objections to it happening? > > I still don't see a reason why git/git needs to be the one that is > used, when somebody > so interested (and I seem to see very many of them in the thread) can > sacrifice his or > her own fork and enable it him or herself. To state it a bit differently. If somebody says "I've been maintaining a clone of git/git with Travis webhooks enabled and as the result caught this many glitches during the past two months without any ill side effect. Here are the patches to fix them, and by the way, the first patch in this series is not a fix but the configuration to tell Travis how to run tests so that other people can enable it on _their_ own fork before they send their own series to the mailing list." in the cover letter of a patch series, I would appreciate such a series greatly and would not mind carrying one extra yml file in the tree at all. But that is not what I am seeing in this thread at all. I am tired of hearing people telling others to help them by doing more without doing the grunt work themselves. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v1] Add Travis CI support
On Sat, Oct 3, 2015 at 3:23 PM, Roberto Tyley wrote: > > Given this, enabling Travis CI for git/git seems pretty low risk, > are there any strong objections to it happening? I still don't see a reason why git/git needs to be the one that is used, when somebody so interested (and I seem to see very many of them in the thread) can sacrifice his or her own fork and enable it him or herself. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v1] Add Travis CI support
On 28 September 2015 at 19:47, Junio C Hamano wrote: > I won't enable it on github.com:gitster/git anyway, so I do not > think that is a concern. I thought what people are talking about > was to add it on github.com:git/git, but have I been misreading the > thread? I do not even own the latter repository (I only can push > into it). I was momentarily surprised to hear that Junio doesn't own github.com/git/git but I had a quick look at the github.com/git organisation, and it turns out that Peff and Scott Chacon are the current owners - so at the moment I think they're the only ones who could switch on the GitHub webhook to hit Travis. For what it's worth, I'd love to see Travis CI - or any form of CI - running for the core Git project. It doesn't require giving write access to Travis, and beyond the good reasons given by Lars, I'm also personally interested because it opens up the possibility of some useful enhancements to the submitGit flow - so that you can't send email to the list without knowing you've broken tests first. Regarding Luke's concerns about excess emails coming from CI, default Travis behaviour is for emails to be sent to the committer and author, but only if they have write access to the repository the commit was pushed to: http://docs.travis-ci.com/user/notifications/#How-is-the-build-email-receiver-determined%3F If Travis emails do become problematic, you can disable them completely by adding 2 lines of config to the .travis.yml: http://docs.travis-ci.com/user/notifications/#Email-notifications Given this, enabling Travis CI for git/git seems pretty low risk, are there any strong objections to it happening? -- 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 41/68] init: use strbufs to store paths
On 03.10.15 18:54, Junio C Hamano wrote:
> Torsten Bögershausen writes:
>
>> On 30.09.15 02:23, Jeff King wrote:
>>> On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote:
>>>
I see compile errors on my mac:
>>
>> This is my attempt, passing the test, but not fully polished.
>
> Thanks.
>
>> diff --git a/builtin/init-db.c b/builtin/init-db.c
>> index 89f2c05..60b559c 100644
>> --- a/builtin/init-db.c
>> +++ b/builtin/init-db.c
>> @@ -276,7 +276,9 @@ static int create_default_files(const char
>> *template_path)
>> path = git_path_buf(&buf, "CoNfIg");
>> if (!access(path, F_OK))
>> git_config_set("core.ignorecase", "true");
>> -probe_utf8_pathname_composition(path);
>> +/* Probe utf-8 normalization withou mangling CoNfIG */
>> +path = git_path_buf(&buf, "config");
>> +probe_utf8_pathname_composition(path, strlen(path));
>
> Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly
> to probe_utf8_pathname_composition() without changing its signature.
True, ( I was thinking that the test did only work on case insensitive FS).
We can skip that change.
Beside that, I later realized, that a better signature could be:
+void probe_utf8_pathname_composition(const char *path, size_t len)
I can send a proper patch the next days.
--
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 01/43] refs.c: create a public version of verify_refname_available
On 03.10.15 18:50, David Turner wrote: > On Sat, 2015-10-03 at 07:02 +0200, Torsten Bögershausen wrote: >> On 29.09.15 00:01, David Turner wrote: >>> >> (Not sure if this is the right thread to report on) >> >> In file included from builtin/commit.c:20: >> ./refs.h:695:16: warning: redefinition of typedef 'ref_transaction_free_fn' >> is a C11 feature >> [-Wtypedef-redefinition] >> typedef void (*ref_transaction_free_fn)(struct ref_transaction *transaction); >>^ > > > Fixed, thanks. > > What compiler flag did you turn on to see that warning? Mac OS X, 10.9: gcc --version Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/usr/include/c++/4.2.1 Apple LLVM version 6.0 (clang-600.0.57) (based on LLVM 3.5svn) Target: x86_64-apple-darwin13.4.0 Thread model: posix -- 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: Git feature request: mark a commit as minor
You are right. It could be useful to fix old commits (already pushed) but it could encourage bad practices. Minor changes should be avoided, it is an exception, not a rule. Thank you Fredrik. On 03/10/2015 15:12, Fredrik Gustafsson wrote: > On Fri, Oct 02, 2015 at 06:38:46PM -0300, Felipe Micaroni Lalli wrote: >> A minor change (also called "cosmetic") usually is a typo fix, doc >> improvement, a little code refactoring that don't change the behavior etc. >> >> In Wikipedia we can mark an edition as "minor". >> >> It would be nice to have an argument like "--minor" in git-commit to >> mark the commit as minor. Also, filter in git-log (like --hide-minor) to >> hide the minor changes. The git-log could be optimized to show minor >> commits more discreetly. > > I can see your problem and implement your suggest is a solution that > would work. However since this is a common problem, git already has a > solution, that is the interactive rebase. > > You can read a discussion about when to use merge and rebase here: > http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html > > This work method make the "minor" commits to go away. There shouldn't be > any minor, or "fixup" commits in your history (of course there's > exception). > > Minor things should be caught in your code review process and then > fixed, rebased and the merged again. > > Or do I miss a usecase here? > signature.asc Description: OpenPGP digital signature
[PATCH 2/2] quote: move comment before sq_quote_buf()
A big comment at the beginning of quote.c is really
related to sq_quote_buf(), so let's move it in front
of this function.
---
quote.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/quote.c b/quote.c
index 890885a..fe884d2 100644
--- a/quote.c
+++ b/quote.c
@@ -4,6 +4,11 @@
int quote_path_fully = 1;
+static inline int need_bs_quote(char c)
+{
+ return (c == '\'' || c == '!');
+}
+
/* Help to copy the thing properly quoted for the shell safety.
* any single quote is replaced with '\'', any exclamation point
* is replaced with '\!', and the whole thing is enclosed in a
@@ -16,11 +21,6 @@ int quote_path_fully = 1;
* a'b ==> a'\''b==> 'a'\''b'
* a!b ==> a'\!'b==> 'a'\!'b'
*/
-static inline int need_bs_quote(char c)
-{
- return (c == '\'' || c == '!');
-}
-
void sq_quote_buf(struct strbuf *dst, const char *src)
{
char *to_free = NULL;
--
2.6.0.rc2.23.g0e57679
--
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] quote: fix broken sq_quote_buf() related comment
Since 77d604c (Enhanced sq_quote(), 10 Oct 2005), the comment at the beginning of quote.c is broken. Let's fix it. --- quote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/quote.c b/quote.c index 7920e18..890885a 100644 --- a/quote.c +++ b/quote.c @@ -7,6 +7,7 @@ int quote_path_fully = 1; /* Help to copy the thing properly quoted for the shell safety. * any single quote is replaced with '\'', any exclamation point * is replaced with '\!', and the whole thing is enclosed in a + * single quote pair. * * E.g. * original sq_quote result -- 2.6.0.rc2.23.g0e57679 -- 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: Git feature request: mark a commit as minor
On Fri, Oct 02, 2015 at 06:38:46PM -0300, Felipe Micaroni Lalli wrote: > A minor change (also called "cosmetic") usually is a typo fix, doc > improvement, a little code refactoring that don't change the behavior etc. > > In Wikipedia we can mark an edition as "minor". > > It would be nice to have an argument like "--minor" in git-commit to > mark the commit as minor. Also, filter in git-log (like --hide-minor) to > hide the minor changes. The git-log could be optimized to show minor > commits more discreetly. I can see your problem and implement your suggest is a solution that would work. However since this is a common problem, git already has a solution, that is the interactive rebase. You can read a discussion about when to use merge and rebase here: http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html This work method make the "minor" commits to go away. There shouldn't be any minor, or "fixup" commits in your history (of course there's exception). Minor things should be caught in your code review process and then fixed, rebased and the merged again. Or do I miss a usecase here? -- Fredrik Gustafsson phone: +46 733-608274 e-mail: iv...@iveqy.com website: http://www.iveqy.com -- 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 43/43] refs: tests for db backend
On Mon, Sep 28, 2015 at 06:02:18PM -0400, David Turner wrote:
> Add tests for the database backend.
>
> Signed-off-by: David Turner
> ---
> t/t1460-refs-be-db.sh| 1103
> ++
> t/t1470-refs-be-db-reflog.sh | 353 ++
> 2 files changed, 1456 insertions(+)
> create mode 100755 t/t1460-refs-be-db.sh
> create mode 100755 t/t1470-refs-be-db-reflog.sh
These break 'make test' on builds without the db backend. Maybe squash
in something like the following:
diff --git a/t/t1460-refs-be-db.sh b/t/t1460-refs-be-db.sh
index f13b0f0..c8222ed 100755
--- a/t/t1460-refs-be-db.sh
+++ b/t/t1460-refs-be-db.sh
@@ -9,6 +9,11 @@ test_description='Test lmdb refs backend'
TEST_NO_CREATE_REPO=1
. ./test-lib.sh
+if ! test -e ../../test-refs-be-lmdb; then
+ skip_all="Skipping lmdb refs backend tests, lmdb backend not built"
+ test_done
+fi
+
raw_ref() {
test-refs-be-lmdb "$1"
}
diff --git a/t/t1470-refs-be-db-reflog.sh b/t/t1470-refs-be-db-reflog.sh
index 99a705d..2538a58 100755
--- a/t/t1470-refs-be-db-reflog.sh
+++ b/t/t1470-refs-be-db-reflog.sh
@@ -8,6 +8,11 @@ test_description='Test prune and reflog expiration'
TEST_NO_CREATE_REPO=1
. ./test-lib.sh
+if ! test -e ../../test-refs-be-lmdb; then
+ skip_all="Skipping lmdb refs backend tests, lmdb backend not built"
+ test_done
+fi
+
raw_reflog() {
cat .git/logs/$1 2>/dev/null || test-refs-be-lmdb -l "$1"
}
Also, test 18 in t1460 is broken:
expecting success:
git symbolic-ref refs/heads/self refs/heads/self &&
test_when_finished "delete_ref refs/heads/self" &&
test_must_fail git update-ref -d refs/heads/self
test_must_fail: command succeeded: git update-ref -d refs/heads/self
not ok 18 - update-ref -d is not confused by self-reference
#
# git symbolic-ref refs/heads/self refs/heads/self &&
# test_when_finished "delete_ref refs/heads/self" &&
# test_must_fail git update-ref -d refs/heads/self
#
--
Dennis Kaarsemaker
http://twitter.com/seveas
--
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 v8 0/7] git-p4: add support for large file systems
Junio C Hamano writes:
> Luke Diamand writes:
>
>> All looks good to me, Ack.
>>
>> One tiny thing perhaps Junio could comment on: the git commit message
>> for 75abe9fa5b39980de27dfc33dd5b4f4b5926f34c, "git-p4: add optional
>> type specifier to gitConfig reader" uses what looks like UTF-8 encoded
>> left and right quotes, rather than regular ASCII quotes ("). I don't
>> know if that matters.
>
> Yeah, I noticed them, too. In general, I'd prefer to avoid these
> pretty-quotes myself, as they typically do not add much information
> (unless nesting matters, which is usually not the case in the log
> message, or something) and the primary effect of them is to force us
> to step outside ASCII, which causes editors and pagers to misalign
> for some people.
>
> But it is not too huge an issue that it is worth to go back and fix
> them, either.
Well, I looked at it again and it also replaced double-dash before
option names like --bool and --int with something funny (are these
em-dashes?), which is a more serious bogosity than pretty quotes, so
I ended up amending the message for that commit after all.
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 v8 0/7] git-p4: add support for large file systems
Luke Diamand writes:
> On 26 September 2015 at 08:54, wrote:
>> From: Lars Schneider
>>
>> diff to v7:
>> * fix commit message line length (thanks Junio)
>> * fix sync command for large file system support (thanks Luke!)
>> * add test case for sync command
>> * rename git-p4.pushLargeFiles to git-p4.largeFilePush for consistency with
>> other git-p4.largeFile* configs
>> * fix relative path handling for clone operation which caused a crash in the
>> disk space check and t9819 (thanks Luke!)
>> * use test_path_is_missing instead of !test_path_is_file (thanks Luke!)
>>
>> The patch is again based on maint (ee6ad5f) as patches v1-v6 before.
>
>
> All looks good to me, Ack.
>
> One tiny thing perhaps Junio could comment on: the git commit message
> for 75abe9fa5b39980de27dfc33dd5b4f4b5926f34c, "git-p4: add optional
> type specifier to gitConfig reader" uses what looks like UTF-8 encoded
> left and right quotes, rather than regular ASCII quotes ("). I don't
> know if that matters.
Yeah, I noticed them, too. In general, I'd prefer to avoid these
pretty-quotes myself, as they typically do not add much information
(unless nesting matters, which is usually not the case in the log
message, or something) and the primary effect of them is to force us
to step outside ASCII, which causes editors and pagers to misalign
for some people.
But it is not too huge an issue that it is worth to go back and fix
them, either.
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 1/2] test-path-utils.c: remove incorrect assumption
Ray Donnelly writes: > In normalize_ceiling_entry(), we test that normalized paths end with > slash, *unless* the path to be normalized was already the root > directory. > > However, normalize_path_copy() does not even enforce this condition. Perhaps the real issue to be addressed is the above, and your patch is killing a coalmine canary? Some callers of this function in real code (i.e. not the one you are removing the check) do seem to depend on that condition, e.g. the codepath in clone that leads to add_to_alternates_file() wants to make sure it does not add an duplicate, so it may end up not noticing /foo/bar and /foo/bar/ are the same thing, no? There may be others. -- 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/9] ref-filter: implement %(if), %(then), and %(else) atoms
Matthieu Moy writes:
> My take on it:
>
> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
> format string between %(if) and %(then) expands to an empty string, or
> to only whitespaces, then the string following %(then) is printed.
> Otherwise, the string following %(else), if any, is printed.
>
>> +When a scripting language specific quoting is in effect,
>
> This may not be immediately clear to the reader. I'd add explicitly:
>
> When a scripting language specific quoting is in effect (i.e. one of
> `--shell`, `--perl`, `--python`, `--tcl` is used), ...
I found all the suggestions very good, except that the distinction
between "expands to" and "is printed" bothers me a bit, as they want
to mean exactly the same thing (imagine this whole thing were inside
another %(if)...%(then)).
>> EXAMPLES
>>
>
> This is just the context of the patch, but I read it as a hint that we
> could add some examples with complex --format usage to illustrate the
> theory above.
Yes ;-)
>> +static int is_empty(const char * s){
>
> char * s -> char *s
Also "{" must sit alone on the next line.
>> +static void then_atom_handler(struct atom_value *atomv, struct
>> ref_formatting_state *state)
>> +{
>> +struct ref_formatting_stack *cur = state->stack;
>> +struct if_then_else *if_then_else = (struct if_then_else
>> *)cur->at_end_data;
>> +
>> +if (!if_then_else)
>> +die(_("format: %%(then) atom used without an %%(if) atom"));
>
> You've just casted at_end_data to if_then_else. if_then_else being not
> NULL does not mean that it is properly typed. It can be the at_end_data
> of another opening atom. What happens if you use
> %(align)foo%(then)bar%(end)?
>
> One way to be safer would be to check that cur->at_end does point to
> if_then_else_handler.
Good eyes. Thanks.
> So, while parsing the %(else)...%(end), the stack contains both the
> %(then)...%(else) part, and the %(else)...%(end).
>
> I'm wondering if we can simplify this. We already know if the condition
> is satisfied, and if it's not, we can just drop the %(then) part right
> now, and write to the top of the stack normally (the %(end) atom will
> only have to pop the string normally). But if the condition is not
> satisfied, we need to preserve the %(then) part and need to do something
> about the %(else).
Good suggestion.
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 41/68] init: use strbufs to store paths
Torsten Bögershausen writes:
> On 30.09.15 02:23, Jeff King wrote:
>> On Tue, Sep 29, 2015 at 04:50:39PM -0700, Michael Blume wrote:
>>
>>> I see compile errors on my mac:
>>>
>
> This is my attempt, passing the test, but not fully polished.
Thanks.
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 89f2c05..60b559c 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -276,7 +276,9 @@ static int create_default_files(const char *template_path)
> path = git_path_buf(&buf, "CoNfIg");
> if (!access(path, F_OK))
> git_config_set("core.ignorecase", "true");
> - probe_utf8_pathname_composition(path);
> + /* Probe utf-8 normalization withou mangling CoNfIG */
> + path = git_path_buf(&buf, "config");
> + probe_utf8_pathname_composition(path, strlen(path));
Hmph, Peff's quick-fix passed the original "CoNfIg" in &buf directly
to probe_utf8_pathname_composition() without changing its signature.
What is the reason behind these two changes? i.e. why is it
inappropriate to use "CoNfIg" (and append the auml to it to use for
the checking) and why does the function need to take pointer + len,
only to store it in another strbuf itself?
> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index b4dd3c7..37172a4 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -8,6 +8,7 @@
> #include "cache.h"
> #include "utf8.h"
> #include "precompose_utf8.h"
> +#include "strbuf.h"
>
> typedef char *iconv_ibp;
> static const char *repo_encoding = "UTF-8";
> @@ -36,28 +37,33 @@ static size_t has_non_ascii(const char *s, size_t maxlen,
> size_t *strlen_c)
> }
>
>
> -void probe_utf8_pathname_composition(struct strbuf *path)
> +void probe_utf8_pathname_composition(char *path, int len)
> {
> static const char *auml_nfc = "\xc3\xa4";
> static const char *auml_nfd = "\x61\xcc\x88";
> - size_t baselen = path->len;
> + struct strbuf sbuf;
> int output_fd;
> if (precomposed_unicode != -1)
> return; /* We found it defined in the global config, respect it
> */
> - strbuf_addstr(path, auml_nfc);
> - output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600);
> + strbuf_init(&sbuf, len+3);
> + strbuf_add(&sbuf, path, len);
> + strbuf_addstr(&sbuf, auml_nfc);
> + output_fd = open(sbuf.buf, O_CREAT|O_EXCL|O_RDWR, 0600);
> + fprintf(stderr, "%s/%s:%d sbuf.buf=%s\n",
> + __FILE__, __FUNCTION__, __LINE__,
> sbuf.buf);
> if (output_fd >= 0) {
> close(output_fd);
> - strbuf_setlen(path, baselen);
> - strbuf_addstr(path, auml_nfd);
> + strbuf_setlen(&sbuf, len);
> + strbuf_addstr(&sbuf, auml_nfd);
> + fprintf(stderr, "%s/%s:%d sbuf.buf=%s\n",
> + __FILE__, __FUNCTION__, __LINE__,
> sbuf.buf);
> precomposed_unicode = access(path, R_OK) ? 0 : 1;
> git_config_set("core.precomposeunicode", precomposed_unicode ?
> "true" : "false");
> - strbuf_setlen(path, baselen);
> - strbuf_addstr(path, auml_nfc);
> + strcpy(path + len, auml_nfc);
> if (unlink(path))
> die_errno(_("failed to unlink '%s'"), path);
> }
> - strbuf_setlen(path, baselen);
> + strbuf_release(&sbuf);
> }
>
>
> diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h
> index 7fc7be5..3b73585 100644
> --- a/compat/precompose_utf8.h
> +++ b/compat/precompose_utf8.h
> @@ -27,7 +27,7 @@ typedef struct {
> } PREC_DIR;
>
> void precompose_argv(int argc, const char **argv);
> -void probe_utf8_pathname_composition(struct strbuf *path);
> +void probe_utf8_pathname_composition(char *, int);
>
> PREC_DIR *precompose_utf8_opendir(const char *dirname);
> struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *dirp);
--
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 01/43] refs.c: create a public version of verify_refname_available
On Sat, 2015-10-03 at 07:02 +0200, Torsten Bögershausen wrote: > On 29.09.15 00:01, David Turner wrote: > > > (Not sure if this is the right thread to report on) > > In file included from builtin/commit.c:20: > ./refs.h:695:16: warning: redefinition of typedef 'ref_transaction_free_fn' > is a C11 feature > [-Wtypedef-redefinition] > typedef void (*ref_transaction_free_fn)(struct ref_transaction *transaction); >^ Fixed, thanks. What compiler flag did you turn on to see that warning? -- 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 v8 0/7] git-p4: add support for large file systems
On 26 September 2015 at 08:54, wrote:
> From: Lars Schneider
>
> diff to v7:
> * fix commit message line length (thanks Junio)
> * fix sync command for large file system support (thanks Luke!)
> * add test case for sync command
> * rename git-p4.pushLargeFiles to git-p4.largeFilePush for consistency with
> other git-p4.largeFile* configs
> * fix relative path handling for clone operation which caused a crash in the
> disk space check and t9819 (thanks Luke!)
> * use test_path_is_missing instead of !test_path_is_file (thanks Luke!)
>
> The patch is again based on maint (ee6ad5f) as patches v1-v6 before.
All looks good to me, Ack.
One tiny thing perhaps Junio could comment on: the git commit message
for 75abe9fa5b39980de27dfc33dd5b4f4b5926f34c, "git-p4: add optional
type specifier to gitConfig reader" uses what looks like UTF-8 encoded
left and right quotes, rather than regular ASCII quotes ("). I don't
know if that matters.
Thanks,
Luke
>
> Cheers,
> Lars
>
> Lars Schneider (7):
> git-p4: add optional type specifier to gitConfig reader
> git-p4: add gitConfigInt reader
> git-p4: return an empty list if a list config has no values
> git-p4: add file streaming progress in verbose mode
> git-p4: check free space during streaming
> git-p4: add support for large file systems
> git-p4: add Git LFS backend for large file system
>
> Documentation/git-p4.txt | 32 +
> git-p4.py | 270 +++---
> t/t9823-git-p4-mock-lfs.sh | 192 ++
> t/t9824-git-p4-git-lfs.sh | 288
> +
> 4 files changed, 766 insertions(+), 16 deletions(-)
> create mode 100755 t/t9823-git-p4-mock-lfs.sh
> create mode 100755 t/t9824-git-p4-git-lfs.sh
>
> --
> 2.5.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 1/2] test-path-utils.c: remove incorrect assumption
I'm going to have to attach this as a file, git-send-email isn't
working for me; apologies.
On Sat, Oct 3, 2015 at 1:44 PM, Ray Donnelly wrote:
> In normalize_ceiling_entry(), we test that normalized paths end with
> slash, *unless* the path to be normalized was already the root
> directory.
>
> However, normalize_path_copy() does not even enforce this condition.
>
> Even worse: on Windows, the root directory gets translated into a
> Windows directory by the Bash before being passed to `git.exe` (or
> `test-path-utils.exe`), which means that we cannot even know whether
> the path that was passed to us was the root directory to begin with.
>
> This issue has already caused endless hours of trying to "fix" the
> MSYS2 runtime, only to break other things due to MSYS2 ensuring that
> the converted path maintains the same state as the input path with
> respect to any final '/'.
>
> So let's just forget about this test. It is non-essential to Git's
> operation, anyway.
>
> Ack-by: Johannes Schindelin
> Signed-off-by: Ray Donnelly
> ---
> test-path-utils.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/test-path-utils.c b/test-path-utils.c
> index 3dd3744..c67bf65 100644
> --- a/test-path-utils.c
> +++ b/test-path-utils.c
> @@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct
> string_list_item *item, void *unused)
> if (normalize_path_copy(buf, ceil) < 0)
> die("Path \"%s\" could not be normalized", ceil);
> len = strlen(buf);
> - if (len > 1 && buf[len-1] == '/')
> - die("Normalized path \"%s\" ended with slash", buf);
> free(item->string);
> item->string = xstrdup(buf);
> return 1;
> --
> 2.5.2
0001-test-path-utils.c-remove-incorrect-assumption.patch
Description: Binary data
[PATCH 1/2] test-path-utils.c: remove incorrect assumption
In normalize_ceiling_entry(), we test that normalized paths end with
slash, *unless* the path to be normalized was already the root
directory.
However, normalize_path_copy() does not even enforce this condition.
Even worse: on Windows, the root directory gets translated into a
Windows directory by the Bash before being passed to `git.exe` (or
`test-path-utils.exe`), which means that we cannot even know whether
the path that was passed to us was the root directory to begin with.
This issue has already caused endless hours of trying to "fix" the
MSYS2 runtime, only to break other things due to MSYS2 ensuring that
the converted path maintains the same state as the input path with
respect to any final '/'.
So let's just forget about this test. It is non-essential to Git's
operation, anyway.
Ack-by: Johannes Schindelin
Signed-off-by: Ray Donnelly
---
test-path-utils.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/test-path-utils.c b/test-path-utils.c
index 3dd3744..c67bf65 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -21,8 +21,6 @@ static int normalize_ceiling_entry(struct
string_list_item *item, void *unused)
if (normalize_path_copy(buf, ceil) < 0)
die("Path \"%s\" could not be normalized", ceil);
len = strlen(buf);
- if (len > 1 && buf[len-1] == '/')
- die("Normalized path \"%s\" ended with slash", buf);
free(item->string);
item->string = xstrdup(buf);
return 1;
--
2.5.2
--
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 8/9] branch: use ref-filter printing APIs
Karthik Nayak writes:
> - if (upstream_is_gone) {
> - if (show_upstream_ref)
> - strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
The old string was translated, and you're replacing it with one which
isn't.
I'm not a big fan of translation, so that change doesn't disturb me, but
people used to having "git branch" talk their native language here may
care.
Be careful: translation should happen only in porcelain. Typicall,
"for-each-ref" should anyway continue saying "gone" in english whatever
the configuration is.
See e.g. 7a76c28 (status: disable translation when --porcelain is used,
2014-03-20) for an example of translation only for porcelain (that was
for status, but also for ahead/behind/gone).
> +static char *get_format(struct ref_filter *filter, int maxwidth, const char
> *remote_prefix)
I'd call that "build_format", since "get_..." usually implies that the
value exists already and you're retrieving it.
> +{
> + char *remote = NULL;
> + char *local = NULL;
> + char *final = NULL;
> +
> + if (filter->verbose) {
> + if (filter->verbose > 1)
> + local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)
> %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s"
> + " %%(objectname:short,7)
> %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)"
> +
> "%%(if)%%(upstream:track)%%(then)%%(upstream:track)
> %%(end)%%(contents:subject)",
> + branch_get_color(BRANCH_COLOR_CURRENT),
> maxwidth, branch_get_color(BRANCH_COLOR_RESET),
> +
> branch_get_color(BRANCH_COLOR_UPSTREAM),
> branch_get_color(BRANCH_COLOR_RESET));
OK, that is a hell of a block of code ;-).
You should factor the common portions out of these if/else. C allows you
to write several string litteral next to each other, so you can eg.
#define STAR_IF_HEAD "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)"
#define ALIGNED_REFNAME "%%(align:%d,left)%%(refname:short)%%(end)"
and then
STAR_IF_HEAD ALIGNED_REFNAME ...
Actually, this is not a performance-cricical piece of code at all, so I
think it's even better to build an strbuf little by little using
repeated strbuf_addf calls. This way you can do things like
strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)",
branch_get_color(BRANCH_COLOR_CURRENT));
strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
which makes it much easier to pair the %s and the corresponding
branch_get_color() or the %d and maxwidth.
But fundamentally, I think this function is doing the right thing. The
function is a bit complex (I think it will be much nicer to read when
better factored), but 1) it allows getting rid of a lot of specific code
and 2) it's a proof that --format is expressive enough.
> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
> '
>
> cat >expect <<\EOF
> -b1 [origin/master: ahead 1, behind 1] d
> -b2 [origin/master: ahead 1, behind 1] d
> -b3 [origin/master: behind 1] b
> -b4 [origin/master: ahead 2] f
> -b5 [brokenbase: gone] g
> +b1 [origin/master] [ahead 1, behind 1] d
> +b2 [origin/master] [ahead 1, behind 1] d
> +b3 [origin/master] [behind 1] b
> +b4 [origin/master] [ahead 2] f
> +b5 [brokenbase] [gone] g
This corresponds to this part of the commit message:
> Since branch.c is being ported to use ref-filter APIs to print the
> required data, it is constricted to the constraints of printing as per
> ref-filter. Which means branch.c can only print as per the atoms
> available in ref-filter. Hence the "-vv" option of 'git branch' now
> prints the upstream and its tracking details separately as
> "[] []" instead of "[: info>]".
>
> Make changes in /t/t6040-tracking-info.sh to reflect the change.
nit: /t/t... -> t/t... (remove leading /)
That is a behavior change, and I actually prefered the previous one.
I actually don't understand why you can't get the old syntax: the []
around the refname come from the format string it get_format(), so you
could decide to change "[%s%%(upstream:short)%s]" to
"[%s%%(upstream:short)%s: ". The brackets around the status are a bit
more tricky, since they were here before your series. But we could have
a %(upstream:track,nobracket) to display just the status without the
brackets around.
Then, the code must deal with up-to-date branches for which no
" :ahead/behind" must be displayed. Probably one more if/then/else
nested in the first one.
So, it seems feasible to me to keep the old behavior with the new
system, even though it's going to be a bit tricky.
--
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 7/9] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
Karthik Nayak writes:
> diff --git a/ref-filter.c b/ref-filter.c
> index 099acd6..48b06e3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1133,8 +1133,10 @@ static void populate_value(struct ref_array_item *ref)
> char buf[40];
>
> if (stat_tracking_info(branch, &num_ours,
> -&num_theirs, NULL))
> +&num_theirs, NULL)) {
> + v->s = xstrdup("[gone]");
I think just "[gone]" without the xstrdup is OK, and avoids leaking one
string.
--
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 6/9] ref-filter: introduce format_ref_array_item()
Karthik Nayak writes: > Introduce format_ref_array_item() which will output the details of a > given ref_array_item as per the given format and quote_style to the > given strbuf. Why do you need it in this series and you could do without for tag? Going through PATCH 8/9, I guess there's something related to --column, but tag also has --column so I'm puzzled. -- 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
git svn pushing to wrong branch
Hello dear gitters, I've been trying to use git-svn to work with branches. When I merge branch A into branch B, and then do a dcommit, the change is not pushed to branch B. I've attached a bash script that demonstrates the issue. I've tested with svn 1.9 and git 2.5. The problem goes away when the merge is done with --no-ff. Best regards, Jos van den Oever mergetest.sh Description: application/shellscript
Re: [PATCH 3/9] ref-filter: add support for %(path) atom
Karthik Nayak writes:
> This adds %(path) and %(path:short) atoms. The %(path) atom will print
> the path of the given ref, while %(path:short) will only print the
> subdirectory of the given ref.
What does "path" mean in this context? How is it different from
%(refname)?
I found the answer below, but I could not guess from the doc and commit
message. Actually, I'm not sure %(path) is the right name. If you want
the "file/directory" analogy, then %(dirname) would be better.
> + } else if (match_atom_name(name, "path", &valp)) {
> + const char *sp, *ep;
> +
> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)
> + continue;
> +
> + sp = strchr(ref->refname, '/');
> + ep = strchr(++sp, '/');
This assumes you have two / in the fullrefname. It is normally the case,
but one can also create eg. refs/foo references. AFAIK, in this case sp
will be NULL, and you're then calling strchr(++NULL) which may segfault.
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index d7f7a18..5557657 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=)' '
> test_cmp expect actual
> '
>
> +
> test_expect_success 'check %(if:notequals=)' '
Useless new blank line.
> +test_expect_success 'check %(path)' '
> + git for-each-ref --format="%(path)" >actual &&
> + cat >expect <<-\EOF &&
> + refs/heads
You should add eg.
git update-ref refs/foo HEAD
git update-ref refs/foodir/bar/boz HEAD
before the test to check and document the behavior for such refnames.
--
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/9] ref-filter: implement %(if), %(then), and %(else) atoms
Karthik Nayak writes:
> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).
I prefer ... to .., which often means "interval" as in HEAD^^..HEAD.
> If there is an atom with value or string literal after the %(if)
I find this explanation hard to read, and ambiguous: what does "atom
with value" mean?
> then everything after the %(then) is printed, else if the %(else) atom
> is used, then everything after %(else) is printed. If the string
> contains only whitespaces, then it is not considered.
"the string" is ambiguous again. I guess it's "what's between %(if) and
%(then)", but it could be clearer. And it's not clear what "not
considered" means.
My take on it:
Implement %(if), %(then) and %(else) atoms. Used as
%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
format string between %(if) and %(then) expands to an empty string, or
to only whitespaces, then the string following %(then) is printed.
Otherwise, the string following %(else), if any, is printed.
> +When a scripting language specific quoting is in effect,
This may not be immediately clear to the reader. I'd add explicitly:
When a scripting language specific quoting is in effect (i.e. one of
`--shell`, `--perl`, `--python`, `--tcl` is used), ...
> EXAMPLES
>
This is just the context of the patch, but I read it as a hint that we
could add some examples with complex --format usage to illustrate the
theory above.
> + if (if_then_else->condition_satisfied && if_then_else->else_atom) {
// cs && else
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
> + } else if (if_then_else->else_atom) {
// !cs && else
> + strbuf_swap(&cur->output, &prev->output);
> + strbuf_reset(&cur->output);
> + pop_stack_element(&cur);
> + } else if (!if_then_else->condition_satisfied)
// !cs && !else
> + strbuf_reset(&cur->output);
This if/else if/else if looks hard to read to me. I had to add the
comments above as a note to myself to get the actual full condition for
3 branches.
The reasoning would be clearer to me as:
if (if_then_else->else_atom) {
/*
* There is an %(else) atom: we need to drop one state from the
* stack, either the %(else) branch if the condition is satisfied, or
* the %(then) branch if it isn't.
*/
if (if_then_else->condition_satisfied) {
strbuf_reset(&cur->output);
pop_stack_element(&cur);
} else {
strbuf_swap(&cur->output, &prev->output);
strbuf_reset(&cur->output);
pop_stack_element(&cur);
}
} else if (if_then_else->condition_satisfied)
/*
* No %(else) atom: just drop the %(then) branch if the
* condition is not satisfied.
*/
strbuf_reset(&cur->output);
> +static void if_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *new;
> + struct if_then_else *if_then_else = xcalloc(sizeof(struct
> if_then_else), 1);
> +
> + if_then_else->if_atom = 1;
Do you ever use this "if_atom"? It doesn't seem so in the current patch,
and it seems like a tautology to me: if you have a struct if_then_else,
then you have seen the %(if).
> +static int is_empty(const char * s){
char * s -> char *s
> +static void then_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *cur = state->stack;
> + struct if_then_else *if_then_else = (struct if_then_else
> *)cur->at_end_data;
> +
> + if (!if_then_else)
> + die(_("format: %%(then) atom used without an %%(if) atom"));
You've just casted at_end_data to if_then_else. if_then_else being not
NULL does not mean that it is properly typed. It can be the at_end_data
of another opening atom. What happens if you use
%(align)foo%(then)bar%(end)?
One way to be safer would be to check that cur->at_end does point to
if_then_else_handler. Or add information to struct ref_formatting_stack
(a Boolean is_if_then_else or an enum).
Also, you need to check that if_then_else->then_atom is not 1.
> +static void else_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *prev = state->stack;
> + struct if_then_else *if_then_else = (struct if_then_else
> *)state->stack->at_end_data;
> +
> + if (!if_then_else)
> + die(_("format: %%(else) atom used without an %%(if) atom"));
Same as above, I guess (not tested) %(align)...%(else) is accepted.
> + if (!if_then_else->then_atom)
> + die(_("format: %%(else) atom used without a %%(then) atom"));
> + if_then_else->else_atom = 1;
> + push_stack_element(&state->stack);
So, while parsing the %(else)...%(end), the stack contains both the
%(then)...%(else) pa
Re: [BUG?] applypatch-msg hook no-longer thinks stdin is a tty
Am 03.10.2015 um 09:37 schrieb Chris Packham: On Sat, Oct 3, 2015 at 6:43 AM, Junio C Hamano wrote: If you want to go interactive from the hook, you'd have to open and interact with /dev/tty yourself in your hook anyway. That may be what I have to do, although I have absolutely no idea how. Something like this: exec <>/dev/tty 1>&0 2>&0 printf "what now: " read answer echo the answer was: "$answer" -- Hannes -- 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?] applypatch-msg hook no-longer thinks stdin is a tty
On Sat, Oct 3, 2015 at 6:43 AM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Chris Packham writes:
>>
>>> As of git 2.6 this has stopped working and stdin always fails the tty
>>> check.
>>
>> We now run that hook thru run_hook_ve(), which closes the standard
>> input (as the hook is not reading anything). Perhaps you can check
>> if your output is connected to the tty instead?
Possibly but I still need to be able to push a prompt out to the user
and receive a response.
>
> s|closes the standard input|opens the standard input for /dev/null|;
>
> Having said that, here are some further thoughts:
>
> * Hooks run via run_hook_ve() and run_hook_le() have their standard
>input connected to /dev/null even before these functions were
>introduced at ae98a008 (Move run_hook() from builtin-commit.c
>into run-command.c (libgit), 2009-01-16). The commit
>consolidated the code to run hooks in "checkout", "commit", "gc",
>and "merge", all of which run their hooks with their standard
>input reading from /dev/null.
>
> * Later at dfa7a6c5 (clone: run post-checkout hook when checking
>out, 2009-03-03) "git clone" learned to run post-checkout hook
>the same way.
>
> * "receive-pack" (which accepts and processes an incoming "git
>push") has pre-receive and post-receive hooks, and they do get
>invoked with their standard input open, but they are connected to
>a pipe to be fed with the information about the push from
>"receive-pack" process.
>
> * "post-rewrite" hooks, invoked by "rebase" and "commit", does get
>invoked with its standard input open, but it is fed with the
>information about the original and the rewritten commit.
>
> So in that sense, "am", primarily because it was implemented as a
> script, was an oddball. It should have been connecting the standard
> input to /dev/null in order to be consistent with others, but it did
> not even bother to do so.
>
> We _could_ leave the standard input connected to the original
> process's standard input only for the specific hook by doing
> something along the lines of the attached, but I am not sure if it
> is a good change. Given that the majority of existing hooks are
> spawned with their standard input connected to /dev/null (and also
> after scanning the output from "git hooks --help", I did not find
> any that would want to read from the standard input of the original
> process that spawns it), I tend to consider that the change in 2.6
> done as part of rewriting "am" in C is a bugfix, even though an
> unintended one, to make things more consistent.
>
> Besides "consistency", a hook that tried to read from "am"'s
> standard input would have been incorrect in the first place, as it
> is a normal mode of operation to feed one or more patch e-mails from
> the standard input of "git am", i.e.
>
> $ git am
> If you want to go interactive from the hook, you'd have to open and
> interact with /dev/tty yourself in your hook anyway.
That may be what I have to do, although I have absolutely no idea how.
>
> builtin/am.c | 8 +++-
> run-command.c | 30 ++
> run-command.h | 9 +
> 3 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 4f77e07..3d160d9 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -510,9 +510,15 @@ static void am_destroy(const struct am_state *state)
> static int run_applypatch_msg_hook(struct am_state *state)
> {
> int ret;
> + struct child_process custom = CHILD_PROCESS_INIT;
>
> assert(state->msg);
> - ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
> "final-commit"), NULL);
> +
> + custom.env = NULL;
> + custom.no_stdin = 0;
> + custom.stdout_to_stderr = 1;
> +
> + ret = run_hook_le_opt(&custom, "applypatch-msg", am_path(state,
> "final-commit"), NULL);
>
> if (!ret) {
> free(state->msg);
> diff --git a/run-command.c b/run-command.c
> index 3277cf7..dee86df 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -793,7 +793,7 @@ const char *find_hook(const char *name)
> return path.buf;
> }
>
> -int run_hook_ve(const char *const *env, const char *name, va_list args)
> +int run_hook_ve_opt(struct child_process *custom, const char *name, va_list
> args)
> {
> struct child_process hook = CHILD_PROCESS_INIT;
> const char *p;
> @@ -805,13 +805,35 @@ int run_hook_ve(const char *const *env, const char
> *name, va_list args)
> argv_array_push(&hook.args, p);
> while ((p = va_arg(args, const char *)))
> argv_array_push(&hook.args, p);
> - hook.env = env;
> - hook.no_stdin = 1;
> - hook.stdout_to_stderr = 1;
> + hook.env = custom->env;
> + hook.no_stdin = custom->no_stdin;
> + hook.stdout_to_stderr = custom->stdout_to_stderr;
>
> return run_command(&hook);
> }
>
> +int run_hook_ve(const char *const
Re: [PATCH 4/9] ref-filter: modify "%(objectname:short)" to take length
On Sat, Oct 3, 2015 at 2:32 AM, Junio C Hamano wrote: > Karthik Nayak writes: > >> Add support for %(objectname:short,) which would print the >> abbreviated unique objectname of given length. When no length is >> specified 7 is used. The minimum length is 4. > > It would have to be "short=", not "short,", if I > recall the previous discussion on width=, etc., on the %(align) > atom. Yea, I got confused by "The "align:" is followed by `` and `` in any order separated by a comma". Will change, Thanks. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ref-filter: implement %(if:equals=) and %(if:notequals=)
On Sat, Oct 3, 2015 at 2:24 AM, Junio C Hamano wrote: > Karthik Nayak writes: > >> Implement %(if:equals=) wherein the if condition is only >> satisfied if the value obtained between the %(if:...) and %(then) atom >> is the same as the given ''. >> >> Similarly, implement (if:notequals=) wherein the if condition >> is only satisfied if the value obtained between the %(if:...) and >> %(then) atom is differnt from the given ''. >> >> Add tests and Documentation for the same. >> >> Mentored-by: Christian Couder >> Mentored-by: Matthieu Moy >> Signed-off-by: Karthik Nayak >> --- > > The fast that the patch touches only the narrow parts that are > specific to %(if),%(then) and %(else) and does not have to touch any > generic part (other than the populate_value() parser for obvious > reasons) is a good signal that tells us that the basic structure of > the code is very sound. I very much like the direction in which > this series is going ;-) > Thats nice to hear :) -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms
On Sat, Oct 3, 2015 at 2:15 AM, Junio C Hamano wrote:
> Karthik Nayak writes:
>
>> +static int is_empty(const char * s){
>> + while (*s != '\0') {
>> + if (!isspace(*s))
>> + return 0;
>> + s++;
>> + }
>> + return 1;
>> +}
>
> My knee-jerk reaction was "why is space so special?", but if a
> caller really cared, it can do "%(if:not_equal=)%(something)%(then)"
> to unignore spaces in %(something), so it is not a huge deal. It
> may be that ignoring spaces when checking if something is empty is
> so common that this default is useful---I cannot tell offhand.
>
>
My reason for doing this was the "HEAD" atom which prints "*" or " ",
hence I thought strings with only spaces in general shouldn't be accepted.
>> +if::
>> + Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).
>> + If there is an atom with value or string literal after the
>> + %(if) then everything after the %(then) is printed, else if
>> + the %(else) atom is used, then everything after %(else) is
>> + printed.
>
> I notice that "we ignore space when evaluating the string before
> %(then)" is not mentioned here. That fact, and an example or two
> that illustrates the situation where this "ignore spaces" behaviour
> is useful, would be a good thing to document here.
>
Oh Yes! Will do that :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
