Re: none

2019-03-03 Thread Junio C Hamano
Rohit Ashiwal  writes:

> Just to be clear of what caused the error:
>   1. Path not being file, or
>   2. File not being empty
> I am checking for both.

test -s  makes sure  is file; if it is not a file, then
it won't yield true.

So why do you need to say test_path_is_file yourself, if you are
asking "test -s"?


Re: none

2018-10-10 Thread Junio C Hamano
Mihir Mehta  writes:

> Thanks, Junio. Instead of removing that part of the patch, I opted to
> expand it to make it a little clearer (in my opinion) than it was
> before. Let me know if this works.

I am mildly negative on that change.  "Omitting both would give an
empty diff" would be understandable to anybody who understands that
an omitted end of dot-dot is substituted with HEAD *and* thinks what
range HEAD..HEAD means, so it is just an additional noise to them,
and to those who do not want to waste time on thinking, it is a
statement that reads as if "it will be an error" without saying why
it is an error.  So overall, it seems, at least to me, that the
additional text adds negative value.

So, I dunno.


[PATCH v2 09/11] builtin rebase: start a new rebase only if none is in progress

2018-09-04 Thread Pratik Karki via GitGitGadget
From: Pratik Karki 

To run a new rebase, there needs to be a check to assure that no other
rebase is in progress. New rebase operation cannot start until an
ongoing rebase operation completes or is terminated.

Signed-off-by: Pratik Karki 
Signed-off-by: Johannes Schindelin 
---
 builtin/rebase.c | 48 +++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8a7bf3d468..d45f8f9008 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -87,6 +87,7 @@ struct rebase_options {
REBASE_VERBOSE = 1<<1,
REBASE_DIFFSTAT = 1<<2,
REBASE_FORCE = 1<<3,
+   REBASE_INTERACTIVE_EXPLICIT = 1<<4,
} flags;
struct strbuf git_am_opt;
 };
@@ -392,10 +393,11 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
.git_am_opt = STRBUF_INIT,
};
const char *branch_name;
-   int ret, flags;
+   int ret, flags, in_progress = 0;
int ok_to_skip_pre_rebase = 0;
struct strbuf msg = STRBUF_INIT;
struct strbuf revisions = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
struct object_id merge_base;
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
@@ -447,6 +449,30 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
 
git_config(rebase_config, &options);
 
+   if (is_directory(apply_dir())) {
+   options.type = REBASE_AM;
+   options.state_dir = apply_dir();
+   } else if (is_directory(merge_dir())) {
+   strbuf_reset(&buf);
+   strbuf_addf(&buf, "%s/rewritten", merge_dir());
+   if (is_directory(buf.buf)) {
+   options.type = REBASE_PRESERVE_MERGES;
+   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
+   } else {
+   strbuf_reset(&buf);
+   strbuf_addf(&buf, "%s/interactive", merge_dir());
+   if(file_exists(buf.buf)) {
+   options.type = REBASE_INTERACTIVE;
+   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
+   } else
+   options.type = REBASE_MERGE;
+   }
+   options.state_dir = merge_dir();
+   }
+
+   if (options.type != REBASE_UNSPECIFIED)
+   in_progress = 1;
+
argc = parse_options(argc, argv, prefix,
 builtin_rebase_options,
 builtin_rebase_usage, 0);
@@ -455,6 +481,26 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_rebase_usage,
   builtin_rebase_options);
 
+   /* Make sure no rebase is in progress */
+   if (in_progress) {
+   const char *last_slash = strrchr(options.state_dir, '/');
+   const char *state_dir_base =
+   last_slash ? last_slash + 1 : options.state_dir;
+   const char *cmd_live_rebase =
+   "git rebase (--continue | --abort | --skip)";
+   strbuf_reset(&buf);
+   strbuf_addf(&buf, "rm -fr \"%s\"", options.state_dir);
+   die(_("It seems that there is already a %s directory, and\n"
+ "I wonder if you are in the middle of another rebase.  "
+ "If that is the\n"
+ "case, please try\n\t%s\n"
+ "If that is not the case, please\n\t%s\n"
+ "and run me again.  I am stopping in case you still "
+ "have something\n"
+ "valuable there.\n"),
+   state_dir_base, cmd_live_rebase, buf.buf);
+   }
+
if (!(options.flags & REBASE_NO_QUIET))
strbuf_addstr(&options.git_am_opt, " -q");
 
-- 
gitgitgadget



Re: [PATCH 09/11] builtin rebase: start a new rebase only if none is in progress

2018-08-24 Thread Johannes Schindelin
Hi Stefan,

On Wed, 8 Aug 2018, Stefan Beller wrote:

> On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki  wrote:
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 8a7bf3d468..a261f552f1 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -455,6 +481,26 @@ int cmd_rebase(int argc, const char **argv, const char 
> > *prefix)
> > usage_with_options(builtin_rebase_usage,
> >builtin_rebase_options);
> >
> > +   /* Make sure no rebase is in progress */
> 
> The faithful conversion doesn't even stop at the comments. ;-)

Yes, I insisted on it.

TBH it is a bit of a shame that we cannot fix all those error messages
going to stdout, but... you know... One step after the other.

> I shortly wondered if this is the best place for this comment,
> but let's just keep it here to have the 1:1 rewrite.

It should probably be inside the conditional block, but as you say: the
original had it in a funny spot, and so does the converted code.

> > +   if (in_progress) {
> [...]
> > +   state_dir_base, cmd_live_rebase,buf.buf);
> 
> In case a resend is needed, add a whitespace after the
> comma and buf.buf, please.

I will fix this before sending the next iteration.

Thanks for the review!
Dscho


Re: [PATCH 09/11] builtin rebase: start a new rebase only if none is in progress

2018-08-08 Thread Stefan Beller
On Wed, Aug 8, 2018 at 6:51 AM Pratik Karki  wrote:
>
> To run a new rebase, there needs to be a check to assure that no other
> rebase is in progress. New rebase operation cannot start until an
> ongoing rebase operation completes or is terminated.
>
> Signed-off-by: Pratik Karki 
> ---
>  builtin/rebase.c | 48 +++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 8a7bf3d468..a261f552f1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -87,6 +87,7 @@ struct rebase_options {
> REBASE_VERBOSE = 1<<1,
> REBASE_DIFFSTAT = 1<<2,
> REBASE_FORCE = 1<<3,
> +   REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> } flags;
> struct strbuf git_am_opt;
>  };
> @@ -392,10 +393,11 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
> .git_am_opt = STRBUF_INIT,
> };
> const char *branch_name;
> -   int ret, flags;
> +   int ret, flags, in_progress = 0;
> int ok_to_skip_pre_rebase = 0;
> struct strbuf msg = STRBUF_INIT;
> struct strbuf revisions = STRBUF_INIT;
> +   struct strbuf buf = STRBUF_INIT;
> struct object_id merge_base;
> struct option builtin_rebase_options[] = {
> OPT_STRING(0, "onto", &options.onto_name,
> @@ -447,6 +449,30 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
>
> git_config(rebase_config, &options);
>
> +   if (is_directory(apply_dir())) {
> +   options.type = REBASE_AM;
> +   options.state_dir = apply_dir();
> +   } else if (is_directory(merge_dir())) {
> +   strbuf_reset(&buf);
> +   strbuf_addf(&buf, "%s/rewritten", merge_dir());
> +   if (is_directory(buf.buf)) {
> +   options.type = REBASE_PRESERVE_MERGES;
> +   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
> +   } else {
> +   strbuf_reset(&buf);
> +   strbuf_addf(&buf, "%s/interactive", merge_dir());
> +   if(file_exists(buf.buf)) {
> +   options.type = REBASE_INTERACTIVE;
> +   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
> +   } else
> +   options.type = REBASE_MERGE;
> +   }
> +   options.state_dir = merge_dir();
> +   }
> +
> +   if (options.type != REBASE_UNSPECIFIED)
> +   in_progress = 1;
> +
> argc = parse_options(argc, argv, prefix,
>  builtin_rebase_options,
>  builtin_rebase_usage, 0);
> @@ -455,6 +481,26 @@ int cmd_rebase(int argc, const char **argv, const char 
> *prefix)
> usage_with_options(builtin_rebase_usage,
>builtin_rebase_options);
>
> +   /* Make sure no rebase is in progress */

The faithful conversion doesn't even stop at the comments. ;-)
I shortly wondered if this is the best place for this comment,
but let's just keep it here to have the 1:1 rewrite.


> +   if (in_progress) {
[...]
> +   state_dir_base, cmd_live_rebase,buf.buf);

In case a resend is needed, add a whitespace after the
comma and buf.buf, please.

So far I have not seen anything major that would warrant a resend.

Thanks,
Stefan


[PATCH 09/11] builtin rebase: start a new rebase only if none is in progress

2018-08-08 Thread Pratik Karki
To run a new rebase, there needs to be a check to assure that no other
rebase is in progress. New rebase operation cannot start until an
ongoing rebase operation completes or is terminated.

Signed-off-by: Pratik Karki 
---
 builtin/rebase.c | 48 +++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8a7bf3d468..a261f552f1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -87,6 +87,7 @@ struct rebase_options {
REBASE_VERBOSE = 1<<1,
REBASE_DIFFSTAT = 1<<2,
REBASE_FORCE = 1<<3,
+   REBASE_INTERACTIVE_EXPLICIT = 1<<4,
} flags;
struct strbuf git_am_opt;
 };
@@ -392,10 +393,11 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
.git_am_opt = STRBUF_INIT,
};
const char *branch_name;
-   int ret, flags;
+   int ret, flags, in_progress = 0;
int ok_to_skip_pre_rebase = 0;
struct strbuf msg = STRBUF_INIT;
struct strbuf revisions = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
struct object_id merge_base;
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
@@ -447,6 +449,30 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
 
git_config(rebase_config, &options);
 
+   if (is_directory(apply_dir())) {
+   options.type = REBASE_AM;
+   options.state_dir = apply_dir();
+   } else if (is_directory(merge_dir())) {
+   strbuf_reset(&buf);
+   strbuf_addf(&buf, "%s/rewritten", merge_dir());
+   if (is_directory(buf.buf)) {
+   options.type = REBASE_PRESERVE_MERGES;
+   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
+   } else {
+   strbuf_reset(&buf);
+   strbuf_addf(&buf, "%s/interactive", merge_dir());
+   if(file_exists(buf.buf)) {
+   options.type = REBASE_INTERACTIVE;
+   options.flags |= REBASE_INTERACTIVE_EXPLICIT;
+   } else
+   options.type = REBASE_MERGE;
+   }
+   options.state_dir = merge_dir();
+   }
+
+   if (options.type != REBASE_UNSPECIFIED)
+   in_progress = 1;
+
argc = parse_options(argc, argv, prefix,
 builtin_rebase_options,
 builtin_rebase_usage, 0);
@@ -455,6 +481,26 @@ int cmd_rebase(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_rebase_usage,
   builtin_rebase_options);
 
+   /* Make sure no rebase is in progress */
+   if (in_progress) {
+   const char *last_slash = strrchr(options.state_dir, '/');
+   const char *state_dir_base =
+   last_slash ? last_slash + 1 : options.state_dir;
+   const char *cmd_live_rebase =
+   "git rebase (--continue | --abort | --skip)";
+   strbuf_reset(&buf);
+   strbuf_addf(&buf, "rm -fr \"%s\"", options.state_dir);
+   die(_("It seems that there is already a %s directory, and\n"
+ "I wonder if you are in the middle of another rebase.  "
+ "If that is the\n"
+ "case, please try\n\t%s\n"
+ "If that is not the case, please\n\t%s\n"
+ "and run me again.  I am stopping in case you still "
+ "have something\n"
+ "valuable there.\n"),
+   state_dir_base, cmd_live_rebase,buf.buf);
+   }
+
if (!(options.flags & REBASE_NO_QUIET))
strbuf_addstr(&options.git_am_opt, " -q");
 
-- 
2.18.0



Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-23 Thread Mazo, Andrey
>> This was actually discussed in a separate thread [1] some time ago with 
>> patches proposed by Thandesha and me.
>> I haven't yet got time to cook a final patch, which addresses both 
>> Thandesha's and mine use-cases though,
>> so this wasn't submitted to Junio yet.
>> In the meantime, I guess, one of the patches [2] from that thread can be 
>> taken as is.
>>
>> [1] "[BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key"
>>    
>>https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#mee2ec50a40242089741f808f06214a44278055b3
>> [2] "[PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'"
>>    
>>https://public-inbox.org/git/2e2b2add4e4fffa4228b8ab9f6cd47fa9bf25207.1523981210.git.am...@checkvideo.com/
>
> Should I re-roll my patch without this change then?
If it's the question to me, then I'm fine either way -- I can rebase my changes 
easily.
However, re-rolling your patch would probably make the aforementioned fileSize 
patch apply cleanly to both maint and master branches.


Thank you,
Andrey Mazo

Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-23 Thread Luke Diamand
On 23 May 2018 at 17:41, Mazo, Andrey  wrote:
>> The last one (i.e. "even if it is verbose, if fileSize is not
>> reported, do not write the verbose output") does not look like it is
>> limited to the unshelve feature, so it might, even though it is a
>> one-liner, deserve to be a separate preparatory patch if you want.
>> But I do not feel strongly about either way.
>
> This was actually discussed in a separate thread [1] some time ago with 
> patches proposed by Thandesha and me.
> I haven't yet got time to cook a final patch, which addresses both 
> Thandesha's and mine use-cases though,
> so this wasn't submitted to Junio yet.
> In the meantime, I guess, one of the patches [2] from that thread can be 
> taken as is.
>
> [1] "[BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key"
>   
> https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#mee2ec50a40242089741f808f06214a44278055b3
> [2] "[PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'"
>   
> https://public-inbox.org/git/2e2b2add4e4fffa4228b8ab9f6cd47fa9bf25207.1523981210.git.am...@checkvideo.com/

Should I re-roll my patch without this change then?

Luke


Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-23 Thread Mazo, Andrey
> The last one (i.e. "even if it is verbose, if fileSize is not
> reported, do not write the verbose output") does not look like it is
> limited to the unshelve feature, so it might, even though it is a
> one-liner, deserve to be a separate preparatory patch if you want.
> But I do not feel strongly about either way.

This was actually discussed in a separate thread [1] some time ago with patches 
proposed by Thandesha and me.
I haven't yet got time to cook a final patch, which addresses both Thandesha's 
and mine use-cases though,
so this wasn't submitted to Junio yet.
In the meantime, I guess, one of the patches [2] from that thread can be taken 
as is.

[1] "[BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key"
  
https://public-inbox.org/git/cajjpmi-plb4qcka5alkxa8b1vozfff+oaq0fguq9yviobrp...@mail.gmail.com/t/#mee2ec50a40242089741f808f06214a44278055b3
[2] "[PATCH 1/1] git-p4: fix `sync --verbose` traceback due to 'fileSize'"
  
https://public-inbox.org/git/2e2b2add4e4fffa4228b8ab9f6cd47fa9bf25207.1523981210.git.am...@checkvideo.com/

Thank you,
Andrey Mazo

Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-22 Thread Junio C Hamano
Luke Diamand  writes:

>> However, instead of a separate patch, wouldn't it be better to squash
>> it into the previous one?  So 'make test' would succeed on every
>> commit even with a newer p4 version.
>
> Junio?
>
> I can squash together the original commit and the two fixes if that
> would be better?

Among the three hunks in this fix-up patch, the first two are
strictly fixing what you had in the previous patch, so it make sense
to fix them at the source by squashing.

The last one (i.e. "even if it is verbose, if fileSize is not
reported, do not write the verbose output") does not look like it is
limited to the unshelve feature, so it might, even though it is a
one-liner, deserve to be a separate preparatory patch if you want.
But I do not feel strongly about either way.

Thanks.


Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-22 Thread Luke Diamand
On 22 May 2018 at 11:15, SZEDER Gábor  wrote:
> On Tue, May 22, 2018 at 10:41 AM, Luke Diamand  wrote:
>> SZEDER Gábor found that the unshelve tests fail with newer
>> versions of Perforce (2016 vs 2015).
>>
>> The problem arises because when a file is added in a P4
>> shelved changelist, the depot revision is shown as "none"
>> if using the older p4d (which makes sense - the file doesn't
>> yet exist, so can't have a revision), but as "1" in the newer
>> versions of p4d.
>>
>> For example, adding a file called "new" with 2015.1 and then
>> shelving that change gives this from "p4 describe" :
>>
>> ... //depot/new#none add
>>
>> Using the 2018.1 server gives this:
>>
>> ... //depot/new#1 add
>>
>> We can detect that a file has been added simply by using the
>> file status ("add") instead, rather than the depot revision,
>> which is what this change does.
>>
>> This also fixes a few verbose prints used for debugging this
>> to be more friendly.
>>
>> Signed-off-by: Luke Diamand 
>
> For what it's worth, I can confirm that 't9832-unshelve.sh' passes
> with these changes, here and in all Linux and OSX build jobs on Travis
> CI.

Thanks!

>
> However, instead of a separate patch, wouldn't it be better to squash
> it into the previous one?  So 'make test' would succeed on every
> commit even with a newer p4 version.

Junio?

I can squash together the original commit and the two fixes if that
would be better?

Thanks!
Luke


Re: [PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-22 Thread SZEDER Gábor
On Tue, May 22, 2018 at 10:41 AM, Luke Diamand  wrote:
> SZEDER Gábor found that the unshelve tests fail with newer
> versions of Perforce (2016 vs 2015).
>
> The problem arises because when a file is added in a P4
> shelved changelist, the depot revision is shown as "none"
> if using the older p4d (which makes sense - the file doesn't
> yet exist, so can't have a revision), but as "1" in the newer
> versions of p4d.
>
> For example, adding a file called "new" with 2015.1 and then
> shelving that change gives this from "p4 describe" :
>
> ... //depot/new#none add
>
> Using the 2018.1 server gives this:
>
> ... //depot/new#1 add
>
> We can detect that a file has been added simply by using the
> file status ("add") instead, rather than the depot revision,
> which is what this change does.
>
> This also fixes a few verbose prints used for debugging this
> to be more friendly.
>
> Signed-off-by: Luke Diamand 

For what it's worth, I can confirm that 't9832-unshelve.sh' passes
with these changes, here and in all Linux and OSX build jobs on Travis
CI.

However, instead of a separate patch, wouldn't it be better to squash
it into the previous one?  So 'make test' would succeed on every
commit even with a newer p4 version.


[PATCH 1/1] git-p4: unshelve: use action==add instead of rev==none

2018-05-22 Thread Luke Diamand
SZEDER Gábor found that the unshelve tests fail with newer
versions of Perforce (2016 vs 2015).

The problem arises because when a file is added in a P4
shelved changelist, the depot revision is shown as "none"
if using the older p4d (which makes sense - the file doesn't
yet exist, so can't have a revision), but as "1" in the newer
versions of p4d.

For example, adding a file called "new" with 2015.1 and then
shelving that change gives this from "p4 describe" :

... //depot/new#none add

Using the 2018.1 server gives this:

... //depot/new#1 add

We can detect that a file has been added simply by using the
file status ("add") instead, rather than the depot revision,
which is what this change does.

This also fixes a few verbose prints used for debugging this
to be more friendly.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 364d86dbcc..c80d85af89 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2463,7 +2463,7 @@ class P4Sync(Command, P4UserMap):
 """
 ret = p4Cmd(["diff2", "{0}#{1}".format(path, filerev), 
"{0}@{1}".format(path, revision)])
 if verbose:
-print("p4 diff2 %s %s %s => %s" % (path, filerev, revision, ret))
+print("p4 diff2 path %s filerev %s revision %s => %s" % (path, 
filerev, revision, ret))
 return ret["status"] == "identical"
 
 def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0, 
origin_revision = 0):
@@ -2492,7 +2492,12 @@ class P4Sync(Command, P4UserMap):
 if shelved:
 file["shelved_cl"] = int(shelved_cl)
 
-if file["rev"] != "none" and \
+# For shelved changelists, check that the revision of each 
file that the
+# shelve was based on matches the revision that we are using 
for the
+# starting point for git-fast-import (self.initialParent). 
Otherwise
+# the resulting diff will contain deltas from multiple commits.
+
+if file["action"] != "add" and \
 not self.cmp_shelved(path, file["rev"], origin_revision):
 sys.exit("change {0} not based on {1} for {2}, cannot 
unshelve".format(
 commit["change"], self.initialParent, path))
@@ -2610,7 +2615,7 @@ class P4Sync(Command, P4UserMap):
 def streamOneP4File(self, file, contents):
 relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
 relPath = self.encodeWithUTF8(relPath)
-if verbose:
+if verbose and 'fileSize' in self.stream_file:
 size = int(self.stream_file['fileSize'])
 sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], 
relPath, size/1024/1024))
 sys.stdout.flush()
-- 
2.17.0.392.gdeb1a6e9b7



Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-05 Thread Ævar Arnfjörð Bjarmason

On Sun, Feb 04 2018, Lucas Werkmeister jotted:

> On 04.02.2018 19:55, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Sun, Feb 04 2018, Lucas Werkmeister jotted:
>> 
>>>  [--inetd |
>>>   [--listen=] [--port=]
>>>   [--user= [--group=]]]
>>> +[--log-destination=(stderr|syslog|none)]
>> 
>> I micronit, but maybe worthwhile to have a preceeding commit to fix up
>> that indentation of --listen and --user.
>
> I thought the indentation here is intentional, since we’re still inside
> the [] pair (either --inetd or --listen, --port, …).

Yes, sorry. Nevrmind.

>> 
>>> +--log-destination=::
>>> +   Send log messages to the specified destination.
>>> +   Note that this option does not imply --verbose,
>> 
>> Should `` quote --verbose, although I see similar to the WS change I
>> noted above there's plenty of existing stuff in that doc doing it wrong.
>
> I could send a follow-up to consistently ``-quote all options in
> git-daemon.txt… or would that be rejected as unnecessarily cluttering
> the history or the `git blame`?

I don't think anyone would mind. Tiny doc cleanups are neat-o.

>>> +   } else
>>> +   die("unknown log destination '%s'", v);
>> 
>> Should be die(_("unknown..., i.e. use the _() macro.
>> 
>> Anyway, this looks fine to be with our without these proposed
>> bikeshedding changes above. Thanks.
>>



Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-05 Thread Junio C Hamano
Eric Sunshine  writes:

> On Sun, Feb 4, 2018 at 1:30 PM, Lucas Werkmeister
>  wrote:
>> This new option can be used to override the implicit --syslog of
>> ...
> Thanks. With the 'log_destination' initialization bug fixed, this
> version looks good; I didn't find anything else worth commenting upon.
> Ævar's micronits[1] could be addressed by a follow-up patch (if
> desirable), but probably needn't hold up this patch.
>
> [1]: https://public-inbox.org/git/871si0mvo0@evledraar.gmail.com/

Nicely done.  Thanks, all.

Will queue.


Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-04 Thread Eric Sunshine
On Sun, Feb 4, 2018 at 1:55 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Sun, Feb 04 2018, Lucas Werkmeister jotted:
>>[--inetd |
>> [--listen=] [--port=]
>> [--user= [--group=]]]
>> +  [--log-destination=(stderr|syslog|none)]
>
> I micronit, but maybe worthwhile to have a preceeding commit to fix up
> that indentation of --listen and --user.

The '--listen' and '--user' lines are in the "alternate" ('|') branch
of '--inetd' so, as Lucas points out, this indentation appears
intentional, thus seems okay as-is.

>> +--log-destination=::
>> + Send log messages to the specified destination.
>> + Note that this option does not imply --verbose,
>
> Should `` quote --verbose, although I see similar to the WS change I
> noted above there's plenty of existing stuff in that doc doing it wrong.

As you mention, there are plenty of existing offenders already in this
file, so probably not worth a re-roll (perhaps Junio can fix this new
instance locally), but certainly good fodder for a follow-up patch.

>> + } else
>> + die("unknown log destination '%s'", v);
>
> Should be die(_("unknown..., i.e. use the _() macro.

No message in this source file use _(...) yet so probably not worth a
re-roll, but definitely something for a follow-up patch (by someone).


Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-04 Thread Eric Sunshine
On Sun, Feb 4, 2018 at 1:30 PM, Lucas Werkmeister
 wrote:
> This new option can be used to override the implicit --syslog of
> --inetd, or to disable all logging. (While --detach also implies
> --syslog, --log-destination=stderr with --detach is useless since
> --detach disassociates the process from the original stderr.) --syslog
> is retained as an alias for --log-destination=syslog.
> [...]
> Signed-off-by: Lucas Werkmeister 
> ---
> Notes:
> Fixes log_destination not being initialized correctly and some minor
> style issues.

Thanks. With the 'log_destination' initialization bug fixed, this
version looks good; I didn't find anything else worth commenting upon.
Ævar's micronits[1] could be addressed by a follow-up patch (if
desirable), but probably needn't hold up this patch.

[1]: https://public-inbox.org/git/871si0mvo0@evledraar.gmail.com/


Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-04 Thread Lucas Werkmeister
On 04.02.2018 19:55, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Feb 04 2018, Lucas Werkmeister jotted:
> 
>>   [--inetd |
>>[--listen=] [--port=]
>>[--user= [--group=]]]
>> + [--log-destination=(stderr|syslog|none)]
> 
> I micronit, but maybe worthwhile to have a preceeding commit to fix up
> that indentation of --listen and --user.

I thought the indentation here is intentional, since we’re still inside
the [] pair (either --inetd or --listen, --port, …).

> 
>> +--log-destination=::
>> +Send log messages to the specified destination.
>> +Note that this option does not imply --verbose,
> 
> Should `` quote --verbose, although I see similar to the WS change I
> noted above there's plenty of existing stuff in that doc doing it wrong.

I could send a follow-up to consistently ``-quote all options in
git-daemon.txt… or would that be rejected as unnecessarily cluttering
the history or the `git blame`?

>> +} else
>> +die("unknown log destination '%s'", v);
> 
> Should be die(_("unknown..., i.e. use the _() macro.
> 
> Anyway, this looks fine to be with our without these proposed
> bikeshedding changes above. Thanks.
>



smime.p7s
Description: S/MIME Cryptographic Signature


Re: [PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-04 Thread Ævar Arnfjörð Bjarmason

On Sun, Feb 04 2018, Lucas Werkmeister jotted:

>[--inetd |
> [--listen=] [--port=]
> [--user= [--group=]]]
> +  [--log-destination=(stderr|syslog|none)]

I micronit, but maybe worthwhile to have a preceeding commit to fix up
that indentation of --listen and --user.

> +--log-destination=::
> + Send log messages to the specified destination.
> + Note that this option does not imply --verbose,

Should `` quote --verbose, although I see similar to the WS change I
noted above there's plenty of existing stuff in that doc doing it wrong.
> + } else
> + die("unknown log destination '%s'", v);

Should be die(_("unknown..., i.e. use the _() macro.

Anyway, this looks fine to be with our without these proposed
bikeshedding changes above. Thanks.


[PATCH v4] daemon: add --log-destination=(stderr|syslog|none)

2018-02-04 Thread Lucas Werkmeister
This new option can be used to override the implicit --syslog of
--inetd, or to disable all logging. (While --detach also implies
--syslog, --log-destination=stderr with --detach is useless since
--detach disassociates the process from the original stderr.) --syslog
is retained as an alias for --log-destination=syslog.

--log-destination always overrides implicit --syslog regardless of
option order. This is different than the “last one wins” logic that
applies to some implicit options elsewhere in Git, but should hopefully
be less confusing. (I also don’t know if *all* implicit options in Git
follow “last one wins”.)

The combination of --inetd with --log-destination=stderr is useful, for
instance, when running `git daemon` as an instanced systemd service
(with associated socket unit). In this case, log messages sent via
syslog are received by the journal daemon, but run the risk of being
processed at a time when the `git daemon` process has already exited
(especially if the process was very short-lived, e.g. due to client
error), so that the journal daemon can no longer read its cgroup and
attach the message to the correct systemd unit (see systemd/systemd#2913
[1]). Logging to stderr instead can solve this problem, because systemd
can connect stderr directly to the journal daemon, which then already
knows which unit is associated with this stream.

[1]: https://github.com/systemd/systemd/issues/2913

Helped-by: Ævar Arnfjörð Bjarmason 
Helped-by: Junio C Hamano 
Helped-by: Eric Sunshine 
Signed-off-by: Lucas Werkmeister 
---

Notes:
Fixes log_destination not being initialized correctly and some minor
style issues.

 Documentation/git-daemon.txt | 28 ---
 daemon.c | 46 +---
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..56d54a489 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 [--inetd |
  [--listen=] [--port=]
  [--user= [--group=]]]
+[--log-destination=(stderr|syslog|none)]
 [...]
 
 DESCRIPTION
@@ -80,7 +81,8 @@ OPTIONS
do not have the 'git-daemon-export-ok' file.
 
 --inetd::
-   Have the server run as an inetd service. Implies --syslog.
+   Have the server run as an inetd service. Implies --syslog (may be
+   overridden with `--log-destination=`).
Incompatible with --detach, --port, --listen, --user and --group
options.
 
@@ -110,8 +112,28 @@ OPTIONS
zero for no limit.
 
 --syslog::
-   Log to syslog instead of stderr. Note that this option does not imply
-   --verbose, thus by default only error conditions will be logged.
+   Short for `--log-destination=syslog`.
+
+--log-destination=::
+   Send log messages to the specified destination.
+   Note that this option does not imply --verbose,
+   thus by default only error conditions will be logged.
+   The  must be one of:
++
+--
+stderr::
+   Write to standard error.
+   Note that if `--detach` is specified,
+   the process disconnects from the real standard error,
+   making this destination effectively equivalent to `none`.
+syslog::
+   Write to syslog, using the `git-daemon` identifier.
+none::
+   Disable all logging.
+--
++
+The default destination is `syslog` if `--inetd` or `--detach` is specified,
+otherwise `stderr`.
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..fb538e367 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,12 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
-static int log_syslog;
+static enum log_destination {
+   LOG_DESTINATION_UNSET = -1,
+   LOG_DESTINATION_NONE = 0,
+   LOG_DESTINATION_STDERR = 1,
+   LOG_DESTINATION_SYSLOG = 2,
+} log_destination = LOG_DESTINATION_UNSET;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -25,6 +30,7 @@ static const char daemon_usage[] =
 "   [--access-hook=]\n"
 "   [--inetd | [--listen=] [--port=]\n"
 "  [--detach] [--user= [--group=]]\n"
+"   [--log-destination=(stderr|syslog|none)]\n"
 "   [...]";
 
 /* List of acceptable pathname prefixes */
@@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
 
 static void logreport(int priority, const char *err, va_list params)
 {
-   if (log_syslog) {
+   switch (log_destination) {
+   case LOG_DESTINATION_SYSLOG: {
char buf[1024];
vsnprintf(buf, sizeof(buf), err, params);
syslog(priority, "%s", buf);
-   } else {
+   break;
+   }
+   case LOG_DESTINATION_STDERR:
/*
 * Since stderr is set to buffered mode, 

Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

2018-02-04 Thread Lucas Werkmeister
On 04.02.2018 07:36, Eric Sunshine wrote:
> On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister
>  wrote:
>> This new option can be used to override the implicit --syslog of
>> --inetd, or to disable all logging. (While --detach also implies
>> --syslog, --log-destination=stderr with --detach is useless since
>> --detach disassociates the process from the original stderr.) --syslog
>> is retained as an alias for --log-destination=syslog.
>> [...]
>> Signed-off-by: Lucas Werkmeister 
> 
> Thanks for the re-roll. There are a few comments below. Except for one
> apparent bug, I'm not sure the others are worth a re-roll...
> 
>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> @@ -110,8 +112,26 @@ OPTIONS
>> +--log-destination=::
>> +   Send log messages to the specified destination.
>> +   Note that this option does not imply --verbose,
>> +   thus by default only error conditions will be logged.
>> +   The  defaults to `stderr`, and must be one of:
> 
> I wonder if this should say instead:
> 
> The default destination is `stderr` unless `syslog`
> is implied by `--inetd` or `--detach`, ...
> 
>> diff --git a/daemon.c b/daemon.c
>> @@ -9,7 +9,12 @@
>> -static int log_syslog;
>> +static enum log_destination {
>> +   LOG_DESTINATION_UNSET = -1,
>> +   LOG_DESTINATION_NONE = 0,
>> +   LOG_DESTINATION_STDERR = 1,
>> +   LOG_DESTINATION_SYSLOG = 2,
>> +} log_destination;
> 
> Doesn't log_destination need to be initialized to
> LOG_DESTINATION_UNSET (see [1])? As it stands, being static, it's
> initialized automatically to 0 (LOG_DESTINATION_NONE), which borks the
> logic below.

Thanks, I knew I’d forgotten something :)

> 
>> @@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>>  static void logreport(int priority, const char *err, va_list params)
>>  {
>> +   switch (log_destination) {
>> +   case LOG_DESTINATION_SYSLOG: {
>> char buf[1024];
>> vsnprintf(buf, sizeof(buf), err, params);
>> syslog(priority, "%s", buf);
>> +   break;
>> +   }
>> +   case LOG_DESTINATION_STDERR:
>> /*
>>  * Since stderr is set to buffered mode, the
>>  * logging of different processes will not overlap
>> @@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, 
>> va_list params)
>> vfprintf(stderr, err, params);
>> fputc('\n', stderr);
>> fflush(stderr);
>> +   break;
>> +   case LOG_DESTINATION_NONE:
>> +   case LOG_DESTINATION_UNSET:
>> +   break;
> 
> Since LOG_DESTINATION_UNSET should never occur, perhaps this should be
> written as:
> 
> case LOG_DESTINATION_NONE:
> break;
> case LOG_DESTINATION_UNSET:
> BUG("impossible destination: %d", log_destination);

Good point, I didn’t know about the BUG() macro. But putting the
destination in the message seems unnecessary if it can only be a single
value – or would you make this a default: case?

> 
>> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)
>> +   if (skip_prefix(arg, "--log-destination=", &v)) {
>> +   if (!strcmp(v, "syslog")) {
>> +   log_destination = LOG_DESTINATION_SYSLOG;
>> +   continue;
>> +   } else if (!strcmp(v, "stderr")) {
>> +   log_destination = LOG_DESTINATION_STDERR;
>> +   continue;
>> +   } else if (!strcmp(v, "none")) {
>> +   log_destination = LOG_DESTINATION_NONE;
>> +   continue;
>> +   } else
>> +   die("Unknown log destination %s", v);
> 
> Mentioned previously[1], this probably ought to start with lowercase.
> It also would be more readable to set off the unknown value with a
> colon or quotes:
> 
> die("unknown log destination '%s', v);
> 
>> @@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)
>> -   if (log_syslog) {
>> +   if (log_destination == LOG_DESTINATION_UNSET) {
>> +   if (inetd_mode || detach)
>> +   log_destination = LOG_DESTINATION_SYSLOG;
>> +   else
>> +   log_destination = LOG_DESTINATION_STDERR;
>> +   }
>> +
>> +   if (log_destination == LOG_DESTINATION_SYSLOG) {
>> openlog("git-daemon", LOG_PID, LOG_DAEMON);
>> set_die_routine(daemon_die);
> 
> [1]: 
> https://public-inbox.org/git/capig+ctetjq9lsh68fe5otcj9twq9gsbgzdrjzhohtavfvr...@mail.gmail.com/
> 

I’ll send a new version shortly, also addressing your other comments
which I didn’t reply to here.

Cheers,
Lucas



smime.p7s
Description: S/MIME Cryptographic Signature


Re: [PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

2018-02-03 Thread Eric Sunshine
On Sat, Feb 3, 2018 at 6:08 PM, Lucas Werkmeister
 wrote:
> This new option can be used to override the implicit --syslog of
> --inetd, or to disable all logging. (While --detach also implies
> --syslog, --log-destination=stderr with --detach is useless since
> --detach disassociates the process from the original stderr.) --syslog
> is retained as an alias for --log-destination=syslog.
> [...]
> Signed-off-by: Lucas Werkmeister 

Thanks for the re-roll. There are a few comments below. Except for one
apparent bug, I'm not sure the others are worth a re-roll...

> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> @@ -110,8 +112,26 @@ OPTIONS
> +--log-destination=::
> +   Send log messages to the specified destination.
> +   Note that this option does not imply --verbose,
> +   thus by default only error conditions will be logged.
> +   The  defaults to `stderr`, and must be one of:

I wonder if this should say instead:

The default destination is `stderr` unless `syslog`
is implied by `--inetd` or `--detach`, ...

> diff --git a/daemon.c b/daemon.c
> @@ -9,7 +9,12 @@
> -static int log_syslog;
> +static enum log_destination {
> +   LOG_DESTINATION_UNSET = -1,
> +   LOG_DESTINATION_NONE = 0,
> +   LOG_DESTINATION_STDERR = 1,
> +   LOG_DESTINATION_SYSLOG = 2,
> +} log_destination;

Doesn't log_destination need to be initialized to
LOG_DESTINATION_UNSET (see [1])? As it stands, being static, it's
initialized automatically to 0 (LOG_DESTINATION_NONE), which borks the
logic below.

> @@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>  static void logreport(int priority, const char *err, va_list params)
>  {
> +   switch (log_destination) {
> +   case LOG_DESTINATION_SYSLOG: {
> char buf[1024];
> vsnprintf(buf, sizeof(buf), err, params);
> syslog(priority, "%s", buf);
> +   break;
> +   }
> +   case LOG_DESTINATION_STDERR:
> /*
>  * Since stderr is set to buffered mode, the
>  * logging of different processes will not overlap
> @@ -88,6 +97,10 @@ static void logreport(int priority, const char *err, 
> va_list params)
> vfprintf(stderr, err, params);
> fputc('\n', stderr);
> fflush(stderr);
> +   break;
> +   case LOG_DESTINATION_NONE:
> +   case LOG_DESTINATION_UNSET:
> +   break;

Since LOG_DESTINATION_UNSET should never occur, perhaps this should be
written as:

case LOG_DESTINATION_NONE:
break;
case LOG_DESTINATION_UNSET:
BUG("impossible destination: %d", log_destination);

> @@ -1297,9 +1309,22 @@ int cmd_main(int argc, const char **argv)
> +   if (skip_prefix(arg, "--log-destination=", &v)) {
> +   if (!strcmp(v, "syslog")) {
> +   log_destination = LOG_DESTINATION_SYSLOG;
> +   continue;
> +   } else if (!strcmp(v, "stderr")) {
> +   log_destination = LOG_DESTINATION_STDERR;
> +   continue;
> +   } else if (!strcmp(v, "none")) {
> +   log_destination = LOG_DESTINATION_NONE;
> +   continue;
> +   } else
> +   die("Unknown log destination %s", v);

Mentioned previously[1], this probably ought to start with lowercase.
It also would be more readable to set off the unknown value with a
colon or quotes:

die("unknown log destination '%s', v);

> @@ -1402,7 +1426,14 @@ int cmd_main(int argc, const char **argv)
> -   if (log_syslog) {
> +   if (log_destination == LOG_DESTINATION_UNSET) {
> +   if (inetd_mode || detach)
> +   log_destination = LOG_DESTINATION_SYSLOG;
> +   else
> +   log_destination = LOG_DESTINATION_STDERR;
> +   }
> +
> +   if (log_destination == LOG_DESTINATION_SYSLOG) {
> openlog("git-daemon", LOG_PID, LOG_DAEMON);
> set_die_routine(daemon_die);

[1]: 
https://public-inbox.org/git/capig+ctetjq9lsh68fe5otcj9twq9gsbgzdrjzhohtavfvr...@mail.gmail.com/


[PATCH v3] daemon: add --log-destination=(stderr|syslog|none)

2018-02-03 Thread Lucas Werkmeister
This new option can be used to override the implicit --syslog of
--inetd, or to disable all logging. (While --detach also implies
--syslog, --log-destination=stderr with --detach is useless since
--detach disassociates the process from the original stderr.) --syslog
is retained as an alias for --log-destination=syslog.

--log-destination always overrides implicit --syslog regardless of
option order. This is different than the “last one wins” logic that
applies to some implicit options elsewhere in Git, but should hopefully
be less confusing. (I also don’t know if *all* implicit options in Git
follow “last one wins”.)

The combination of --inetd with --log-destination=stderr is useful, for
instance, when running `git daemon` as an instanced systemd service
(with associated socket unit). In this case, log messages sent via
syslog are received by the journal daemon, but run the risk of being
processed at a time when the `git daemon` process has already exited
(especially if the process was very short-lived, e.g. due to client
error), so that the journal daemon can no longer read its cgroup and
attach the message to the correct systemd unit (see systemd/systemd#2913
[1]). Logging to stderr instead can solve this problem, because systemd
can connect stderr directly to the journal daemon, which then already
knows which unit is associated with this stream.

[1]: https://github.com/systemd/systemd/issues/2913

Helped-by: Ævar Arnfjörð Bjarmason 
Helped-by: Junio C Hamano 
Helped-by: Eric Sunshine 
Signed-off-by: Lucas Werkmeister 
---

Notes:
Next version on my quest to make git-daemon logging more reliable :)
many thanks to Eric Sunshine for review. Main changes this version:

- Rename --send-log-to to --log-destination, following the
  postgresql option. A cursory search didn’t turn up any other
  daemons with a similar option – suggestions welcome!
- Make explicit --log-destination always override implicit --syslog,
  regardless of option order.

 Documentation/git-daemon.txt | 26 ++---
 daemon.c | 45 +---
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..a0106e6aa 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 [--inetd |
  [--listen=] [--port=]
  [--user= [--group=]]]
+[--log-destination=(stderr|syslog|none)]
 [...]
 
 DESCRIPTION
@@ -80,7 +81,8 @@ OPTIONS
do not have the 'git-daemon-export-ok' file.
 
 --inetd::
-   Have the server run as an inetd service. Implies --syslog.
+   Have the server run as an inetd service. Implies --syslog (may be
+   overridden with `--log-destination=`).
Incompatible with --detach, --port, --listen, --user and --group
options.
 
@@ -110,8 +112,26 @@ OPTIONS
zero for no limit.
 
 --syslog::
-   Log to syslog instead of stderr. Note that this option does not imply
-   --verbose, thus by default only error conditions will be logged.
+   Short for `--log-destination=syslog`.
+
+--log-destination=::
+   Send log messages to the specified destination.
+   Note that this option does not imply --verbose,
+   thus by default only error conditions will be logged.
+   The  defaults to `stderr`, and must be one of:
++
+--
+stderr::
+   Write to standard error.
+   Note that if `--detach` is specified,
+   the process disconnects from the real standard error,
+   making this destination effectively equivalent to `none`.
+syslog::
+   Write to syslog, using the `git-daemon` identifier.
+none::
+   Disable all logging.
+--
++
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..f28400e3f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,12 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
-static int log_syslog;
+static enum log_destination {
+   LOG_DESTINATION_UNSET = -1,
+   LOG_DESTINATION_NONE = 0,
+   LOG_DESTINATION_STDERR = 1,
+   LOG_DESTINATION_SYSLOG = 2,
+} log_destination;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -25,6 +30,7 @@ static const char daemon_usage[] =
 "   [--access-hook=]\n"
 "   [--inetd | [--listen=] [--port=]\n"
 "  [--detach] [--user= [--group=]]\n"
+"   [--log-destination=(stderr|syslog|none)]\n"
 "   [...]";
 
 /* List of acceptable pathname prefixes */
@@ -74,11 +80,14 @@ static const char *get_ip_address(struct hostinfo *hi)
 
 static void logreport(int priority, const char *err, va_list params)
 {
-   if (log_syslog) {
+   switch (log_destination) {
+   case LOG_DESTINATION_SYSLOG: {
char buf[1024];
vsnp

Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)

2018-01-28 Thread Eric Sunshine
On Sun, Jan 28, 2018 at 5:58 PM, Lucas Werkmeister
 wrote:
> On 28.01.2018 07:40, Eric Sunshine wrote:
>> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
>>  wrote:
>>> This makes it possible to use --inetd while still logging to standard
>>> error. --syslog is retained as an alias for --send-log-to=syslog. A mode
>>> to disable logging explicitly is also provided.
>>>
>>> The combination of --inetd with --send-log-to=stderr is useful, for
>>> instance, when running `git daemon` as an instanced systemd service
>>> (with associated socket unit). In this case, log messages sent via
>>> syslog are received by the journal daemon, but run the risk of being
>>> processed at a time when the `git daemon` process has already exited
>>> (especially if the process was very short-lived, e.g. due to client
>>> error), so that the journal daemon can no longer read its cgroup and
>>> attach the message to the correct systemd unit (see systemd/systemd#2913
>>> [1]). Logging to stderr instead can solve this problem, because systemd
>>> can connect stderr directly to the journal daemon, which then already
>>> knows which unit is associated with this stream.
>>
>> The purpose of this patch would be easier to fathom if the problem was
>> presented first (systemd race condition), followed by the solution
>> (ability to log to stderr even when using --inetd), followed finally
>> by incidental notes ("--syslog is retained as an alias..." and ability
>> to disable logging).
>>
>> Not sure, though, if it's worth a re-roll.
>
> I didn’t want to sound like I was just scratching my own itch ;) I hope
> this option is useful for other use-cases as well?

If the reader does not know that --inetd implies --syslog, then

This makes it possible to use --inetd while still logging to
standard error.

leads to a "Huh?" moment since it is not self-contained. Had it said

Add new option --send-log-to=(stderr|syslog|none) which
allows the implied --syslog by --inetd to be overridden.

it would have provided enough information to understand the purpose of
the patch at a glance. Talking about the systemd race-condition first
would also have explained the patch's purpose, and since the proposed
solution is general (not specific to your use-case), scratching an
itch is not a point against it.

Anyhow, it's not that big of a deal, but it did give me a bit of a
pause when reading the first paragraph since it's customary on this
project to start by explaining the problem.

>> I understand that Junio suggested the name --send-log-to=, but I
>> wonder if the more concise --log= would be an possibility.
>
> --log sounds to me like it could also indicate *what* to log (e. g. “log
> verbosely” or “don’t log client IPs”). But perhaps --log-to= ?

Perhaps we can take into consideration precedent by other (non-Git)
daemon-like commands when naming this option. (None come to my mind
immediately, but I'm sure they're out there.)

>>> if (!strcmp(arg, "--inetd")) {
>>> inetd_mode = 1;
>>> -   log_syslog = 1;
>>> +   log_destination = LOG_TO_SYSLOG;
>>
>> Hmm, so an invocation "--inetd --send-log-to=stderr" works as
>> expected, but "--send-log-to=stderr --inetd" doesn't; output goes to
>> syslog despite the explicit request for stderr. Counterintuitive. This
>> should probably distinguish between 'log_destination' being unset and
>> set explicitly; if unset, then, and only then, have --inetd imply
>> syslog. Perhaps something like this:
>>
>> static enum log_destination {
>> LOG_TO_UNSET = -1
>> LOG_TO_NONE,
>> LOG_TO_STDERR,
>> LOG_TO_SYSLOG,
>> } log_destination = LOG_TO_UNSET;
>>
>> if (!strcmp(arg, "--inetd")) {
>> inetd_mode = 1;
>> if (log_destination == LOG_TO_UNSET)
>> log_destination = LOG_TO_SYSLOG;
>> ...
>> }
>> ...
>> if (log_destination == LOG_TO_UNSET)
>> log_destination = LOG_TO_STDERR
>>
>
> I’m not sure if that’s worth the extra complication… some existing
> options behave the same way already, e. g. in `git rebase --stat
> --quiet`, the `--stat` is ignored.

I took "last one wins" into consideration when writing the above but
was not convinced that it applies to this case since --inetd and
--send-log-to= have no obvious relation to one another (unlike, say,
--verbose and --quiet or other similar combinations). Unless

Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)

2018-01-28 Thread Lucas Werkmeister
On 28.01.2018 07:40, Eric Sunshine wrote:
> On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
>  wrote:
>> This makes it possible to use --inetd while still logging to standard
>> error. --syslog is retained as an alias for --send-log-to=syslog. A mode
>> to disable logging explicitly is also provided.
>>
>> The combination of --inetd with --send-log-to=stderr is useful, for
>> instance, when running `git daemon` as an instanced systemd service
>> (with associated socket unit). In this case, log messages sent via
>> syslog are received by the journal daemon, but run the risk of being
>> processed at a time when the `git daemon` process has already exited
>> (especially if the process was very short-lived, e.g. due to client
>> error), so that the journal daemon can no longer read its cgroup and
>> attach the message to the correct systemd unit (see systemd/systemd#2913
>> [1]). Logging to stderr instead can solve this problem, because systemd
>> can connect stderr directly to the journal daemon, which then already
>> knows which unit is associated with this stream.
> 
> The purpose of this patch would be easier to fathom if the problem was
> presented first (systemd race condition), followed by the solution
> (ability to log to stderr even when using --inetd), followed finally
> by incidental notes ("--syslog is retained as an alias..." and ability
> to disable logging).
> 
> Not sure, though, if it's worth a re-roll.

I didn’t want to sound like I was just scratching my own itch ;) I hope
this option is useful for other use-cases as well?

> 
>> Signed-off-by: Lucas Werkmeister 
>> Helped-by: Ævar Arnfjörð Bjarmason 
>> Helped-by: Junio C Hamano 
>> ---
>>
>> Notes:
>> This was originally “daemon: add --no-syslog to undo implicit
>> --syslog”, but Junio pointed out that combining --no-syslog with
>> --detach isn’t especially useful and suggested --send-log-to=
>> instead. Is Helped-by: the right credit for this or should it be
>> something else?
> 
> Helped-by: is fine, though typically your Signed-off-by: would be last.
> 
> I understand that Junio suggested the name --send-log-to=, but I
> wonder if the more concise --log= would be an possibility.

--log sounds to me like it could also indicate *what* to log (e. g. “log
verbosely” or “don’t log client IPs”). But perhaps --log-to= ?

> 
> More below...
> 
>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> @@ -110,8 +111,26 @@ OPTIONS
>> +--send-log-to=::
>> +   Send log messages to the specified destination.
>> +   Note that this option does not imply --verbose,
>> +   thus by default only error conditions will be logged.
>> +   The  defaults to `stderr`, and must be one of:
> 
> Perhaps also update the documentation of --inetd to mention that its
> implied --syslog can be overridden by --send-log-to=.

Very good idea!

> 
>> diff --git a/daemon.c b/daemon.c
>> @@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>>
>>  static void logreport(int priority, const char *err, va_list params)
>>  {
>> -   if (log_syslog) {
>> +   switch (log_destination) {
>> +   case LOG_TO_SYSLOG: {
>> char buf[1024];
>> vsnprintf(buf, sizeof(buf), err, params);
>> syslog(priority, "%s", buf);
>> -   } else {
>> +   break;
>> +   }
>> +   case LOG_TO_STDERR: {
> 
> There aren't many instances of:
> 
> case FOO: {
> 
> in the code-base, but those that exist don't use braces around cases
> which don't need it, so perhaps drop it from the STDERR and NONE
> cases. (Probably not worth a re-roll, though.)

Good point, forgot that part of the coding guidelines. Only the syslog
case needs it because the buf declaration can’t be labeled directly.

> 
>> /*
>>  * Since stderr is set to buffered mode, the
>>  * logging of different processes will not overlap
>> @@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, 
>> va_list params)
>> vfprintf(stderr, err, params);
>> fputc('\n', stderr);
>> fflush(stderr);
>> +   break;
>> +   }
>> +   case LOG_TO_NONE: {
>> +   break;
>> +   }
>> }
> 
> Consecutive lines with braces at the same indentation level is rather
> odd (but see previous comment).
> 
>>  }
>>
>> @@ -1289,7 

Re: [PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)

2018-01-27 Thread Eric Sunshine
On Sat, Jan 27, 2018 at 1:31 PM, Lucas Werkmeister
 wrote:
> This makes it possible to use --inetd while still logging to standard
> error. --syslog is retained as an alias for --send-log-to=syslog. A mode
> to disable logging explicitly is also provided.
>
> The combination of --inetd with --send-log-to=stderr is useful, for
> instance, when running `git daemon` as an instanced systemd service
> (with associated socket unit). In this case, log messages sent via
> syslog are received by the journal daemon, but run the risk of being
> processed at a time when the `git daemon` process has already exited
> (especially if the process was very short-lived, e.g. due to client
> error), so that the journal daemon can no longer read its cgroup and
> attach the message to the correct systemd unit (see systemd/systemd#2913
> [1]). Logging to stderr instead can solve this problem, because systemd
> can connect stderr directly to the journal daemon, which then already
> knows which unit is associated with this stream.

The purpose of this patch would be easier to fathom if the problem was
presented first (systemd race condition), followed by the solution
(ability to log to stderr even when using --inetd), followed finally
by incidental notes ("--syslog is retained as an alias..." and ability
to disable logging).

Not sure, though, if it's worth a re-roll.

> Signed-off-by: Lucas Werkmeister 
> Helped-by: Ævar Arnfjörð Bjarmason 
> Helped-by: Junio C Hamano 
> ---
>
> Notes:
> This was originally “daemon: add --no-syslog to undo implicit
> --syslog”, but Junio pointed out that combining --no-syslog with
> --detach isn’t especially useful and suggested --send-log-to=
> instead. Is Helped-by: the right credit for this or should it be
> something else?

Helped-by: is fine, though typically your Signed-off-by: would be last.

I understand that Junio suggested the name --send-log-to=, but I
wonder if the more concise --log= would be an possibility.

More below...

> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> @@ -110,8 +111,26 @@ OPTIONS
> +--send-log-to=::
> +   Send log messages to the specified destination.
> +   Note that this option does not imply --verbose,
> +   thus by default only error conditions will be logged.
> +   The  defaults to `stderr`, and must be one of:

Perhaps also update the documentation of --inetd to mention that its
implied --syslog can be overridden by --send-log-to=.

> diff --git a/daemon.c b/daemon.c
> @@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)
>
>  static void logreport(int priority, const char *err, va_list params)
>  {
> -   if (log_syslog) {
> +   switch (log_destination) {
> +   case LOG_TO_SYSLOG: {
> char buf[1024];
> vsnprintf(buf, sizeof(buf), err, params);
> syslog(priority, "%s", buf);
> -   } else {
> +   break;
> +   }
> +   case LOG_TO_STDERR: {

There aren't many instances of:

case FOO: {

in the code-base, but those that exist don't use braces around cases
which don't need it, so perhaps drop it from the STDERR and NONE
cases. (Probably not worth a re-roll, though.)

> /*
>  * Since stderr is set to buffered mode, the
>  * logging of different processes will not overlap
> @@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, 
> va_list params)
> vfprintf(stderr, err, params);
> fputc('\n', stderr);
> fflush(stderr);
> +   break;
> +   }
> +   case LOG_TO_NONE: {
> +   break;
> +   }
> }

Consecutive lines with braces at the same indentation level is rather
odd (but see previous comment).

>  }
>
> @@ -1289,7 +1302,7 @@ int cmd_main(int argc, const char **argv)
> }
> if (!strcmp(arg, "--inetd")) {
> inetd_mode = 1;
> -   log_syslog = 1;
> +   log_destination = LOG_TO_SYSLOG;

Hmm, so an invocation "--inetd --send-log-to=stderr" works as
expected, but "--send-log-to=stderr --inetd" doesn't; output goes to
syslog despite the explicit request for stderr. Counterintuitive. This
should probably distinguish between 'log_destination' being unset and
set explicitly; if unset, then, and only then, have --inetd imply
syslog. Perhaps something like this:

static enum log_destination {
LOG_TO_UNSET = -1
LOG_TO_NONE,
LOG_TO_STDERR,
LOG_TO_SYSLOG,
} log_destination = LOG_TO_UNSET;

if (!strcmp(arg, "--inetd&q

[PATCH v2] daemon: add --send-log-to=(stderr|syslog|none)

2018-01-27 Thread Lucas Werkmeister
This makes it possible to use --inetd while still logging to standard
error. --syslog is retained as an alias for --send-log-to=syslog. A mode
to disable logging explicitly is also provided.

The combination of --inetd with --send-log-to=stderr is useful, for
instance, when running `git daemon` as an instanced systemd service
(with associated socket unit). In this case, log messages sent via
syslog are received by the journal daemon, but run the risk of being
processed at a time when the `git daemon` process has already exited
(especially if the process was very short-lived, e.g. due to client
error), so that the journal daemon can no longer read its cgroup and
attach the message to the correct systemd unit (see systemd/systemd#2913
[1]). Logging to stderr instead can solve this problem, because systemd
can connect stderr directly to the journal daemon, which then already
knows which unit is associated with this stream.

[1]: https://github.com/systemd/systemd/issues/2913

Signed-off-by: Lucas Werkmeister 
Helped-by: Ævar Arnfjörð Bjarmason 
Helped-by: Junio C Hamano 
---

Notes:
This was originally “daemon: add --no-syslog to undo implicit
--syslog”, but Junio pointed out that combining --no-syslog with
--detach isn’t especially useful and suggested --send-log-to=
instead. Is Helped-by: the right credit for this or should it be
something else?

I’m also not quite sure if the systemd part of the commit message is
accurate – see my comment on the linked issue [2]. TL;DR: this might
no longer be necessary on systemd v235. (I’m experiencing the
problem on Debian Stretch, systemd v232.) As in the last patch, feel
free to remove that part of the commit message.

[2]: https://github.com/systemd/systemd/issues/2913#issuecomment-361002589

 Documentation/git-daemon.txt | 23 +--
 daemon.c | 43 ---
 2 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 3c91db7be..e973f4390 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 [--inetd |
  [--listen=] [--port=]
  [--user= [--group=]]]
+[--send-log-to=(stderr|syslog|none)]
 [...]
 
 DESCRIPTION
@@ -110,8 +111,26 @@ OPTIONS
zero for no limit.
 
 --syslog::
-   Log to syslog instead of stderr. Note that this option does not imply
-   --verbose, thus by default only error conditions will be logged.
+   Short for `--send-log-to=syslog`.
+
+--send-log-to=::
+   Send log messages to the specified destination.
+   Note that this option does not imply --verbose,
+   thus by default only error conditions will be logged.
+   The  defaults to `stderr`, and must be one of:
++
+--
+stderr::
+   Write to standard error.
+   Note that if `--detach` is specified,
+   the process disconnects from the real standard error,
+   making this destination effectively equivalent to `none`.
+syslog::
+   Write to syslog, using the `git-daemon` identifier.
+none::
+   Disable all logging.
+--
++
 
 --user-path::
 --user-path=::
diff --git a/daemon.c b/daemon.c
index e37e343d0..3d8e16ede 100644
--- a/daemon.c
+++ b/daemon.c
@@ -9,7 +9,11 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
-static int log_syslog;
+static enum log_destination {
+   LOG_TO_NONE = -1,
+   LOG_TO_STDERR = 0,
+   LOG_TO_SYSLOG = 1,
+} log_destination;
 static int verbose;
 static int reuseaddr;
 static int informative_errors;
@@ -25,6 +29,7 @@ static const char daemon_usage[] =
 "   [--access-hook=]\n"
 "   [--inetd | [--listen=] [--port=]\n"
 "  [--detach] [--user= [--group=]]\n"
+"   [--send-log-to=(stderr|syslog|none)]\n"
 "   [...]";
 
 /* List of acceptable pathname prefixes */
@@ -74,11 +79,14 @@ static const char *get_ip_address(struct hostinfo *hi)
 
 static void logreport(int priority, const char *err, va_list params)
 {
-   if (log_syslog) {
+   switch (log_destination) {
+   case LOG_TO_SYSLOG: {
char buf[1024];
vsnprintf(buf, sizeof(buf), err, params);
syslog(priority, "%s", buf);
-   } else {
+   break;
+   }
+   case LOG_TO_STDERR: {
/*
 * Since stderr is set to buffered mode, the
 * logging of different processes will not overlap
@@ -88,6 +96,11 @@ static void logreport(int priority, const char *err, va_list 
params)
vfprintf(stderr, err, params);
fputc('\n', stderr);
fflush(stderr);
+   break;
+   }
+   case LOG_TO_NONE: {
+   break;
+   }
   

[PATCH 04/13] list-objects-filter-blobs-none: add filter to omit all blobs

2017-10-24 Thread Jeff Hostetler
From: Jeff Hostetler 

Create a simple filter for traverse_commit_list_worker() to omit
all blobs from the result.

This filter will be used in a future commit by rev-list and pack-objects
to create a "commits and trees" result.  This is intended for partial
clone and fetch support.

Signed-off-by: Jeff Hostetler 
---
 Makefile |  1 +
 list-objects-filter-blobs-none.c | 83 
 list-objects-filter-blobs-none.h | 18 +
 3 files changed, 102 insertions(+)
 create mode 100644 list-objects-filter-blobs-none.c
 create mode 100644 list-objects-filter-blobs-none.h

diff --git a/Makefile b/Makefile
index e59f12d..7e9d1f4 100644
--- a/Makefile
+++ b/Makefile
@@ -807,6 +807,7 @@ LIB_OBJS += levenshtein.o
 LIB_OBJS += line-log.o
 LIB_OBJS += line-range.o
 LIB_OBJS += list-objects.o
+LIB_OBJS += list-objects-filter-blobs-none.o
 LIB_OBJS += list-objects-filter-map.o
 LIB_OBJS += ll-merge.o
 LIB_OBJS += lockfile.o
diff --git a/list-objects-filter-blobs-none.c b/list-objects-filter-blobs-none.c
new file mode 100644
index 000..1b548b9
--- /dev/null
+++ b/list-objects-filter-blobs-none.c
@@ -0,0 +1,83 @@
+#include "cache.h"
+#include "dir.h"
+#include "tag.h"
+#include "commit.h"
+#include "tree.h"
+#include "blob.h"
+#include "diff.h"
+#include "tree-walk.h"
+#include "revision.h"
+#include "list-objects.h"
+#include "list-objects-filter-blobs-none.h"
+
+#define DEFAULT_MAP_SIZE (16*1024)
+
+/*
+ * A filter for list-objects to omit ALL blobs from the traversal.
+ * And to OPTIONALLY collect a list of the omitted OIDs.
+ */
+struct filter_blobs_none_data {
+   struct oidmap *omits;
+};
+
+static list_objects_filter_result filter_blobs_none(
+   list_objects_filter_type filter_type,
+   struct object *obj,
+   const char *pathname,
+   const char *filename,
+   void *filter_data_)
+{
+   struct filter_blobs_none_data *filter_data = filter_data_;
+
+   switch (filter_type) {
+   default:
+   die("unkown filter_type");
+   return LOFR_ZERO;
+
+   case LOFT_BEGIN_TREE:
+   assert(obj->type == OBJ_TREE);
+   /* always include all tree objects */
+   return LOFR_MARK_SEEN | LOFR_SHOW;
+
+   case LOFT_END_TREE:
+   assert(obj->type == OBJ_TREE);
+   return LOFR_ZERO;
+
+   case LOFT_BLOB:
+   assert(obj->type == OBJ_BLOB);
+   assert((obj->flags & SEEN) == 0);
+
+   if (filter_data->omits)
+   list_objects_filter_map_insert(
+   filter_data->omits, &obj->oid, pathname,
+   obj->type);
+
+   return LOFR_MARK_SEEN; /* but not LOFR_SHOW (hard omit) */
+   }
+}
+
+void traverse_commit_list__blobs_none(
+   struct rev_info *revs,
+   show_commit_fn show_commit,
+   show_object_fn show_object,
+   list_objects_filter_map_foreach_cb print_omitted_object,
+   void *ctx_data)
+{
+   struct filter_blobs_none_data d;
+
+   memset(&d, 0, sizeof(d));
+   if (print_omitted_object) {
+   d.omits = xcalloc(1, sizeof(*d.omits));
+   oidmap_init(d.omits, DEFAULT_MAP_SIZE);
+   }
+
+   traverse_commit_list_worker(revs, show_commit, show_object, ctx_data,
+   filter_blobs_none, &d);
+
+   if (print_omitted_object) {
+   list_objects_filter_map_foreach(d.omits,
+   print_omitted_object,
+   ctx_data);
+   oidmap_free(d.omits, 1);
+   }
+}
diff --git a/list-objects-filter-blobs-none.h b/list-objects-filter-blobs-none.h
new file mode 100644
index 000..363c9de
--- /dev/null
+++ b/list-objects-filter-blobs-none.h
@@ -0,0 +1,18 @@
+#ifndef LIST_OBJECTS_FILTER_BLOBS_NONE_H
+#define LIST_OBJECTS_FILTER_BLOBS_NONE_H
+
+#include "list-objects-filter-map.h"
+
+/*
+ * A filter for list-objects to omit ALL blobs
+ * from the traversal.
+ */
+void traverse_commit_list__blobs_none(
+   struct rev_info *revs,
+   show_commit_fn show_commit,
+   show_object_fn show_object,
+   list_objects_filter_map_foreach_cb print_omitted_object,
+   void *ctx_data);
+
+#endif /* LIST_OBJECTS_FILTER_BLOBS_NONE_H */
+
-- 
2.9.3



Re: [PATCH v2] doc: add missing values "none" and "default" for diff.wsErrorHighlight

2017-07-25 Thread Junio C Hamano
Andreas Heiduk  writes:

> The values have eluded documentation so far. While at it streamline
> the wording by grouping relevant parts together.
>
> Signed-off-by: Andreas Heiduk 
> ---
>  Documentation/diff-config.txt  | 11 +++
>  Documentation/diff-options.txt | 17 -
>  2 files changed, 15 insertions(+), 13 deletions(-)

Looks sensible; thanks.  Will queue.


[PATCH v2] doc: add missing values "none" and "default" for diff.wsErrorHighlight

2017-07-25 Thread Andreas Heiduk
The values have eluded documentation so far. While at it streamline
the wording by grouping relevant parts together.

Signed-off-by: Andreas Heiduk 
---
 Documentation/diff-config.txt  | 11 +++
 Documentation/diff-options.txt | 17 -
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index cbce8ec63..5ca942ab5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -200,7 +200,10 @@ diff.algorithm::
 +
 
 diff.wsErrorHighlight::
-   A comma separated list of `old`, `new`, `context`, that
-   specifies how whitespace errors on lines are highlighted
-   with `color.diff.whitespace`.  Can be overridden by the
-   command line option `--ws-error-highlight=`
+   Highlight whitespace errors in the `context`, `old` or `new`
+   lines of the diff.  Multiple values are separated by comma,
+   `none` resets previous values, `default` reset the list to
+   `new` and `all` is a shorthand for `old,new,context`.  The
+   whitespace errors are colored with `color.diff.whitespace`.
+   The command line option `--ws-error-highlight=`
+   overrides this setting.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 89cc0f48d..d60f61ad4 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -300,15 +300,14 @@ ifndef::git-format-patch[]
with --exit-code.
 
 --ws-error-highlight=::
-   Highlight whitespace errors on lines specified by 
-   in the color specified by `color.diff.whitespace`.  
-   is a comma separated list of `old`, `new`, `context`.  When
-   this option is not given, only whitespace errors in `new`
-   lines are highlighted.  E.g. `--ws-error-highlight=new,old`
-   highlights whitespace errors on both deleted and added lines.
-   `all` can be used as a short-hand for `old,new,context`.
-   The `diff.wsErrorHighlight` configuration variable can be
-   used to specify the default behaviour.
+   Highlight whitespace errors in the `context`, `old` or `new`
+   lines of the diff.  Multiple values are separated by comma,
+   `none` resets previous values, `default` reset the list to
+   `new` and `all` is a shorthand for `old,new,context`.  When
+   this option is not given, and the configuration variable
+   `diff.wsErrorHighlight` is not set, only whitespace errors in
+   `new` lines are highlighted. The whitespace errors are colored
+   whith `color.diff.whitespace`.
 
 endif::git-format-patch[]
 
-- 
2.13.3



Re: [PATCH] doc: add missing "none" value for diff.wsErrorHighlight

2017-07-25 Thread Junio C Hamano
Andreas Heiduk  writes:

> The value has not eluded documentation so far.

I am not sure what "has not eluded" means in this context (did you
mean "has eluded"?).  

The patch text itself is not wrong per-se, but if we are to add
documentation for 'none', diff-options.txt must also document that
it clears the default and previously given values, unlike new, old
and context that are cumulative.  For that matter, we do not list
'default' and 'all' (which also clears the previous ones before
setting their own) in that three-item list, either.

I think we need to either 

 - make it to a six-item list and then describe that 'none', 'all'
   and 'default' clear the slate before taking any effect, or 

 - keep it three-item list of cumulative things, and then in the
   sentence that talks about `all` in Documentation/diff-options.txt
   to also explain what 'default' and 'none' do.

Thanks.

> Signed-off-by: Andreas Heiduk 
> ---
>  Documentation/diff-config.txt  | 2 +-
>  Documentation/diff-options.txt | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index cbce8ec63..c84ced8f6 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -200,7 +200,7 @@ diff.algorithm::
>  +
>  
>  diff.wsErrorHighlight::
> - A comma separated list of `old`, `new`, `context`, that
> + A comma separated list of `old`, `new`, `context` and `none`, that
>   specifies how whitespace errors on lines are highlighted
>   with `color.diff.whitespace`.  Can be overridden by the
>   command line option `--ws-error-highlight=`
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 89cc0f48d..903d68eb7 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -302,7 +302,7 @@ ifndef::git-format-patch[]
>  --ws-error-highlight=::
>   Highlight whitespace errors on lines specified by 
>   in the color specified by `color.diff.whitespace`.  
> - is a comma separated list of `old`, `new`, `context`.  When
> + is a comma separated list of `old`, `new`, `context` and `none`.  When
>   this option is not given, only whitespace errors in `new`
>   lines are highlighted.  E.g. `--ws-error-highlight=new,old`
>   highlights whitespace errors on both deleted and added lines.


[PATCH] doc: add missing "none" value for diff.wsErrorHighlight

2017-07-24 Thread Andreas Heiduk
The value has not eluded documentation so far.

Signed-off-by: Andreas Heiduk 
---
 Documentation/diff-config.txt  | 2 +-
 Documentation/diff-options.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index cbce8ec63..c84ced8f6 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -200,7 +200,7 @@ diff.algorithm::
 +
 
 diff.wsErrorHighlight::
-   A comma separated list of `old`, `new`, `context`, that
+   A comma separated list of `old`, `new`, `context` and `none`, that
specifies how whitespace errors on lines are highlighted
with `color.diff.whitespace`.  Can be overridden by the
command line option `--ws-error-highlight=`
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 89cc0f48d..903d68eb7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -302,7 +302,7 @@ ifndef::git-format-patch[]
 --ws-error-highlight=::
Highlight whitespace errors on lines specified by 
in the color specified by `color.diff.whitespace`.  
-   is a comma separated list of `old`, `new`, `context`.  When
+   is a comma separated list of `old`, `new`, `context` and `none`.  When
this option is not given, only whitespace errors in `new`
lines are highlighted.  E.g. `--ws-error-highlight=new,old`
highlights whitespace errors on both deleted and added lines.
-- 
2.13.3



Re: none

2016-04-11 Thread Matthieu Moy
miwilli...@google.com writes:

> From 7201fe08ede76e502211a781250c9a0b702a78b2 Mon Sep 17 00:00:00 2001
> From: Mike Williams 
> Date: Mon, 11 Apr 2016 14:18:39 -0400
> Subject: [PATCH 1/1] wt-status: Remove '!!' from
> wt_status_collect_changed_cb
>
> The wt_status_collect_changed_cb function uses an extraneous double
> negation (!!)
> when determining whether or not a submodule has new commits.

It's not just a double negation, it's a way to ensure that the value is
0 or 1 (it's a relatively common idiom in C at least in Git's codebase).

new_submodule_commits is a 1-bit bitfield, and you don't want to assign
anything other than 1 or 0 (or you'll get modulo 2^n semantics, with
n==1).

So the old code is correct and your patch would introduce a bug.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 08/12] git submodule update: check for "none" in C

2015-10-15 Thread Stefan Beller
Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 38 --
 git-submodule.sh|  8 +---
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f81f37a..73954ac 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,9 +255,15 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+static int git_submodule_config(const char *var, const char *value, void *cb)
+{
+   return parse_submodule_config_option(var, value);
+}
+
 static int module_list_or_clone(int argc, const char **argv, const char 
*prefix)
 {
int i;
+   char *update = NULL;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
 
@@ -265,6 +271,9 @@ static int module_list_or_clone(int argc, const char 
**argv, const char *prefix)
OPT_STRING(0, "prefix", &prefix,
   N_("path"),
   N_("alternative anchor for relative paths")),
+   OPT_STRING(0, "update", &update,
+  N_("string"),
+  N_("update command for submodules")),
OPT_END()
};
 
@@ -281,20 +290,45 @@ static int module_list_or_clone(int argc, const char 
**argv, const char *prefix)
return 1;
}
 
+   gitmodules_config();
+   /* Overlay the parsed .gitmodules file with .git/config */
+   git_config(git_submodule_config, NULL);
+
for (i = 0; i < list.nr; i++) {
+   const struct submodule *sub = NULL;
+   const char *displaypath = NULL;
const struct cache_entry *ce = list.entries[i];
+   struct strbuf sb = STRBUF_INIT;
+   const char *update_module = NULL;
 
char *env_prefix = getenv("prefix");
if (ce_stage(ce)) {
if (env_prefix)
-   fprintf(stderr, "Skipping unmerged submodule 
%s/%s",
+   fprintf(stderr, "Skipping unmerged submodule 
%s/%s\n",
env_prefix, ce->name);
else
-   fprintf(stderr, "Skipping unmerged submodule 
%s",
+   fprintf(stderr, "Skipping unmerged submodule 
%s\n",
ce->name);
continue;
}
 
+   sub = submodule_from_path(null_sha1, ce->name);
+   if (env_prefix)
+   displaypath = relative_path(env_prefix, ce->name, &sb);
+   else
+   displaypath = ce->name;
+
+   if (update)
+   update_module = update;
+   if (!update_module)
+   update_module = sub->update;
+   if (!update_module)
+   update_module = "checkout";
+   if (!strcmp(update_module, "none")) {
+   fprintf(stderr, "Skipping submodule '%s'\n", 
displaypath);
+   continue;
+   }
+
printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), 
ce_stage(ce));
utf8_fprintf(stdout, "%s\n", ce->name);
}
diff --git a/git-submodule.sh b/git-submodule.sh
index 0754ecd..227fed6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -656,7 +656,7 @@ cmd_update()
fi
 
cloned_modules=
-   git submodule--helper list-or-clone --prefix "$wt_prefix" "$@" | {
+   git submodule--helper list-or-clone --prefix "$wt_prefix" 
${update:+--update "$update"} "$@" | {
err=
while read mode sha1 stage sm_path
do
@@ -677,12 +677,6 @@ cmd_update()
 
displaypath=$(relative_path "$prefix$sm_path")
 
-   if test "$update_module" = "none"
-   then
-   echo >&2 "Skipping submodule '$displaypath'"
-   continue
-   fi
-
if test -z "$url"
then
# Only mention uninitialized submodules when its
-- 
2.5.0.277.gfdc362b.dirty

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


Re: [PATCH] gitk: Fix bad English grammar "Matches none Commit Info"

2015-04-05 Thread Paul Mackerras
On Thu, Apr 02, 2015 at 03:05:06PM -0600, Alex Henrie wrote:
> Signed-off-by: Alex Henrie 
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gitk b/gitk
> index 9a2daf3..30fcd30 100755
> --- a/gitk
> +++ b/gitk
> @@ -4066,7 +4066,7 @@ set known_view_options {
>  {committer t15  .  "--committer=*"  {mc "Committer:"}}
>  {loginfo   t15  .. "--grep=*"   {mc "Commit Message:"}}
>  {allmatch  b.. "--all-match"{mc "Matches all Commit Info 
> criteria"}}
> -{igrep b.. "--invert-grep"  {mc "Matches none Commit Info 
> criteria"}}
> +{igrep b.. "--invert-grep"  {mc "Matches no Commit Info 
> criteria"}}

Thanks, applied.

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


[PATCH] gitk: Fix bad English grammar "Matches none Commit Info"

2015-04-02 Thread Alex Henrie
Signed-off-by: Alex Henrie 
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 9a2daf3..30fcd30 100755
--- a/gitk
+++ b/gitk
@@ -4066,7 +4066,7 @@ set known_view_options {
 {committer t15  .  "--committer=*"  {mc "Committer:"}}
 {loginfo   t15  .. "--grep=*"   {mc "Commit Message:"}}
 {allmatch  b.. "--all-match"{mc "Matches all Commit Info 
criteria"}}
-{igrep b.. "--invert-grep"  {mc "Matches none Commit Info 
criteria"}}
+{igrep b.. "--invert-grep"  {mc "Matches no Commit Info criteria"}}
 {changes_l l+  {}   {mc "Changes to Files:"}}
 {pickaxe_s r0   .  {}   {mc "Fixed String"}}
 {pickaxe_t r1   .  "--pickaxe-regex"  {mc "Regular Expression"}}
-- 
2.3.5

--
To unsubscribe from this list: send the line "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-log: added --none-match option

2015-01-12 Thread Junio C Hamano
Junio C Hamano  writes:

> Christoph Junghans  writes:
>
>> The only useful thing I could image is using it in conjunction with
>> --files-with-matches, but that is what --files-without-match is for.
>
> Yes, "-l" was exactly what I had in mind and I was hoping that "git
> grep -l --no-match -e WIP -e TODO -e FIXME -e NEEDSWORK" may be a
> way to find perfect files without needing any work.
>
> You can say "git grep -L -e WIP -e TODO -e FIXME -e NEEDSWORK"
> instead.  I missed that "-L" option.

Thanks for your patience.  I should have realized that this not only
can be "log-only" but _should_ be "log-only".  As you pointed out,
when we already have "-L", trying to extend it to "grep" does not
make much sense.

Continuing this line of thought, as we determined that it is
pointless to have this at "grep" level and it is needed only in the
"log" family of commands, I would very much prefer the approach
taken by your original "log --invert-grep" patch.  I would further
say that I prefer not to touch grep_opt at all.

The new "global-invert bit" is about "we'd run the usual grep thing
on the log message, and instead of filtering to only show the
commits with matching message, we only show the ones with messages
that do not match".  That logically belongs to the revision struct
that is used to interpret what the underlying grep machinery figured
out, and should not have to affect the way how underlying grep
machinery works.

We would want a few test to make sure that we do not break the
feature in the future changes.  Here is an attempt.  The patch would
apply any commit your original "--invert-grep" option would have
applied.

Thanks again.

-- >8 --
Subject: log: teach --invert-grep option

"git log --grep=" shows only commits with messages that
match the given string, but sometimes it is useful to be able to
show only commits that do *not* have certain messages (e.g. "show me
ones that are not FIXUP commits").

Signed-off-by: Christoph Junghans 
Signed-off-by: Junio C Hamano 
---

 jc: Christoph originally had the invert-grep flag in grep_opt, but
 because "git grep --invert-grep" does not make sense except in
 conjunction with "--files-with-matches", which is already
 covered by "--files-without-matches", I moved it to revisions
 structure.  I think it expresses what the feature is about
 better to have the flag there.

 When the newly inserted two tests run, the history would have
 commits with messages "initial", "second", "third", "fourth",
 "fifth", "sixth" and Second", committed in this order.  The
 first commit that does not match either "th" or "Sec" is
 "second", and "initial" is the one that does not match either
 "th" or "Sec" case insensitively.

 Documentation/rev-list-options.txt |  4 
 contrib/completion/git-completion.bash |  2 +-
 revision.c |  4 +++-
 revision.h |  2 ++
 t/t4202-log.sh | 12 
 5 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index deb8cca..05aa997 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -66,6 +66,10 @@ if it is part of the log message.
Limit the commits output to ones that match all given `--grep`,
instead of ones that match at least one.
 
+--invert-grep::
+   Limit the commits output to ones with log message that do not
+   match the pattern specified with `--grep=`.
+
 -i::
 --regexp-ignore-case::
Match the regular expression limiting patterns without regard to letter
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 06bf262..53857f0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1428,7 +1428,7 @@ __git_log_gitk_options="
 # Options that go well for log and shortlog (not gitk)
 __git_log_shortlog_options="
--author= --committer= --grep=
-   --all-match
+   --all-match --invert-grep
 "
 
 __git_log_pretty_formats="oneline short medium full fuller email raw format:"
diff --git a/revision.c b/revision.c
index 615535c..84b33a3 100644
--- a/revision.c
+++ b/revision.c
@@ -1952,6 +1952,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
    grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, 
&revs->grep_filter);
} else if (!strcmp(arg, "--all-match")) {
  

[PATCH v2] git-log: added --none-match option

2015-01-11 Thread Christoph Junghans
Implements a none match for git log, which is useful from time to
time to e.g. filter FIXUP message out of git log.

Internally, the bol 'all_match' was changed to an int 'all_or_none'
taken the values 0, GREP_ALL_MATCH or GREP_NONE_MATCH.

For git grep a similar functionality can achieved by the existing
--files-without-match option.

Signed-off-by: Christoph Junghans 
---
 Documentation/rev-list-options.txt |  4 
 builtin/grep.c |  5 +++--
 contrib/completion/git-completion.bash |  2 +-
 gitk-git/gitk  |  1 +
 grep.c | 24 ++--
 grep.h |  4 +++-
 revision.c |  4 +++-
 7 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index afccfdc..08e4ed8 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -66,6 +66,10 @@ if it is part of the log message.
Limit the commits output to ones that match all given `--grep`,
instead of ones that match at least one.
 
+--none-match::
+   Limit the commits output to ones that do not match any of the 
+   given `--grep`, instead of ones that match at least one.
+
 -i::
 --regexp-ignore-case::
Match the regular expression limiting patterns without regard to letter
diff --git a/builtin/grep.c b/builtin/grep.c
index 4063882..1ec8ce1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -727,8 +727,9 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
  close_callback },
OPT__QUIET(&opt.status_only,
   N_("indicate hit with exit status without output")),
-   OPT_BOOL(0, "all-match", &opt.all_match,
-   N_("show only matches from files that match all 
patterns")),
+   OPT_SET_INT(0, "all-match", &opt.all_or_none,
+   N_("show only matches from files that match all 
patterns"),
+   GREP_ALL_MATCH),
{ OPTION_SET_INT, 0, "debug", &opt.debug, NULL,
  N_("show parse tree for grep expression"),
  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1 },
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index cd76579..47ed970 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1425,7 +1425,7 @@ __git_log_gitk_options="
 # Options that go well for log and shortlog (not gitk)
 __git_log_shortlog_options="
--author= --committer= --grep=
-   --all-match
+   --all-match --none-match
 "
 
 __git_log_pretty_formats="oneline short medium full fuller email raw format:"
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 78358a7..c67674f 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -3977,6 +3977,7 @@ set known_view_options {
 {committer t15  .  "--committer=*"  {mc "Committer:"}}
 {loginfo   t15  .. "--grep=*"   {mc "Commit Message:"}}
 {allmatch  b.. "--all-match"{mc "Matches all Commit Info 
criteria"}}
+{nonematch b.. "--none-match"   {mc "Matches none Commit Info 
criteria"}}
 {changes_l l+  {}   {mc "Changes to Files:"}}
 {pickaxe_s r0   .  {}   {mc "Fixed String"}}
 {pickaxe_t r1   .  "--pickaxe-regex"  {mc "Regular Expression"}}
diff --git a/grep.c b/grep.c
index 6e085f8..f6eb044 100644
--- a/grep.c
+++ b/grep.c
@@ -609,8 +609,14 @@ static void dump_grep_expression(struct grep_opt *opt)
 {
struct grep_expr *x = opt->pattern_expression;
 
-   if (opt->all_match)
+   switch (opt->all_or_none) {
+   case GREP_ALL_MATCH:
fprintf(stderr, "[all-match]\n");
+   case GREP_NONE_MATCH:
+   fprintf(stderr, "[none-match]\n");
+   default:
+   break;
+   }
dump_grep_expression_1(x, 0);
fflush(NULL);
 }
@@ -713,7 +719,7 @@ static void compile_grep_patterns_real(struct grep_opt *opt)
}
}
 
-   if (opt->all_match || header_expr)
+   if ((opt->all_or_none == GREP_ALL_MATCH) || header_expr)
opt->extended = 1;
else if (!opt->extended && !opt->debug)
return;
@@ -729,13 +735,13 @@ static void compile_grep_patterns_real(struct grep_opt 
*opt)
 
if (!opt->pattern_expression)
opt->pattern_expression = header_expr;
-   else if (opt->all_match)
+   else if (opt->all_or_none == GREP_ALL_MATCH)
opt->pat

Re: [PATCH] git-log: added --none-match option

2015-01-09 Thread Junio C Hamano
Christoph Junghans  writes:

> The only useful thing I could image is using it in conjunction with
> --files-with-matches, but that is what --files-without-match is for.

Yes, "-l" was exactly what I had in mind and I was hoping that "git
grep -l --no-match -e WIP -e TODO -e FIXME -e NEEDSWORK" may be a
way to find perfect files without needing any work.

You can say "git grep -L -e WIP -e TODO -e FIXME -e NEEDSWORK"
instead.  I missed that "-L" option.

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] git-log: added --none-match option

2015-01-09 Thread Christoph Junghans
2015-01-06 16:02 GMT-07:00 Junio C Hamano :
> Christoph Junghans  writes:
>
>> Implements a inverted match for "git log", like in the case of
>> "git grep -v", which is useful from time to time to e.g. filter
>> FIXUP message out of "git log".
>>
>> Internally, a new bol 'none_match' has been introduces as
>> revs->grep_filter.invert inverts the match line-wise, which cannot
>> work as i.e. empty line always not match the pattern given.
>>
>> Signed-off-by: Christoph Junghans 
>> ---
>
> The patch itself looks like a good start, except that the above
> description no longer matches the implementation.
>
> I further suspect it would be better to rename all_match to
> all_or_none and then you can lose the "these two are mutually
> incompatible" check that is placed together with a wrong existing
> comment.  I also notice that you forgot to update the "git grep"
> where the original "--all-match" came from.
That was on purpose. I am not quite sure what would be the point of
"showing only matches from files that match no patterns" (option
description from your patch below).
If a file matches none of the patterns, what matches are there to show?

The only useful thing I could image is using it in conjunction with
--files-with-matches, but that is what --files-without-match is for.

>
> A partial fix-up may start like this on top of your version.  By
> renaming the variable used in the existing code, the compiler will
> remind you that there are a few more places that your patch did not
> touch that does something special for --all-match, which are a good
> candidates you need to think if doing something similarly special
> for the --none-match case is necessary.
>
> Thanks.
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 4063882..9ba4254 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -727,8 +727,12 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>   close_callback },
> OPT__QUIET(&opt.status_only,
>N_("indicate hit with exit status without 
> output")),
> -   OPT_BOOL(0, "all-match", &opt.all_match,
> -   N_("show only matches from files that match all 
> patterns")),
> +   OPT_SET_INT(0, "all-match", &opt.all_or_none,
> +   N_("show only matches from files that match all 
> patterns"),
> +   GREP_ALL_MATCH),
> +   OPT_SET_INT(0, "none-match", &opt.all_or_none,
> +   N_("show only matches from files that match no 
> patterns"),
> +   GREP_NONE_MATCH),
> { OPTION_SET_INT, 0, "debug", &opt.debug, NULL,
>   N_("show parse tree for grep expression"),
>   PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1 },
> diff --git a/grep.c b/grep.c
> index f486ee5..1ff5dea 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1622,14 +1622,18 @@ static int chk_hit_marker(struct grep_expr *x)
>
>  int grep_source(struct grep_opt *opt, struct grep_source *gs)
>  {
> -   if(opt->none_match)
> -   return !grep_source_1(opt, gs, 0);
> /*
>  * we do not have to do the two-pass grep when we do not check
> -* buffer-wide "all-match".
> +* buffer-wide "all-match" or "none-match".
>  */
> -   if (!opt->all_match)
> +   switch (opt->all_or_none) {
> +   case GREP_ALL_MATCH:
> return grep_source_1(opt, gs, 0);
> +   case GREP_NONE_MATCH:
> +   return !grep_source_1(opt, gs, 0);
> +   default:
> +   break;
> +   }
>
> /* Otherwise the toplevel "or" terms hit a bit differently.
>  * We first clear hit markers from them.
> diff --git a/grep.h b/grep.h
> index 8e50c95..2cdabf2 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -101,8 +101,9 @@ struct grep_opt {
> int count;
> int word_regexp;
> int fixed;
> -   int all_match;
> -   int none_match;
> +#define GREP_ALL_MATCH 1
> +#define GREP_NONE_MATCH 2
> +   int all_or_none;
> int debug;
>  #define GREP_BINARY_DEFAULT0
>  #define GREP_BINARY_NOMATCH1
> diff --git a/revision.c b/revision.c
> index d43779e..b955848 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2010,9 +2010,9 @@ static int handle_revision_opt(struct rev_info *revs, 
> int ar

Re: [PATCH] git-log: added --none-match option

2015-01-06 Thread Junio C Hamano
Christoph Junghans  writes:

> Implements a inverted match for "git log", like in the case of
> "git grep -v", which is useful from time to time to e.g. filter
> FIXUP message out of "git log".
>
> Internally, a new bol 'none_match' has been introduces as
> revs->grep_filter.invert inverts the match line-wise, which cannot
> work as i.e. empty line always not match the pattern given.
>
> Signed-off-by: Christoph Junghans 
> ---

The patch itself looks like a good start, except that the above
description no longer matches the implementation.

I further suspect it would be better to rename all_match to
all_or_none and then you can lose the "these two are mutually
incompatible" check that is placed together with a wrong existing
comment.  I also notice that you forgot to update the "git grep"
where the original "--all-match" came from.

A partial fix-up may start like this on top of your version.  By
renaming the variable used in the existing code, the compiler will
remind you that there are a few more places that your patch did not
touch that does something special for --all-match, which are a good
candidates you need to think if doing something similarly special
for the --none-match case is necessary.

Thanks.

diff --git a/builtin/grep.c b/builtin/grep.c
index 4063882..9ba4254 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -727,8 +727,12 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
  close_callback },
OPT__QUIET(&opt.status_only,
   N_("indicate hit with exit status without output")),
-   OPT_BOOL(0, "all-match", &opt.all_match,
-   N_("show only matches from files that match all 
patterns")),
+   OPT_SET_INT(0, "all-match", &opt.all_or_none,
+   N_("show only matches from files that match all 
patterns"),
+   GREP_ALL_MATCH),
+   OPT_SET_INT(0, "none-match", &opt.all_or_none,
+   N_("show only matches from files that match no 
patterns"),
+   GREP_NONE_MATCH),
{ OPTION_SET_INT, 0, "debug", &opt.debug, NULL,
  N_("show parse tree for grep expression"),
  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 1 },
diff --git a/grep.c b/grep.c
index f486ee5..1ff5dea 100644
--- a/grep.c
+++ b/grep.c
@@ -1622,14 +1622,18 @@ static int chk_hit_marker(struct grep_expr *x)
 
 int grep_source(struct grep_opt *opt, struct grep_source *gs)
 {
-   if(opt->none_match)
-   return !grep_source_1(opt, gs, 0);
/*
 * we do not have to do the two-pass grep when we do not check
-* buffer-wide "all-match".
+* buffer-wide "all-match" or "none-match".
 */
-   if (!opt->all_match)
+   switch (opt->all_or_none) {
+   case GREP_ALL_MATCH:
return grep_source_1(opt, gs, 0);
+   case GREP_NONE_MATCH:
+   return !grep_source_1(opt, gs, 0);
+   default:
+   break;
+   }
 
/* Otherwise the toplevel "or" terms hit a bit differently.
 * We first clear hit markers from them.
diff --git a/grep.h b/grep.h
index 8e50c95..2cdabf2 100644
--- a/grep.h
+++ b/grep.h
@@ -101,8 +101,9 @@ struct grep_opt {
int count;
int word_regexp;
int fixed;
-   int all_match;
-   int none_match;
+#define GREP_ALL_MATCH 1
+#define GREP_NONE_MATCH 2
+   int all_or_none;
int debug;
 #define GREP_BINARY_DEFAULT0
 #define GREP_BINARY_NOMATCH1
diff --git a/revision.c b/revision.c
index d43779e..b955848 100644
--- a/revision.c
+++ b/revision.c
@@ -2010,9 +2010,9 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
} else if (!strcmp(arg, "--perl-regexp")) {
grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, 
&revs->grep_filter);
} else if (!strcmp(arg, "--all-match")) {
-   revs->grep_filter.all_match = 1;
+   revs->grep_filter.all_or_none = GREP_ALL_MATCH;
} else if (!strcmp(arg, "--none-match")) {
-   revs->grep_filter.none_match = 1;
+   revs->grep_filter.all_or_none = GREP_NONE_MATCH;
} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
if (strcmp(optarg, "none"))
git_log_output_encoding = xstrdup(optarg);
@@ -2335,8 +2335,6 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
    die("cannot combine --walk-reflogs with --graph");
if (!rev

[PATCH] git-log: added --none-match option

2015-01-03 Thread Christoph Junghans
Implements a inverted match for "git log", like in the case of
"git grep -v", which is useful from time to time to e.g. filter
FIXUP message out of "git log".

Internally, a new bol 'none_match' has been introduces as
revs->grep_filter.invert inverts the match line-wise, which cannot
work as i.e. empty line always not match the pattern given.

Signed-off-by: Christoph Junghans 
---
 Documentation/rev-list-options.txt | 4 
 contrib/completion/git-completion.bash | 2 +-
 grep.c | 2 ++
 grep.h | 1 +
 revision.c | 4 
 5 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index afccfdc..08e4ed8 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -66,6 +66,10 @@ if it is part of the log message.
Limit the commits output to ones that match all given `--grep`,
instead of ones that match at least one.
 
+--none-match::
+   Limit the commits output to ones that do not match any of the 
+   given `--grep`, instead of ones that match at least one.
+
 -i::
 --regexp-ignore-case::
Match the regular expression limiting patterns without regard to letter
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 23988ec..b0720e9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1425,7 +1425,7 @@ __git_log_gitk_options="
 # Options that go well for log and shortlog (not gitk)
 __git_log_shortlog_options="
--author= --committer= --grep=
-   --all-match
+   --all-match --none-match
 "
 
 __git_log_pretty_formats="oneline short medium full fuller email raw format:"
diff --git a/grep.c b/grep.c
index 6e085f8..eadf8d9 100644
--- a/grep.c
+++ b/grep.c
@@ -1622,6 +1622,8 @@ static int chk_hit_marker(struct grep_expr *x)
 
 int grep_source(struct grep_opt *opt, struct grep_source *gs)
 {
+   if(opt->none_match)
+   return !grep_source_1(opt, gs, 0);  
/*
 * we do not have to do the two-pass grep when we do not check
 * buffer-wide "all-match".
diff --git a/grep.h b/grep.h
index 95f197a..8e50c95 100644
--- a/grep.h
+++ b/grep.h
@@ -102,6 +102,7 @@ struct grep_opt {
int word_regexp;
int fixed;
int all_match;
+   int none_match;
int debug;
 #define GREP_BINARY_DEFAULT0
 #define GREP_BINARY_NOMATCH1
diff --git a/revision.c b/revision.c
index 75dda92..d43779e 100644
--- a/revision.c
+++ b/revision.c
@@ -2011,6 +2011,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
grep_set_pattern_type_option(GREP_PATTERN_TYPE_PCRE, 
&revs->grep_filter);
} else if (!strcmp(arg, "--all-match")) {
revs->grep_filter.all_match = 1;
+   } else if (!strcmp(arg, "--none-match")) {
+   revs->grep_filter.none_match = 1;
    } else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
if (strcmp(optarg, "none"))
git_log_output_encoding = xstrdup(optarg);
@@ -2333,6 +2335,8 @@ int setup_revisions(int argc, const char **argv, struct 
rev_info *revs, struct s
die("cannot combine --walk-reflogs with --graph");
if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
die("cannot use --grep-reflog without --walk-reflogs");
+   if (revs->grep_filter.all_match && revs->grep_filter.none_match)
+   die("cannot combine --all-match with --none-match");
 
return left;
 }
-- 
2.0.5

--
To unsubscribe from this list: send the line "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_CONFIG should either apply to all commands, or none at all

2014-10-02 Thread Junio C Hamano
Jonathan Nieder  writes:

> Yep.  One possibility would be to do something like the following (A):
>
>  1) advertise in the git-config(1) manpage that the GIT_CONFIG
> environment variable only affects the behavior of the 'git config'
> command
>
>  2) introduce an environment variable GIT_I_AM_PORCELAIN.  (If doing
> this, we could come up with a better name, but this is just an
> illustration.)  Set and export that envvar in git-sh-setup.sh.
> When that environment variable is set, make git-config stop paying
> attention to GIT_CONFIG.
>
> That way, git commands that happen to be scripts would not be
> affected by the GIT_CONFIG setting any more.

At the places you plan to update porcelains to set and export
GIT_I_AM_PORCELAIN, you could unset GIT_CONFIG if set.  Would that
achieve the same goal?

And you can stop there without doing 3 or 4, no?
--
To unsubscribe from this list: send the line "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_CONFIG should either apply to all commands, or none at all

2014-10-02 Thread Jeff King
On Wed, Oct 01, 2014 at 06:15:46PM -0700, Jonathan Nieder wrote:

>  3) Warn when 'git config' is called with GIT_CONFIG set, explaining
> that support will eventually be removed and that callers should
> pass --file= instead.
> 
>  4) Once we're confident there are no scripts in the wild relying on
> that envvar, remove support for it.

I think you could do just these two without worrying about the
I_AM_PORCELAIN setting. It's completely redundant with `git config
--file` these days.

> Another possibility (B):
> 
>  1) Teach git's commands in C to respect the GIT_CONFIG environment
> variable.  Semantics: only configuration from that file would be
> respected and all other configuration will be ignored.  Advertise
> it in the git(1) manpage.

I think this is a bad idea. It originally _did_ impact each command, but
there were a lot of confusing corner cases to the semantics, and it led
to bugs and misbehavior. That's what led to dc87183. I wish we had just
dropped it for git-config then, too. We kept it for backwards
compatibility, but we probably should have deprecated it more clearly.

> Yet another possibility (C):
> 
>  1) Just skip to step (4) from plan (A).

I agree this is tempting. We have never deprecated it formally, but it
has been a little-used feature.

> C is kind of temping.  Do you know if there are scripts in the wild
> that rely on the GIT_CONFIG setting working?

Searching here:

  https://github.com/search?q=%22export+GIT_CONFIG%22&type=Code

reveals that some people do set it, but from the handful I investigated,
it is probably not doing what they want. For example, in:

  
https://github.com/GNOME/sysadmin-bin/blob/8ef4165a4b38fd1488c194f0c562c7fe24545bca/git/gnome-post-receive

they are trying to use it as if it affects all git commands, but as we
just discussed, it does not. So their script is potentially buggy as-is.
Getting rid of GIT_CONFIG would make it _more_ buggy, so perhaps that is
not an excuse, but I think it points to actually doing something.

-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: $GIT_CONFIG should either apply to all commands, or none at all

2014-10-01 Thread Jonathan Nieder
Hi,

Frédéric Brière wrote[1]:

> This kind of stuff caused me a lot of hair-pulling:
>
>   $ git config core.abbrev
>   32
>   git log --pretty=oneline --abbrev-commit
>   89be foo
>
> Here's the source of the discrepancy:
>
>   $ grep abbrev $GIT_CONFIG .git/config
>   git.conf:   abbrev=32
>   .git/config:abbrev=4
>
> Since dc87183, $GIT_CONFIG is ignored by any other Git command, but it
> *still* applies to git-config.  This basically means that values
> obtained via git-config are not necessarily those which are actually in
> effect.
>
> The really frustrating part (for me, at least) is that for any tool
> (gitweb in my case) which uses git-config, values from $GIT_CONFIG will
> take effect for that tool, but not for any subsequent Git command.
>
> git-config(1) doesn't make this clear either; it mentions $GIT_CONFIG as
> "the configuration", without saying explicitly that this environment
> variable only applies to git-config.

Yep.  One possibility would be to do something like the following (A):

 1) advertise in the git-config(1) manpage that the GIT_CONFIG
environment variable only affects the behavior of the 'git config'
command

 2) introduce an environment variable GIT_I_AM_PORCELAIN.  (If doing
this, we could come up with a better name, but this is just an
illustration.)  Set and export that envvar in git-sh-setup.sh.
When that environment variable is set, make git-config stop paying
attention to GIT_CONFIG.

That way, git commands that happen to be scripts would not be
affected by the GIT_CONFIG setting any more.

 3) Warn when 'git config' is called with GIT_CONFIG set, explaining
that support will eventually be removed and that callers should
pass --file= instead.

 4) Once we're confident there are no scripts in the wild relying on
that envvar, remove support for it.

Another possibility (B):

 1) Teach git's commands in C to respect the GIT_CONFIG environment
variable.  Semantics: only configuration from that file would be
respected and all other configuration will be ignored.  Advertise
it in the git(1) manpage.

 2) Gnash teeth a little but continue to support it.

Yet another possibility (C):

 1) Just skip to step (4) from plan (A).

C is kind of temping.  Do you know if there are scripts in the wild
that rely on the GIT_CONFIG setting working?

Thanks for reporting,
Jonathan

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


Re: [PATCH/RFC] log doc: explain --encoding=none and default output encoding

2013-08-05 Thread Junio C Hamano
Jonathan Nieder  writes:

> Signed-off-by: Jonathan Nieder 
> ---
> I'm not thrilled with the wording.  This can probably be explained
> more simply.  Ideas?

Some random thoughts on text, both before and after this patch.

 - The first stentence in this paragraph up to the semicolon is
   irrelevant (it is an implementation detail that allows us to
   re-encode in the first place, and the user does not care).

 - The use of word "default" is fuzzy.  With this description, we
   want to tell the user to what encoding we reencode to by default,
   but it is easy to miss that the reencoding always happen for
   output with or without --encoding= option (which is not
   clear from the text, especially the original) and incorrectly
   assume that without --encoding= the recorded text is
   spit out without mangling.

So perhaps along this line?

--encoding=::

The encoding used to output the log messages in commit
objects.  When this option is not given, non-plumbing
commands output messages in the commit log encoding
(`i18n.commitEncoding`, or UTF-8 if the configuration
variable is not set).  `--encoding=none` lets you view the
raw log message without any reencoding.

>
>  Documentation/pretty-options.txt | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/pretty-options.txt 
> b/Documentation/pretty-options.txt
> index 5e499421..e31fd494 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -32,8 +32,14 @@ people using 80-column terminals.
>   The commit objects record the encoding used for the log message
>   in their encoding header; this option can be used to tell the
>   command to re-code the commit log message in the encoding
> - preferred by the user.  For non plumbing commands this
> - defaults to UTF-8.
> + preferred by the user.  "--encoding=none" means to use the
> + raw log message without paying attention to its encoding header.
> ++
> +For non plumbing commands, the output encoding defaults to the commit
> +encoding (as set using the `i18n.commitEncoding` variable, or UTF-8
> +by default).  This default can be overridden using the
> +`i18n.logOutputEncoding` configuration item. See linkgit:git-config[1]
> +for details.
>  
>  --notes[=]::
>   Show the notes (see linkgit:git-notes[1]) that annotate the
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH/RFC] log doc: explain --encoding=none and default output encoding

2013-08-02 Thread Jonathan Nieder
Signed-off-by: Jonathan Nieder 
---
I'm not thrilled with the wording.  This can probably be explained
more simply.  Ideas?

 Documentation/pretty-options.txt | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 5e499421..e31fd494 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -32,8 +32,14 @@ people using 80-column terminals.
The commit objects record the encoding used for the log message
in their encoding header; this option can be used to tell the
command to re-code the commit log message in the encoding
-   preferred by the user.  For non plumbing commands this
-   defaults to UTF-8.
+   preferred by the user.  "--encoding=none" means to use the
+   raw log message without paying attention to its encoding header.
++
+For non plumbing commands, the output encoding defaults to the commit
+encoding (as set using the `i18n.commitEncoding` variable, or UTF-8
+by default).  This default can be overridden using the
+`i18n.logOutputEncoding` configuration item. See linkgit:git-config[1]
+for details.
 
 --notes[=]::
Show the notes (see linkgit:git-notes[1]) that annotate the
-- 
1.8.4.rc1

--
To unsubscribe from this list: send the line "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/2] mergetools/p4merge: create a base if none available

2013-03-25 Thread Junio C Hamano
Kevin Bracey  writes:

> Minor change from v3: that version moved initialisation of src1 higher up,
> detaching it from its associated comment. This move was only required by
> earlier versions, so v4 leaves src1 in its original position.

The "funny filename" comment was from b539c5e8fbd3 (git-merge-one:
new merge world order., 2005-12-07) where the removed code just
before that new comment ended with:

merge "$4" "$orig" "$src2"

(yes, we used to use "merge" program from the RCS suite).  The
comment refers to one of the bad side effect the old code used to
have and warns against such a practice, i.e. it was talking about
the code that no longer existed.

I think the two-line comment should simply go.

Given that, I _think_ it is OK to move the initialization of src1
next to that of src2; that may make the result easier to read.

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


[PATCH v4 2/2] mergetools/p4merge: create a base if none available

2013-03-24 Thread Kevin Bracey
Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:

   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"

Commit 0a0ec7bd changed this to:

   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances with
similar inputs.

Unfortunately this approach produces much worse results on differing
inputs. P4Merge really regards the blank file as the base, and once you
have just a couple of differences between the two branches you end up
with one a massive full-file conflict. The 3-way diff is not readable,
and you have to invoke "difftool MERGE_HEAD HEAD" manually to get a
useful view.

The original approach appears to have invoked special 2-way merge
behaviour in P4Merge that occurs only if the base filename is "" or
equal to the left input.  You get a good visual comparison, and it does
not auto-resolve differences. (Normally if one branch matched the base,
it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being
able to reliably save. Having base==left appears to be triggering other
assumptions. There are tricks the user can use to force the save icon
on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion:
generate a virtual base, consisting of the lines common to the two
branches. This is the same as the technique used in resolve and octopus
merges, so we relocate that code to a shared function.

Note that if there are no differences at the same location, this
technique can lead to automatic resolution without conflict, combining
everything from the 2 files.  As with the other merges using this
technique, we assume the user will inspect the result before saving.

Signed-off-by: Kevin Bracey 
Reviewed-by: David Aguilar 
---
Minor change from v3: that version moved initialisation of src1 higher up,
detaching it from its associated comment. This move was only required by
earlier versions, so v4 leaves src1 in its original position.

Added Reviewed-by footer.

 Documentation/git-sh-setup.txt |  6 ++
 git-merge-one-file.sh  | 18 +-
 git-sh-setup.sh| 12 
 mergetools/p4merge |  6 +-
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 6a9f66d..5d709d0 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -82,6 +82,12 @@ get_author_ident_from_commit::
outputs code for use with eval to set the GIT_AUTHOR_NAME,
GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE variables for a given commit.
 
+create_virtual_base::
+   modifies the first file so only lines in common with the
+   second file remain. If there is insufficient common material,
+   then the first file is left empty. The result is suitable
+   as a virtual base input for a 3-way merge.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 3373c04..255c07a 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,30 +104,22 @@ case "${1:-.}${2:-.}${3:-.}" in
;;
esac
 
-   src2=`git-unpack-file $3`
+   src2=$(git-unpack-file $3)
case "$1" in
'')
echo "Added $4 in both, but differently."
-   # This extracts OUR file in $orig, and uses git apply to
-   # remove lines that are unique to ours.
-   orig=`git-unpack-file $2`
-   sz0=`wc -c <"$orig"`
-   @@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
-   sz1=`wc -c <"$orig"`
-
-   # If we do not have enough common material, it is not
-   # worth trying two-file merge using common subsections.
-   expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+   orig=$(git-unpack-file $2)
+   create_virtual_base "$orig" "$src2"
;;
*)
echo "Auto-merging $4"
-   orig=`git-unpack-file $1`
+   orig=$(git-unpack-file $1)
;;
esac
 
# Be careful for funny filename such as "-L" in "$4", which
# would confuse "merge" greatly.
-   src1=`git-unpack-file $2`
+   src1=$(git-unpack-file $2)
git merge-file "$src1" "$orig" "$src2"
ret=$?
msg=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 9cfbe7f..2f78359 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -249,6 +249,18 @@ clear_local_git_env() {
unset $(git rev-parse --local-env-vars)
 }
 
+# Generate a virtual base file for a two-file merge. Uses git apply to
+# remove lines from $1 that are not in $2, leaving only common lines.
+create_virtual_base() {
+   sz0=$(wc -c <"$1")
+   @@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
+   sz1=$(wc -c <"$1")
+
+   # If we do n

[PATCH v3 2/3] mergetools/p4merge: create a base if none available

2013-03-12 Thread Kevin Bracey
Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:

   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"

Commit 0a0ec7bd changed this to:

   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances with
similar inputs.

Unfortunately this approach produces much worse results on differing
inputs. P4Merge really regards the blank file as the base, and once you
have just a couple of differences between the two branches you end up
with one a massive full-file conflict. The 3-way diff is not readable,
and you have to invoke "difftool MERGE_HEAD HEAD" manually to get a
useful view.

The original approach appears to have invoked special 2-way merge
behaviour in P4Merge that occurs only if the base filename is "" or
equal to the left input.  You get a good visual comparison, and it does
not auto-resolve differences. (Normally if one branch matched the base,
it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being
able to reliably save. Having base==left appears to be triggering other
assumptions. There are tricks the user can use to force the save icon
on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion:
generate a virtual base, consisting of the lines common to the two
branches. This is the same as the technique used in resolve and octopus
merges, so we relocate that code to a shared function.

Note that if there are no differences at the same location, this
technique can lead to automatic resolution without conflict, combining
everything from the 2 files.  As with the other merges using this
technique, we assume the user will inspect the result before saving.

Signed-off-by: Kevin Bracey 
---
 Documentation/git-sh-setup.txt |  6 ++
 git-merge-one-file.sh  | 18 +-
 git-sh-setup.sh| 12 
 mergetools/p4merge |  6 +-
 4 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 6a9f66d..5d709d0 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -82,6 +82,12 @@ get_author_ident_from_commit::
outputs code for use with eval to set the GIT_AUTHOR_NAME,
GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE variables for a given commit.
 
+create_virtual_base::
+   modifies the first file so only lines in common with the
+   second file remain. If there is insufficient common material,
+   then the first file is left empty. The result is suitable
+   as a virtual base input for a 3-way merge.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index f612cb8..0f164e5 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,30 +104,22 @@ case "${1:-.}${2:-.}${3:-.}" in
;;
esac
 
-   src2=`git-unpack-file $3`
+   src1=$(git-unpack-file $2)
+   src2=$(git-unpack-file $3)
case "$1" in
'')
echo "Added $4 in both, but differently."
-   # This extracts OUR file in $orig, and uses git apply to
-   # remove lines that are unique to ours.
-   orig=`git-unpack-file $2`
-   sz0=`wc -c <"$orig"`
-   @@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
-   sz1=`wc -c <"$orig"`
-
-   # If we do not have enough common material, it is not
-   # worth trying two-file merge using common subsections.
-   expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+   orig=$(git-unpack-file $2)
+   create_virtual_base "$orig" "$src2"
;;
*)
echo "Auto-merging $4"
-   orig=`git-unpack-file $1`
+   orig=$(git-unpack-file $1)
;;
esac
 
# Be careful for funny filename such as "-L" in "$4", which
# would confuse "merge" greatly.
-   src1=`git-unpack-file $2`
git merge-file "$src1" "$orig" "$src2"
ret=$?
msg=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 795edd2..349a5d4 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -249,6 +249,18 @@ clear_local_git_env() {
unset $(git rev-parse --local-env-vars)
 }
 
+# Generate a virtual base file for a two-file merge. Uses git apply to
+# remove lines from $1 that are not in $2, leaving only common lines.
+create_virtual_base() {
+   sz0=$(wc -c <"$1")
+   @@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
+   sz1=$(wc -c <"$1")
+
+   # If we do not have enough common material, it is not
+   # worth trying two-file merge using common subsections.
+   expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
+}
+
 
 # Platform specific tweaks to work around some commands
 case $(uname -s) in
diff --git a/mergetool

Re: [PATCH v2 2/3] mergetools/p4merge: create a base if none available

2013-03-09 Thread Junio C Hamano
Kevin Bracey  writes:

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 795edd2..aa9a732 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -249,6 +249,19 @@ clear_local_git_env() {
>   unset $(git rev-parse --local-env-vars)
>  }
>  
> +# Generate a virtual base file for a two-file merge. On entry the
> +# base file $1 should be a copy of $2. Uses git apply to remove
> +# lines from $1 that are not in $3, leaving only common lines.
> +create_virtual_base() {
> + sz0=$(wc -c <"$1")
> + @@DIFF@@ -u -La/"$1" -Lb/"$1" "$2" "$3" | git apply --no-add
> + sz1=$(wc -c <"$1")
> +
> + # If we do not have enough common material, it is not
> + # worth trying two-file merge using common subsections.
> + expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
> +}
> +

This rewrite is wrong.  It should be

> + sz0=$(wc -c <"$1")
> + @@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$3" | git apply --no-add
> + sz1=$(wc -c <"$1")

for it to make sense.  "diff $1 $3" is a change to go from $1 to $3;
with "-La/$1 -Lb/$1", we declare that the change is to be applied to
$1, and use --no-add to only use the removal from the diff when we
edit $1 using this mechanism.

The end effect is to in-place edit "$1" to remove what is not common
with "$3", and sz0/sz1 computation is done on "$1" for this reason.
Does it (i.e. "$1") shrink sufficiently when we remove the material
that is not common in it (i.e. "$1") and "$3"?

This part is a two-file operation between $1 and $3; there is
nothing you would want to pass $2 to influence what the above three
lines do.

It may happen that the caller has two copies of the same thing,
$orig and $src1, and uses one for $1 and the other for $2, so you
won't observe the damage from the incorrect rewriting of the above
logic, but it invites the next caller to incorrectly feed something
totally unrelated to $1 and $2.

Please fix it to a function that takes two temporary paths, not
three.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2 2/3] mergetools/p4merge: create a base if none available

2013-03-09 Thread Kevin Bracey
Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:

   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"

Commit 0a0ec7bd changed this to:

   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances with
similar inputs.

Unfortunately this approach produces much worse results on differing
inputs. P4Merge really regards the blank file as the base, and once you
have just a couple of differences between the two branches you end up
with one a massive full-file conflict. The 3-way diff is not readable,
and you have to invoke "difftool MERGE_HEAD HEAD" manually to get a
useful view.

The original approach appears to have invoked special 2-way merge
behaviour in P4Merge that occurs only if the base filename is "" or
equal to the left input.  You get a good visual comparison, and it does
not auto-resolve differences. (Normally if one branch matched the base,
it would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being
able to reliably save. Having base==left appears to be triggering other
assumptions. There are tricks the user can use to force the save icon
on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion:
generate a virtual base, consisting of the lines common to the two
branches. This is the same as the technique used in resolve and octopus
merges, so we relocate that code to a shared function.

Note that if there are no differences at the same location, this
technique can lead to automatic resolution without conflict, combining
everything from the 2 files.  As with the other merges using this
technique, we assume the user will inspect the result before saving.

Signed-off-by: Kevin Bracey 
---
 git-merge-one-file.sh | 18 +-
 git-sh-setup.sh   | 13 +
 mergetools/p4merge|  6 +-
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index f612cb8..1236fbf 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -104,30 +104,22 @@ case "${1:-.}${2:-.}${3:-.}" in
;;
esac
 
-   src2=`git-unpack-file $3`
+   src1=$(git-unpack-file $2)
+   src2=$(git-unpack-file $3)
case "$1" in
'')
echo "Added $4 in both, but differently."
-   # This extracts OUR file in $orig, and uses git apply to
-   # remove lines that are unique to ours.
-   orig=`git-unpack-file $2`
-   sz0=`wc -c <"$orig"`
-   @@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
-   sz1=`wc -c <"$orig"`
-
-   # If we do not have enough common material, it is not
-   # worth trying two-file merge using common subsections.
-   expr $sz0 \< $sz1 \* 2 >/dev/null || : >$orig
+   orig=$(git-unpack-file $2)
+   create_virtual_base "$orig" "$src1" "$src2"
;;
*)
echo "Auto-merging $4"
-   orig=`git-unpack-file $1`
+   orig=$(git-unpack-file $1)
;;
esac
 
# Be careful for funny filename such as "-L" in "$4", which
# would confuse "merge" greatly.
-   src1=`git-unpack-file $2`
git merge-file "$src1" "$orig" "$src2"
ret=$?
msg=
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 795edd2..aa9a732 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -249,6 +249,19 @@ clear_local_git_env() {
unset $(git rev-parse --local-env-vars)
 }
 
+# Generate a virtual base file for a two-file merge. On entry the
+# base file $1 should be a copy of $2. Uses git apply to remove
+# lines from $1 that are not in $3, leaving only common lines.
+create_virtual_base() {
+   sz0=$(wc -c <"$1")
+   @@DIFF@@ -u -La/"$1" -Lb/"$1" "$2" "$3" | git apply --no-add
+   sz1=$(wc -c <"$1")
+
+   # If we do not have enough common material, it is not
+   # worth trying two-file merge using common subsections.
+   expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$1"
+}
+
 
 # Platform specific tweaks to work around some commands
 case $(uname -s) in
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 46b3a5a..16ae0cc 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -21,7 +21,11 @@ diff_cmd () {
 
 merge_cmd () {
touch "$BACKUP"
-   $base_present || >"$BASE"
+   if ! $base_present
+   then
+   cp -- "$LOCAL" "$BASE"
+   create_virtual_base "$BASE" "$LOCAL" "$REMOTE"
+   fi
"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
check_unchanged
 }
-- 
1.8.2.rc3.7.g77aeedb

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


Re: [PATCH 2/2] p4merge: create a virtual base if none available

2013-03-07 Thread Kevin Bracey

On 07/03/2013 04:23, David Aguilar wrote:

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey  wrote:

+make_virtual_base() {
+   # Copied from git-merge-one-file.sh.

I think the reasoning behind these patches is good.

How do we feel about this duplication?

Bad.

Should we make a common function in the git-sh-setup.sh,
or is it okay to have a slightly modified version of this
function in two places?
I'd prefer to have a common function, I just didn't know if there was 
somewhere appropriate to place it, available from both files. And I'm 
going to have to learn a bit more sh to get it right.

Also, the "@@DIFF@@" string may not work here.
This is a template string that is replaced by the Makefile.


It does work in git-mergetool--lib.sh, but not in mergetools/p4merge.


We prefer $(command) instead of `command`.
These should be adjusted.

Can the same thing be accomplished using "git diff --no-index"
so that we do not need a dependency on an external "diff" command here?
Do these comments still apply if it's a common function in 
git-sh-setup.sh that git-one-merge-file.sh will use? I'm wary of 
layering violations.


Kevin


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


Re: [PATCH 2/2] p4merge: create a virtual base if none available

2013-03-06 Thread Junio C Hamano
David Aguilar  writes:

> How do we feel about this duplication?
> Should we make a common function in the git-sh-setup.sh,
> or is it okay to have a slightly modified version of this
> function in two places?

It probably is a good idea to have it in one place.  That would also
solve the @@DIFF@@ replacement issue you noticed at the same time.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] p4merge: create a virtual base if none available

2013-03-06 Thread David Aguilar
On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey  wrote:
> +make_virtual_base() {
> +   # Copied from git-merge-one-file.sh.
> +   # This starts with $LOCAL, and uses git apply to
> +   # remove lines that are not in $REMOTE.
> +   cp -- "$LOCAL" "$BASE"
> +   sz0=`wc -c <"$BASE"`
> +   @@DIFF@@ -u -L"a/$BASE" -L"b/$BASE" "$BASE" "$REMOTE" | git 
> apply --no-add
> +   sz1=`wc -c <"$BASE"`
> +
> +   # If we do not have enough common material, it is not
> +   # worth trying two-file merge using common subsections.
> +   expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$BASE"
> +}

This seems to be indented deeper then the other functions
(or gmail is whitespace damaging my view).
Please use one hard tab to indent here.

We prefer $(command) instead of `command`.
These should be adjusted.

Also, the "@@DIFF@@" string may not work here.
This is a template string that is replaced by the Makefile.

I don't think the tools in the mergetools/ directory go through
cmd_munge_script so this is not going to work as-is.

Can the same thing be accomplished using "git diff --no-index"
so that we do not need a dependency on an external "diff" command here?


I am not a regular p4merge user myself so I'll defer to others
on the cc: list for their thoughts.  It does seem like a good idea, though.
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] p4merge: create a virtual base if none available

2013-03-06 Thread David Aguilar
On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey  wrote:
> Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:
>
>p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
>
> Commit 0a0ec7bd changed this to:
>
>p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"
>
> to avoid the problem of being unable to save in some circumstances.
>
> Unfortunately this approach does not produce good results at all on
> differing inputs. P4Merge really regards the blank file as the base, and
> once you have just a couple of differences between the two branches you
> end up with one a massive full-file conflict. The diff is not readable,
> and you have to invoke "difftool MERGE_HEAD HEAD" manually to see a
> 2-way diff.
>
> The original form appears to have invoked special 2-way comparison
> behaviour that occurs only if the base filename is "" or equal to the
> left input.  You get a good diff, and it does not auto-resolve in one
> direction or the other. (Normally if one branch equals the base, it
> would autoresolve to the other branch).
>
> But there appears to be no way of getting this 2-way behaviour and being
> able to reliably save. Having base=left appears to be triggering other
> assumptions. There are tricks the user can use to force the save icon
> on, but it's not intuitive.
>
> So we now follow a suggestion given in the original patch's discussion:
> generate a virtual base, consisting of the lines common to the two
> branches. It produces a much nicer 3-way diff view than either of the
> original forms, and than I suspect other mergetools are managing.
>
> Signed-off-by: Kevin Bracey 
> ---
>  git-mergetool--lib.sh | 14 ++
>  mergetools/p4merge|  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index e338be5..5b60cf5 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -108,6 +108,20 @@ check_unchanged () {
> fi
>  }
>
> +make_virtual_base() {
> +   # Copied from git-merge-one-file.sh.

I think the reasoning behind these patches is good.

How do we feel about this duplication?
Should we make a common function in the git-sh-setup.sh,
or is it okay to have a slightly modified version of this
function in two places?

> +   # This starts with $LOCAL, and uses git apply to
> +   # remove lines that are not in $REMOTE.
> +   cp -- "$LOCAL" "$BASE"
> +   sz0=`wc -c <"$BASE"`
> +   @@DIFF@@ -u -L"a/$BASE" -L"b/$BASE" "$BASE" "$REMOTE" | git 
> apply --no-add
> +   sz1=`wc -c <"$BASE"`
> +
> +   # If we do not have enough common material, it is not
> +   # worth trying two-file merge using common subsections.
> +   expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$BASE"
> +}
> +
>  valid_tool () {
> setup_tool "$1" && return 0
> cmd=$(get_merge_tool_cmd "$1")
> diff --git a/mergetools/p4merge b/mergetools/p4merge
> index 46b3a5a..f0a893b 100644
> --- a/mergetools/p4merge
> +++ b/mergetools/p4merge
> @@ -21,7 +21,7 @@ diff_cmd () {
>
>  merge_cmd () {
> touch "$BACKUP"
> -   $base_present || >"$BASE"
> +   $base_present || make_virtual_base
> "$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
> check_unchanged
>  }
> --
> 1.8.2.rc2.5.g1a80410.dirty
>



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


[PATCH 2/2] p4merge: create a virtual base if none available

2013-03-06 Thread Kevin Bracey
Originally, with no base, Git gave P4Merge $LOCAL as a dummy base:

   p4merge "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"

Commit 0a0ec7bd changed this to:

   p4merge "empty file" "$LOCAL" "$REMOTE" "$MERGED"

to avoid the problem of being unable to save in some circumstances.

Unfortunately this approach does not produce good results at all on
differing inputs. P4Merge really regards the blank file as the base, and
once you have just a couple of differences between the two branches you
end up with one a massive full-file conflict. The diff is not readable,
and you have to invoke "difftool MERGE_HEAD HEAD" manually to see a
2-way diff.

The original form appears to have invoked special 2-way comparison
behaviour that occurs only if the base filename is "" or equal to the
left input.  You get a good diff, and it does not auto-resolve in one
direction or the other. (Normally if one branch equals the base, it
would autoresolve to the other branch).

But there appears to be no way of getting this 2-way behaviour and being
able to reliably save. Having base=left appears to be triggering other
assumptions. There are tricks the user can use to force the save icon
on, but it's not intuitive.

So we now follow a suggestion given in the original patch's discussion:
generate a virtual base, consisting of the lines common to the two
branches. It produces a much nicer 3-way diff view than either of the
original forms, and than I suspect other mergetools are managing.

Signed-off-by: Kevin Bracey 
---
 git-mergetool--lib.sh | 14 ++
 mergetools/p4merge|  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index e338be5..5b60cf5 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -108,6 +108,20 @@ check_unchanged () {
fi
 }
 
+make_virtual_base() {
+   # Copied from git-merge-one-file.sh.
+   # This starts with $LOCAL, and uses git apply to
+   # remove lines that are not in $REMOTE.
+   cp -- "$LOCAL" "$BASE"
+   sz0=`wc -c <"$BASE"`
+   @@DIFF@@ -u -L"a/$BASE" -L"b/$BASE" "$BASE" "$REMOTE" | git 
apply --no-add
+   sz1=`wc -c <"$BASE"`
+
+   # If we do not have enough common material, it is not
+   # worth trying two-file merge using common subsections.
+   expr $sz0 \< $sz1 \* 2 >/dev/null || : >"$BASE"
+}
+
 valid_tool () {
setup_tool "$1" && return 0
cmd=$(get_merge_tool_cmd "$1")
diff --git a/mergetools/p4merge b/mergetools/p4merge
index 46b3a5a..f0a893b 100644
--- a/mergetools/p4merge
+++ b/mergetools/p4merge
@@ -21,7 +21,7 @@ diff_cmd () {
 
 merge_cmd () {
touch "$BACKUP"
-   $base_present || >"$BASE"
+   $base_present || make_virtual_base
"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
check_unchanged
 }
-- 
1.8.2.rc2.5.g1a80410.dirty

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


[PATCH v6 09/16] remote-hg: fake bookmark when there's none

2012-11-03 Thread Felipe Contreras
Or at least no current bookmark.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-helpers/git-remote-hg | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg 
b/contrib/remote-helpers/git-remote-hg
index 9db4b7e..dbe309a 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -26,7 +26,7 @@ import urllib
 # git:
 # Sensible defaults for git.
 # hg bookmarks are exported as git branches, hg branches are prefixed
-# with 'branches/'.
+# with 'branches/', HEAD is a special case.
 #
 # hg:
 # Emulate hg-git.
@@ -430,12 +430,21 @@ def get_branch_tip(repo, branch):
 return heads[0]
 
 def list_head(repo, cur):
-global g_head
+global g_head, bmarks
 
 head = bookmarks.readcurrent(repo)
-if not head:
-return
-node = repo[head]
+if head:
+node = repo[head]
+else:
+# fake bookmark from current branch
+head = cur
+node = repo['.']
+if not node:
+return
+if head == 'default':
+head = 'master'
+bmarks[head] = node
+
 print "@refs/heads/%s HEAD" % head
 g_head = (head, node)
 
-- 
1.8.0

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


[PATCH v5 10/14] remote-hg: fake bookmark when there's none

2012-10-29 Thread Felipe Contreras
Or at least no current bookmark.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-hg/git-remote-hg | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/contrib/remote-hg/git-remote-hg b/contrib/remote-hg/git-remote-hg
index c2efadf..c41ec95 100755
--- a/contrib/remote-hg/git-remote-hg
+++ b/contrib/remote-hg/git-remote-hg
@@ -422,12 +422,20 @@ def list_branch_head(repo, cur):
 g_head = (head, 'branches', repo[tip])
 
 def list_bookmark_head(repo, cur):
-global g_head
+global g_head, bmarks
 
 head = bookmarks.readcurrent(repo)
-if not head:
-return
-node = repo[head]
+if head:
+node = repo[head]
+else:
+# fake bookmark from current branch
+head = cur
+tip = get_branch_tip(repo, head)
+if not tip:
+return
+node = repo[tip]
+bmarks[head] = node
+
 print "@refs/heads/%s HEAD" % head
 g_head = (head, 'bookmarks', node)
 
-- 
1.8.0

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


[PATCH v4 10/13] remote-hg: fake bookmark when there's none

2012-10-27 Thread Felipe Contreras
Or at least no current bookmark.

Signed-off-by: Felipe Contreras 
---
 contrib/remote-hg/git-remote-hg | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/contrib/remote-hg/git-remote-hg b/contrib/remote-hg/git-remote-hg
index 3bb3192..e8e3791 100755
--- a/contrib/remote-hg/git-remote-hg
+++ b/contrib/remote-hg/git-remote-hg
@@ -419,12 +419,20 @@ def list_branch_head(repo, cur):
 g_head = (head, 'branches', repo[tip])
 
 def list_bookmark_head(repo, cur):
-global g_head
+global g_head, bmarks
 
 head = bookmarks.readcurrent(repo)
-if not head:
-return
-node = repo[head]
+if head:
+node = repo[head]
+else:
+# fake bookmark from current branch
+head = cur
+tip = get_branch_tip(repo, head)
+if not tip:
+return
+node = repo[tip]
+bmarks[head] = node
+
 print "@refs/heads/%s HEAD" % head
 g_head = (head, 'bookmarks', node)
 
-- 
1.8.0

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