Re: [PATCH v3 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 13 Feb 2016, at 18:44, Jeff King <p...@peff.net> wrote:

> On Sat, Feb 13, 2016 at 03:24:16PM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> If config values are queried using 'git config' (e.g. via --get,
>> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
>> find the configuration file where the values were defined.
>> 
>> Teach 'git config' the '--show-origin' option to print the source
>> configuration file for every printed value.
> 
> Thanks, I think this version fixes the correctness issues I mentioned
> earlier. I do still have nits to pick (of course :) ), that we may or
> may not want to deal with.
> 
>> +static void show_config_origin(struct strbuf *buf)
>> +{
>> +const char term = end_null ? '\0' : '\t';
>> +const char *type;
>> +const char *name;
>> +
>> +current_config_type_name(, );
> 
> This double out-parameter feels like a clunky interface.
> 
> I was tempted to suggest that we simply make the "struct config_source"
> available to builtin/config.c (which is already pretty intimate with the
> rest of the config code), and then it can pick out what it wants. But
> there _is_ some logic in the function to convert the NULL "cf" into
> "cmdline".
> 
> Perhaps it would be simpler to just have two accessor functions, and do:
> 
>  strbuf_addstr(buf, current_config_type());
>  ...
>  strbuf_addstr(buf, current_config_name());
> 
> I admit it is a pretty minor point, though.
Agreed, this looks nicer.


> 
>> static int show_all_config(const char *key_, const char *value_, void *cb)
>> {
>> +if (show_origin) {
>> +   struct strbuf buf = STRBUF_INIT;
>> +   show_config_origin();
>> +   fwrite(buf.buf, 1, buf.len, stdout);
>> +   strbuf_release();
>> +}
> 
> The indentation is funky here.
True! Indentation without intention :-) 

> 
> The use of fwrite() to catch the embedded NULs is subtle enough that it
> might be worth a comment.
> 
> It also made me wonder how format_config() handles this. It looks like
> we send the result eventually to fwrite() there, so it all works (and it
> does _not_ have the comment I mentioned :) ).
I will add a comment in both places :-)


> 
>> +test_expect_success '--show-origin' '
>> +>.git/config &&
>> +>"$HOME"/.gitconfig &&
>> +INCLUDE_DIR="$HOME/include" &&
>> +mkdir -p "$INCLUDE_DIR" &&
>> +cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +[user]
>> +absolute = include
>> +EOF
>> +cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +[user]
>> +relative = include
>> +EOF
>> +test_config_global user.global "true" &&
>> +test_config_global user.override "global" &&
>> +test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
>> +test_config include.path ../include/relative.include &&
>> +test_config user.local "true" &&
>> +test_config user.override "local" &&
>> +
>> +cat >expect <<-EOF &&
>> +file:$HOME/.gitconfig   user.global=true
>> +file:$HOME/.gitconfig   user.override=global
>> +file:$HOME/.gitconfig   
>> include.path=$INCLUDE_DIR/absolute.include
>> +file:$INCLUDE_DIR/absolute.include  user.absolute=include
>> +file:.git/configinclude.path=../include/relative.include
>> +file:.git/../include/relative.include   user.relative=include
>> +file:.git/configuser.local=true
>> +file:.git/configuser.override=local
>> +cmdline:user.cmdline=true
>> +EOF
>> +git -c user.cmdline=true config --list --show-origin >output &&
>> +test_cmp expect output &&
>> +
>> +cat >expect <<-EOF &&
>> +file:$HOME/.gitconfigQuser.global
>> +trueQfile:$HOME/.gitconfigQuser.override
>> +globalQfile:$HOME/.gitconfigQinclude.path
>> +
>> $INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
>> +includeQfile:.git/configQinclude.path
>> +
>> ../include/relative.includeQfile:.git/../include

Re: GSoC 2016: applications open, deadline = Fri, 19/2

[Lars Schneider]

On 12 Feb 2016, at 08:10, Matthieu Moy  wrote:

> Christian Couder  writes:
> 
>> Hi,
>> 
>> On Wed, Feb 10, 2016 at 10:31 AM, Matthieu Moy
>>  wrote:
>>> 
>>> So, the first question is: are there volunteers to be GSoC mentors this
>>> year?
>> 
>> I can co-mentor this year too, with you or someone else.
>> With you I think it will work out even if you have less time than last year.
> 
> So, that makes it 4 possible co-mentors, i.e. 2 potential slots. Not
> much, but it starts looking like last year ... ;-).
> 
> Peff, would you be willing to co-admin with me (that would be cool, you
> are the one with most experience here and you know the SFC stuff for
> payment)? Are there any other co-admin volunteer?

I don't know what level of Git development knowledge and what amount of time
is necessary but I would be available as junior co-mentor :-)

Cheers,
Lars--
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] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement

[Lars Schneider]

On 08 Feb 2016, at 13:25, Jeff King <p...@peff.net> wrote:

> On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> The global Travis-CI environment variable CFLAGS did not override the
>> CFLAGS variable in the makefile. Pass CFLAGS as make variable to
>> override it properly.
> 
> Makes sense.
> 
>> In addition to that, add '-Wdeclaration-after-statement' to make a
>> Travis-CI build fail (because of '-Werror') if the code does not adhere
>> to the Git coding style.
> 
> I think this is a good step, but is probably the tip of the iceberg. I
> typically use:
> 
>  -Wall -Werror -Wdeclaration-after-statement
>  -Wpointer-arith -Wstrict-prototypes -Wvla
>  -Wold-style-declaration -Wold-style-definition
> 
> Junio does his integration testing with a similar set, which I think you
> can find in one of the scripts in his "todo" branch.

I collected the warnings from Junio's Make [1] script and merged them with 
yours. This is the resulting warning list for clang and gcc:

-Wdeclaration-after-statement -Wno-format-zero-length -Wold-style-definition 
-Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla

Gcc processes one extra warning that clang doesn't like:
-Wold-style-declaration

The build runs clean with all these checks enabled.

[1] https://github.com/git/git/blob/todo/Make


>> I made this patch because Peff pointed out to me that "git style doesn't
>> allow declaration-after-statement" [1]. I wonder if it would make sense
>> to add this check even in the makefile [2]?
> 
> I think we keep the default CFLAGS to a pretty tame set, so that we
> build out of the box on a large number of compilers. I know I have run
> into problems with -Wold-style-* on clang (though perhaps that is no
> longer the case, as I haven't tried recently), let alone truly antique
> compilers.
> 
> I have, however, wondered if it would make sense to codify this
> knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1
> roughly mean "you are a git dev, you have a reasonably modern compiler,
> and want to be as careful as possible before sending your patches".

That make sense. However, in "development mode" I don't like Werror.
How about two knobs? One called DEVELOPER which enables all the warnings
above and one called CONTINUOUS_INTEGRATION that enables Werror
in addition?


>> I am no make expert, but I
>> also wonder why we don't use the override directive [3] for the CFLAGS?
>> AFAIK this would allow a make invocation like this:
>> 
>> make target CFLAGS+=-Wdeclaration-after-statement
> 
> I think it is rather the opposite. If we used:
> 
>  override CFLAGS+= ...
> 
> in the Makefile, then if a user did:
> 
>  make CFLAGS=...
> 
> we would add to their CFLAGS (whereas without the override, our
> appending is ignored). Our Makefile solves that in a different way,
> though. We pass $(ALL_CFLAGS) to the compiler, and that in turn is made
> up of $(CFLAGS) and $(BASIC_CFLAGS). We assume the user tweaks the
> former, and we set the latter based on Makefile knobs (e.g., NO_CURL,
> etc).
I see. Thanks for the explanation.

Regarding CI checks:

Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]
where I found checkpatch.pl [3]. Would it make sense to check all commits
that are not in next/master/maint with this script on Travis-CI?

Stefan Beller recently mentioned "Adhere to the common coding style of 
Git" [4] where he removed explicit NULL checks. This kind of stuff could be
checked automatically with checkpatch.pl as far as I can see.

[2] http://www.spinics.net/lists/git/msg267445.html
[3] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
[4] http://permalink.gmane.org/gmane.comp.version-control.git/280842

Thanks,
Lars
--
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] On the --depth argument when fetching with submodules

[Lars Schneider]

On 06 Feb 2016, at 01:05, Junio C Hamano  wrote:

> Stefan Beller  writes:
> 
>> Currently when cloning a project, including submodules, the --depth argument
>> is passed on recursively, i.e. when cloning with "--depth 2", both the
>> superproject as well as the submodule will have a depth of 2.  It is not
>> garantueed that the commits as specified by the superproject are included
>> in these 2 commits of the submodule.
>> 
>> Illustration:
>> (superproject with depth 2, so A would have more parents, not shown)
>> 
>> superproject/master: A <- B
>>/  \
>> submodule/master:  C <- D <- E <- F <- G
>> 
>> (Current behavior is to fetch G and F)
> 
> I think the issue is deeper than merely "--depth 2", and you would
> be better off stepping back and think about various use cases to
> make sure that we know what kind of behaviour we want to support
> before delving into one particular corner case.  We currently pass
> the depth recursively, and I do not think it makes much sense, but I
> view it as a secondary question "among the behaviours we want to
> support, which one should be the default?"  It may turn out that not
> passing it recursively at all, or even passing a different depth, is
> a better default, but we wouldn't know until we know what are the
> desirable behaviour in various workflows.
> 
> If you are actively working on the superproject plus some submodules
> but you are merely using the submodule you depicted above, not
> working on changing it, even when you want the full history of the
> superproject (i.e. no "--depth 2"), you may not want history of the
> submodule.  Even though we have a way to say "I am not interested in
> this submodule AT ALL" by not doing "submodule init", not having
> anything at all at the path submodule/ may not allow you to build
> the whole thing, and we currently lack a way to express "I am not
> interested in the history of this thing, but I need at least the
> tree that matches the commit referred to by the superproject".
> 
> If you are working on a single submodule, trying to fix a bug in the
> context of the whole project, you might want to have a single-depth
> clone of the superproject and all other submodules, plus the whole
> history of the single submodule.
> 
> In either of these examples, the top-level "--depth" does not have
> much to do with what depth the user wants to use when cloning or
> fetching the submodule repositories.
> 
> I have a feeling (but I would not be surprised if somebody who uses
> submodules heavily has a counter-example from real life) that
> regardless of "--depth" or full clone, fetching the tip of matching
> branch is not a good default behaviour.  In your picture, even when
> depth is not given at all, there isn't much point fetching F or G.

I really wonder in what cases people use the "--depth" option, too. 
For instance I have never used it in either one of the two cases you
described above. I don't worry about a long running "clone" as it 
usually is a one-time operation.

However, in case of a continuous integration system that starts with
a clean state in the beginning of every run (e.g. Travis CI) a
"clone" operation is no one-time operation anymore. In this case the
"--depth 1" option makes very much sense to me. This was the situation
where I realized the problem that Stefan wants to tackle here and I
tried to make it tangible with a test case [1]. 

On top of that I think Git's error message is really confusing if
you clone a repo with "--depth" that has submodules and Git is not
fetching the necessary submodule commits:

Unable to checkout '$SHA' in submodule path 'path/to/submodule'

I tried to tackle that with [2] which would detect this case and
print the following error instead (slightly changed from the patch):

Unable to checkout '$SHA' in submodule path '/path/to/commit'.
Try to remove the '--depth' argument on clone!

[1] https://www.mail-archive.com/git%40vger.kernel.org/msg82614.html
[2] https://www.mail-archive.com/git%40vger.kernel.org/msg82613.html


> 
>> So to fetch the correct submodule commits, we need to
>> * traverse the superproject and list all submodule commits.
>> * fetch these submodule commits (C and E) by sha1
> 
> I do not think requiring that C to be fetched when the superproject
> is cloned with --depth=2 (hence A and B are present in the result)
> is a good definition of "correct submodule commits".  The initial
> clone could be "superproject follows --depth, all submodules are
> cloned with --depth=1 at the commits referenced by the superproject
> tree"--by that definition, you need E but you do not want C.
> 
> As a specification of the behaviour, the above two might work, but I
> do not think that should be the implementation.  In other words,
> "The implementation should behave as if it did the above two" is OK,
> and it is also OK to qualify with further conditions to help the
> implementation.  For example, the 

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Lars Schneider]

On 05 Feb 2016, at 14:58, Jeff King  wrote:

> On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote:
> 
>>> I'm not sure returning here is the best idea. We won't have a config
>>> filename if we are reading from "-c", but if we return early from this
>>> function, it parses differently than every other line. E.g., with your
>>> patch, if I do:
>>> 
>>>  git config -c foo.bar=true config --sources --list
>>> 
>>> I'll get:
>>> 
>>>  /home/peff/.gitconfig  user.name=Jeff King
>>>  /home/peff/.gitconfig  user.email=p...@peff.net
>>>  ...etc...
>>>  foo.bar=true
>>> 
>>> If somebody is parsing this as a tab-delimited list, then instead of the
>>> source field for that line being empty, it is missing (and it looks like
>>> "foo.bar=true" is the source file). I think it would be more friendly to
>>> consumers of the output to have a blank (i.e., set "fn" to the empty
>>> string and continue in the function).
>> 
>> Or to come up with a special string to denote config values specified on the
>> command line. Maybe somehting like
>> 
>>  foo.bar=true
>> 
>> I acknowledge that "" would be a valid filename on some
>> filesystems, but I think the risk is rather low that someone would actually
>> be using that name for a Git config file.
> 
> Yeah, I agree it's unlikely. And the output is already ambiguous, as the
> first field could be a blob (though I guess the caller knows if they
> passed "--blob" or not). If we really wanted an unambiguous output, we
> could have something like "file:...", "blob:...", etc. But that's a bit
> less readable for humans, and I don't think solves any real-world
> problems.
> 
> So I think it would be OK to use "" here, as long as the
> token is documented.
Sounds good to me. I'll add it!

> Are there any other reasons why current_config_filename() would return
> NULL, besides command-line config? I don't think so. It looks like we
> can read config from stdin, but we use the token "" there for the
> name field (another ambiguity!).
During my testing I haven't found any other case either. To be honest
I didn't know the "stdin" way to set the config! I added a test case for 
that, too!

Thanks,
Lars--
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] config: add '--sources' option to print the source of a config value

[Lars Schneider]

On 05 Feb 2016, at 12:20, Jeff King  wrote:

> On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschnei...@gmail.com wrote:
> 
>> @@ -538,6 +569,17 @@ int cmd_config(int argc, const char **argv, const char 
>> *prefix)
>>  error("--name-only is only applicable to --list or 
>> --get-regexp");
>>  usage_with_options(builtin_config_usage, 
>> builtin_config_options);
>>  }
>> +
>> +const int is_query_action = actions & (
>> +ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|
>> +ACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH
>> +);
>> +
>> +if (show_sources && !is_query_action) {
>> +error("--sources is only applicable to --list or --get-* 
>> actions");
>> +usage_with_options(builtin_config_usage, 
>> builtin_config_options);
>> +}
> 
> Hmm. I had originally envisioned this only being used with "--list", but
> I guess it makes sense to say "--sources --get" to show where the value
> for a particular option is coming from.
> 
> The plural "sources" is a little funny there, though, as we list only
> the "last one wins" final value, not all of them (for that, you can use
> "--sources --get-all", which seems handy). I think it would be
> sufficient for the documentation to make this clear (speaking of which,
> this patch needs documentation...).
Oops. I will add documentation.

> 
> Also, I don't think the feature works with --get-color, --get-colorbool,
> or --get-urlmatch (which don't do their output in quite the same way).
> I think it would be fine to simply disallow --sources with those options
> rather than worry about making it work.
OK, I'll remove them. I don't have experience with these options as I 
have never really used them, yet.


>> +/* output to either fp or buf; only one should be non-NULL */
>> +static void show_config_source(struct strbuf *buf, FILE *fp)
>> +{
>> +const char *fn = current_config_filename();
>> +if (!fn)
>> +return;
> 
> I'm not sure returning here is the best idea. We won't have a config
> filename if we are reading from "-c", but if we return early from this
> function, it parses differently than every other line. E.g., with your
> patch, if I do:
> 
>  git config -c foo.bar=true config --sources --list
> 
> I'll get:
> 
>  /home/peff/.gitconfig  user.name=Jeff King
>  /home/peff/.gitconfig  user.email=p...@peff.net
>  ...etc...
>  foo.bar=true
> 
> If somebody is parsing this as a tab-delimited list, then instead of the
> source field for that line being empty, it is missing (and it looks like
> "foo.bar=true" is the source file). I think it would be more friendly to
> consumers of the output to have a blank (i.e., set "fn" to the empty
> string and continue in the function).
I actually wondered about that exact point in your original patch but
"parsing" did not come to my mind. Now I understand your reasoning and I
agree.


> 
>> +
>> +char term = '\t';
> 
> This declaration comes after the "if" above, but git style doesn't allow
> declaration-after-statement (to support ancient compilers).
Interesting, I noticed the style and wondered about it! Should we add 
"-Werror=declaration-after-statement" to the TravisCI [1] build to catch these
kind of cases automatically?

After enabling this flag the compiler showed me that I did the same
error a few lines above in "const int is_query_action ...".

[1] https://travis-ci.org/larsxschneider/git/jobs/107610347


> 
>> +test_expect_success '--sources' '
>> +>.git/config &&
>> +>"$HOME"/.gitconfig &&
>> +INCLUDE_DIR="$HOME/include" &&
>> +mkdir -p "$INCLUDE_DIR" &&
>> +cat >"$INCLUDE_DIR"/include.conf <<-EOF &&
>> +[user]
>> +include = true
>> +EOF
>> +cat >"$HOME"/file.conf <<-EOF &&
>> +[user]
>> +custom = true
>> +EOF
>> +test_config_global user.global "true" &&
>> +test_config_global user.override "global" &&
>> +test_config_global include.path "$INCLUDE_DIR"/include.conf &&
> 
> Here you include the file by its absolute path. I wondered what would
> happen if we used a relative path. E.g.:
> 
>  git config include.path=foo
>  git config -f .git/foo included.config=true
>  git config --sources --list
> 
> which shows it as ".git/foo", because we resolved it by manipulating the
> relative path ".git/config". Whereas including it from ~/.gitconfig will
> show the absolute path, because we use the absolute path to get to
> ~/.gitconfig in the first place.
> 
> I think that's all sane. I don't know if it's worth noting it in the
> documentation or not.
I agree, this is the behavior I would expect and therefore I don't think
any additional documentation is necessary. The relative include is a good
idea! I added it to the test case.

> 
>> +cat >expect <<-EOF &&
>> +$HOME/.gitconfiguser.global=true
>> +$HOME/.gitconfiguser.override=global

Re: [PATCH v1] config: add '--sources' option to print the source of a config value

[Lars Schneider]

On 05 Feb 2016, at 12:22, Jeff King  wrote:

> On Fri, Feb 05, 2016 at 12:13:04PM +0100, Sebastian Schuberth wrote:
> 
>> On 2/5/2016 9:42, larsxschnei...@gmail.com wrote:
>> 
>>> Teach 'git config' the '--sources' option to print the source
>>> configuration file for every printed value.
>> 
>> Yay, not being able to see where a config setting originates from has
>> bothered me in the past, too. So thanks for working on this.
>> 
>> However, the naming of the '--sources' option sounds a bit misleading to me.
>> It has nothing to do with source code. So maybe better name it '--origin',
>> or even more verbose '--show-origin' or '--show-filename'?
> 
> I think he inherited the "--sources" name from me. :) I agree it could
> be better. I think "--show-filename" is not right as there are
> non-filename cases.  Just "--origin" sounds funny to me, perhaps because
> of git's normal use of the word "origin".
> 
> I like "--show-origin" the best of the ones suggested.

I understand your reasoning and I agree that "--show-origin" is better than
"--sources". However, I think just the word "origin" could be misleading in 
this context because people associate it with Git remotes. How about
"--show-config-origin" then? Or would that be too verbose?

Thanks,
Lars


--
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

[RFC] Identify where a Git config is defined

[Lars Schneider]

Hi,

Using "git config --list" shows me all configs but sometimes I have a hard time 
to figure out where a certain config is defined. This is especially true on 
Windows as I found the system config in various places. I wonder if other 
people would find it useful to enable something like "git config --list 
--print-source" where every config value is printed with the file where it 
originates from.

If I read the source correctly this would mean I would need to change 
"config_fn_t" to pass not only key and value but also the config source file in 
addition. Since "config_fn_t" is used in many places this would be a big change 
that probably is not worth the effort?!

Alternatively I was thinking about "git config --print-source-files" to print 
all config files that Git would parse. This would already help to find the 
configs and would probably be a smaller change.

Thoughts?

Thanks,
Lars--
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] convert: legitimately disable clean/smudge filter with an empty override

[Lars Schneider]

> On 29 Jan 2016, at 19:20, Junio C Hamano <gits...@pobox.com> wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> If the clean/smudge command of a Git filter driver (filter..smudge 
>> and
>> filter..clean) is set to an empty string ("") and the filter driver 
>> is
>> not required (filter..required=false) then Git will run successfully.
>> However, Git will print an error for every file that is affected by the 
>> filter.
>> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message if the filter is not required.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> convert.c |  2 +-
>> t/t0021-conversion.sh | 16 
>> 2 files changed, 17 insertions(+), 1 deletion(-)
>> 
>> diff --git a/convert.c b/convert.c
>> index 814e814..02d5f1e 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char 
>> *src, size_t len, int fd,
>>struct async async;
>>struct filter_params params;
>> 
>> -if (!cmd)
>> +if (!cmd || !*cmd)
>>return 0;
> 
> This is certainly simpler than v1.  I was initially worried about
> the fact that slightly changes the semantics around the "required"
> variable relative to v1, which said:
> 
>if (ca.drv && ca.drv->clean && *ca.drv->clean) {
>filter = ca.drv->clean;
>required = ca.drv->required;
>}
>ret |= apply_filter(path, src, len, -1, dst, filter);
>if (!ret && required)
>die;
> 
> but in v2, this part of the code is just as before, i.e.
> 
>if (ca.drv) {
>filter = ca.drv->clean;
>required = ca.drv->required;
>}
>ret |= apply_filter(path, src, len, -1, dst, filter);
>if (!ret && required)
>die;
> 
> So unlike v1, 'required' is set to true in the function, which is a
> good thing, but because in v2, apply_filter knows that an extrernal
> filter command that is an empty string is a no-op success, the above
> codepath behaves identically to v1 when observed from outside, i.e.
> "an empty string given as clean/smudge filter is a no-op success".
> 
> Good.
> 
> By the way, I find it somewhat annoying to see "legitimately" twice
> in the log message.  It makes it sound like there are legitimate way
> and not-so-kosher way to disable the filters.  Perhaps something
> like this instead?
> 
> -- >8 --
> convert: treat an empty string for clean/smudge filters as "cat"
> 
> Once a lower-priority configuration file defines a clean or smudge
> filter, there is no convenient way to override it.  Even though the
> configuration mechanism implements "the last one wins" semantics,
> you cannot set them to an empty string and expect them to work, as
> apply_filter() would try to run the empty string as an external
> command and fail.  The conversion is not done, but the function
> would still report a failure to convert.
> 
> Even though resetting the variable to "cat" (i.e. pass the data back
> as-is and report success) is an obvious and a viable way to solve
> this, it is wasteful to spawn an external process just as a
> workaround.
> 
> Instead, teach apply_filter() to treat an empty string given as a
> filter means the input must be returned as-is without conversion,
> and the operation must always succeed.
> -- >8 --

That reads perfect. I am sorry that I caused so much work for you with this 
patch. 

I really appreciate your editing as this helps me to improve my commit message 
writing skills!

Thanks,
Lars

> 
>> 
>>if (!dst)
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 718efa0..7bac2bc 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -252,4 +252,20 @@ test_expect_success "filter: smudge empty file" '
>>test_cmp expected filtered-empty-in-repo
>> '
>> 
>> +test_expect_success 'disable filter with empty override' '
>> +test_config_global filter.disable.smudge false &&
>> +test_config_global filter.disable.clean false &&
>> +test_config filter.disable.smudge false &&
>> +test_config filter.disable.clean false &&
>> +
>> +echo "*.disable filter=disable" >.gitattributes &&
>> +
>> +echo test >test.disable &&
>> +git -c filter.disable.clean= add test.disable 2>err &&
>> +test_must_be_empty err &&
>> +rm -f test.disable &&
>> +git -c filter.disable.smudge= checkout -- test.disable 2>err &&
>> +test_must_be_empty err
>> +'
>> +
>> test_done
--
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: Starting on a microproject for GSoC

[Lars Schneider]

On 28 Jan 2016, at 01:40, Moritz Neeb  wrote:

> As the list of available microprojects 2016 is still to be created, I
> might need your help in finding a project to work on.

As Stefan already pointed out, working on something that scratches your (Git) 
itch is probably the best way to find a project.
My recent itch was that I broke a test on Linux which I did not realize as I 
primarily work on OSX. As a solution for myself I suggested a TravisCI patch to 
the mailing list and it was accepted:
https://travis-ci.org/git/git/branches 

I see a number of ways to improve the Git TravisCI integration:

* install CVS on the build machines to run t94?? and t96?? tests
* install SVN on the build machines to run t91?? tests
* install Apache Web Server to run 5539, 5550, and 5561
* investigate if it is possible to run t1509 root worktree test
* investigate if it is possible to add jgit to run t5310
* investigate why GIT_TEST_LONG=YesPlease does not work on TravisCI
* investigate if we can use pylint to analyze the git-p4 Python code
* investigate if we can trigger Coverity static code analysis for the Git master
  branch (hint: Stefan Beller already looked into this)
  https://scan.coverity.com/travis_ci

I think all of these tasks can be done without deep Git knowledge. However, 
working with the tests is quite a good way to learn more about a complex 
project like Git.

Cheers,
Lars--
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: legitimately disable clean/smudge filter with an empty override

[Lars Schneider]

On 25 Jan 2016, at 02:25, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> A clean/smudge filter can be disabled if set to an empty string. However,
>> Git will try to run the empty string as command which results in a error
>> message per processed file.
> 
> The above two sentences do not make any sense to me.  You observe
> that the command refuses to work when the variable is set to an
> empty string--why then can you claim "filter can be disabled if set
> to an empty string"?  I'd consider that the system is not working
> with such a configuration, i.e. "filter cannot be disabled by
> setting it to empty; such a request will result in an error".

If I am not mistaken then Git exits with 0 (success) and an error message
if the filter command is empty and the filter is not required. If the filter
command is empty and the filter is required then Git will exit with 1 (error).

How about this?

If the clean/smudge command of a Git filter driver (filter..smudge and
filter..clean) is set to an empty string ("") and the filter driver is
not required (filter..required=false) then Git will run successfully.
However, Git will print an error for every file that is affected by the filter.

Teach Git to consider an empty clean/smudge filter as legitimately disabled
and do not print an error message if the filter is not required.

Thanks,
Lars


> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message.
> 
> On the other hand, this does make sense to me, as I do not think of
> a good way to say "earlier configuration entry said we should use
> this filter, but we actually do not want to use that one (or any)"
> because a configuration, unlike attributes entry, cannot be reset.
> The closest you can do is to set it to empty, so it may be a good
> new feature to do this.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jan 2016, #04; Wed, 20)

[Lars Schneider]

On 26 Jan 2016, at 23:58, Junio C Hamano <gits...@pobox.com> wrote:

> Lars Schneider <larsxschnei...@gmail.com> writes:
> 
>> Hi Junio,
>> 
>> Did you miss the topic "submodule: extend die message on failed
>> checkout with depth argument" or do you not agree with it
>> ($gmane/282779)? Stefan Beller reviewed the commits ($gmane/283666
>> and $gmane/283851).
> 
> No, these three are still in my inbox.
> 
> Quite honestly, "test to demonstrate breakage" alone is not all that
> interesting to me unless I get a firm impression that somebody is
> (or will soon be) working on fixing the breakage--which would make
> it worth keeping track of it in my tree, to allow the eventual fix
> to be more easily verified and protected from getting broken again.
> Also "these look good to me" without much explanation is not much of
> a review, and such a comment on a "demonstrate breakage" did not add
> to the sense of urgency to the issue, either.
> 
> That was why it has been sitting near the bottom of the pile.

OK, I get your point. I got the impression that Stefan is on to a fix 
($gmane/281260). Looks like he is not and I need to learn more about Git to 
tackle fixes of this scope. Would it be an option for you to just drop patch 
1/3 from this series and look at the remaining ones? 2/3 fixes an "&& chain 
issue" ($gmane/282776) and 3/3 prints a explanatory user warning instead of an 
incomprehensible error ($gmane/282777).

Thanks,
Lars--
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: legitimately disable clean/smudge filter with an empty override

[Lars Schneider]

On 24 Jan 2016, at 22:45, Jeff King <p...@peff.net> wrote:

> On Sun, Jan 24, 2016 at 01:22:50PM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> A clean/smudge filter can be disabled if set to an empty string. However,
>> Git will try to run the empty string as command which results in a error
>> message per processed file.
>> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message.
> 
> That makes sense to me, as I do not think the empty filter name can
> possibly do anything useful. You omitted the real motivation here, but I
> know what it is from past discussions: you want to be able to
> temporarily disable a filter with "git -c filter.foo.clean= ...". Which
> I think makes it more immediately obvious that this is a useful thing to
> have, and not just user error.
> 
>> diff --git a/convert.c b/convert.c
>> index 814e814..58af965 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, 
>> size_t len,
>>  struct conv_attrs ca;
>> 
>>  convert_attrs(, path);
>> -if (ca.drv) {
>> +if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
>>  filter = ca.drv->clean;
>>  required = ca.drv->required;
>>  }
>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char 
>> *path, const char *src,
>>  struct conv_attrs ca;
>> 
>>  convert_attrs(, path);
>> -if (ca.drv) {
>> +if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
>>  filter = ca.drv->smudge;
>>  required = ca.drv->required;
>>  }
> 
> This catches two calls, but I think there are others. What about
> would_convert_to_git_filter_fd and convert_to_git_filter_fd?
> 
> Would it make more sense for apply_filter() to treat the empty string as
> a noop, just as it does for NULL?

Yes :-)

Thanks,
Lars

> 
> I.e.:
> 
> 
> diff --git a/convert.c b/convert.c
> index 814e814..02d5f1e 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -395,7 +395,7 @@ static int apply_filter(const char *path, const char 
> *src, size_t len, int fd,
>   struct async async;
>   struct filter_params params;
> 
> - if (!cmd)
> + if (!cmd || !*cmd)
>   return 0;
> 
>   if (!dst)
> 
> which I think would cover all callers?
> 
> -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] convert: legitimately disable clean/smudge filter with an empty override

[Lars Schneider]

On 24 Jan 2016, at 22:35, Eric Sunshine <sunsh...@sunshineco.com> wrote:

> On Sun, Jan 24, 2016 at 10:06 AM, Torsten Bögershausen <tbo...@web.de> wrote:
>> On 24.01.16 13:22, larsxschnei...@gmail.com wrote:
>>> From: Lars Schneider <larsxschnei...@gmail.com>
>>> diff --git a/convert.c b/convert.c
>>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, 
>>> size_t len,
>>>  struct conv_attrs ca;
>>> 
>>>  convert_attrs(, path);
>>> - if (ca.drv) {
>>> + if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
> 
> More idiomatic:
> 
>if (ca.drv && ca.drv->clean && *ca.drv->clean) {
Agreed! Although I will go with Jeff's suggestion to implement that
logic in apply_filter.


> 
>>>  filter = ca.drv->clean;
>>>  required = ca.drv->required;
>>>  }
>>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char 
>>> *path, const char *src,
>>>  struct conv_attrs ca;
>>> 
>>>  convert_attrs(, path);
>>> - if (ca.drv) {
>>> + if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
> 
> Ditto.
> 
>>>  filter = ca.drv->smudge;
>>>  required = ca.drv->required;
>>>  }
>>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>>> @@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
>>> +test_expect_success 'disable filter with empty override' '
>>> + git config filter.disable.smudge false &&
>>> + git config filter.disable.clean false &&
> 
> test_config perhaps?

I was wondering about that, too. Especially because I could also use 
test_config_global.
Why does no test in t0021 use these functions? Should that be fixed?


> 
>>> + echo "*.disable filter=disable" >.gitattributes &&
>>> +
>>> + echo test >test.disable &&
>>> + git -c filter.disable.clean= add test.disable 2>err &&
>>> + ! test -s err &&
>> How about
>> test_cmp /dev/null err
>> to make debugging easier ?
> 
> Even better:
> 
>test_must_be_empty err &&

True! I will use that.

Thanks,
Lars
--
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: legitimately disable clean/smudge filter with an empty override

[Lars Schneider]

On 24 Jan 2016, at 16:06, Torsten Bögershausen <tbo...@web.de> wrote:

> On 24.01.16 13:22, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
> Some minor nits inside: 
>> 
>> A clean/smudge filter can be disabled if set to an empty string. 
> "set to an empty string" refers to "git config" (in opposite to the
> filter as such, which is specified in .gitattributes.
> Does it make sense to write 
> "git config filter.XXX.smudge ''" or so ?

I am not sure I get what you want here. Would this work for you?

The clean/smudge filter commands (filter.XYZ.smudge and filter.XYZ.clean)
can be disabled if set to an empty string. However, Git will still consider the 
empty string as command which results in a error message per processed 
file.


> 
>> However,
>> Git will try to run the empty string as command which results in a error
>> message per processed file.
>> 
>> Teach Git to consider an empty clean/smudge filter as legitimately disabled
>> and do not print an error message.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> convert.c |  4 ++--
>> t/t0021-conversion.sh | 14 ++
>> 2 files changed, 16 insertions(+), 2 deletions(-)
>> 
>> diff --git a/convert.c b/convert.c
>> index 814e814..58af965 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -786,7 +786,7 @@ int convert_to_git(const char *path, const char *src, 
>> size_t len,
>>  struct conv_attrs ca;
>> 
>>  convert_attrs(, path);
>> -if (ca.drv) {
>> +if (ca.drv && ca.drv->clean && strlen(ca.drv->clean)) {
>>  filter = ca.drv->clean;
>>  required = ca.drv->required;
>>  }
>> @@ -835,7 +835,7 @@ static int convert_to_working_tree_internal(const char 
>> *path, const char *src,
>>  struct conv_attrs ca;
>> 
>>  convert_attrs(, path);
>> -if (ca.drv) {
>> +if (ca.drv && ca.drv->smudge && strlen(ca.drv->smudge)) {
>>  filter = ca.drv->smudge;
>>  required = ca.drv->required;
>>  }
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 718efa0..56e385c 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -252,4 +252,18 @@ test_expect_success "filter: smudge empty file" '
>>  test_cmp expected filtered-empty-in-repo
>> '
>> 
>> +test_expect_success 'disable filter with empty override' '
>> +git config filter.disable.smudge false &&
>> +git config filter.disable.clean false &&
>> +
>> +echo "*.disable filter=disable" >.gitattributes &&
>> +
>> +echo test >test.disable &&
>> +git -c filter.disable.clean= add test.disable 2>err &&
>> +! test -s err &&
> How about 
> test_cmp /dev/null err
> to make debugging easier ?
Right. Although I would probably go with Eric's suggestion and use 
"test_must_be_empty".

That being said, I copied "! test -s" from the "filter large file" test in this 
file (added with 0b6806b).
How does the list handle these cases? Should I add another commit to replace "! 
test -s" there, 
too?

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jan 2016, #04; Wed, 20)

[Lars Schneider]

On 21 Jan 2016, at 00:33, Junio C Hamano  wrote:

> Here are the topics that have been cooking.  Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'.  The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.
> 
> The tip of 'master' now has second batch of topics merged, some of
> which should later be merged to 'maint'.  There are a few topics
> that are v2.7.0 regression fixes still cooking outside 'master',
> which also need to be merged to 'maint' for the maintenance release.
> 
> You can find the changes described here in the integration branches of the
> repositories listed at
> 
>http://git-blame.blogspot.com/p/git-public-repositories.html

Hi Junio,

Did you miss the topic "submodule: extend die message on failed checkout with 
depth argument" or do you not agree with it ($gmane/282779)? Stefan Beller 
reviewed the commits ($gmane/283666 and $gmane/283851).

Thanks,
Lars--
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 0/2] git-p4: kill watchdog and suppress irrelevant output

[Lars Schneider]

On 21 Dec 2015, at 21:31, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> Hi,
>> 
>> these patches extend "git-p4: add trap to kill p4d on test exit" (dfe90e8)
>> and therefore should be applied on master.
> 
> Wait, wait.  Please be a bit more careful when you use such a
> phrasing.  Did somebody review these and said that these are
> trivially correct improvements?
> 
> This depends on what you exactly meant "extend" and "improve" (for
> the other one); if the "improvement" were to make something that
> used to be unusable to usable, then the more sensible way forward
> during the -rc stabilization period might be to revert the earlier
> merges to 'master' that brought in undercooked topic.

Oh. Thanks or making me aware of the inappropriate phrases! 
Nobody reviewed these patches, yet! 

My intention was to let the reviewer know that this patch series is based on 
"master" and not on "maint". Sorry for the confusion!

The patch series "git-p4: kill watchdog and suppress irrelevant output" fixes
something annoying that I did not spot earlier. In other words it changes
"usable but a bit annoying (dfe90e8)" to "usable".

The patch series "git-p4: ignore P4 changelists that only touch files" changes 
"usable (4ae048e)" to "maybe even better" depending on the reviewers
opinion.

Thanks,
Lars

--
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 2/2] git-p4: suppress non test relevant output

[Lars Schneider]

On 21 Dec 2015, at 21:38, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> If tests are executed in verbose mode then the retry logic clutters the
>> test output. Suppress that clutter.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> t/lib-git-p4.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index 30bf7ae..03f29c1 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -174,7 +174,7 @@ retry_until_fail() {
>>  until ! "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
>>  do
>>  sleep 1
>> -done
>> +done >/dev/null 2>&1
> 
> Eh, what does this squelch?  The sleep in the body of the loop is
> silent, "test A -gt B" on the loop condition would be silent too, so
> you are squelching the invocation of "$@" whose standard error
> stream is already sent to 2>/dev/null?
> 
> If so, why not do it there instead?  You seem to run only "kill" to
> send some signal to a process using this helper function, and it
> would be silent on its standard output stream (even though it may
> say "no such process" etc. on its standard error), so it is not
> clear to me what you are doing with this change here...

If I run git-p4 tests in verbose mode (e.g. "./t9823-git-p4-mock-lfs.sh -v") 
without this patch then the last lines of the output look like this:

>>> Output Start >>>
expecting success:
kill_p4d

./lib-git-p4.sh: line 172: 26289 Killed: 9   while true; do
if test $(time_in_seconds) -gt $timeout; then
kill -9 $pid; exit 1;
fi; sleep 1;
done
ok 8 - kill p4d

# passed all 8 test(s)
1..8 
<<< Output end <<<

However, I want them to look like this:

>>> Output Start >>>
expecting success:
kill_p4d

ok 8 - kill p4d

# passed all 8 test(s)
1..8
<<< Output end <<<

This is achieved with the patch. I am no shell expert ... is there a nicer way 
to achieve the same?

Thanks,
Lars


--
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] git-p4: ignore P4 changelists that only touch files

[Lars Schneider]

On 20 Dec 2015, at 15:59, larsxschnei...@gmail.com wrote:

> From: Lars Schneider <larsxschnei...@gmail.com>
> 
> Hi,
> 
> this patch improves "git-p4: add option to keep empty commits" (4ae048e)
> and therefore should be applied on master.
> 
> Thanks,
> Lars

Junio just made me aware of the inappropriate phrasing here.
This is what I wanted to express:

This patch is based on master as it depends on "git-p4: add option to keep 
empty commits" (4ae048e)"


Sorry,
Lars--
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: Where does http.sslcainfo get set in Windows (2.6.3)?

[Lars Schneider]

Hi Titus,

try to look here:
C:\Users\All Users\Git\config

(that's where I found it... maybe different on your end).

Cheers,
Lars

> On 14 Dec 2015, at 16:45, Titus Barik  wrote:
> 
> Hi all,
> 
> I'm in Windows using git version: git version 2.6.3.windows.1. Git is
> installed to /c/Users/tbarik/AppData/Local/Programs/Git/cmd/git.
> 
> However, when I look for the config name http.sslcainfo, it returns:
> 
> $ git config --get-all http.sslcainfo
> C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt
> 
> Although I can override the name, I'm trying to figure out where this is
> being set, since the correct location should be (in this case)
> C:/Users/tbarik/AppData/Local/Programs/Git/mingw64/ssl/certs/ca-bundle.crt.
> 
> I don't see C:/Program Files/Git/mingw64/ssl/certs/ca-bundle.crt in
> either --global, --system, or --local, hence my confusion as to where
> this path is coming from.
> 
> Thanks,
> 
> Titus
> 
> -- 
> Titus Barik, PE 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] Case insensitive URL rewrite

[Lars Schneider]

Hi,

the "url..insteadOf" git config is case sensitive. I understand that 
this makes sense on case sensitive file systems. However, URLs are mostly case 
insensitive:

Consider this:
git clone https://GIThub.com/GIT/GIT
git clone https://github.com/git/git

Both commands will clone the same repository.

Interestingly enough this works, too:
git clone g...@github.com:GIT/GIT

What do you think about a flag that makes these rewrites case insensitive? E.g. 
with the following config flag:

[url ""]
insteadOf = 
ignorecase = true

Thanks,
Lars

--
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] git-p4: support multiple depot paths in p4 submit

[Lars Schneider]

On 07 Dec 2015, at 19:51, Sam Hocevar <s...@hocevar.net> wrote:

> On Sun, Dec 06, 2015, Lars Schneider wrote:
>> Thanks for the patch! Do you see a way to demonstrate the bug in a test case 
>> similar to t9821 [1]?
> 
>   Not yet, I'm afraid. It's proving trickier than expected because for
> now I can only reproduce the bug when the view uses multiples depots
> (solution #2 on http://answers.perforce.com/articles/KB/2437), and
> unfortunately the test case system in Git was designed for a single
> depot.
> 
>   Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to
> support multiple depots be acceptable and/or welcome? I prefer to ask
> before I dig into the task.

Can you outline your idea a bit? Are you aware of the following way to define 
client specs: [1] ? Would that help?
I haven't used multiple depots, yet. Therefore please bare with me :-)

Thanks,
Lars

[1] 
https://github.com/git/git/blob/362d2fc2f8ab9ee22072f76fb36ec16918511944/t/t9821-git-p4-path-variations.sh#L109-L111

--
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] git-p4: support multiple depot paths in p4 submit

[Lars Schneider]

Thanks for the patch! Do you see a way to demonstrate the bug in a test case 
similar to t9821 [1]?

Cheers,
Lars

[1] https://github.com/git/git/blob/master/t/t9821-git-p4-path-variations.sh

> On 05 Dec 2015, at 12:22, Sam Hocevar  wrote:
> 
> When submitting from a repository that was cloned using a client spec,
> use the full list of paths when ruling out files that are outside the
> view.  This fixes a bug where only files pertaining to the first path
> would be included in the p4 submit.
> 
> Signed-off-by: Sam Hocevar 
> ---
> git-p4.py | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index a79b6d8..210f100 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1253,6 +1253,8 @@ class P4Submit(Command, P4UserMap):
>Remove lines in the Files section that show changes to files
>outside the depot path we're committing into."""
> 
> +[upstream, settings] = findUpstreamBranchPoint()
> +
> template = ""
> inFilesSection = False
> for line in p4_read_pipe_lines(['change', '-o']):
> @@ -1265,8 +1267,13 @@ class P4Submit(Command, P4UserMap):
> lastTab = path.rfind("\t")
> if lastTab != -1:
> path = path[:lastTab]
> -if not p4PathStartsWith(path, self.depotPath):
> -continue
> +if settings.has_key('depot-paths'):
> +if not [p for p in settings['depot-paths']
> +if p4PathStartsWith(path, p)]:
> +continue
> +else:
> +if not p4PathStartsWith(path, self.depotPath):
> +continue
> else:
> inFilesSection = False
> else:
> -- 
> 2.6.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 v8] Add Travis CI support

[Lars Schneider]

On 28 Nov 2015, at 18:12, Jeff King <p...@peff.net> wrote:

> On Fri, Nov 27, 2015 at 10:23:26AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> diff to v7:
>> * remove NO_GETTEXT patch and install gettext on OS X to compile with
>>  no additional flags (thanks Torsten)
>> * fix P4 version (15.2 is the latest one available)
> 
> Thanks. I don't have any other comments on this one. I guess the next
> step is for me to get git/git signed up for Travis, and then merging
> this to 'master' will have the desired effect.

Thanks! You're right, signing up git/git for Travis should do it. Right now it 
is configured to build all branches on git/git that contain a travis.yml. That 
means if you merge it to pu or next then we should see builds already.

Cheers,
Lars--
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 2/2] add "ok=sigpipe" to test_must_fail and use it to fix flaky tests

[Lars Schneider]

On 28 Nov 2015, at 18:10, Jeff King <p...@peff.net> wrote:

> On Fri, Nov 27, 2015 at 10:15:14AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" is
>> flaky in the following case:
>> 1. remote upload-pack finds out "not our ref"
>> 2. remote sends a response and closes the pipe
>> 3. fetch-pack still tries to write commands to the remote upload-pack
>> 4. write call in wrapper.c dies with SIGPIPE
>> 
>> t5504 "9 - push with transfer.fsckobjects" is flaky, too, and returns
>> SIGPIPE once in a while. I had to remove the final "To dst..." output
>> check because there is no output if the process dies with SIGPUPE.
> 
> s/PUPE/PIPE/ :)
> 
> I think it would be nice for future readers to understand a bit better
> _why_ this is flaky, and why the fix is to the test suite and not to git
> itself. I added this paragraph in between the two above:
> 
>The test is flaky because the sending fetch-pack may or may not have
>finished writing its output by step (3). If it did, then we see a
>closed pipe on the next read() call. If it didn't, then we get the
>SIGPIPE from step (4) above. Both are fine, but the latter fools
>test_must_fail.
> 
Sounds good! Thank you :-)

- Lars

--
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] OS X El Capitan + Xcode ships without SSL header?!

[Lars Schneider]

On 29 Nov 2015, at 18:04, Torsten Bögershausen <tbo...@web.de> wrote:

> On 21/11/15 19:58, Lars Schneider wrote:
>> Hi,
>> 
>> I cannot build Git on a clean machine with OS X El Capitan 10.11, Xcode 
>> 7.1.1 and Xcode command line tools because of missing OpenSSL headers.
>> 
>> It looks like as there are no OpenSSL headers at all. I only found this 
>> weird non working version:
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift-migrator/sdk/MacOSX.sdk/usr/include/openssl/ssl.h
>> 
>> I installed OpenSSL with brew, added the include path and it works.
>> 
>> Can anyone confirm?
>> 
>> Thanks,
>> Lars
>> 
> (Does it make sense that you send a patch which auto-detects brew similar to 
> fink or mac ports?)
I think that would make sense. I'll look into it if I find some free cycles...

> 
> After some proper updating of one test machine I ran into the same problem.
> A possible patch may look like this:
> 
> 
> commit 5e7c16f3350e8e62bfdb181b0b5da7352945d046
> Author: Torsten Bögershausen <tbo...@web.de>
> Date:   Sun Nov 29 17:29:22 2015 +0100
> 
>Mac OS X 10.11: set NO_OPENSSL
> 
>There is no openssl/ directory any more in Mac OS X 10.11,
>openssl is depracated since Mac OS X 10.7
> 
>Set NO_OPENSSL to YesPlease as default under Mac OS X, and make it
>possible to override this and use openssl by defining DARWIN_OPENSSL
> 
> diff --git a/config.mak.uname b/config.mak.uname
> index f34dcaa..a8a8b07 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -105,6 +105,12 @@ ifeq ($(uname_S),Darwin)
>ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 
> && echo 1),1)
>HAVE_GETDELIM = YesPlease
>endif
> +   # MacOS 10.11  and higher
> +   ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 15 
> && echo 1),1)
> +   ifndef DARWIN_OPENSSL
> +   NO_OPENSSL = YesPlease
> +   endif
> +   endif

That would propably do it. However, what are the ramifications here? Does this 
affect `git clone` over HTTPS or even SSH? Sorry if this question sounds stupid 
but I am not too familiar with these systems. If there are indeed these severe 
ramifications then I would probably print a little tutorial how to install 
OpenSSL via brew.

Maybe one could use Apple's TLS implementation [1] on OS X alternatively. 
However, I don't know how different the APIs are and the resulting "ifdefs" are 
probably undesired.

Cheers,
Lars

[1] 
https://developer.apple.com/library/mac/documentation/Security/Reference/secureTransportRef/


 --
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 super slow on Windows 7

[Lars Schneider]

After some investigation I figured that ~50 Submodules are the culprit.
Does anyone have an idea how to speed up Git on Windows while keeping 50 
Submodules?

Thanks,
Lars


> On 25 Nov 2015, at 13:35, Lars Schneider <larsxschnei...@gmail.com> wrote:
> 
> Hi Johannes,
> 
> I am working with Git for Windows right now and it is dramatically slower 
> than on OS X.
> I executed "time git status" on Windows and OS X with the same repository and 
> the following results:
> 
> ## Windows git version 2.6.3.windows.1 (with enabled experimental flag on 
> install):
> real0m1.327s
> user0m0.000s
> sys 0m0.015s
> 
> 
> ## OS X git version 2.4.9 (Apple Git-60):
> git status  0.06s user 0.13s system 102% cpu 0.186 total
> 
> 
> Initially it was even slower on Windows (~1.6s). According to [1] I used the 
> following settings to make it faster:
> $ git config --global core.preloadindex true
> $ git config --global core.fscache true
> 
> Is this behavior normal/expected?
> If it is not normal, how would you debug the issue? How can I find out why it 
> is so slow?
> 
> My user drive is not on a net share and the machine has a SSD.
> 
> Thanks,
> Lars
> 
> 
> [1] 
> http://stackoverflow.com/questions/4485059/git-bash-is-extremely-slow-in-windows-7-x64

--
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 super slow on Windows 7

[Lars Schneider]

Hi Johannes,

I am working with Git for Windows right now and it is dramatically slower than 
on OS X.
I executed "time git status" on Windows and OS X with the same repository and 
the following results:

## Windows git version 2.6.3.windows.1 (with enabled experimental flag on 
install):
real0m1.327s
user0m0.000s
sys 0m0.015s


## OS X git version 2.4.9 (Apple Git-60):
git status  0.06s user 0.13s system 102% cpu 0.186 total


Initially it was even slower on Windows (~1.6s). According to [1] I used the 
following settings to make it faster:
$ git config --global core.preloadindex true
$ git config --global core.fscache true

Is this behavior normal/expected?
If it is not normal, how would you debug the issue? How can I find out why it 
is so slow?

My user drive is not on a net share and the machine has a SSD.

Thanks,
Lars


[1] 
http://stackoverflow.com/questions/4485059/git-bash-is-extremely-slow-in-windows-7-x64--
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/2] Add Travis CI support

[Lars Schneider]

> On 24 Nov 2015, at 21:40, Jeff King <p...@peff.net> wrote:
> 
> On Mon, Nov 23, 2015 at 09:25:08AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
>> 64 bit" and on "OS X Mavericks" using gcc and clang.
>> 
>> Perforce and Git-LFS are installed and therefore available for the
>> respective tests.
> 
> Thanks, I find this one a little easier to digest.
> 
> I'm iffy on the NO_GETTEXT change from patch 1. I had hoped we could
> just build out of the box everywhere, but I think the "do we have
> libintl" decision is a hard one. Most people _do_ have it and want it,
> but it sounds like the Travis environment does not. So maybe it is a
> place where it is worth doing the tweak inside travis.yml and leaving
> the stock build alone.

OK, I'll try to fix it in the next roll.

Thanks,
Lars
--
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

[RFC] OS X El Capitan + Xcode ships without SSL header?!

[Lars Schneider]

Hi,

I cannot build Git on a clean machine with OS X El Capitan 10.11, Xcode 7.1.1 
and Xcode command line tools because of missing OpenSSL headers.

It looks like as there are no OpenSSL headers at all. I only found this weird 
non working version:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift-migrator/sdk/MacOSX.sdk/usr/include/openssl/ssl.h

I installed OpenSSL with brew, added the include path and it works.

Can anyone confirm?

Thanks,
Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/6] Add Travis CI support

[Lars Schneider]

On 19 Nov 2015, at 15:35, Jeff King <p...@peff.net> wrote:

> On Thu, Nov 19, 2015 at 09:58:11AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
>> 64 bit" and on "OS X Mavericks" using gcc and clang.
>> 
>> Perforce and Git-LFS are installed and therefore available for the
>> respective tests.
> 
> My overall impression is that this is a lot more complicated than I was
> expecting. Can we start with something simpler to gain experience with
> Travis?  And then we can add in more complexity later.
> 
> For example:
> 
>> +addons:
>> +  apt:
>> +packages:
>> +- language-pack-is
> 
> I doubt most people are running the t0204 right now. Maybe we should
> start without it.

Well, I think the entire point of a CI is to run automatically tests that most 
people are *not* running. Plus I can't recall any problems with 
language-pack-is and t0204.

> 
>> +env:
>> +  global:
>> +- P4_VERSION="15.1"
>> +- GIT_LFS_VERSION="1.0.2"
> 
> I know p4 is your area of interest, but from a project perspective, it
> seems like an odd choice for the initial set of tests.

See above. Most people do not execute the p4 tests because they don't have p4 
installed. Therefore these tests have the highest risk to brake unnoticed.

> 
>> +- DEFAULT_TEST_TARGET=prove
>> +- GIT_PROVE_OPTS="--timer --jobs 3"
>> +- GIT_TEST_OPTS="--verbose --tee"
> 
> These all make sense, I guess.
> 
>> +- GETTEXT_ISO_LOCALE=YesPlease
>> +- GETTEXT_LOCALE=YesPlease
> 
> What are these? I don't think we have any such Makefile knobs. These
> look like variables that get used internally by the test suite, but we
> shouldn't need to set them.

These are used in t020*. I enabled them to run the tests and I haven't seen any 
problems. However, I am no expert in that area. I'll remove them.

> 
>> +# - GETTEXT_POISON=YesPlease
> 
> I'm assuming the "#" means this is commented out. Can we just drop such
> cruft?

Yes, for the final submission of course. I just wanted to make it stick out as 
I wasn't able to run the tests successfully with this flag. I wonder if someone 
can explain to me how it works.

> 
>> +- GIT_TEST_CHAIN_LINT=YesPlease
> 
> This is the default, and we don't need to specify it.

OK

> 
>> +- GIT_TEST_CLONE_2GB=YesPlease
> 
> Is it a good idea to run such an expensive test?

Well, it's a CI machine that does not block anyone. I doubt that people run 
these tests on their local machines for exactly the reason you just described. 
Therefore I would rather run these tests.

> 
>> +  matrix:
>> +-
>> +  # NO_ICONV=YesPlease
> 
> Ditto here on the commenting.

OK. Setting NO_ICONV brakes a lot of tests. Do we really "support" that flag?

> 
>> +- >
>> +  NO_CURL=YesPlease
> 
> So if I understand correctly, the point of this list is to test
> alternate configurations. So compiling without curl makes some sense, I
> guess. Though mostly it will just shut off a bunch off tests.
> 
>> +  NO_D_INO_IN_DIRENT=YesPlease
> 
> But setting platform compat knobs like this seems weird. Nobody sets
> this manually. Either your platform has it, or it does not. And we are
> already testing on alternate platforms to cover such things.
> 
> If there's a specific config setup of interest, it makes sense to me to
> try to increase test coverage there. But it feels like you've just
> picked a random laundry list of Makefile knobs and tweaked them, without
> any sense of whether they even make sense together, or match the
> platform.
> 
> For instance:
> 
>> +  NO_STRCASESTR=YesPlease
>> +  NO_STRLCPY=YesPlease
> 
> I wouldn't not be surprised if these cause problems building on some
> platforms that _do_ have these functions.

Agreed. As mentioned in gmane/280963 [1] I am no expert in these flags.  Should 
I remove the "NO_*" test configuration all together?

[1] http://article.gmane.org/gmane.comp.version-control.git/280963/match=no_

> 
>> +echo "$(tput setaf 6)Perfoce Server Version$(tput sgr0)";
>> +p4d -V | grep Rev.;
>> +echo "$(tput setaf 6)Perfoce Client Version$(tput sgr0)";
>> +p4 -V | grep Rev.;
> 
> s/Perfoce/Perforce/ :)

Thanks :)


>> +before_script: make configure && ./configure && make --jobs=2
> 
> Hmm. I wonder if we actually want to use autoconf here at all; most devs
> do not use it, and Git 

Re: [PATCH v6 0/6] Add Travis CI support

[Lars Schneider]

On 19 Nov 2015, at 15:14, Jeff King <p...@peff.net> wrote:

> On Thu, Nov 19, 2015 at 09:58:05AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> diff to v5:
>> * check if PID file still exists on P4D cleanup (thanks Luke)
>> * fix space/tab formatting error
>> * add sleep to timeout loops (thanks Luke)
>> * replace 'date +%s' with platform independent Python function (thanks Eric 
>> and Luke)
>> 
>> With the patches of this series the Travis CI test stability increases.
>> However, as I am "stress testing" the Travis CI infrastructure you can
>> see that it is not perfect: https://travis-ci.org/larsxschneider/git/builds
> 
> I peeked at a few, and it looks like just p4 tests failing now?

Yes, in particular t9810-git-p4-rcs.sh and t9816-git-p4-locked.sh. I would 
probably disable these test in Travis CI until I've found a way to make it 
stable.

> 
>> Nevertheless, I believe that Travis CI integration has still value as
>> contributors can test their patches easily on Linux and OS X before
>> posting them.
>> 
>> @junio / @peff: Do you consider merging this?
> 
> I think I'd prefer to split it into 3 separate topics (de-flaking
> test_must_fail, p4 test improvements, and the Travis file). Then they
> can proceed independently. I can take care of that split when applying.

Sounds good to me!

> 
>> Lars Schneider (6):
>>  implement test_might_fail using a refactored test_must_fail
> 
> You mentioned in the v5 cover that this one was from Junio. Should it be
> "From: Junio ..." in the pseudo-header?

Yes, this one was from Junio with a minor fix from my end if I recall 
correctly. What do you mean by "pseudo-header"? The "email-header" in the patch 
file? 

> 
>>  add "ok=sigpipe" to test_must_fail and use it to fix flaky tests
> 
> Looks OK.

"Looks OK" means I can/should add "Acked-by: Jeff King <p...@peff.net>" ? Bare 
with me, I am still learning ;-)

> 
>>  git-p4: retry kill/cleanup operations in tests with timeout
>>  git-p4: add p4d timeout in tests
>>  git-p4: add trap to kill p4d on test exit
> 
> These are all fairly gross, and I don't have p4d to test with myself.
> But if we assume they're all necessary, I suppose it's the best we can
> do.

Unfortunately I think they are necessary. However, if someone finds a better 
way for stable p4d tests then I would be happy to see them go away, again.

> 
>>  Add Travis CI support
> 
> I'll leave some comments directly in response to this one.
> 

Thanks for taking the time to review this!

- Lars

--
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

Three dot notion used inconsitent?

[Lars Schneider]

Hi,

I just stumbled across the this:

git diff branchA...branchB
--> gives me the diff between (the common ancestor of A and B) and B. That 
means I never see changes on branchA.

git log branchA...branchB
--> gives me the commits reachable from A and B. That includes changes from 
branchA.

Is this because of a design decision that I do not (yet) understand or is this 
inconsistent for historical reasons?

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

On 16 Nov 2015, at 22:14, Eric Sunshine <sunsh...@sunshineco.com> wrote:

> On Sun, Nov 15, 2015 at 8:08 AM,  <larsxschnei...@gmail.com> wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> @@ -121,22 +125,33 @@ p4_add_user() {
>>EOF
>> }
>> 
>> +retry_until_success() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
> 
> There was some discussion previously[1] about detecting dynamically
> whether 'date +%s' was supported. Was this something that you intended
> to do, or did you decide against it since p4 is not supported on such
> platforms?
> 
> Same question also applies to patch 4/6.

While implementing it I thought more about it. P4D is only supported on 
platforms that support the date function. That means these tests will only run 
on platforms that support the date function. Consequently I wondered if this 
would justify the slightly more complicated code. However, if you think this 
change would help the patch to get accepted then I will add it.

Thanks,
Lars


> 
> [1]: 
> http://article.gmane.org/gmane.comp.version-control.git/280978/match=lazy+prerequisite
> 
>> +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
>> +done
>> +}
>> +
>> +retry_until_fail() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
>> +done
>> +}

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/6] git-p4: add trap to kill p4d on test exit

[Lars Schneider]

On 16 Nov 2015, at 09:43, Luke Diamand <l...@diamand.org> wrote:

> On 15/11/15 13:08, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> Sometimes the "prove" test runner hangs on test exit because p4d is
>> still running. Add a trap to always kill "p4d" on test exit.
> 
> With this change, I've started seeing this when running the tests:
> 
> cat: /home/lgd/git/git/t/trash directory.t9819-git-p4-case-folding/p4d.pid: 
> No such file or directory
> 
> Probably just needs the obvious "test -f" adding.
> 
> Other than, all looks good to me. Particularly nice that I can now do:
> 
> $ make T=t98* -j10
> 
> and it actually works!

Great! I can see where the cat error comes from. I will add the "test -f" 
condition in the next roll.

Thanks,
Lars

> 
> 
> 
> 
> 
>> 
>> You can reproduce the problem by commenting "P4D_TIMEOUT" in
>> "lib-git-p4.sh" and running "prove ./t9800-git-p4-basic.sh".
>> ---
>>  t/lib-git-p4.sh | 6 ++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index f2a009c..f9c68d4 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -65,6 +65,12 @@ cli="$TRASH_DIRECTORY/cli"
>>  git="$TRASH_DIRECTORY/git"
>>  pidfile="$TRASH_DIRECTORY/p4d.pid"
>> 
>> +# Sometimes "prove" seems to hang on exit because p4d is still running
>> +cleanup() {
>> +kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
>> +}
>> +trap cleanup EXIT
>> +
>>  # git p4 submit generates a temp file, which will
>>  # not get cleaned up if the submission fails.  Don't
>>  # clutter up /tmp on the test machine.
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/6] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

On 16 Nov 2015, at 09:36, Luke Diamand <l...@diamand.org> wrote:

> On 15/11/15 13:08, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
> 
> Should there be a sleep in that retry_until_success loop so that it doesn't 
> spin sending signals to p4d?
Agreed. I will add a sleep in the next roll!


> 
> Do we need to worry about the time offset being updated (e.g. NTP) while this 
> is running?
Interesting question! I would consider this an edge case but I can see how it 
could happen.
Do you see a way to handle that in an easy way?

Thanks,
Lars

> 
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  t/lib-git-p4.sh | 31 +++
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>> 
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> index 7548225..8d6b48f 100644
>> --- a/t/lib-git-p4.sh
>> +++ b/t/lib-git-p4.sh
>> @@ -6,6 +6,10 @@
>>  # a subdirectory called "$git"
>>  TEST_NO_CREATE_REPO=NoThanks
>> 
>> +# Some operations require multiple attempts to be successful. Define
>> +# here the maximal retry timeout in seconds.
>> +RETRY_TIMEOUT=60
>> +
>>  . ./test-lib.sh
>> 
>>  if ! test_have_prereq PYTHON
>> @@ -121,22 +125,33 @@ p4_add_user() {
>>  EOF
>>  }
>> 
>> +retry_until_success() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
> 
> sleep here?
> 
>> +done
>> +}
>> +
>> +retry_until_fail() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
> 
> sleep here?
> 
>> +done
>> +}
>> +
>>  kill_p4d() {
>>  pid=$(cat "$pidfile")
>> -# it had better exist for the first kill
>> -kill $pid &&
>> -for i in 1 2 3 4 5 ; do
>> -kill $pid >/dev/null 2>&1 || break
>> -sleep 1
>> -done &&
>> +retry_until_fail kill $pid
>> +retry_until_fail kill -9 $pid
>>  # complain if it would not die
>>  test_must_fail kill $pid >/dev/null 2>&1 &&
>>  rm -rf "$db" "$cli" "$pidfile"
>>  }
>> 
>>  cleanup_git() {
>> -rm -rf "$git" &&
>> -mkdir "$git"
>> +retry_until_success rm -r "$git"
>> +test_must_fail test -d "$git" &&
>> +retry_until_success mkdir "$git"
>>  }
>> 
>>  marshal_dump() {

--
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] add test to demonstrate that shallow recursive clones fail

[Lars Schneider]

On 13 Nov 2015, at 00:34, Stefan Beller <sbel...@google.com> wrote:

> On Thu, Nov 12, 2015 at 1:37 AM,  <larsxschnei...@gmail.com> wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> "git clone --recursive --depth 1 --single-branch " clones the
>> submodules successfully. However, it does not obey "--depth 1" for
>> submodule cloning.
>> 
>> The following workaround does only work if the used submodule pointer
>> is on the default branch. Otherwise "git submodule update" fails with
>> "fatal: reference is not a tree:" and "Unable to checkout".
>> git clone --depth 1 --single-branch 
>> cd 
>> git submodule update --init --recursive --depth 1
>> 
>> The workaround does not fail using the "--remote" flag. However, in that
>> case the wrong commit is checked out.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
> 
> Thanks for writing these tests. :)
Thanks for looking into the issue :)


> 
>> +test_expect_failure shallow-clone-recursive-workaround '
>> +   URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
>> +   echo $URL &&
>> +   git clone --depth 1 --single-branch $URL clone-recursive-workaround 
>> &&
>> +   (
>> +   cd "clone-recursive-workaround" &&
>> +   git log --oneline >lines &&
>> +   test_line_count = 1 lines &&
>> +   git submodule update --init --recursive --depth 1
> 
> Should we prepend the lines with git submodule update with test_must_fail 
> here?
Wouldn't the test fail then? The test is expected to fail (see 
"test_expect_failure"). Am I missing something?


> 
>> +   )
>> +'
>> +
>> +test_expect_failure shallow-clone-recursive-with-remote-workaround '
>> +   URL="file://$(pwd | sed "s/[[:space:]]/%20/g")/repo" &&
>> +   echo $URL &&
>> +   git clone --depth 1 --single-branch $URL 
>> clone-recursive-remote-workaround &&
>> +   (
>> +   cd "clone-recursive-remote-workaround" &&
>> +   git log --oneline >lines &&
>> +   test_line_count = 1 lines &&
>> +   git submodule update --init --remote --recursive --depth 1 &&
>> +   git status submodule >status &&
>> +   test_must_fail grep "modified:" status
> 
> Use ! here instead of test_must_fail.
> 
> IIUC we use test_must_fail for git commands (to test that git does
> return a non null value instead of segfaulting).
> But on the other hand we trust grep to not segfault, so just negating
> its output is enough here.

OK! I will fix that in the next series!

Thanks,
Lars--
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] add test to demonstrate that shallow recursive clones fail

[Lars Schneider]

On 13 Nov 2015, at 06:35, Jeff King <p...@peff.net> wrote:

> On Thu, Nov 12, 2015 at 10:37:41AM +0100, larsxschnei...@gmail.com wrote:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> "git clone --recursive --depth 1 --single-branch " clones the
>> submodules successfully. However, it does not obey "--depth 1" for
>> submodule cloning.
>> 
>> The following workaround does only work if the used submodule pointer
>> is on the default branch. Otherwise "git submodule update" fails with
>> "fatal: reference is not a tree:" and "Unable to checkout".
>> git clone --depth 1 --single-branch 
>> cd 
>> git submodule update --init --recursive --depth 1
>> 
>> The workaround does not fail using the "--remote" flag. However, in that
>> case the wrong commit is checked out.
> 
> Hrm. Do we want to make these workarounds work correctly? Or is the
> final solution going to be that the first command you gave simply works,
> and no workarounds are needed.  If the latter, I wonder if we want to be
> adding tests for the workarounds in the first place.
> 
> I'm not clear on the expected endgame.

I see your point. I'll remove the workaround tests in the next roll. That being 
said, I think the we should do something about the workarounds, too, because it 
certainly confused me as Git user. Would you merge a patch that prints a 
warning message like "--depth parameter not supported for submodules update 
command" or something if a user tries this command?

Thanks,
Lars--
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] URL rewrite in .gitmodules

[Lars Schneider]

On 26 Oct 2015, at 17:52, Jens Lehmann <jens.lehm...@web.de> wrote:

> Am 26.10.2015 um 17:34 schrieb Stefan Beller:
>> On Sun, Oct 25, 2015 at 8:12 AM, Lars Schneider <larsxschnei...@gmail.com> 
>> wrote:
>>> On 20 Oct 2015, at 19:33, Junio C Hamano <gits...@pobox.com> wrote:
>>>> I do not think this topic is specific to use of submodules.  If you
>>>> want to encourage your engineers to fetch from nearby mirrors you
>>>> maintain, you would want a forest of url.mine.insteadof=theirs for
>>>> the external repositories that matter to you specified by
>>>> everybody's $HOME/.gitconfig, and one way to do so would be to have
>>>> them use the configuration inclusion.  An item in your engineer
>>>> orientation material could tell them to add
>>>> 
>>>>   [include]
>>>>   path = /usr/local/etc/git/mycompany.urlrewrite
>>>> 
>>>> when they set up their "[user] name/email" in there.
>>>> 
>>>> And you can update /usr/local/etc/git/mycompany.urlrewrite as
>>>> needed.
>>> Oh nice, I didn't know about "include". However, as mentioned to Stefan in 
>>> this thread, I fear that our engineers will miss that. I would prefer a 
>>> solution that does not need any additional setup. Therefore the suggestion 
>>> to add rewrites in the .gitmodules file.
>> 
>> How do you distribute new copies of Git to your engineers?
>> Maybe you could ship them a version which has the "include" line
>> already builtin as default? So your distributed copy of Git
>> would not just check the default places for configs, but also
>> some complied in /net/share/mycompany.gitconfig
> 
> Which is just what we do at $DAYJOB, that way you can easily
> distribute all kinds of settings, customizations and hooks
> company-wide.

That's a very good idea. I will try to establish this practice, too.

Thanks,
Lars--
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] http: fix some printf format warnings on 32-bit builds

[Lars Schneider]

On 13 Nov 2015, at 21:02, Ramsay Jones <ram...@ramsayjones.plus.com> wrote:

> 
> 
> On 13/11/15 08:57, Eric Sunshine wrote:
>> On Fri, Nov 13, 2015 at 3:46 AM, Lars Schneider
>> <larsxschnei...@gmail.com> wrote:
>>> On 11 Nov 2015, at 18:49, Ramsay Jones <ram...@ramsayjones.plus.com> wrote:
>>>> On 11/11/15 02:00, Stefan Beller wrote:
>>>>> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine <sunsh...@sunshineco.com> 
>>>>> wrote:
>>>>>> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>>>>>> <ram...@ramsayjones.plus.com> wrote:
>>>>>>> Commit f8117f55 ("http: use off_t to store partial file size",
>>>>>>> 02-11-2015) changed the type of some variables from long to off_t.
>>>>>>> The 32-bit build, which enables the large filesystem interface
>>>>>>> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
>>>>>>> integer, whereas long is a 32-bit integer. This results in a couple
>>>>>>> of printf format warnings.
>>>>>> 
>>>>>> My machine is 64-bit, though, so perhaps it's misleading to
>>>>>> characterize this as a fix for 32-bit builds. In particular, off_t is
>>>>>> 'long long' on this machine, so it complains about the "long" format
>>>>>> specifier.
>>>>> 
>>>>> I wonder if 32 bit compilation can be part of travis.
>>>> 
>>>> Did this warning show up on the OS X build?
>>> 
>>> Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI
>>> build and it breaks the build on OS X.
>>> See here (you need to scroll all the way down):
>>> https://travis-ci.org/larsxschneider/git/jobs/90899656
>>> 
>>> BTW: I tried to set "-Werror" but then I got a bunch of macro redefined 
>>> errors like this:
>>> ./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]
>>> 
>>> Is this a known issue? Is this an issue at all?
>> 
>> Odd. I don't experience anything like that on my Mac.
>> 
> 
> Hmm, from the output, it looks like the configure script is
> not detecting that 'strlcpy' is available (so setting
> NO_STRLCPY=YesPlease in the config.mak.autogen file).
> However, it seems to be a 'macro redirect' set in the
> /usr/include/secure/_string.h header file (presumably it
> redirects between a more or less secure version ;-)
> 
> Unfortunately, I don't have access to a mac - so I can't
> help you with the debugging. :(
> 

I don't have any experience with autotools at all. However, here is what I 
found out on OS X Mavericks (10.9.5):
1.) In config.mak.uname, line 103, "NO_STRLCPY = YesPlease" is set for some old 
OS X version. However, it looks like these settings have no impact at all.
2.) In configure.ac, line 1010, the AC_CHECK_FUNC macros (via GIT_CHECK_FUNC) 
is used to detect strlcpy. That detection fails on OS X Mavericks.
3.) I tried to use "AC_CHECK_DECLS" to detect strlcpy declarations on OS X and 
this works.

Can you give me a few hints how to debug this further? Why do have the values 
in config.mak.uname no impact? My idea was to detect OS X Mavericks there and 
unset NO_STRLCPY. Could that work?

Thanks,
Lars 

--
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] http: fix some printf format warnings on 32-bit builds

[Lars Schneider]

> On 13 Nov 2015, at 11:32, Torsten Bögershausen <tbo...@web.de> wrote:
> 
> On 2015-11-13 09.57, Eric Sunshine wrote:
>> On Fri, Nov 13, 2015 at 3:46 AM, Lars Schneider
>> <larsxschnei...@gmail.com> wrote:
>>> On 11 Nov 2015, at 18:49, Ramsay Jones <ram...@ramsayjones.plus.com> wrote:
>>>> On 11/11/15 02:00, Stefan Beller wrote:
>>>>> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine <sunsh...@sunshineco.com> 
>>>>> wrote:
>>>>>> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>>>>>> <ram...@ramsayjones.plus.com> wrote:
>>>>>>> Commit f8117f55 ("http: use off_t to store partial file size",
>>>>>>> 02-11-2015) changed the type of some variables from long to off_t.
>>>>>>> The 32-bit build, which enables the large filesystem interface
>>>>>>> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
>>>>>>> integer, whereas long is a 32-bit integer. This results in a couple
>>>>>>> of printf format warnings.
>>>>>> 
>>>>>> My machine is 64-bit, though, so perhaps it's misleading to
>>>>>> characterize this as a fix for 32-bit builds. In particular, off_t is
>>>>>> 'long long' on this machine, so it complains about the "long" format
>>>>>> specifier.
>>>>> 
>>>>> I wonder if 32 bit compilation can be part of travis.
>>>> 
>>>> Did this warning show up on the OS X build?
>>> 
>>> Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI
>>> build and it breaks the build on OS X.
>>> See here (you need to scroll all the way down):
>>> https://travis-ci.org/larsxschneider/git/jobs/90899656
>>> 
>>> BTW: I tried to set "-Werror" but then I got a bunch of macro redefined 
>>> errors like this:
>>> ./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]
>>> 
>>> Is this a known issue? Is this an issue at all?
>> 
>> Odd. I don't experience anything like that on my Mac.
> 
> Could it be, that strlcpy is present on your system ?
> And where does it come from ?
> 
> Which OS ?
> Which compiler ?
> What does `uname -r` say ?
> Do you have Macports, Fink, Brew... installed ?
> 

Looks like this is a OS X only issue. Happens with clang and gcc on the OS X 
Mavericks TravisCI machines [1]:
https://travis-ci.org/larsxschneider/git/jobs/90919078
https://travis-ci.org/larsxschneider/git/jobs/90919080

On Linux+gcc the following error happens if "-Werror" is present:
https://travis-ci.org/larsxschneider/git/jobs/90919076
Do you have an idea what that might be?

Linux+clang works fine:
https://travis-ci.org/larsxschneider/git/jobs/90919074

- Lars

[1] http://docs.travis-ci.com/user/ci-environment/


--
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] http: fix some printf format warnings on 32-bit builds

[Lars Schneider]

On 11 Nov 2015, at 18:49, Ramsay Jones  wrote:

> 
> 
> On 11/11/15 02:00, Stefan Beller wrote:
>> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
>> wrote:
>>> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>>>  wrote:
 Commit f8117f55 ("http: use off_t to store partial file size",
 02-11-2015) changed the type of some variables from long to off_t.
 The 32-bit build, which enables the large filesystem interface
 (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
 integer, whereas long is a 32-bit integer. This results in a couple
 of printf format warnings.
 
 In order to suppress the warnings, change the format specifier to use
 the PRIuMAX macro and cast the off_t argument to uintmax_t. (See also
 the http_opt_request_remainder() function, which uses the same
 solution).
>>> 
>>> I just ran across the problem when building 'next' on my Mac and was
>>> about to investigate, so am happy to find that the work has already
>>> been done. Thanks.
>>> 
>>> My machine is 64-bit, though, so perhaps it's misleading to
>>> characterize this as a fix for 32-bit builds. In particular, off_t is
>>> 'long long' on this machine, so it complains about the "long" format
>>> specifier.
>> 
>> +Lars
>> 
>> I wonder if 32 bit compilation can be part of travis.
>> 
> 
> Did this warning show up on the OS X build?

Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI build and 
it breaks the build on OS X.
See here (you need to scroll all the way down):
https://travis-ci.org/larsxschneider/git/jobs/90899656

BTW: I tried to set "-Werror" but then I got a bunch of macro redefined errors 
like this:
./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]

Is this a known issue? Is this an issue at all?

Thanks,
Lars--
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] Clone repositories recursive with depth 1

[Lars Schneider]

On 11 Nov 2015, at 21:09, Stefan Beller <sbel...@google.com> wrote:

> On Wed, Nov 11, 2015 at 11:19 AM, Stefan Beller <sbel...@google.com> wrote:
>> On Wed, Nov 11, 2015 at 6:09 AM, Lars Schneider
>> <larsxschnei...@gmail.com> wrote:
>>> Hi,
>>> 
>>> I have a clean build machine and I want to clone my source code to this 
>>> machine while transferring only the minimal necessary amount of data. 
>>> Therefore I use this command:
>>> 
>>> git clone --recursive --depth 1 --single-branch 
>> 
>> That *should* work, actually.
>> However looking at the code it does not.
>> 
>> citing from builtin/clone.c:
>> 
>>static struct option builtin_clone_options[] = {
>>...
>>OPT_BOOL(0, "recursive", _recursive,
>>   N_("initialize submodules in the clone")),
>>OPT_BOOL(0, "recurse-submodules", _recursive,
>>  N_("initialize submodules in the clone")),
>>...
>>};
>>...
>>static const char *argv_submodule[] = {
>>"submodule", "update", "--init", "--recursive", NULL
>>};
>> 
>>if (!err && option_recursive)
>>err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
>> 
>> So the --depth argument is not passed on, although "git submodule update"
>> definitely supports --depth.
>> 
>> In an upcoming series (next version of origin/sb/submodule-parallel-update),
>> this will slightly change, such it will be even easier to add the
>> depth argument in
>> there as we construct the argument list in code instead of hard coding
>> argv_submodule.
>> 
>> This may require some discussion whether you expect --depth to be recursed.
>> (What if you only want a top level shallow thing?, What if you want to have 
>> only
>> submodules shallow? What is the user expectation here?)
>> 
>>> 
>>> Apparently this does not clone the submodules with "--depth 1" (using Git 
>>> 2.4.9). As a workaround I tried:
>>> 
>>> git clone --depth 1 --single-branch 
>>> cd 
>>> git submodule update --init --recursive --depth 1
>>> 
> 
> The workaround works with the origin/master version for me.
> 
> Notice the other email thread, which suggests to include --remote into the
> call to  git submodule update depending on a branch config option being
> present in the .gitmodules file.

Can you check "[PATCH v2] add test to demonstrate that shallow recursive clones 
fail"? This demonstrates the failure that I see. I also tried the "--remote" 
flag but this does not work either (see test case).
Can you confirm this behavior?

Cheers,
Lars--
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

[RFC] Clone repositories recursive with depth 1

[Lars Schneider]

Hi,

I have a clean build machine and I want to clone my source code to this machine 
while transferring only the minimal necessary amount of data. Therefore I use 
this command:

git clone --recursive --depth 1 --single-branch 

Apparently this does not clone the submodules with "--depth 1" (using Git 
2.4.9). As a workaround I tried:

git clone --depth 1 --single-branch 
cd 
git submodule update --init --recursive --depth 1

However, this does not work either as I get:
fatal: reference is not a tree: 
Unable to checkout  in submodule path 

How would you clone the repo? Is the behavior above expected? If not, should 
the "--depth 1" flag be applied recursively to all submodules on a clone 
--recursive? Has a patch implementing this a chance to get in?

Thanks,
Lars--
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] http: fix some printf format warnings on 32-bit builds

[Lars Schneider]

On 11 Nov 2015, at 03:00, Stefan Beller  wrote:

> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
> wrote:
>> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>>  wrote:
>>> Commit f8117f55 ("http: use off_t to store partial file size",
>>> 02-11-2015) changed the type of some variables from long to off_t.
>>> The 32-bit build, which enables the large filesystem interface
>>> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
>>> integer, whereas long is a 32-bit integer. This results in a couple
>>> of printf format warnings.
>>> 
>>> In order to suppress the warnings, change the format specifier to use
>>> the PRIuMAX macro and cast the off_t argument to uintmax_t. (See also
>>> the http_opt_request_remainder() function, which uses the same
>>> solution).
>> 
>> I just ran across the problem when building 'next' on my Mac and was
>> about to investigate, so am happy to find that the work has already
>> been done. Thanks.
>> 
>> My machine is 64-bit, though, so perhaps it's misleading to
>> characterize this as a fix for 32-bit builds. In particular, off_t is
>> 'long long' on this machine, so it complains about the "long" format
>> specifier.
> 
> +Lars
> 
> I wonder if 32 bit compilation can be part of travis.

Unfortunately no. All their machines are 64-bit [1] and they have "no immediate 
plans to add this feature" [2].
However, we could the following build configuration on a 64-bit machine:

export CFLAGS="-m32"
export LDFLAGS="-m32"

I think then we could catch these kind of warnings. Do you see a downside with 
this approach?

- Lars

[1] http://docs.travis-ci.com/user/ci-environment/
[2] https://github.com/travis-ci/travis-ci/issues/986--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

> On 06 Nov 2015, at 10:28, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschnei...@gmail.com> wrote:
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> +retry_until_success() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
>> +done
>> +}
>> +
>> +retry_until_fail() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
>> +done
>> +}
> 
> I'm confused by this. Patch 2/4 was already calling
> retry_until_fail(), but it's introduction seems to be here in patch
> 3/4. Am I missing something obvious?

No, my fault. I reordered the commits and forgot about this. I will fix the 
order in the next roll.

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/4] git-p4: add p4d timeout in tests

[Lars Schneider]

> On 06 Nov 2015, at 10:23, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschnei...@gmail.com> wrote:
>> In rare cases p4d seems to hang. This watchdog will kill the p4d
>> process after 300s in any case. That means each individual git p4 test
>> needs to finish before 300s or it will fail.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> @@ -81,6 +85,19 @@ start_p4d() {
>># will be caught with the "kill -0" check below.
>>i=${P4D_START_PATIENCE:-300}
>>pid=$(cat "$pidfile")
>> +
>> +   timeout=$(($(date +%s) + $P4D_TIMEOUT))
>> +while true
> 
> The 'while' line is incorrectly indented with spaces rather than tabs.'

Oh, you're right. One more thing that I will add to my pre mailing list submit 
check script :-)

> 
>> +   do
>> +   if test $(date +%s) -gt $timeout
> 
> I don't know how portable you intend this to be, but 'date +%s' is not
> universally supported (it's missing on Solaris, for instance, and
> perhaps AIX). For 6a9d16a (filter-branch: add passed/remaining seconds
> on progress, 2015-09-07), we ultimately settled upon detecting +%s
> support dynamically:
> 
>if date '+%s' 2>/dev/null | grep -q '^[0-9][0-9]*$'
># it's supported
>fi
> 
> Perhaps you'd want to detect this via a lazy prerequisite and skip
> this if not supported?

AFAIK Perforce does not run on Solaris and AIX anyways (see supported platforms 
[1]). Therefore these tests should not be executed on these platforms. A lazy 
prerequisite sounds like a good idea to make this explicit!

Thanks,
Lars

[1] https://www.perforce.com/perforce/doc.current/user/relnotes.txt

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 14:57, Sebastian Schuberth <sschube...@gmail.com> wrote:
> 
> On Fri, Nov 6, 2015 at 2:55 PM, Lars Schneider <larsxschnei...@gmail.com> 
> wrote:
> 
>>> I think running different configuration per branch makes sense, yes.
>> 
>> If the list decides to accept this patch. Do you think that would be a 
>> necessary requirement for the first iteration?
> 
> No. I think this could be addressed later as an improvements. To me
> it's more important to finally get *some* sane Travis CI configuration
> in.
True. However, as I stated in my v4 cover letter the Travis CI integration is 
not yet perfect. I am constantly running builds to find flaky tests. Eg. here 
is one of them in git-p4 area that I will tackle next:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/89603763/log.txt

I also see a weird "prove Tests our of sequence" error one in a while:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/89603770/log.txt

Does anyone have an idea what could cause this?

Thanks,
Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 14:20, Sebastian Schuberth <sschube...@gmail.com> wrote:
> 
> On Fri, Nov 6, 2015 at 2:11 PM, Lars Schneider <larsxschnei...@gmail.com> 
> wrote:
> 
>> Per platform/compiler (Linux/clang) we run two configurations. One
>> normal configuration (see the lonely "-" right under "matrix:") and one
>> configuration with all sorts of things are disabled ("NO...").
>> 
>> You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal,
>> NO_...]) here:
>> https://travis-ci.org/larsxschneider/git/builds/89598194
> 
> Aren't these 8 configurations a bit too much? I see the total running
> time is about 2 hours. For my taste, this is way to much to give
> developers feedback about the status of their PR. It should be
> something < 30 minutes.
> 
> IMO, the purpose of the Travis CI configuration mainly is to 1) save
> developers work by not requiring to build manually, 2) build on other
> platforms than the developer has access to. I doubt that the average
> developer manually builds anything close to these 8 configurations
> before we had this job, so I wouldn't make it a requirement for Travis
> to do much more than a developer could / would to manually.
> 
> On the other hand, I see the point in letting a CI system test more
> configurations than manually feasible. But that should be done as part
> of a different job then. E.g. we could have a "fast" PR validation
> job, and a "slow" nightly build job.
> 

Well, I partly agree. Right now the running time is ~20 min (that means less 
than your 30min target!). After ~10min you even have all Linux results, Mac 
takes a bit longer. Travis shows you 2h because that is the time that would be 
required if all builds where run one after another (we run builds in parallel).

That being said, I see your point of to avoiding to burn Travis CI resources 
meaningless. If I am not mistaken then you can configure Travis in a way that 
it runs different configurations for different branches. E.g. I would like to 
run all 8 configurations on maint, master, next and maybe pu. All other 
branches on peoples own forks should be fine with the default Linux build 
(~10min).

What do you think?

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 14:36, Sebastian Schuberth <sschube...@gmail.com> wrote:
> 
> On Fri, Nov 6, 2015 at 2:28 PM, Lars Schneider <larsxschnei...@gmail.com> 
> wrote:
> 
>> Well, I partly agree. Right now the running time is ~20 min (that means less 
>> than your 30min target!). After ~10min you even have all Linux results, Mac 
>> takes a bit longer. Travis shows you 2h because that is the time that would 
>> be required if all builds where run one after another (we run builds in 
>> parallel).
> 
> Are you sure about than? I mean, what sense does it make to show how
> long it *would* have taken *if* the builds were running serially? I
> can see that the longest of the jobs took 21 minutes, which is ok. But
> that does not mean that all jobs completed in within 21 minutes. It
> could be that not all jobs started at (about) the same time due to a
> lack of resources, and that the last job did not compete before the 2
> hours were over because it only started to run 1 hours and 40 minutes
> befor ethe first job was started.
I am fairly certain about this. 

See, here is the first configuration and the first test case of a job:
https://travis-ci.org/larsxschneider/git/jobs/89598195
[08:21:06] t0002-gitfile.sh 

Here is the last configuration and the last test case of the same job:
[08:51:22] t9903-bash-prompt.sh

~30 min for all 8 configurations. You can enable Travis CI for you GitHub 
account and try it easily yourself :-)

> 
>> That being said, I see your point of to avoiding to burn Travis CI resources 
>> meaningless. If I am not mistaken then you can configure Travis in a way 
>> that it runs different configurations for different branches. E.g. I would 
>> like to run all 8 configurations on maint, master, next and maybe pu. All 
>> other branches on peoples own forks should be fine with the default Linux 
>> build (~10min).
>> 
>> What do you think?
> 
> I think running different configuration per branch makes sense, yes.
If the list decides to accept this patch. Do you think that would be a 
necessary requirement for the first iteration?

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] git-p4: retry kill/cleanup operations in tests with timeout

[Lars Schneider]

> On 06 Nov 2015, at 10:28, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschnei...@gmail.com> wrote:
>> In rare cases kill/cleanup operations in tests fail. Retry these
>> operations with a timeout to make the test less flaky.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
>> +retry_until_success() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
>> +done
>> +}
>> +
>> +retry_until_fail() {
>> +timeout=$(($(date +%s) + $RETRY_TIMEOUT))
>> +until ! "$@" 2>/dev/null || test $(date +%s) -gt $timeout
>> +do :
>> +done
>> +}
> 
> I'm confused by this. Patch 2/4 was already calling
> retry_until_fail(), but it's introduction seems to be here in patch
> 3/4. Am I missing something obvious?
No, my fault. I reordered the commits and forgot about this. I will fix the 
order in the next roll.

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/4] Add Travis CI support

[Lars Schneider]

> On 06 Nov 2015, at 10:56, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> 
> On Fri, Nov 6, 2015 at 3:58 AM,  <larsxschnei...@gmail.com> wrote:
>> The tests are currently executed on "Ubuntu 12.04 LTS Server Edition
>> 64 bit" and on "OS X Mavericks" using gcc and clang.
>> 
>> Perforce and Git-LFS are installed and therefore available for the
>> respective tests.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> diff --git a/.travis.yml b/.travis.yml
>> @@ -0,0 +1,131 @@
>> +  matrix:
>> +-
>> +  # NO_ICONV=YesPlease
>> +- >
>> +  NO_CURL=YesPlease
>> +  NO_D_INO_IN_DIRENT=YesPlease
>> +  NO_DEFLATE_BOUND=YesPlease
>> +  NO_EXPAT=YesPlease
>> +  NO_GECOS_IN_PWENT=YesPlease
>> +  NO_GETTEXT=YesPlease
>> +  NO_HMAC_CTX_CLEANUP=YesPlease
>> +  NO_HSTRERROR=YesPlease
>> +  NO_INET_NTOP=YesPlease
>> +  NO_INET_PTON=YesPlease
>> +  NO_INITGROUPS=YesPlease
>> +  NO_INTTYPES_H=YesPlease
>> +  NO_IPV6=YesPlease
>> +  NO_IPV6=YesPlease
>> +  NO_LIBGEN_H=YesPlease
>> +  NO_MEMMEM=YesPlease
>> +  NO_MKDTEMP=YesPlease
>> +  NO_MKSTEMPS=YesPlease
>> +  NO_MMAP=YesPlease
>> +  NO_NSEC=YesPlease
>> +  NO_OPENSSL=YesPlease
>> +  NO_PERL=YesPlease
>> +  NO_PTHREADS=YesPlease
>> +  NO_REGEX=YesPlease
>> +  NO_SETENV=YesPlease
>> +  NO_SETITIMER=YesPlease
>> +  NO_SOCKADDR_STORAGE=YesPlease
>> +  NO_STRCASESTR=YesPlease
>> +  NO_STRLCPY=YesPlease
>> +  NO_STRTOUMAX=YesPlease
>> +  NO_STRUCT_ITIMERVAL=YesPlease
>> +  NO_SYMLINK_HEAD=YesPlease
>> +  NO_SYS_POLL_H=YesPlease
>> +  NO_SYS_SELECT_H=YesPlease
>> +  NO_UINTMAX_T=YesPlease
>> +  NO_UNSETENV=YesPlease
> 
> Hmm, aren't you losing test coverage by disabling some of these? Is
> that desirable for continuous integration testing? Am I missing
> something?
Per platform/compiler (Linux/clang) we run two configurations. One 
normal configuration (see the lonely "-" right under "matrix:") and one 
configuration with all sorts of things are disabled ("NO...").

You can see all 8 configurations ([linux, mac] * [clang, gcc] * [normal, 
NO_...]) here:
https://travis-ci.org/larsxschneider/git/builds/89598194

Cheers,
Lars 

--
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] test: accept death by SIGPIPE as a valid failure mode

[Lars Schneider]

> On 05 Nov 2015, at 08:47, Jeff King  wrote:
> 
> On Fri, Oct 30, 2015 at 02:22:14PM -0700, Junio C Hamano wrote:
> 
>> On a local host, the object/history transport code often talks over
>> pipe with the other side.  The other side may notice some (expected)
>> failure, send the error message either to our process or to the
>> standard error and hung up.  In such codepaths, if timing were not
>> unfortunate, our side would receive the report of (expected) failure
>> from the other side over the pipe and die().  Otherwise, our side
>> may still be trying to talk to it and would die with a SIGPIPE.
>> 
>> This was observed as an intermittent breakage in t5516 by a few
>> people.
>> 
>> In the real-life scenario, either mode of death exits with a
>> non-zero status, and the user would learn that the command failed.
>> The test_must_fail helper should also know that dying with SIGPIPE
>> is one of the valid failure modes when we are expecting the tested
>> operation to notice problem and fail.
> 
> Sorry for the slow review; before commenting I wanted to dig into
> whether this SIGPIPE ambiguity was avoidable in the first place.
> 
> I think the answer is "probably not". We do call write_or_die() pretty
> consistently in the network-aware programs. So we could ignore SIGPIPE,
> and then we would catch EPIPE (of course, we convert that into SIGPIPE
> in many places, but we do not have to do so). But since the SIGPIPE
> behavior is global, that carries the risk of us failing to check a write
> against some other descriptor. It's probably not worth it.
> 
> Teaching the tests to handle both cases seems like a reasonable
> workaround. Changing test_must_fail covers a lot of cases; I wondered if
> there are other tests that would not want to silently cover up a SIGPIPE
> death. But I could not really think of a plausible reason.
> 
> So I think your patch is the best thing to do.
> 
> -Peff

Oh, I missed this email thread. I am still working on a stable Travis-CI 
integration and I ran into this issue a few times. I fixed it in my (not yet 
published) patch with an additional function "test_must_fail_or_sigpipe" that 
I've used for all tests affected by this issue. Modifying the "test_must_fail" 
function seemed too risky for me as I don't understand all possible 
implications. However, if you don't see a problem then this is fine with me.

- Lars--
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] git-p4: Add option to ignore empty commits

[Lars Schneider]

On 26 Oct 2015, at 21:40, Luke Diamand <l...@diamand.org> wrote:

> On 24/10/15 19:08, Lars Schneider wrote:
>> 
>> On 21 Oct 2015, at 08:32, Luke Diamand <l...@diamand.org> wrote:
>> 
>>> On 19/10/15 19:43, larsxschnei...@gmail.com wrote:
>>>> From: Lars Schneider <larsxschnei...@gmail.com>
>>>> 
-- snip --
>> 
>>> Also, could you use python3 style print stmnts, print("whatever") ?
>> Sure. How do you prefer the formatting? Using "format" would be true Python 
>> 3 style I think:
>> print('Ignoring file outside of client spec: {}'.format(path))
> 
> Will that breaker older versions of python? There's a statement somewhere 
> about how far back we support.
The "format" method requires Python 2.6 according to the Python docs:
https://docs.python.org/2/library/functions.html#format

Luckily this is also the version we aim to support according to 
Documentation/CodingGuidelines

Thanks,
Lars

--
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] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?

[Lars Schneider]

Thanks for your explanation Fredrik! However, I believe your 4. step is not 
what happens in the current implementation as the write call in wrapper.c dies 
directly. I see three ways to fix the problem:

(1) Make upload-pack wait for a response (with timeout) before it closes the 
pipe. However, I believe this would not be in line with the general Git 
philosophy stated in "git.c" (added in 7559a1be):
"Many parts of Git have subprograms communicate via pipe, expect the upstream 
of a pipe to die with SIGPIPE when the downstream of a pipe does not need to 
read all that is written."

(2) Ignore SIGPIPE errors when "fetch-pack" sends a "done" using 
"sigchain_push(SIGPIPE, SIG_IGN)" / "sigchain_pop(SIGPIPE)". However, then the 
check_pipe function (added in 756e676c) kicks in and we would need to work 
around that as well.

(3) Add a method "test_must_fail_or_die" to "test-lib-functions.sh". This 
method accepts exit codes 129<x<192, too. Use the new method in t5516.

All this code is fairly new to me and I don't understand all the ramifications. 
That being said I prefer solution (3) as it is the simplest solution.

Thanks,
Lars

On 25 Oct 2015, at 08:18, Fredrik Medley <fredrik.med...@gmail.com> wrote:

> I think the following happens:
> 1. The remote upload-pack finds out "not our ref"
> 2. The remote send a response and close the pipe
> 3. fetch-pack still tries to write commands to the remote upload-pack
> 4. Because the connection has already been closed, writing will fail
> with EPIPE which is detected by write_or_die() -> check_pipe()
> resulting in die(141)
> 
> The reason for the test to succeed, i.e. git-fetch fails with 128 (or
> 1?), in most cases must be because it manages to write everything
> before the context switch to the remote upload-pack occurs.
> 
> What is actually the wanted outcome? Should git-fetch try to continue
> to see if the already received response is enough to continue as
> normal?
> 
> Best regards,
> Fredrik
> 
> 2015-10-24 15:08 GMT+02:00 Lars Schneider <larsxschnei...@gmail.com>:
>> Hi,
>> 
>> while working on the Git CI integration I noticed that t5516 "75 - deny fetch
>> unreachable SHA1, allowtipsha1inwant=true" (introduced in 68ee628) seems to 
>> be
>> flaky on TravisCI. I get the following output in verbose mode:
>> 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>  
>> expecting success:
>>mk_empty testrepo &&
>>(
>>cd testrepo &&
>>git config uploadpack.allowtipsha1inwant 
>> $configallowtipsha1inwant &&
>>git commit --allow-empty -m foo &&
>>git commit --allow-empty -m bar &&
>>git commit --allow-empty -m xyz
>>) &&
>>SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&
>>SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
>>SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&
>>(
>>cd testrepo &&
>>git reset --hard $SHA1_2 &&
>>git cat-file commit $SHA1_1 &&
>>git cat-file commit $SHA1_3
>>) &&
>>mk_empty shallow &&
>>(
>>cd shallow &&
>>test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
>>test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
>>git --git-dir=../testrepo/.git config 
>> uploadpack.allowreachablesha1inwant true &&
>>git fetch ../testrepo/.git $SHA1_1 &&
>>git cat-file commit $SHA1_1 &&
>> 

Re: [RFC] URL rewrite in .gitmodules

[Lars Schneider]

On 20 Oct 2015, at 00:07, Stefan Beller <sbel...@google.com> wrote:

> On Mon, Oct 19, 2015 at 12:28 PM, Lars Schneider
> <larsxschnei...@gmail.com> wrote:
>> Hi,
>> 
>> I have a closed source Git repo which references an Open Source Git repo as 
>> Submodule. The Open Source Git repo references yet another Open Source repo 
>> as submodule. In order to avoid failing builds due to external services I 
>> mirrored the Open Source repos in my company network. That works great with 
>> the first level of Submodules. Unfortunately it does not work with the 
>> second level because the first level still references the "outside of 
>> company" repos. I know I can rewrite Git URLs with the git config 
>> "url..insteadOf" option. However, git configs are client specific.
> 
> I feel like this is working as intended. You only want to improve your
> one client (say the buildbot) to not goto the open source site, while
> the developer may do want to fetch from external sources ("Hey shiny
> new code!";)
Well, that's a good argument. However, our developers have usually no write 
access to these repos. If they want to push a commit then they need to fork the 
open source repo and change the submodule URL in the parent repo. I fear that 
this kind of process might overwhelm them and/or troubles them (changing Git 
submodules URLs has a few pitfalls). As a result they might be less inclined to 
make a contribution or - even worse - they copy the code in the parent repo, 
don't use Submodules and make no contribution at all. 


> 
>> I would prefer a solution that works without setup on any client. I also 
>> know that I could update the .gitmodules file in the Open Source repo on the 
>> first level. I also would prefer not to do this as I want to use the very 
>> same hashes as defined by the "upstream" Open Source repos.
> 
> You could carry a patch on top of the tip of the first submodule
> re-pointing the nested submodule. This requires good workflows
> available to deal with submodules though. (Fetch and merge or rebase,
> git submodule update should be able to do that?)
True. However, we have many Git beginners and I fear that this workflow would 
overwhelm them.


>> 
>> Is there yet another way in Git to change URLs of Submodules in the way I 
>> want it?
>> 
>> If not, what do you think about a patch that adds a "url" section similar to 
>> the one in git config to a .gitmodules file?
>> 
> 
> So we have different kinds of git configs. within one repository, in
> the home director (global to the one machine),
> maybe you would want to have one "global" config on a network share,
> such that every box in your company
> reads that "company-wide" global config and acts upon that?
That could actually work. The only downside I see is that the devs need to 
intentionally update their "company" git config. We have +4000 engineers and 
therefore I want to establish processes that are as easy and fault-tolerant as 
possible.  

> 
>> Example:
>> --
>> [submodule "git"]
>>path = git
>>url=git://github.com/larsxschneider/git.git
>> 
>> [url "mycompany.com"]
>>insteadOf = outside.com
> 
> Wouldn't that be better put into say a global git config instead of
> repeating it for every submodule?
See answer above. The git config setup could be an obstacle.

> 
> In case of the nested submodule you would need to carry the last lines
> as an extra patch anyway
> if this was done in the .gitmodules files? Or do you expect this to be
> applied recursively (i.e. nested
> submodules all the way down also substitute outside.com)
Yes, my intention was to apply these recursively.


> Am I missing your point?
I don't think so :-)

Thanks,
Lars--
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] URL rewrite in .gitmodules

[Lars Schneider]

On 20 Oct 2015, at 19:33, Junio C Hamano <gits...@pobox.com> wrote:

> Lars Schneider <larsxschnei...@gmail.com> writes:
> 
>> If not, what do you think about a patch that adds a "url" section
>> similar to the one in git config to a .gitmodules file?
>> 
>> Example:
>> --
>> [submodule "git"]
>>  path = git
>>url=git://github.com/larsxschneider/git.git
>> 
>> [url "mycompany.com"]
>>insteadOf = outside.com
>> --
> 
> It is unclear to me if you are adding the last two (or three,
> counting the blank before) lines to your company's private fork of
> the opensource project, but if that is the case, then that would
> defeat your earlier desire:
> 
>> ... I also would prefer not to do this as I want to use the
>> very same hashes as defined by the "upstream" ...
> 
> wouldn't it?
The last three lines are added to my companies closed source Git repo. In this 
example the company repo references git://github.com/larsxschneider/git.git as 
submodule. This submodule in turn references another submodule with a URL 
"outside.com". This is the URL I want to rewrite. Do you think this could be 
useful to others as well?


> I do not think this topic is specific to use of submodules.  If you
> want to encourage your engineers to fetch from nearby mirrors you
> maintain, you would want a forest of url.mine.insteadof=theirs for
> the external repositories that matter to you specified by
> everybody's $HOME/.gitconfig, and one way to do so would be to have
> them use the configuration inclusion.  An item in your engineer
> orientation material could tell them to add
> 
>   [include]
>   path = /usr/local/etc/git/mycompany.urlrewrite
> 
> when they set up their "[user] name/email" in there.
> 
> And you can update /usr/local/etc/git/mycompany.urlrewrite as
> needed.
Oh nice, I didn't know about "include". However, as mentioned to Stefan in this 
thread, I fear that our engineers will miss that. I would prefer a solution 
that does not need any additional setup. Therefore the suggestion to add 
rewrites in the .gitmodules file.

Thanks,
Lars

--
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] git-p4: Add option to ignore empty commits

[Lars Schneider]

On 20 Oct 2015, at 19:27, Junio C Hamano  wrote:

> larsxschnei...@gmail.com writes:
> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 0093fa3..6c50c74 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
>> filesToDelete = []
>> 
>> for f in files:
>> -# if using a client spec, only add the files that have
>> -# a path in the client
>> -if self.clientSpecDirs:
>> -if self.clientSpecDirs.map_in_client(f['path']) == "":
>> -continue
>> -
>> filesForCommit.append(f)
>> if f['action'] in self.delete_actions:
>> filesToDelete.append(f)
> 
> Earlier, the paths outside the clientspec were not in filesToDelete
> (or filesToRead that is below the context here).  Now they all go to
> these arrays, and will hit this loop beyond the context:
> 
># deleted files...
>for f in filesToDelete:
>self.streamOneP4Deletion(f)
> 
> after leaving the above for loop.  I cannot quite see where this
> "stream one deletion" is turned into a no-op for paths outside after
> this patch gets applied.

Earlier the client spec filtering happened in "def streamP4Files(self, files)". 
I moved the code up to the caller of this function into "def commit(...)" which 
now calls "streamP4Files" with an already filtered file list. Therefore the 
logic should be exactly the same.


> Also I have this suspicion that those who do want to use client spec
> to get a narrowed view into the history would almost always want
> this "ignore empty" behaviour (I'd even say the current behaviour to
> leave empty commits by default is a bug).  What are the advantages
> of keeping empty commits?  If there aren't many, perhaps git-p4
> should by the default skip empties and require p4.keepEmpty
> configuration to keep them?

I agree. 
@Luke: What option do you prefer? "git-p4.keepEmptyCommits" or 
"git-p4.ignoreEmptyCommits" ?

Thanks,
Lars--
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] git-p4: Add option to ignore empty commits

[Lars Schneider]

On 21 Oct 2015, at 08:32, Luke Diamand <l...@diamand.org> wrote:

> On 19/10/15 19:43, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> A changelist that contains only excluded files (e.g. via client spec or
>> branch prefix) will be imported as empty commit. Add option
>> "git-p4.ignoreEmptyCommits" to ignore these commits.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  Documentation/git-p4.txt   |   5 ++
>>  git-p4.py  |  41 -
>>  t/t9826-git-p4-ignore-empty-commits.sh | 103 
>> +
>>  3 files changed, 133 insertions(+), 16 deletions(-)
>>  create mode 100755 t/t9826-git-p4-ignore-empty-commits.sh
>> 
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 82aa5d6..f096a6a 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -510,6 +510,11 @@ git-p4.useClientSpec::
>>  option '--use-client-spec'.  See the "CLIENT SPEC" section above.
>>  This variable is a boolean, not the name of a p4 client.
>> 
>> +git-p4.ignoreEmptyCommits::
>> +A changelist that contains only excluded files will be imported
>> +as empty commit. To ignore these commits set this boolean option
>> +to 'true'.
> 
> s/as empty/as an empty/
> 
> Possibly putting 'true' in quotes could be confusing.
OK. Will fix.

> 
>> +
>>  Submit variables
>>  
>>  git-p4.detectRenames::
>> diff --git a/git-p4.py b/git-p4.py
>> index 0093fa3..6c50c74 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2288,12 +2288,6 @@ class P4Sync(Command, P4UserMap):
>>  filesToDelete = []
>> 
>>  for f in files:
>> -# if using a client spec, only add the files that have
>> -# a path in the client
>> -if self.clientSpecDirs:
>> -if self.clientSpecDirs.map_in_client(f['path']) == "":
>> -continue
>> -
>>  filesForCommit.append(f)
>>  if f['action'] in self.delete_actions:
>>  filesToDelete.append(f)
>> @@ -2368,18 +2362,33 @@ class P4Sync(Command, P4UserMap):
>>  if self.verbose:
>>  print "commit into %s" % branch
>> 
>> -# start with reading files; if that fails, we should not
>> -# create a commit.
>> -new_files = []
>> -for f in files:
>> -if [p for p in self.branchPrefixes if 
>> p4PathStartsWith(f['path'], p)]:
>> -new_files.append (f)
>> -else:
>> -sys.stderr.write("Ignoring file outside of prefix: %s\n" % 
>> f['path'])
>> -
>>  if self.clientSpecDirs:
>>  self.clientSpecDirs.update_client_spec_path_cache(files)
>> 
>> +def inClientSpec(path):
> 
> This seems to be adding a new function in the middle of an existing function. 
> Is that right?
That is true. I could move these functions into the P4Sync class if you don't 
like them here. I added them right there because it is the only place where 
they are used/useful.


> 
>> +if not self.clientSpecDirs:
>> +return True
>> +inClientSpec = self.clientSpecDirs.map_in_client(path)
>> +if not inClientSpec and self.verbose:
>> +print '\n  Ignoring file outside of client spec' % path
>> +return inClientSpec
> 
> Any particular reason for putting a \n at the start of the message?
I did this because "sys.stdout.write("\rImporting revision ..." (line 2724) 
does not add a newline. However, I agree that this looks stupid. I will remove 
the "\n" and fix the "Import revision" print. Speaking of that one: this is 
only printed if "not self.silent". Is there a particular reason to have 
"self.silent" and "self.verbose"? Should we merge the two? 


> 
> Also, could you use python3 style print stmnts, print("whatever") ?
Sure. How do you prefer the formatting? Using "format" would be true Python 3 
style I think:
print('Ignoring file outside of client spec: {}'.format(path))
OK?

> 
>> +
>> +def hasBranchPrefix(path):
>> +if not self.branchPrefixes:
>> +return True
>> +hasPrefix = [p for p in self.branchPrefixes
>> +

[RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?

[Lars Schneider]

Hi,

while working on the Git CI integration I noticed that t5516 "75 - deny fetch 
unreachable SHA1, allowtipsha1inwant=true" (introduced in 68ee628) seems to be 
flaky on TravisCI. I get the following output in verbose mode:

>>>
expecting success: 
mk_empty testrepo &&
(
cd testrepo &&
git config uploadpack.allowtipsha1inwant $configallowtipsha1inwant 
&&
git commit --allow-empty -m foo &&
git commit --allow-empty -m bar &&
git commit --allow-empty -m xyz
) &&
SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&
SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&
(
cd testrepo &&
git reset --hard $SHA1_2 &&
git cat-file commit $SHA1_1 &&
git cat-file commit $SHA1_3
) &&
mk_empty shallow &&
(
cd shallow &&
test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
git --git-dir=../testrepo/.git config 
uploadpack.allowreachablesha1inwant true &&
git fetch ../testrepo/.git $SHA1_1 &&
git cat-file commit $SHA1_1 &&
test_must_fail git cat-file commit $SHA1_2 &&
git fetch ../testrepo/.git $SHA1_2 &&
git cat-file commit $SHA1_2 &&
test_must_fail git fetch ../testrepo/.git $SHA1_3
)

Initialized empty Git repository in 
/home/travis/build/larsxschneider/git/t/trash 
directory.t5516-fetch-push/testrepo/.git/
[master (root-commit) a6b22ca] foo
 Author: A U Thor 
[master 5e26403] bar
 Author: A U Thor 
[master 64ea4c1] xyz
 Author: A U Thor 
HEAD is now at 5e26403 bar
tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
author A U Thor  1112912053 -0700
committer C O Mitter  1112912053 -0700
foo
tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
parent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad
author A U Thor  1112912053 -0700
committer C O Mitter  1112912053 -0700
xyz
Initialized empty Git repository in 
/home/travis/build/larsxschneider/git/t/trash 
directory.t5516-fetch-push/shallow/.git/
fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886
test_must_fail: died by signal: git fetch ../testrepo/.git 
64ea4c133d59fa98e86a771eda009872d6ab2886
not ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true 
<<<

"git fetch ../testrepo/.git $SHA1_3" seems to die as follows:
1. fetch-pack.c:408 goto done;
2. fetch-pack.c:447 done:
3. fetch-pack.c:229 static void send_request... (send "0009done\n" --> 9 bytes)
4. write_or_die.c:74 write_or_die
5. wrapper.c:331 write_in_full
6. wrapper.c:281 xwrite
7. wrapper.c:287 write --> just dies with exit code 141?!
(use 4688abf to match the line numbers)

You can find the full logs about the CI run here:
https://travis-ci.org/larsxschneider/git/jobs/86984110

I repeatedly executed this test to demonstrate the flakiness:
failed after 100 attempts: 
https://travis-ci.org/larsxschneider/git/jobs/87181753
failed after 1876 attempts: 
https://travis-ci.org/larsxschneider/git/jobs/87181754
(all executed on bbd2929 from https://github.com/larsxschneider/git)

Unfortunately I was not able to make it fail on my local machine (OS X 10.9) 
nor on my VM (Ubuntu 14.10). Travis CI uses Ubuntu 12.04 LTS Server Edition 64 
bit with a AUFS file system.

Has anyone an idea what might causes these failures or can give me pointers how
to investigate it?

Thank you,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-p4: import the ctypes module

[Lars Schneider]

Hi Etienne,

thanks for reporting this! Junio is right, I messed that up on my Windows 
testing box! :-( Sorry! 
If you have any questions around submitting patches I am happy to help as I 
just recently went through the learning process myself!

@Dennis: Thanks for the quick patch!

Thanks,
Lars

On 21 Oct 2015, at 10:23, Etienne Girard  wrote:

> Hello,
> 
> I couldn't work further on this yesterday (but I read
> Documentation/SubmittingPatches, which is a good start I guess). The
> diff proposed by Dennis works on my machine, I'll try to figure out
> why the original script worked with 2.7.10.
> 
> Thanks
> 
> 2015-10-21 1:00 GMT+02:00 Luke Diamand :
>> On 20/10/15 20:36, Junio C Hamano wrote:
>>> 
>>> Dennis Kaarsemaker  writes:
>>> 
> I do not follow Python development, but does the above mean that
> with recent 2.x you can say ctypes without first saying "import
> ctypes"?  It feels somewhat non-pythonesque that identifiers like
> this is given to you without you asking with an explicit 'import',
> so I am puzzled.
 
 
 No, you cannot do that. The reason others may not have noticed this bug
 is that
 in git-p4.py, ctypes is only used on windows.
 
  111 if platform.system() == 'Windows':
  112 free_bytes = ctypes.c_ulonglong(0)
  113
 ctypes.windll.kernel32.GetDiskFreeSpaceExW(ctypes.c_wchar_p(os.getcwd()),
 None, None, ctypes.pointer(free_bytes))
 
 The fact that it works for the OP with 2.7.10 is puzzling (assuming that
 it's
 on the same system).
>>> 
>>> 
>>> Exactly.  That is where my "I am puzzled" comes from.
>>> 
>>> The patch looks obviously the right thing to do.  Luke?  Lars?
>> 
>> 
>> It looks sensible to me, and works fine on Linux, thanks. ack.
>> 
>> I can't test on Windows today but I can't see why it wouldn't work.
>> 
>> Luke
>> 
>> 

--
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] fix flaky untracked-cache test

[Lars Schneider]

Looks good to me, Ack.

Test run with 74301d6 + my TravisCI patch:
https://travis-ci.org/larsxschneider/git/builds/86702932
... on Linux it failed in 1/2 cases after 53min
... on OSX it failed in 2/2 cases after 6min

Test run with 74301d6 + my TravisCI patch + David's t7063 patch:
https://travis-ci.org/larsxschneider/git/builds/86707133
.. on Linux it failed in 0/2 cases after 77min
...on OSX it failed in 0/2 cases after 48min (no error, CI system timed out, 
click on the builds to see detailed log output)

Cheers,
Lars

On 19 Oct 2015, at 21:48, David Turner  wrote:

> Dirty the test worktree's root directory, as the test expects.
> 
> When testing the untracked-cache, we previously assumed that checking
> out master would be sufficient to mark the mtime of the worktree's
> root directory as racily-dirty.  But sometimes, the checkout would
> happen at 12345.999 seconds and the status at 12346.001 seconds,
> meaning that the worktree's root directory would not be racily-dirty.
> And since it was not truly dirty, occasionally the test would fail.
> By making the root truly dirty, the test will always succeed.
> 
> Tested by running a few hundred times.
> 
> Signed-off-by: David Turner 
> ---
> t/t7063-status-untracked-cache.sh | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t7063-status-untracked-cache.sh 
> b/t/t7063-status-untracked-cache.sh
> index 37a24c1..0e8d0d4 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -412,7 +412,9 @@ test_expect_success 'create/modify files, some of which 
> are gitignored' '
>   echo two bis >done/two &&
>   echo three >done/three && # three is gitignored
>   echo four >done/four && # four is gitignored at a higher level
> - echo five >done/five # five is not gitignored
> + echo five >done/five && # five is not gitignored
> + echo test >base && #we need to ensure that the root dir is touched
> + rm base
> '
> 
> test_expect_success 'test sparse status with untracked cache' '
> -- 
> 2.4.2.644.g97b850b-twtrsrc
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] URL rewrite in .gitmodules

[Lars Schneider]

Hi,

I have a closed source Git repo which references an Open Source Git repo as 
Submodule. The Open Source Git repo references yet another Open Source repo as 
submodule. In order to avoid failing builds due to external services I mirrored 
the Open Source repos in my company network. That works great with the first 
level of Submodules. Unfortunately it does not work with the second level 
because the first level still references the "outside of company" repos. I know 
I can rewrite Git URLs with the git config "url..insteadOf" option. 
However, git configs are client specific. I would prefer a solution that works 
without setup on any client. I also know that I could update the .gitmodules 
file in the Open Source repo on the first level. I also would prefer not to do 
this as I want to use the very same hashes as defined by the "upstream" Open 
Source repos.

Is there yet another way in Git to change URLs of Submodules in the way I want 
it?

If not, what do you think about a patch that adds a "url" section similar to 
the one in git config to a .gitmodules file?

Example:
--
[submodule "git"]
path = git
url=git://github.com/larsxschneider/git.git

[url "mycompany.com"]
insteadOf = outside.com
--

Thanks,
Lars--
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] Add Travis CI support

[Lars Schneider]

On 15 Oct 2015, at 10:12, Matthieu Moy <matthieu@grenoble-inp.fr> wrote:

> Lars Schneider <larsxschnei...@gmail.com> writes:
> 
>> I was reluctant to this because I feared problems. Especially while
>> running tests in parallel.
> 
> Isn't the point of using a CI tool to notice problems? ;-)
> 
> More seriously, running tests in parallel shouldn't be a problem since
> each test runs in its own directory with HOME set to this private
> directory, so two diffent tests should not interfer. If there's an issue
> with parallel tests, we probably prefer discovering them than avoiding
> them.
OK. Great!


>> make -j2 9min 11sec:
>> https://travis-ci.org/larsxschneider/git/jobs/85478022
>> 
>> make 17min 20sec:
>> https://travis-ci.org/larsxschneider/git/jobs/85432398
> 
> Since the tests are essentially IO-bound and not CPU-bound, it may even
> make sense to use -j3 here.
Hehe you're right.

make -j3 6min 2sec
https://travis-ci.org/larsxschneider/git/jobs/85497307

just for fun I tried a few more values and -j5 seems to be the best with 4min 
27sec
https://travis-ci.org/larsxschneider/git/jobs/85501015

-j6 is slower again. Do you see a reason not to use "-j5"?

Thanks,
Lars

--
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

[BUG] t7063-status-untracked-cache flaky?

[Lars Schneider]

Hi,

I noticed that "t7063-status-untracked-cache.sh" occasionally fails with "not 
ok 24 - test sparse status with untracked cache".

E.g. on OS X compiled with gcc:
https://travis-ci.org/larsxschneider/git/jobs/85432514

E.g. on Linux compiled with gcc:
https://travis-ci.org/larsxschneider/git/jobs/84986975

The test was added with commit 7687252. I have not really investigated the 
problem yet but the "avoid_racy" method caught my attention. Is this test known 
to be flaky? Would an increased sleep time in "avoid_racy" help?

Thanks,
Lars--
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] Add Travis CI support

[Lars Schneider]

On 13 Oct 2015, at 12:32, Jean-Noël Avila  wrote:

> Le 11/10/2015 19:55, larsxschnei...@gmail.com a écrit :
>> +
>> +before_script: make
>> +
>> +script: make --quiet test
> 
> Travis can be used in container mode but that would need getting rid of
> "sudo" command and only installing from white-listed sources
> (https://github.com/travis-ci/apt-source-whitelist/blob/master/ubuntu.json)
> 
> Anyway, even within the present VM mode, 1.5 cores are available, so it
> makes sense to add "-j2" to every make commands.
> 
I was reluctant to this because I feared problems. Especially while running 
tests in parallel. However, the result looks quite good.

make -j2 9min 11sec:
https://travis-ci.org/larsxschneider/git/jobs/85478022

make 17min 20sec:
https://travis-ci.org/larsxschneider/git/jobs/85432398

If there is no argument against running test in parallel then I will add it to 
the next roll.

Thanks,
Lars--
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] Add Travis CI support

[Lars Schneider]

On 12 Oct 2015, at 22:20, Matthieu Moy  wrote:

> larsxschnei...@gmail.com writes:
> 
>> --- /dev/null
>> +++ b/.travis.yml
>> @@ -0,0 +1,46 @@
>> +language: c
>> +
>> +os:
>> +  - linux
>> +  - osx
>> +
>> +compiler:
>> +  - clang
>> +  - gcc
>> +
>> +before_install:
>> +  - >
>> +export GIT_TEST_OPTS=" --quiet";
>> +case "${TRAVIS_OS_NAME:-linux}" in
>> +linux)
>> +  wget -q https://package.perforce.com/perforce.pubkey -O - \
>> +| sudo apt-key add -
>> +  echo 'deb http://package.perforce.com/apt/ubuntu precise release' \
>> +| sudo tee -a /etc/apt/sources.list
>> +  wget -q https://packagecloud.io/gpg.key -O - | sudo apt-key add -
>> +  echo 'deb https://packagecloud.io/github/git-lfs/debian/ jessie main' 
>> \
>> +| sudo tee -a /etc/apt/sources.list
>> +  sudo apt-get update -qq
>> +  sudo apt-get install -y apt-transport-https
>> +  sudo apt-get install perforce-server git-lfs
> 
> Sorry if this has been discussed already, but do you really need these
> "sudo" calls?
> 
> They trigger builds on the legacy Travis CI infrastructure:
> 
>  
> http://docs.travis-ci.com/user/migrating-from-legacy/?utm_source=legacy-notice_medium=banner_campaign=legacy-upgrade
> 
> No big deal, but getting rid of sudo would be cool, and documenting why
> it can't easily be done in commit message and/or comments would be nice
> otherwise.
I would like to get rid of the "sudo" calls, too. Unfortunately I wasn't able 
to achieve this so far because these packages are not white listed on Travis CI 
(see Jean-Noël answer in this thread). I tried to download and install the 
*.deb files manually using dpkg without luck. Any idea or hint?
Nevertheless I don't think this is a problem as Travis CI states that "sudo 
isn't possible (__right now__)" on the new infrastructure. They need to find a 
solutions because I believe many projects will run into this issue...
http://docs.travis-ci.com/user/migrating-from-legacy/?utm_source=legacy-notice_medium=banner_campaign=legacy-upgrade#Using-sudo-isn%E2%80%99t-possible-(right-now)

- Lars--
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] Add Travis CI support

[Lars Schneider]

>> +  brew_force_set_latest_binary_hash () {
>> +FORUMULA=$1
> 
> Is this spelling intentional or is it a misspelling of "formula"?

This is a misspelling. I will fix it.

Thanks,
Lars
--
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] Add Travis CI support

[Lars Schneider]

On 12 Oct 2015, at 01:05, Sebastian Schuberth  wrote:

> On 10/11/2015 19:55, larsxschnei...@gmail.com wrote:
> 
>> +  sudo apt-get update -qq
>> +  sudo apt-get install -y apt-transport-https
>> +  sudo apt-get install perforce-server git-lfs
> 
> Why no "-y" also in this line, or append these to the previous line?
> 
> Or maybe even better, like [1] does, also use "--qq" (which implies "-y") for 
> "apt-get install"?
Agreed!

> 
>> +install: make configure && ./configure
>> +
>> +before_script: make
>> +
>> +script: make --quiet test
> 
> Semantically, it does not seem correct to me that configuarion goes to the 
> install step. As "make test" will build git anyway, I'd instead propose to 
> get rid of "install" and just say:
> 
> before_script: make configure && ./configure
> 
> script: make --quiet test

I understand your point. I did this to make the "make" logs easily accessible 
(no option "--quite"). By default Travis CI automatically collapses the logs 
from all stages prior to the "script" stage. You can uncollapse these logs by 
clicking on the little triangle on the left border of the log. Therefore the 
"make" logs are available without noise.

Do you see value in "make" logs? 

If yes then we could also do:
before_script: make configure && ./configure && make

If no then I will take your suggestion.

Thanks,
Lars--
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] Add Travis CI support

[Lars Schneider]

On 12 Oct 2015, at 12:37, Sebastian Schuberth <sschube...@gmail.com> wrote:

> On Mon, Oct 12, 2015 at 7:12 PM, Lars Schneider
> <larsxschnei...@gmail.com> wrote:
> 
>>>> +install: make configure && ./configure
>>>> +
>>>> +before_script: make
>>>> +
>>>> +script: make --quiet test
>>> 
>>> Semantically, it does not seem correct to me that configuarion goes to the 
>>> install step. As "make test" will build git anyway, I'd instead propose to 
>>> get rid of "install" and just say:
>>> 
>>> before_script: make configure && ./configure
>>> 
>>> script: make --quiet test
>> 
>> I understand your point. I did this to make the "make" logs easily 
>> accessible (no option "--quite"). By default Travis CI automatically 
>> collapses the logs from all stages prior to the "script" stage. You can 
>> uncollapse these logs by clicking on the little triangle on the left border 
>> of the log. Therefore the "make" logs are available without noise.
> 
> To make this more clear, I guess what you're referring to is the
> visual difference between [1] and [2], correct?
correct!

> 
>> Do you see value in "make" logs?
>> 
>> If yes then we could also do:
>> before_script: make configure && ./configure && make
> 
> Reading through Travis' docs [3] again, "before_script" is documented
> to "return a non-zero exit code, the build is errored and stops
> immediately", while "script" is documented as "returns a non-zero exit
> code, the build is failed, but continues to run before being marked as
> failed". As it does not make much sense to continue the build or even
> start testing if the build failed, maybe it's indeed best to do:
> 
> before_script: make configure && ./configure && make
> 
> script: make --quiet test

Ok, then I will make it so :-)

Thanks,
Lars--
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] Add Travis CI support

[Lars Schneider]

On 12 Oct 2015, at 09:02, Junio C Hamano  wrote:

> Sebastian Schuberth  writes:
> 
>> Semantically, it does not seem correct to me that configuarion goes to
>> the install step. As "make test" will build git anyway, I'd instead
>> propose to get rid of "install" and just say:
>> 
>> before_script: make configure && ./configure
>> 
>> script: make --quiet test
> 
> Very good point.  Do we even need to do anything in the "install"
> target?  We aim to be able to testable without any installed Git,
> and not running "make install" at all, ever, would be one way to
> make sure that works.
The Travis CI "install" stage is independent of "make install". AFAIK you can 
use the Travis lifecycle stages pretty much as you want. However, I agree we 
should not use the "install" stage to avoid confusion.


> This is a slightly related tangent, but we saw a few build issues
> reported recently on customized configurations like NO_PTHREAD.  If
> we are to start using automated tests, I wonder if we want to build
> (and optionally test) with various combinations of the customization
> options (e.g. NO_CURL, NO_OPENSSL, NO_MMAP, NO_IPV6, NO_PERL etc.)
This easy to do. However, the more we environment settings we define the longer 
the build runs. I created a test matrix that runs the following combinations:
{Linux | OSX} * {gcc | clang} * {Default, NO_PTHREAD, NO_CURL, NO_OPENSSL, 
NO_MMAP, NO_IPV6, NO_PERL}

These result in 28 (= 2*2*7) combinations. I created a build without the 
"Default" environment (=24 combination) here:
https://travis-ci.org/larsxschneider/git/builds/84978673

Should I add them them to the Travis CI patch?

Thanks,
Lars

--
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 3/4] git-p4: Fix t9815 git-p4-submit-fail test case on OS X

[Lars Schneider]

On 04 Oct 2015, at 11:23, Junio C Hamano  wrote:

> larsxschnei...@gmail.com writes:
> 
>> +if test_have_prereq CYGWIN; then
>> +: # NOOP
>> +elif test_have_prereq DARWIN; then
>> +stat -f %Sp text | egrep ^-r-- &&
>> +stat -f %Sp text+x | egrep ^-r-x
>> +else
>>  stat --format=%A text | egrep ^-r-- &&
>>  stat --format=%A text+x | egrep ^-r-x
>>  fi
> 
> Not a new problem but why do we need "stat" here?
> 
> Shouldn't "test -r", "! test -x", and their usual friends be
> sufficient for the purpose of the test and are more portable?

Good question. The stat call was introduced with df9c545 by Pete Wyckoff.
@Pete, @Luke: Are you aware of any particular reason for stat?

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] Add prereq to detect OS X

[Lars Schneider]

On 04 Oct 2015, at 11:28, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> Some Unix tools differ between Linux and Mac OS X. Add this prereq to
>> detect OS X and handle it appropriately.
> 
> If my comment on 4/4 turns out to be to the point, then we do not
> need this, I presume?  In general, we really should think three
> times before adding prerequisite based on the platform.  A prereq
> based on a specific feature of the platform is often what we want
> instead.
Agreed. I will remove this commit in the next roll.

Thanks,
Lars

--
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

[Lars Schneider]

On 03 Oct 2015, at 10:23, Junio C Hamano  wrote:

> 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.
> 
Oh. This was not intentional. I wonder how that happened. I will watch out for 
it in the future.

Thanks for fixing,
Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] git-p4: Disable t9819 git-p4-case-folding test on OS X

[Lars Schneider]

On 04 Oct 2015, at 11:26, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> The OS X file system is case insensitive by default. Consequently this
>> test does not apply.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> t/t9819-git-p4-case-folding.sh | 5 +
>> 1 file changed, 5 insertions(+)
>> 
>> diff --git a/t/t9819-git-p4-case-folding.sh b/t/t9819-git-p4-case-folding.sh
>> index 78f1d0f..c69ae47 100755
>> --- a/t/t9819-git-p4-case-folding.sh
>> +++ b/t/t9819-git-p4-case-folding.sh
>> @@ -4,6 +4,11 @@ test_description='interaction with P4 case-folding'
>> 
>> . ./lib-git-p4.sh
>> 
>> +if test_have_prereq DARWIN; then
>> +skip_all='skipping P4 case-folding tests; OS X file system is case 
>> insensitive by default'
>> +test_done
>> +fi
> 
> Makes one wonder what should happen on Windows, or vfat mounted on
> Linux for that matter.  IOW, shouldn't the prerequisite be more like
> "do not run any of these tests if the filesystem does not allow us
> to have two files in different cases at the same time"?
> 
> Perhaps
> 
>if ! test_have_prereq CASE_INSENSITIVE_FS
>then
>skip_all=...
>test_done
>fi
> 
> instead, or something?
Agreed! Although I think the “!” in the if clause is not correct.
By the way... what formatting should I use?

if foo
then
  bar

or

if foo; then
  bar

I think the latter is more used in the code base.

- Lars--
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

[Lars Schneider]

On 25 Sep 2015, at 10:05, Luke Diamand  wrote:

> On 25 September 2015 at 08:27, Johannes Schindelin
>  wrote:
>> Hi,
>> 
>> On 2015-09-25 05:14, Dennis Kaarsemaker wrote:
>>> On do, 2015-09-24 at 17:41 -0700, Junio C Hamano wrote:
 larsxschnei...@gmail.com writes:
 
> My idea is that the owner of "https://github.com/git/git; enables this 
> account
> for Travis (it's free!). Then we would automatically get the test state 
> for all
> official branches.
 
 The last time I heard about this "it's free" thing, I thought I
 heard that it wants write access to the repository.
>>> 
>>> It does not need write access to the git data, only to auxiliary GitHub
>>> data: commit status and deployment status (where it can put "this
>>> commit failed tests"), repository hooks (to set up build triggers),
>>> team membership (ro) and email addresses (ro).
>> 
>> If that still elicits concerns, a fork could be set up that is automatically 
>> kept up-to-date via a web hook, and enable Travis CI there.
>> 
>> Junio, if that is something with which you feel more comfortable, I would be 
>> willing to set it up. Even if the visibility (read: impact) would be higher 
>> if the badges were attached to https://github.com/git/git proper...
>> 
> 
> It would be less intrusive for the CI system to have a fork. Otherwise
> other people using git with the same CI system will get annoying merge
> conflicts, and we'll also end up with a repo littered with the control
> files from past CI systems if the CI system is ever changed.
> 
> From past experience, if it's configured to email people when things
> break, sooner or later it will email the wrong people, probably once
> every few seconds over a weekend.
> 
> Automated testing is a Good Thing, but it's still software, so needs
> maintenance or it will break.

I completely agree with your argument about emails and that software needs 
maintenance. We could setup this CI to not send any emails. We still could 
inspect the build/test state of each branch on the Travis CI website. I believe 
this is valuable because not everyone has e.g. a Mac system at hand to run all 
tests. This is no theoretical example because t9819 is broken on maint using a 
Mac.

Lars--
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-p4: t9819 failing

[Lars Schneider]

Good catch. The case-handling test is actually fine. There was a bug in my 
implementation!

If you do this:
git p4 clone //depot/something/...

Then git p4 generates a directory “something” and clones into that (similar to 
Git). Since I set “cloneDirectory” to the current working directory that logic 
never kicked in. Therefore the depot was cloned into the current working 
directory instead of a new directory “something” and the test broke.

Thanks,
Lars

On 24 Sep 2015, at 22:29, Luke Diamand <l...@diamand.org> wrote:

> OK, slight correction there - it now doesn't crash getting the disk
> usage, but I think it still needs to be updated following the other
> changes to case-handling.
> 
> Luke
> 
> On 24 September 2015 at 08:45, Luke Diamand <l...@diamand.org> wrote:
>> On 23 September 2015 at 13:28, Lars Schneider <larsxschnei...@gmail.com> 
>> wrote:
>>> 
>>>> Here's the last bit of the crash dump from git-p4 I get:
>>>> 
>>>> File "/home/ldiamand/git/git/git-p4", line 2580, in streamP4FilesCbSelf
>>>>   self.streamP4FilesCb(entry)
>>>> File "/home/ldiamand/git/git/git-p4", line 2497, in streamP4FilesCb
>>>>   required_bytes = int((4 * int(self.stream_file["fileSize"])) -
>>>> calcDiskFree(self.cloneDestination))
>>>> File "/home/ldiamand/git/git/git-p4", line 116, in calcDiskFree
>>>>   st = os.statvfs(dirname)
>>>> OSError: [Errno 2] No such file or directory: 'lc'
>>>> 
>>>> Luke
>>> Confirmed. What do you think about this fix?
>> 
>> Works for me!
>> 
>> 
>> 
>>> 
>>> Thank you,
>>> Lars
>>> 
>>> ---
>>> git-p4.py | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 1d1bb87..66c0a4e 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -3478,6 +3478,7 @@ class P4Clone(P4Sync):
>>> 
>>> print "Importing from %s into %s" % (', '.join(depotPaths), 
>>> self.cloneDestination)
>>> 
>>> +self.cloneDestination = os.path.abspath(self.cloneDestination)
>>> if not os.path.exists(self.cloneDestination):
>>> os.makedirs(self.cloneDestination)
>>> chdir(self.cloneDestination)
>>> --

--
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/7] git-p4: add support for large file systems

[Lars Schneider]

On 23 Sep 2015, at 13:25, Luke Diamand <l...@diamand.org> wrote:

> Adding back git@vger.kernel.org, which I inadvertently dropped off the thread.
> 
> On 23 September 2015 at 12:22, Luke Diamand <l...@diamand.org> wrote:
>> On 23 September 2015 at 11:09, Lars Schneider <larsxschnei...@gmail.com> 
>> wrote:
>>> 
>>> On 23 Sep 2015, at 11:22, Luke Diamand <l...@diamand.org> wrote:
>>> 
>>>> On 23 September 2015 at 09:50, Lars Schneider <larsxschnei...@gmail.com> 
>>>> wrote:
>>>>> 
>>>>> On 23 Sep 2015, at 10:18, Lars Schneider <larsxschnei...@gmail.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> 
>>>>> I think I found an easy fix. Can you try it?
>>>>> 
>>>>> (in case my mail app messes something up: I changed line 2240 in 
>>>>> git-p4.py to 'self.cloneDestination = os.getcwd()’)
>>>> 
>>>> It fixes the problem, but in re-running the tests, I'm seeing t9808
>>>> fail which doesn't happen on next.
>>> Confirmed. I fixed it.
>>> Do you think it makes sense to create a new roll v8 for Junio?
>> 
>> How about we leave it a day or two in case anything else crawls out of
>> the woodwork?
>> 
>> Thanks!
>> Luke
sounds good to me!

Thanks,
Lars--
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-p4: t9819 failing

[Lars Schneider]

On 23 Sep 2015, at 11:27, Luke Diamand  wrote:

> Lars,
> 
> I just noticed that your change "git-p4: honor core.ignorecase when
> using P4 client specs" seems to break t9819.
> 
> I suspect that the problem is just that t9819 needs to be updated to
> reflect your change - do you have any thoughts on that?
> 
> Thanks!
> Luke

What OS and what commit do you use for testing? Here is what I got on my 
machine:

Ubuntu, next (c07a1e8) -> OK
Ubuntu, maint (ee6ad5f) -> OK

OS X, next (c07a1e8) -> broken
OS X, maint (ee6ad5f) -> broken

Thanks,
Lars--
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-p4: t9819 failing

[Lars Schneider]

On 23 Sep 2015, at 13:11, Luke Diamand <l...@diamand.org> wrote:

> On 23 September 2015 at 11:27, Lars Schneider <larsxschnei...@gmail.com> 
> wrote:
>> 
>> On 23 Sep 2015, at 11:27, Luke Diamand <l...@diamand.org> wrote:
>> 
>>> Lars,
>>> 
>>> I just noticed that your change "git-p4: honor core.ignorecase when
>>> using P4 client specs" seems to break t9819.
>>> 
>>> I suspect that the problem is just that t9819 needs to be updated to
>>> reflect your change - do you have any thoughts on that?
>>> 
>>> Thanks!
>>> Luke
>> 
>> What OS and what commit do you use for testing? Here is what I got on my 
>> machine:
>> 
>> Ubuntu, next (c07a1e8) -> OK
>> Ubuntu, maint (ee6ad5f) -> OK
>> 
>> OS X, next (c07a1e8) -> broken
>> OS X, maint (ee6ad5f) -> broken
> 
> The next branch is good:
> c07a1e8 Merge branch 'bb/remote-get-url' into next
> 
> The pu branch isn't:
> f44e3df Merge branch 'jk/notes-dwim-doc' into pu
> 
> This is on Debian.
> 
> Actually I think the problem may be in calcDiskFree().
> 
> Here's the last bit of the crash dump from git-p4 I get:
> 
>  File "/home/ldiamand/git/git/git-p4", line 2580, in streamP4FilesCbSelf
>self.streamP4FilesCb(entry)
>  File "/home/ldiamand/git/git/git-p4", line 2497, in streamP4FilesCb
>required_bytes = int((4 * int(self.stream_file["fileSize"])) -
> calcDiskFree(self.cloneDestination))
>  File "/home/ldiamand/git/git/git-p4", line 116, in calcDiskFree
>st = os.statvfs(dirname)
> OSError: [Errno 2] No such file or directory: 'lc'
> 
> Luke
Confirmed. What do you think about this fix?

Thank you,
Lars

---
 git-p4.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-p4.py b/git-p4.py
index 1d1bb87..66c0a4e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3478,6 +3478,7 @@ class P4Clone(P4Sync):
 
 print "Importing from %s into %s" % (', '.join(depotPaths), 
self.cloneDestination)
 
+self.cloneDestination = os.path.abspath(self.cloneDestination)
 if not os.path.exists(self.cloneDestination):
 os.makedirs(self.cloneDestination)
 chdir(self.cloneDestination)
-- --
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

Thanks a lot for taking care of this!

- Lars

On 22 Sep 2015, at 21:17, Junio C Hamano <gits...@pobox.com> wrote:

> Yup, this was privately reported and I just squashed a fix in right now ;-)
> 
> Thanks. "cd t && make test-lint" would have caught it.
> 
> On Tue, Sep 22, 2015 at 12:11 PM, Michael Blume <blume.m...@gmail.com> wrote:
>> I'm seeing test failures
>> 
>> non-executable tests: t9825-git-p4-handle-utf16-without-bom.sh
>> 
>> ls -l shows that all the other tests are executable but t9825 isn't.
>> 
>> On Tue, Sep 22, 2015 at 9:02 AM, Junio C Hamano <gits...@pobox.com> wrote:
>>> Lars Schneider <larsxschnei...@gmail.com> writes:
>>> 
>>>> This works.
>>> 
>>> OK, and thanks; as I don't do perforce, the squash was without any
>>> testing.
>>> 
>>>> Do we need the “-e” option?
>>> 
>>> In syntactic sense, no, but our codebase tends to prefer to have
>>> one, because it is easier to spot which ones are the instructions if
>>> you consistently have "-e" even when you give only one.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe git" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 0/7] git-p4: add support for large file systems

[Lars Schneider]

On 23 Sep 2015, at 00:03, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> diff to v6:
>> * fix spaces in .gitattributes for Git-LFS files (old "[:space:]", new 
>> "[[:space:]]")
>> * generated patch on top of next (95c4325) to resolve merge conflicts
>> 
>> I am sorry about the "[:space:]" bug this late. I had the fix in my 
>> development
>> branch but missed to apply it in this patch series.
>> 
>> @Junio:
>> Your conflict resolution in pu looks good. Nevertheless, I made this patch on
>> top of next to ease the integration. I hope this is ok.
> 
> Please don't rebase without a good reason, especially after you
> checked the conflict resolution is OK and your reroll does not
> affect the way the conflicts are resolved.  If you based your
> v6 patch on v2.6.0-rc0 and then your v7 patch needs to use something
> that did not exist v2.6.0-rc3, you cannot avoid rebasing on top of
> that newer codebase to use that new feature, but otherwise, no.
> 
> This is because I prefer to apply the new series to the same base
> version so that each step can be compared with the corresponding
> step in the previous round.
> 
> I even try hard to keep the commits from the older round if the
> patch text and log message are unchanged.  This time, I had to
> backport [v7 6/7] to apply to the same base before noticing and
> verifying that [v7 7/7] is the only thing that was changed in this
> round.  All the other ones turned out to be identical.
> 
> Hence, the end result for me was
> 
>$ git checkout ls/p4-lfs
>$ git reset --hard HEAD^
>$ git am -s git-p4-lfs-7-of-7.mbox
> 
> but it took me a lot longer than necessary because of the rebase.
> 
Understood. Thanks for taking the time and explaining me your workflow. That 
helps a lot as my intention was to save your time, not waste it.

Sorry,
Lars
--
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 1/7] git-p4: add optional type specifier to gitConfig reader

[Lars Schneider]

On 22 Sep 2015, at 23:49, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> The functions “gitConfig” and “gitConfigBool” are almost identical. Make 
>> “gitConfig” more generic by adding an optional type specifier. Use the type 
>> specifier “—bool” with “gitConfig” to implement “gitConfigBool. This 
>> prepares the implementation of other type specifiers such as “—int”.
> 
> What is this overlong single line paragraph?  Is this a MUA artifact
> on my end?
> 
No. It looks like I messed that up. Same is true for the commit “git-p4: add 
gitConfigInt reader”. I’ll be more careful in the future.

Sorry,
Lars

--
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/7] git-p4: add support for large file systems

[Lars Schneider]

On 23 Sep 2015, at 10:18, Lars Schneider <larsxschnei...@gmail.com> wrote:

> 
> On 23 Sep 2015, at 09:58, Luke Diamand <l...@diamand.org> wrote:
> 
>> On 21 September 2015 at 23:41,  <larsxschnei...@gmail.com> wrote:
>>> From: Lars Schneider <larsxschnei...@gmail.com>
>>> 
>>> diff to v6:
>>> * fix spaces in .gitattributes for Git-LFS files (old "[:space:]", new 
>>> "[[:space:]]")
>>> * generated patch on top of next (95c4325) to resolve merge conflicts
>> 
>> Trying this out by hand (rather than using the test cases) it crashes
>> for me. Am I doing something wrong?
>> 
>> - I've got the version of git-p4.py from
>> ef93b8df71f6d8a7936786c989714868c2d3540c in Junio's ls/p4-lfs branch.
>> - I have git-lfs v0.60 (just downloaded the binary distribution).
>> 
>> Then I created a p4 repo, and added two files in Perforce, small.txt
>> and bigfile. bigfile is 400k.
>> 
>> Doing a regular git clone works fine:
>> 
>> $ mkdir depot
>> $ cd depot
>> $ git init .
>> $ git p4 sync //depot
>> $ git checkout p4/master
>> 
>> (I'm using git init + git sync rather than git clone so that I can
>> setup the git config variables I need in the next step).
>> 
>> Doing a clone with LFS support though doesn't work:
>> 
>> $ mkdir depot2
>> $ cd depot2
>> $ git init
>> $ git config git-p4.largeFileSystem GitLFS
>> $ git config git-p4.largeFileThreshold 100k
>> $ git p4 sync //depot
>> Doing initial import of //depot/ from revision #head into 
>> refs/remotes/p4/master
>> Traceback (most recent call last):
>> File "/home/ldiamand/git/git/git-p4.py", line 3624, in 
>>   main()
>> File "/home/ldiamand/git/git/git-p4.py", line 3618, in main
>>   if not cmd.run(args):
>> File "/home/ldiamand/git/git/git-p4.py", line 3298, in run
>>   self.importHeadRevision(revision)
>> File "/home/ldiamand/git/git/git-p4.py", line 3085, in importHeadRevision
>>   self.commit(details, self.extractFilesFromCommit(details), self.branch)
>> File "/home/ldiamand/git/git/git-p4.py", line 2651, in commit
>>   self.streamP4Files(new_files)
>> File "/home/ldiamand/git/git/git-p4.py", line 2565, in streamP4Files
>>   cb=streamP4FilesCbSelf)
>> File "/home/ldiamand/git/git/git-p4.py", line 501, in p4CmdList
>>   cb(entry)
>> File "/home/ldiamand/git/git/git-p4.py", line 2559, in streamP4FilesCbSelf
>>   self.streamP4FilesCb(entry)
>> File "/home/ldiamand/git/git/git-p4.py", line 2501, in streamP4FilesCb
>>   self.streamOneP4File(self.stream_file, self.stream_contents)
>> File "/home/ldiamand/git/git/git-p4.py", line 2451, in streamOneP4File
>>   (git_mode, contents) =
>> self.largeFileSystem.processContent(self.cloneDestination, git_mode,
>> relPath, contents)
>> File "/home/ldiamand/git/git/git-p4.py", line , in processContent
>>   return LargeFileSystem.processContent(self, cloneDestination,
>> git_mode, relPath, contents)
>> File "/home/ldiamand/git/git/git-p4.py", line 1004, in processContent
>>   (git_mode, contents, localLargeFile) =
>> self.generatePointer(cloneDestination, contentTempFile)
>> File "/home/ldiamand/git/git/git-p4.py", line 1068, in generatePointer
>>   oid,
>> File "/usr/lib/python2.7/posixpath.py", line 70, in join
>>   elif path == '' or path.endswith('/'):
>> AttributeError: 'NoneType' object has no attribute 'endswith'
>> 
> Confirmed. This is a bug!
> 
> Can you try the following sequence of commands?
> 
> $ mkdir depot2
> $ cd depot2
> $ git init
> $ git config git-p4.largeFileSystem GitLFS
> $ git config git-p4.largeFileThreshold 100k
> $ git p4 clone --destination=. //depot
> 
> I’ve always used “clone” and all test cases uses “clone”. I will fix it for 
> “sync”.
> 
> Thanks for reporting,
> Lars

I think I found an easy fix. Can you try it?

(in case my mail app messes something up: I changed line 2240 in git-p4.py to 
'self.cloneDestination = os.getcwd()’)

Thanks,
Lars


diff --git a/git-p4.py b/git-p4.py
index 8c7496d..1d1bb87 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2237,7 +2237,7 @@ class P4Sync(Command, P4UserMap):
 self.tempBranches = []
 self.tempBranchLocation = "git-p4-tmp"
 self.largeFileSystem = None
-self.cloneDestination = None
+self.cloneDestination = os.getcwd()

 if gitConfig('git-p4.largeFileSystem'):
 largeFileSystemConstructor = 
globals(

Re: [PATCH v7 0/7] git-p4: add support for large file systems

[Lars Schneider]

On 23 Sep 2015, at 09:58, Luke Diamand <l...@diamand.org> wrote:

> On 21 September 2015 at 23:41,  <larsxschnei...@gmail.com> wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> diff to v6:
>> * fix spaces in .gitattributes for Git-LFS files (old "[:space:]", new 
>> "[[:space:]]")
>> * generated patch on top of next (95c4325) to resolve merge conflicts
> 
> Trying this out by hand (rather than using the test cases) it crashes
> for me. Am I doing something wrong?
> 
> - I've got the version of git-p4.py from
> ef93b8df71f6d8a7936786c989714868c2d3540c in Junio's ls/p4-lfs branch.
> - I have git-lfs v0.60 (just downloaded the binary distribution).
> 
> Then I created a p4 repo, and added two files in Perforce, small.txt
> and bigfile. bigfile is 400k.
> 
> Doing a regular git clone works fine:
> 
> $ mkdir depot
> $ cd depot
> $ git init .
> $ git p4 sync //depot
> $ git checkout p4/master
> 
> (I'm using git init + git sync rather than git clone so that I can
> setup the git config variables I need in the next step).
> 
> Doing a clone with LFS support though doesn't work:
> 
> $ mkdir depot2
> $ cd depot2
> $ git init
> $ git config git-p4.largeFileSystem GitLFS
> $ git config git-p4.largeFileThreshold 100k
> $ git p4 sync //depot
> Doing initial import of //depot/ from revision #head into 
> refs/remotes/p4/master
> Traceback (most recent call last):
>  File "/home/ldiamand/git/git/git-p4.py", line 3624, in 
>main()
>  File "/home/ldiamand/git/git/git-p4.py", line 3618, in main
>if not cmd.run(args):
>  File "/home/ldiamand/git/git/git-p4.py", line 3298, in run
>self.importHeadRevision(revision)
>  File "/home/ldiamand/git/git/git-p4.py", line 3085, in importHeadRevision
>self.commit(details, self.extractFilesFromCommit(details), self.branch)
>  File "/home/ldiamand/git/git/git-p4.py", line 2651, in commit
>self.streamP4Files(new_files)
>  File "/home/ldiamand/git/git/git-p4.py", line 2565, in streamP4Files
>cb=streamP4FilesCbSelf)
>  File "/home/ldiamand/git/git/git-p4.py", line 501, in p4CmdList
>cb(entry)
>  File "/home/ldiamand/git/git/git-p4.py", line 2559, in streamP4FilesCbSelf
>self.streamP4FilesCb(entry)
>  File "/home/ldiamand/git/git/git-p4.py", line 2501, in streamP4FilesCb
>self.streamOneP4File(self.stream_file, self.stream_contents)
>  File "/home/ldiamand/git/git/git-p4.py", line 2451, in streamOneP4File
>(git_mode, contents) =
> self.largeFileSystem.processContent(self.cloneDestination, git_mode,
> relPath, contents)
>  File "/home/ldiamand/git/git/git-p4.py", line , in processContent
>return LargeFileSystem.processContent(self, cloneDestination,
> git_mode, relPath, contents)
>  File "/home/ldiamand/git/git/git-p4.py", line 1004, in processContent
>(git_mode, contents, localLargeFile) =
> self.generatePointer(cloneDestination, contentTempFile)
>  File "/home/ldiamand/git/git/git-p4.py", line 1068, in generatePointer
>oid,
>  File "/usr/lib/python2.7/posixpath.py", line 70, in join
>elif path == '' or path.endswith('/'):
> AttributeError: 'NoneType' object has no attribute 'endswith'
> 
Confirmed. This is a bug!

Can you try the following sequence of commands?

$ mkdir depot2
$ cd depot2
$ git init
$ git config git-p4.largeFileSystem GitLFS
$ git config git-p4.largeFileThreshold 100k
$ git p4 clone --destination=. //depot

I’ve always used “clone” and all test cases uses “clone”. I will fix it for 
“sync”.

Thanks for reporting,
Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 22 Sep 2015, at 03:10, Junio C Hamano  wrote:

> Eric Sunshine  writes:
> 
>> Yes, it's because $d is a variable reference, even within double
>> quotes.
> 
> s/even/especially/ ;-)
> 
> Here is what I queued as SQUASH???
> 
> diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh 
> b/t/t9825-git-p4-handle-utf16-without-bom.sh
> index 65c3c4e..735c0bb 100644
> --- a/t/t9825-git-p4-handle-utf16-without-bom.sh
> +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
> @@ -22,8 +22,8 @@ test_expect_success 'init depot with UTF-16 encoded file 
> and artificially remove
>   cd "db" &&
>   p4d -jc &&
>   # P4D automatically adds a BOM. Remove it here to make the file 
> invalid.
> - sed -e "$ d" depot/file1,v >depot/file1,v.new &&
> - mv -- depot/file1,v.new depot/file1,v &&
> + sed -e "\$d" depot/file1,v >depot/file1,v.new &&
> + mv depot/file1,v.new depot/file1,v &&
>   printf "@$UTF16@" >>depot/file1,v &&
>   p4d -jrF checkpoint.1
>   )

This works. I even tested successfully this one:

sed \$d depot/file1,v >depot/file1,v.new &&

Do we need the “-e” option?

Thanks,
Lars


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 22 Sep 2015, at 01:54, Eric Sunshine <sunsh...@sunshineco.com> wrote:

> On Mon, Sep 21, 2015 at 7:03 PM, Lars Schneider
> <larsxschnei...@gmail.com> wrote:
>> On 21 Sep 2015, at 20:09, Junio C Hamano <gits...@pobox.com> wrote:
>>> larsxschnei...@gmail.com writes:
>>>> +test_expect_success 'init depot with UTF-16 encoded file and artificially 
>>>> remove BOM' '
>>>> +(
>>>> +cd "db" &&
>>>> +p4d -jc &&
>>>> +# P4D automatically adds a BOM. Remove it here to make the 
>>>> file invalid.
>>>> +sed -e "$ d" depot/file1,v >depot/file1,v.new &&
>>> 
>>> Do you need the space between the address $ (i.e. the last line) and
>>> operation 'd' (i.e. delete it)?  I am asking because that looks very
>>> unusual at least in our codebase.
>> 
>> Well, I am no “sed” pro. I have to admit that I found this snippet
>> on the Internet and it just worked. If I remove the space then it
>> does not work. I was not yet able to figure out why… anyone an idea?
> 
> Yes, it's because $d is a variable reference, even within double
> quotes. Typically, one uses single quotes around the sed argument to
> suppress this sort of undesired behavior. Since the entire test body
> is already within single quotes, however, changing the sed argument to
> use single quotes, rather than double, will require escaping them:
> 
>sed -e \'$d\' depot/file...
> 
> Aside: You could also drop the unnecessary quotes from the 'cd' argument.

Thanks for the explanation. Plus you are correct with the quotes around “db”… 
just a habit.

@Junio: 
If it is no extra work for you, you can remove the quotes around “db”. I can 
also create a new patch roll including the sed change and the quote change if 
it is easier for you.

Best,
Lars

--
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-p4: nicodeDecodeError in ./t9822-git-p4-path-encoding.sh

[Lars Schneider]

On 21 Sep 2015, at 09:52, Luke Diamand  wrote:

> On 21/09/15 08:01, Luke Diamand wrote:
>> Lars,
>> 
>> When I run t9822-git-p4-path-encoding.sh, the last test fails (it's
>> supposed to pass) with the following backtrace.
>> 
>> This is with 'next' at 3dd15c02a81a280c83c8d5e32c6cb71a64177ca6.
>> 
>> Any ideas as to what I'm doing wrong?
>> 
> 
> I think this is probably user error!
> 
> Luke
> 

No user error. It’s is a bug :-(
I will provide a fix shortly!

Thanks a lot,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 21 Sep 2015, at 20:09, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> A P4 repository can get into a state where it contains a file with
>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
>> 
>> More info here: http://answers.perforce.com/articles/KB/3117
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> t/t9825-git-p4-handle-utf16-without-bom.sh | 50 
>> ++
>> 1 file changed, 50 insertions(+)
>> create mode 100755 t/t9825-git-p4-handle-utf16-without-bom.sh
>> 
>> diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh
>> b/t/t9825-git-p4-handle-utf16-without-bom.sh
>> new file mode 100755
>> index 000..65c3c4e
>> --- /dev/null
>> +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh
>> @@ -0,0 +1,50 @@
>> +#!/bin/sh
>> +
>> +test_description='git p4 handling of UTF-16 files without BOM'
>> +
>> +. ./lib-git-p4.sh
>> +
>> +UTF16="\227\000\227\000"
>> +
>> +test_expect_success 'start p4d' '
>> +start_p4d
>> +'
>> +
>> +test_expect_success 'init depot with UTF-16 encoded file and artificially 
>> remove BOM' '
>> +(
>> +cd "$cli" &&
>> +printf "$UTF16" >file1 &&
>> +p4 add -t utf16 file1 &&
>> +p4 submit -d "file1"
>> +) &&
>> +
>> +(
>> +cd "db" &&
>> +p4d -jc &&
>> +# P4D automatically adds a BOM. Remove it here to make the file 
>> invalid.
>> +sed -e "$ d" depot/file1,v >depot/file1,v.new &&
> 
> Do you need the space between the address $ (i.e. the last line) and
> operation 'd' (i.e. delete it)?  I am asking because that looks very
> unusual at least in our codebase.
Well, I am no “sed” pro. I have to admit that I found this snippet on the 
Internet and it just worked. If I remove the space then it does not work. I was 
not yet able to figure out why… anyone an idea?

Thanks,
Lars



> 
>> +mv -- depot/file1,v.new depot/file1,v &&
>> +printf "@$UTF16@" >>depot/file1,v &&
>> +p4d -jrF checkpoint.1
>> +)
>> +'
>> +
>> +test_expect_failure 'clone depot with invalid UTF-16 file in verbose mode' '
>> +git p4 clone --dest="$git" --verbose //depot &&
>> +test_when_finished cleanup_git &&
>> +(
>> +cd "$git" &&
>> +printf "$UTF16" >expect &&
>> +test_cmp_bin expect file1
>> +)
>> +'
>> +
>> +test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose 
>> mode' '
>> +git p4 clone --dest="$git" //depot
>> +'
>> +
>> +test_expect_success 'kill p4d' '
>> +kill_p4d
>> +'
>> +
>> +test_done

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 21 Sep 2015, at 09:49, Luke Diamand <l...@diamand.org> wrote:

> On 20/09/15 17:22, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
> 
> When I run this, I get errors reported on the sed usage:
> 
> t9824-git-p4-handle-utf16-without-bom.sh:25: error: sed -i is not portable:   
>   sed -i.bak "$ d" depot/file1,v &&
> t9824-git-p4-handle-utf16-without-bom.sh:25: error: sed -i is not portable:   
>   sed -i.bak "$ d" depot/file1,v &&

I tried it on OS X 10.9.5 and on Ubuntu Linux 14.04.1 (sed version 4.2.2). 

The “-i” option is mentioned in the GNU sed docs here: 
https://www.gnu.org/software/sed/manual/sed.html

The OS X sed man page indeed lists “-i” as non-standard option:
https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/sed.1.html

What OS/sed version are you using?

Thanks,
Lars


> 
> 
> Luke
> 
> 
>> 
>> A P4 repository can get into a state where it contains a file with
>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
>> 
>> More info here: http://answers.perforce.com/articles/KB/3117
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  t/t9824-git-p4-handle-utf16-without-bom.sh | 49 
>> ++
>>  1 file changed, 49 insertions(+)
>>  create mode 100755 t/t9824-git-p4-handle-utf16-without-bom.sh
>> 
>> diff --git a/t/t9824-git-p4-handle-utf16-without-bom.sh 
>> b/t/t9824-git-p4-handle-utf16-without-bom.sh
>> new file mode 100755
>> index 000..517f6da
>> --- /dev/null
>> +++ b/t/t9824-git-p4-handle-utf16-without-bom.sh
>> @@ -0,0 +1,49 @@
>> +#!/bin/sh
>> +
>> +test_description='git p4 handling of UTF-16 files without BOM'
>> +
>> +. ./lib-git-p4.sh
>> +
>> +UTF16="\227\000\227\000"
>> +
>> +test_expect_success 'start p4d' '
>> +start_p4d
>> +'
>> +
>> +test_expect_success 'init depot with UTF-16 encoded file and artificially 
>> remove BOM' '
>> +(
>> +cd "$cli" &&
>> +printf "$UTF16" >file1 &&
>> +p4 add -t utf16 file1 &&
>> +p4 submit -d "file1"
>> +) &&
>> +
>> +(
>> +cd "db" &&
>> +p4d -jc &&
>> +# P4D automatically adds a BOM. Remove it here to make the file 
>> invalid.
>> +sed -i.bak "$ d" depot/file1,v &&
> 
> This line is the problem I think.
> 
> 
>> +printf "@$UTF16@" >>depot/file1,v &&
>> +p4d -jrF checkpoint.1
>> +)
>> +'
>> +
>> +test_expect_success 'clone depot with invalid UTF-16 file in verbose mode' '
>> +git p4 clone --dest="$git" --verbose //depot &&
>> +test_when_finished cleanup_git &&
>> +(
>> +cd "$git" &&
>> +printf "$UTF16" >expect &&
>> +test_cmp_bin expect file1
>> +)
>> +'
>> +
>> +test_expect_failure 'clone depot with invalid UTF-16 file in non-verbose 
>> mode' '
>> +git p4 clone --dest="$git" //depot
>> +'
>> +
>> +test_expect_success 'kill p4d' '
>> +kill_p4d
>> +'
>> +
>> +test_done

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 6/7] git-p4: add support for large file systems

[Lars Schneider]

On 16 Sep 2015, at 17:20, Junio C Hamano <gits...@pobox.com> wrote:

> Lars Schneider <larsxschnei...@gmail.com> writes:
> 
>>>> +git-p4.largeFileSystem::
>>>> +  Specify the system that is used for large (binary) files. Please note
>>>> +  that large file systems do not support the 'git p4 submit' command.
>>> 
>>> Why is that? Is it just that you haven't implemented support, or
>>> is it fundamentally impossible?
>> 
>> If we detect LFS files only by file extension then we could make
>> it work. But then we must not use any git-p4 settings. We would
>> need to rely only on the “.gitattributes” file that is stored in
>> the P4 repository. My implementation also looks at the file size
>> and decides on a individual file basis if a file is stored in
>> LFS. That means all clients need the same file size threshold.
>> 
>> Junio explained the problem in the v4 thread:
>>> ...
> 
> Hmm, I am not sure if Luke's question was answered with the above,
> and I do not think I explained anything, either.  I did point out
> that with _your_ code I didn't see how "submit" would not work, but
> that is quite different from the problem being fundamentally not
> solvable.

OK, to answer Luke’s question after some thoughts: I think it is possible but I 
haven’t implemented it.

I see one issue right away:
Some large file systems depend on gitattributes filters (e.g. Git-LFS). Git-p4 
would need to run the filters on clone/sync/submit. Right now this does not 
happen and the user sees the raw LFS pointer files in the Git repository 
instead of the content after the initial git-p4 clone (the Git-LFS test cases 
shows this). This is no problem for a git-p4 one time usage as you usually 
upload the created Git repo to some Git server. Nevertheless, I was not able to 
find a quick and easy way to fix this. Anyone an idea?

If we get these filters working then we could create a new large file system 
similar to “Git-LFS”. Instead of pointing to large files on a “Git-LFS” server 
we could point right to their P4 location. That would be pretty neat.

- Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 20 Sep 2015, at 23:16, Eric Sunshine  wrote:

> On Sun, Sep 20, 2015 at 12:22 PM,   wrote:
>> A P4 repository can get into a state where it contains a file with
>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
> 
> Hmm, are these tests going to succeed only after patch 2/2 is applied?
> If so, the order of these patches is backward since you want each
> patch to be able to stand on its own and not introduce any sort of
> breakage.
Yes, these tests succeed only after 2/2. I think I saw this approach somewhere 
in the Git history. I thought it would ease the reviewing process: show the 
problem in the first commit, fix it in a subsequent commit.
However, I understand your point as 1/2 would break the build.

What is the preferred way by the Git community? Combine patch and test in one 
commit or a patch commit followed by a test commit? I would prefer to have 
everything in one commit.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 6/7] git-p4: add support for large file systems

[Lars Schneider]

On 16 Sep 2015, at 10:36, Luke Diamand <l...@diamand.org> wrote:

> On 14/09/15 14:26, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> Perforce repositories can contain large (binary) files. Migrating these
>> repositories to Git generates very large local clones. External storage
>> systems such as Git LFS [1], Git Fat [2], Git Media [3], git-annex [4]
>> try to address this problem.
>> 
>> Add a generic mechanism to detect large files based on extension,
>> uncompressed size, and/or compressed size.
> 
> Looks excellent! I've got a small few comments (below) and I need to come 
> back and have another look through if that's OK.
Sure! Thank you :-)

> 
> Thanks!
> Luke
> 
>> 
>> [1] https://git-lfs.github.com/
>> [2] https://github.com/jedbrown/git-fat
>> [3] https://github.com/alebedev/git-media
>> [4] https://git-annex.branchable.com/
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  Documentation/git-p4.txt   |  32 +++
>>  git-p4.py  | 139 
>> +
>>  t/t9823-git-p4-mock-lfs.sh |  96 +++
>>  3 files changed, 257 insertions(+), 10 deletions(-)
>>  create mode 100755 t/t9823-git-p4-mock-lfs.sh
>> 
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 82aa5d6..3f21e95 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -510,6 +510,38 @@ git-p4.useClientSpec::
>>  option '--use-client-spec'.  See the "CLIENT SPEC" section above.
>>  This variable is a boolean, not the name of a p4 client.
>> 
>> +git-p4.largeFileSystem::
>> +Specify the system that is used for large (binary) files. Please note
>> +that large file systems do not support the 'git p4 submit' command.
> 
> Why is that? Is it just that you haven't implemented support, or is it 
> fundamentally impossible?

If we detect LFS files only by file extension then we could make it work. But 
then we must not use any git-p4 settings. We would need to rely only on the 
“.gitattributes” file that is stored in the P4 repository. My implementation 
also looks at the file size and decides on a individual file basis if a file is 
stored in LFS. That means all clients need the same file size threshold. 

Junio explained the problem in the v4 thread:
> How is collaboration between those who talk to the same p4 depot
> backed by GitHub LFS expected to work?  You use config to set size
> limits and list of file extensions in your repository, and grab new
> changes from p4 and turn them into Git commits (with pointers to LFS
> and the .gitattributes file that records your choice of the config).
> I as a new member to the same project come, clone the resulting Git
> repository from you and then what do I do before talking to the same
> p4 depot to further update the Git history?  Are the values recorded
> in .gitattributes (which essentially were derived from your
> configuration) somehow reflected back automatically to my config so
> that our world view would become consistent?  Perhaps you added
> 'iso' to largeFileExtensions before I cloned from you, and that
> would be present in the copy of .gitattributes I obtained from you.
> I may be trying to add a new ".iso" file, and I presume that an
> existing .gitattributes entry you created based on the file
> extension automatically covers it from the LFS side, but does my
> "git p4 submit" also know what to do, or would it be broken until I
> add a set of configrations that matches yours?
 


> 
>> +Only Git LFS [1] is implemented right now. Download
>> +and install the Git LFS command line extension to use this option
>> +and configure it like this:
>> ++
>> +-
>> +git config   git-p4.largeFileSystem GitLFS
>> +-
>> ++
>> +[1] https://git-lfs.github.com/
>> +
>> +git-p4.largeFileExtensions::
>> +All files matching a file extension in the list will be processed
>> +by the large file system. Do not prefix the extensions with '.'.
>> +
>> +git-p4.largeFileThreshold::
>> +All files with an uncompressed size exceeding the threshold will be
>> +processed by the large file system. By default the threshold is
>> +defined in bytes. Add the suffix k, m, or g to change the unit.
>> +
>> +git-p4.largeFileCompressedThreshold::
>> +All files with a compressed size exceeding the threshold will be
>> +processed by the large file system. This option might slow d

Re: [PATCH v2 2/2] git-p4: handle "Translation of file content failed"

[Lars Schneider]

On 16 Sep 2015, at 00:12, Luke Diamand <l...@diamand.org> wrote:

> On 15/09/15 16:38, Lars Schneider wrote:
>> 
>> On 15 Sep 2015, at 08:43, Luke Diamand <l...@diamand.org> wrote:
>> 
> 
> 
>>> Do we know the mechanism by which we end up in this state?
>> Unfortunately no. I tried hard to reproduce the error with “conventional” 
>> methods. As you can see I ended up manipulating the P4 database…
>> 
>> However, it looks like this error happens in the wild, too:
>> https://stackoverflow.com/questions/5156909/translation-of-file-content-failed-error-in-perforce
>> https://stackoverflow.com/questions/887006/perforce-translation-of-file-content-failed-error
> 
> It's described in the Perforce FAQ here:
> 
> http://answers.perforce.com/articles/KB/3117
> 
> i.e. it looks to be caused by mixing old and new P4 clients.
Good find! No idea why I did not find this article before… I will copy the text 
from the KB into the git commit message to explain the problem.
 
> 
>>>> 
>>>> Known issue: This works only if git-p4 is executed in verbose mode.
>>>> In normal mode no exceptions are thrown and git-p4 just exits.
>>> 
>>> Does that mean that the error will only be detected in verbose mode? That 
>>> doesn't seem right!
>> Correct. I don’t like this either but I also don’t want to make huge changes 
>> to git-p4.
>> You can see the root problem here:
>> https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L110-L114
>> 
>> Any idea how to approach that best?
> 
> I guess what we have is not ideal but probably good enough.
ok. thanks!

I will add another test case without “—verbose" to document that there is work 
to do :-)

> 
> 
>>>> +try:
>>>> +text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % 
>>>> (file['depotFile'], file['change'])])
>>>> +except Exception as e:
>>> 
>>> Would it be better to specify which kind of Exception you are catching? 
>>> Looks like you could get OSError, ValueError and CalledProcessError; it's 
>>> the last of these you want (I think).
>> I think it is just a plain exception. See here:
>> https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L111
> 
> OK, you're right (probably less than ideal behaviour from read_pipe() and 
> die() but let's not try to fix that).
ok

> 
> 
>>>> +if p4_version_string().find('/NT') >= 0:
>>>> +text = text.replace('\r\n', '\n')
>>>> +contents = [ text ]
>>> 
>>> The indentation on this bit doesn't look right to me.
>> I believe it is exactly how it was:
>> https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L2397-L2399
> 
> OK.
> 
>> 
>> 
>> In general, what is the appropriate way to reference code in this email 
>> list? Are GitHub links OK?
> 
> I'm not an expert, but it feels possibly a bit ephemeral - if someone is 
> digging through email archives in a future where that github project has been 
> moved elsewhere, the links will all be dead.

Right. However, you could disassemble the URL and use the commit hash, the 
filename and the line number. They are not ephemeral because they are part of 
the repo.

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] git-p4: handle "Translation of file content failed"

[Lars Schneider]

On 15 Sep 2015, at 08:43, Luke Diamand <l...@diamand.org> wrote:

> On 14/09/15 17:55, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> A P4 repository can get into a state where it contains a file with
>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
> 
> Sorry - what's a BOM? I'm assuming it's not a Bill of Materials?
BOM stands for Byte Order Mark. The UTF-16 BOM is a two byte sequence at the 
beginning of a UTF-16 file. It is not part of the actual content. It is only 
used to define the encoding of the remaining file. FEFF stands for UTF-16 
big-endian encoding and FFFE for little-endian encoding.

More info here: http://www.unicode.org/faq/utf_bom.html#bom1


> Do we know the mechanism by which we end up in this state?
Unfortunately no. I tried hard to reproduce the error with “conventional” 
methods. As you can see I ended up manipulating the P4 database…

However, it looks like this error happens in the wild, too:
https://stackoverflow.com/questions/5156909/translation-of-file-content-failed-error-in-perforce
https://stackoverflow.com/questions/887006/perforce-translation-of-file-content-failed-error


>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
>> 
>> Fix this by detecting this error and retrieving the file as binary
>> instead. The result in Git is the same.
>> 
>> Known issue: This works only if git-p4 is executed in verbose mode.
>> In normal mode no exceptions are thrown and git-p4 just exits.
> 
> Does that mean that the error will only be detected in verbose mode? That 
> doesn't seem right!
Correct. I don’t like this either but I also don’t want to make huge changes to 
git-p4.
You can see the root problem here:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L110-L114

Any idea how to approach that best?


> 
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  git-p4.py | 27 ---
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 073f87b..5ae25a6 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -134,13 +134,11 @@ def read_pipe(c, ignore_error=False):
>>  sys.stderr.write('Reading pipe: %s\n' % str(c))
>> 
>>  expand = isinstance(c,basestring)
>> -p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
>> -pipe = p.stdout
>> -val = pipe.read()
>> -if p.wait() and not ignore_error:
>> -die('Command failed: %s' % str(c))
>> -
>> -return val
>> +p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, 
>> shell=expand)
>> +(out, err) = p.communicate()
>> +if p.returncode != 0 and not ignore_error:
>> +die('Command failed: %s\nError: %s' % (str(c), err))
>> +return out
>> 
>>  def p4_read_pipe(c, ignore_error=False):
>>  real_cmd = p4_build_cmd(c)
>> @@ -2186,10 +2184,17 @@ class P4Sync(Command, P4UserMap):
>>  # them back too.  This is not needed to the cygwin windows 
>> version,
>>  # just the native "NT" type.
>>  #
>> -text = p4_read_pipe(['print', '-q', '-o', '-', "%s@%s" % 
>> (file['depotFile'], file['change']) ])
>> -if p4_version_string().find("/NT") >= 0:
>> -text = text.replace("\r\n", "\n")
>> -contents = [ text ]
>> +try:
>> +text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % 
>> (file['depotFile'], file['change'])])
>> +except Exception as e:
> 
> Would it be better to specify which kind of Exception you are catching? Looks 
> like you could get OSError, ValueError and CalledProcessError; it's the last 
> of these you want (I think).
I think it is just a plain exception. See here:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L111


> 
>> +if 'Translation of file content failed' in str(e):
>> +type_base = 'binary'
>> +else:
>> +raise e
>> +else:
>> +if p4_version_string().find('/NT') >= 0:
>> +text = text.replace('\r\n', '\n')
>> +contents = [ text ]
> 
> The indentation on this bit doesn't look right to me.
I believe it is exactly how it was:
https://github.com/git/git/blob/97d7ad75b6fe74960d2a12e4a9151a55a5a87d6d/git-p4.py#L2397-L2399


In general, what is the appropriate way to reference code in this email list? 
Are GitHub links OK?


Thanks,
Lars

--
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] git-p4: improve path encoding verbose output

[Lars Schneider]

On 15 Sep 2015, at 09:31, Luke Diamand <l...@diamand.org> wrote:

> On 14/09/15 18:10, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> If a path with non-ASCII characters is detected then print always the
> 
> s/print always/print/
I will fix it.

> 
> 
>> encoding and the encoded string in verbose mode.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  git-p4.py | 19 +--
>>  1 file changed, 9 insertions(+), 10 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index d45cf2b..da25d3f 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):
>>  text = regexp.sub(r'$\1$', text)
>>  contents = [ text ]
>> 
>> -if gitConfig("git-p4.pathEncoding"):
>> -relPath = 
>> relPath.decode(gitConfig("git-p4.pathEncoding")).encode('utf8', 'replace')
>> -elif self.verbose:
>> -try:
>> -relPath.decode('ascii')
>> -except:
>> -print (
>> -"Path with Non-ASCII characters detected and no path 
>> encoding defined. "
>> -"Please check the encoding: %s" % relPath
>> -)
>> +try:
>> +relPath.decode('ascii')
>> +except:
>> +encoding = 'utf8'
>> +if gitConfig('git-p4.pathEncoding'):
>> +encoding = gitConfig('git-p4.pathEncoding')
> 
> It would be better to query this once at startup. Otherwise we're potentially 
> forking "git config" twice per file which on a large repo could become 
> significant. Make it an instance variable perhaps?
solved in other email

> 
>> +relPath = relPath.decode(encoding).encode('utf8', 'replace')
>> +if self.verbose:
>> +print 'Path with non-ASCII characters detected. Used %s to 
>> encode: %s ' % (encoding, relPath)
>> 
>>  self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))

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] git-p4: improve path encoding verbose output

[Lars Schneider]

On 14 Sep 2015, at 20:40, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> If a path with non-ASCII characters is detected then print always the
>> encoding and the encoded string in verbose mode.
> 
> Earlier if the user tells us that s/he knows what she is doing
> by setting the configuration, we just followed the instruction
> without complaining or notifying.  The differences in this version
> are
> 
> (1) if the path is in ASCII, the configuration is not even
> consulted, and we didn't do any path munging.
Correct!


> (2) for a non-ASCII path, even if the user tells us that s/he knows
> what she is doing, we notify what we did under "--verbose"
> mode.
Correct!


> I think (1) is a definite improvement, but it is not immediately
> obvious why (2) is an improvement.  It is clearly a good thing to
> let the user know when we munged the path without being told, but
> when the configuration is given, it can be argued both ways.  It may
> be a good thing to reassure that the configuration is kicking in, or
> it may be a needless noise to tell the user that we did what we were
> told to do.
I get your point. However, changing file names in a repository is a pretty 
significant action and therefore I would prefer to explicitly tell the user 
about it. Some encodings differ only slightly I would like to have an easy way 
to look at all the changed paths to ensure I picked the right encoding (e.g. 
grep “Path with non-ASCII characters detected”). I also assume the user is OK 
with noise since s/he enabled “verbose” mode :-)


> In any case, I suspectq that the call to decode-encode to munge
> relPath is indented one level too deep in this patch.  You would
> want to use the configured value if exists and utf8 if there is no
> configuration, but in either case you would want to munge relPath
> when it does not decode as ASCII, no?
Good catch! It works with the indented code too because UTF8 is the default 
encoding for relPath later on. However, with your suggestion the code is more 
explicit. I will change it in the next roll

Thanks!

> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> git-p4.py | 19 +--
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index d45cf2b..da25d3f 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -2220,16 +2220,15 @@ class P4Sync(Command, P4UserMap):
>> text = regexp.sub(r'$\1$', text)
>> contents = [ text ]
>> 
>> -if gitConfig("git-p4.pathEncoding"):
>> -relPath = 
>> relPath.decode(gitConfig("git-p4.pathEncoding")).encode('utf8', 'replace')
>> -elif self.verbose:
>> -try:
>> -relPath.decode('ascii')
>> -except:
>> -print (
>> -"Path with Non-ASCII characters detected and no path 
>> encoding defined. "
>> -"Please check the encoding: %s" % relPath
>> -)
>> +try:
>> +relPath.decode('ascii')
>> +except:
>> +encoding = 'utf8'
>> +if gitConfig('git-p4.pathEncoding'):
>> +encoding = gitConfig('git-p4.pathEncoding')
>> +relPath = relPath.decode(encoding).encode('utf8', 'replace')
>> +if self.verbose:
>> +print 'Path with non-ASCII characters detected. Used %s to 
>> encode: %s ' % (encoding, relPath)
>> 
>> self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))

--
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/2] git-p4: add test case for "Translation of file content failed" error

[Lars Schneider]

On 15 Sep 2015, at 06:40, Torsten Bögershausen <tbo...@web.de> wrote:

> On 09/14/2015 06:55 PM, larsxschnei...@gmail.com wrote:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> A P4 repository can get into a state where it contains a file with
>> type UTF-16 that does not contain a valid UTF-16 BOM. If git-p4
>> attempts to retrieve the file then the process crashes with a
>> "Translation of file content failed" error.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>>  t/t9824-git-p4-handle-utf16-without-bom.sh | 47 
>> ++
>>  1 file changed, 47 insertions(+)
>>  create mode 100755 t/t9824-git-p4-handle-utf16-without-bom.sh
>> 
>> diff --git a/t/t9824-git-p4-handle-utf16-without-bom.sh 
>> b/t/t9824-git-p4-handle-utf16-without-bom.sh
>> new file mode 100755
>> index 000..fa8043b
>> --- /dev/null
>> +++ b/t/t9824-git-p4-handle-utf16-without-bom.sh
>> @@ -0,0 +1,47 @@
>> +#!/bin/sh
>> +
>> +test_description='git p4 handle UTF-16 without BOM'
>> +
>> +. ./lib-git-p4.sh
>> +
>> +UTF16="\\x97\\x0\\x97\\x0"
>> +
>> +test_expect_success 'start p4d' '
>> +start_p4d
>> +'
>> +
>> +test_expect_success 'init depot with UTF-16 encoded file and artificially 
>> remove BOM' '
>> +(
>> +cd "$cli" &&
>> +echo "file1 -text" > .gitattributes &&
> Please no space between '>' and the filename,
> (this is our coding standard, and the same further down)
Correct! Sorry, I still need to get used to this style. Thanks for the reminder!

> 
>> +perl -e "printf \"$UTF16\"" >file1 &&
> Ehh, do we need perl here ?
> This will invoke a process-fork, which costs time and cpu load.
> The following works for me:
> printf '\227\000\227\000' >file1
I agree this is better.

Both issues will be fixed v3.

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: [PATCHv1 0/2] git-p4: work on a detached head

[Lars Schneider]

I wanted to play with the patch and apply it to my source but the process is 
really complicated for me. I wonder if you can give me a few recommendations 
how to work efficiently with email patches. I don’t want to start a flame-war 
about what email client is “right", I am just curious how you work and what 
clients, scripts, or tricks you use :-)

Usually I use Apple Mail. I experimented with mutt but it was not dramatically 
more convenient.

Thanks,
Lars

On 05 Sep 2015, at 16:02, Luke Diamand  wrote:

> git-p4 won't submit from a detached head. If you do, it gives a
> cryptic error message, and instead you have to create an endless
> series of throw-away branches, which can get very trying.
> 
> The first patch in this series demonstrates the problem, and the
> second patch fixes it. I've been using it for the last few days.
> 
> Luke Diamand (2):
>  git-p4: add failing test for submit from detached head
>  git-p4: work with a detached head
> 
> git-p4.py   | 18 --
> t/t9800-git-p4-basic.sh | 16 
> 2 files changed, 28 insertions(+), 6 deletions(-)
> 
> -- 
> 2.6.0.rc0.133.ga438a11.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
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 1/2] git-p4: print stderr if P4 read_pipe operation fails

[Lars Schneider]

On 09 Sep 2015, at 18:00, Junio C Hamano <gits...@pobox.com> wrote:

> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> 
>> If read_pipe crashes then the caller can inspect the error and handle
>> it appropriately.
>> 
>> Signed-off-by: Lars Schneider <larsxschnei...@gmail.com>
>> ---
>> git-p4.py | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 073f87b..36a4bcb 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -134,11 +134,11 @@ def read_pipe(c, ignore_error=False):
>> sys.stderr.write('Reading pipe: %s\n' % str(c))
>> 
>> expand = isinstance(c,basestring)
>> -p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
>> +p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, 
>> shell=expand)
>> pipe = p.stdout
>> val = pipe.read()
>> if p.wait() and not ignore_error:
>> -die('Command failed: %s' % str(c))
>> +die('Command failed: %s\nError: %s' % (str(c), p.stderr.read()))
> 
> I don't know enough about the callers of this helper function to
> tell offhand if that is an issue, but this looks unsafe depending on
> what the process on the other side of these pipes are doing.
> 
> If it attempts to spew a lot on its standard error stream first and
> then write some to its standard output, I would not be surprised it
> would get stuck waiting for us to read and drain its standard error
> before it can proceed to write to its standard output, and in the
> meantime we would be waiting for it to say something on its standard
> output, no?
> 
You are right. I will use the “communicate” function here as recommended in the 
Python docs:
https://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait

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