Re: get_maintainer.pl and .mailmap entries with more than 2 addresses

[Mauro Carvalho Chehab]

Em Wed, 03 Aug 2016 00:17:10 +0200
Florian Mickler  escreveu:

> cc'd mche...@s-opensource.com  (Mauro, is your kernel.org address up?)

Yes, my kernel.org address is up. Better to keep it as the canonical address,
as this is unlikely to change as it is not associated to an e-mail provider.

> 
>  Am Tue, 02 Aug 2016 09:36:21 -0700
> schrieb Joe Perches :
> 
> > Hello Florian.
> > 
> > There is at least an oddity with get_maintainer handling of a
> > .mailmap entry form.
> > 
> > For instance:
> > 
> > Mauro's .mailmap entry is:
> > Mauro Carvalho Chehab  
> >   
> >  
> > 
> > Is this a valid form?
> > 
> > get_maintainer output for Mauro is:
> > 
> > $ ./scripts/get_maintainer.pl drivers/media/ -f
> > Mauro Carvalho Chehab  
> >   
> >  (maintainer:MEDIA INPUT INFRASTRUCTURE
> > (V4L/DVB))
> > 
> > I believe the Mauro's and Shuah's .mailmap entries are improper and
> > should be changed, but I'm not completely aware of git .mailmap
> > handling and the documentation seems weakly specified.
> >   
> 
> Hmm.. looking at Mauros last .mailmap commit it seems like your patch is
> ok for Mauro. 
> 
> Although  and  are probably
> missing? (@Mauro) 

The mche...@brturbo.com.br is indeed an old email that I used on my
first submissions.

I don't know a mywin...@gmail.com address... that looks really weird
on my eyes...

Hmm...

$ git log --author "mywin...@gmail.com"
commit fe8b671306c78a963934cb5d6e354178390b8c87
Author: Mauro Carvalho Chehab 
Date:   Thu Sep 30 14:46:47 2010 -0300

[media] saa7134: port Asus P7131 Hybrid to use the new rc-core

The rc map table were corrected thanks to Giorgio input and tests.

Reported-by: Giorgio Vazzana 
Tested-by: Giorgio Vazzana 
Signed-off-by: Mauro Carvalho Chehab 

I don't remember the dirty details about such patch anymore... too
many years ago. From the custody chain, I _suspect_ that Giorgio
proposed a patch to be applied against the saa7134 driver, but the 
remote controller map table was moved to a separate file about 7
months before such patch by those changesets:

6686fa6917ca V4L/DVB: Break Remote Controller keymaps into modules
77b7422d48cd V4L/DVB: ir-common: move IR tables from ir-keymaps.c to a separate 
file

I likely rewrote his patch against the new driver, but somehow
I mangled with the author name/email. In any case, mywin...@gmail.com
should not be associated with me.
 
> 
> $ git shortlog | grep "^Mauro C"
> Mauro Carvalho Chehab (4404):
> $ git log | grep "^Author:.*Mauro Carvalho Chehab" | sort | uniq -c
>   2 Author: Mauro Carvalho Chehab 
> 146 Author: Mauro Carvalho Chehab 
> 645 Author: Mauro Carvalho Chehab 
> 794 Author: Mauro Carvalho Chehab 
>2015 Author: Mauro Carvalho Chehab 
> 448 Author: Mauro Carvalho Chehab 
> 353 Author: Mauro Carvalho Chehab 

All above are my e-mails. Let's point them all to mche...@kernel.org.

>   1 Author: Mauro Carvalho Chehab 

This one is mangled and doesn't belong to me. So, it shouldn't be
at the .mailmap file.

> 
> 
> 
> Anyway, from a technical viewpoint your patches seem to fix
> the .mailmap entry as the author intended. (See Junio's Email for the
> documantation part) 
> But I would wait for the ack from Mauro and Shuah. 

With the above changes,

Acked-by: Mauro Carvalho Chehab 


Thanks,
Mauro
--
To unsubscribe from this list: send the line "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] gitmodules: document shallow recommendation

[Stefan Beller]

Signed-off-by: Stefan Beller 
---

 This goes on top of origin/sb/submodule-recommend-shallowness.
 I just realized we had an extra page for all the configuration options,
 so let's keep that in sync.
 
 Thanks,
 Stefan 

 Documentation/gitmodules.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index ac70eca..b97b331 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt
@@ -79,6 +79,11 @@ submodule..ignore::
"--ignore-submodule" option. The 'git submodule' commands are not
affected by this setting.
 
+submodule..shallow::
+   When set to true, a clone of this submodule will be performed as a
+   shallow clone unless the user explicitely asks for a non-shallow
+   clone.
+
 
 EXAMPLES
 
-- 
2.9.2.525.g1760797

--
To unsubscribe from this list: send the line "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: get_maintainer.pl and .mailmap entries with more than 2 addresses

[Shuah Khan]

On 08/02/2016 04:26 PM, Joe Perches wrote:
> On Wed, 2016-08-03 at 00:17 +0200, Florian Mickler wrote:
>> cc'd mche...@s-opensource.com  (Mauro, is your kernel.org address up?)
>>
>>  Am Tue, 02 Aug 2016 09:36:21 -0700
>> schrieb Joe Perches :
>>
>>>
>>> Hello Florian.
>>> There is at least an oddity with get_maintainer handling of a
>>> .mailmap entry form.
>>>
>>> For instance:
>>>
>>> Mauro's .mailmap entry is:
>>> Mauro Carvalho Chehab  
>>>   
>>>  
>>>
>>> Is this a valid form?
>>>
>>> get_maintainer output for Mauro is:
>>>
>>> $ ./scripts/get_maintainer.pl drivers/media/ -f
>>> Mauro Carvalho Chehab  
>>>   
>>>  (maintainer:MEDIA INPUT INFRASTRUCTURE
>>> (V4L/DVB))
>>>
>>> I believe the Mauro's and Shuah's .mailmap entries are improper and
>>> should be changed, but I'm not completely aware of git .mailmap
>>> handling and the documentation seems weakly specified.
>>>
>> Hmm.. looking at Mauros last .mailmap commit it seems like your patch is
>> ok for Mauro. 
>>
>> Although  and  are probably
>> missing? (@Mauro) 
>>
>>
>> $ git shortlog | grep "^Mauro C"
>> Mauro Carvalho Chehab (4404):
>> $ git log | grep "^Author:.*Mauro Carvalho Chehab" | sort | uniq -c
>>   2 Author: Mauro Carvalho Chehab 
>> 146 Author: Mauro Carvalho Chehab 
>> 645 Author: Mauro Carvalho Chehab 
>> 794 Author: Mauro Carvalho Chehab 
>>2015 Author: Mauro Carvalho Chehab 
>> 448 Author: Mauro Carvalho Chehab 
>> 353 Author: Mauro Carvalho Chehab 
>>   1 Author: Mauro Carvalho Chehab 
>>
>>
>>
>> Anyway, from a technical viewpoint your patches seem to fix
>> the .mailmap entry as the author intended. (See Junio's Email for the
>> documantation part) 
>> But I would wait for the ack from Mauro and Shuah. 
> 
> As far as I understand, a single entry with just their
> name and preferred email address would work too because
> the name parts are all spelled identically.
> 
> 

I am fine with change to my entry. Thanks for fixing it.

Acked-by: Shuah Khan 

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "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] hashmap: clarify that hashmap_entry can safely be discarded

[Junio C Hamano]

The API documentation said that the hashmap_entry structure to be
embedded in the caller's structure is to be treated as opaque, which
left the reader wondering if it can safely be discarded when it no
longer is necessary.  If the hashmap_entry structure had references
to external resources such as allocated memory or an open file
descriptor, merely free(3)ing the containing structure (when the
caller's structure is on the heap) or letting it go out of scope
(when it is on the stack) would end up leaking the external
resource.

Document that there is no need for hashmap_entry_clear() that
corresponds to hashmap_entry_init() to give the API users a little
bit of peace of mind.

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

 * As I wrote the text already, I thought I'd follow-up with a log
   message, too.

 Documentation/technical/api-hashmap.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-hashmap.txt 
b/Documentation/technical/api-hashmap.txt
index ad7a5bd..28f5a8b 100644
--- a/Documentation/technical/api-hashmap.txt
+++ b/Documentation/technical/api-hashmap.txt
@@ -104,6 +104,11 @@ If `free_entries` is true, each hashmap_entry in the map 
is freed as well
 `entry` points to the entry to initialize.
 +
 `hash` is the hash code of the entry.
++
+The hashmap_entry structure does not hold references to external resources,
+and it is safe to just discard it once you are done with it (i.e. if
+your structure was allocated with xmalloc(), you can just free(3) it,
+and if it is on stack, you can just let it go out of scope).
 
 `void *hashmap_get(const struct hashmap *map, const void *key, const void 
*keydata)`::
 
-- 
2.9.2-708-gdb68231

--
To unsubscribe from this list: send the line "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] apply: mark some file-local symbols static

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Christian,

I had intended to ask you to squash this into your 'cc/apply-am'
branch, specifically commit 4d18b33a (apply: move libified code
from builtin/apply.c to apply.{c,h}, 30-07-2016).

However, having read that commit a little closer, it seems that
you deliberately made these symbols public. The commit message
does not mention this issue at all, and it is not clear to me
why these symbols should be public.

What am I missing?

ATB,
Ramsay Jones


 apply.c | 26 +-
 apply.h | 14 --
 2 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/apply.c b/apply.c
index 96f02fa..ec545f6 100644
--- a/apply.c
+++ b/apply.c
@@ -4743,16 +4743,16 @@ static int apply_patch(struct apply_state *state,
return res;
 }
 
-int apply_option_parse_exclude(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 1);
return 0;
 }
 
-int apply_option_parse_include(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 0);
@@ -4760,9 +4760,9 @@ int apply_option_parse_include(const struct option *opt,
return 0;
 }
 
-int apply_option_parse_p(const struct option *opt,
-const char *arg,
-int unset)
+static int apply_option_parse_p(const struct option *opt,
+   const char *arg,
+   int unset)
 {
struct apply_state *state = opt->value;
state->p_value = atoi(arg);
@@ -4770,8 +4770,8 @@ int apply_option_parse_p(const struct option *opt,
return 0;
 }
 
-int apply_option_parse_space_change(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
if (unset)
@@ -4781,8 +4781,8 @@ int apply_option_parse_space_change(const struct option 
*opt,
return 0;
 }
 
-int apply_option_parse_whitespace(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+const char *arg, int unset)
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
@@ -4791,8 +4791,8 @@ int apply_option_parse_whitespace(const struct option 
*opt,
return 0;
 }
 
-int apply_option_parse_directory(const struct option *opt,
-const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+   const char *arg, int unset)
 {
struct apply_state *state = opt->value;
strbuf_reset(>root);
diff --git a/apply.h b/apply.h
index 66ed0c5..010726e 100644
--- a/apply.h
+++ b/apply.h
@@ -107,20 +107,6 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
-extern int apply_option_parse_exclude(const struct option *opt,
- const char *arg, int unset);
-extern int apply_option_parse_include(const struct option *opt,
- const char *arg, int unset);
-extern int apply_option_parse_p(const struct option *opt,
-   const char *arg,
-   int unset);
-extern int apply_option_parse_whitespace(const struct option *opt,
-const char *arg, int unset);
-extern int apply_option_parse_directory(const struct option *opt,
-   const char *arg, int unset);
-extern int apply_option_parse_space_change(const struct option *opt,
-  const char *arg, int unset);
-
 extern int apply_parse_options(int argc, const char **argv,
   struct apply_state *state,
   int *force_apply, int *options,
-- 
2.9.0

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

Re: [PATCH] apply: mark some file-local symbols static

[Junio C Hamano]

On Tue, Aug 2, 2016 at 3:33 PM, Ramsay Jones
 wrote:
>
> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Christian,
>
> I had intended to ask you to squash this into your 'cc/apply-am'
> branch, specifically commit 4d18b33a (apply: move libified code
> from builtin/apply.c to apply.{c,h}, 30-07-2016).
>
> However, having read that commit a little closer, it seems that
> you deliberately made these symbols public. The commit message
> does not mention this issue at all, and it is not clear to me
> why these symbols should be public.
>
> What am I missing?

Their exports have been made obsolete by the reroll we have
in 'pu' when "builtin/am: use apply api in run_apply()" was
redone in a way not to duplicate the argument parsing.

They should have been cleaned with 4820e13, but I think
Christian did not carefully review the whole series before
sending it out and did not notice that they no longer need
to be extern.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C

[Junio C Hamano]

Junio C Hamano  writes:

> Pranit Bauva  writes:
>
>> Reimplement the `bisect_write` shell function in C and add a
>> `bisect-write` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>
> Up to around this step we've seen these patches well enough and I
> think with another reroll or two, they are in good enough shape to
> be split out and frozen for 'next'.  We may not be there quite yet,
> but I think we are getting pretty close.
>
> Thanks.

By the way, this series applied as a whole seems to break t6030.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Johannes Schindelin  writes:

> I tend to think that the underscore is correct: this change is not so much
> about the builtin (which is written with a dash) but about the function
> (written with an underscore, used by more than just merge-recursive, e.g.
> cherry-pick).

Yes, I agree.  "merge-recursive:" prefix is about either the
built-in command, or the machinery as a whole to support that
built-in command.  It is preferrable to use "merge_recursive():"
if we are talking about a single function.

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: get_maintainer.pl and .mailmap entries with more than 2 addresses

[Joe Perches]

On Wed, 2016-08-03 at 00:17 +0200, Florian Mickler wrote:
> cc'd mche...@s-opensource.com  (Mauro, is your kernel.org address up?)
> 
>  Am Tue, 02 Aug 2016 09:36:21 -0700
> schrieb Joe Perches :
> 
> > 
> > Hello Florian.
> > There is at least an oddity with get_maintainer handling of a
> > .mailmap entry form.
> > 
> > For instance:
> > 
> > Mauro's .mailmap entry is:
> > Mauro Carvalho Chehab  
> >   
> >  
> > 
> > Is this a valid form?
> > 
> > get_maintainer output for Mauro is:
> > 
> > $ ./scripts/get_maintainer.pl drivers/media/ -f
> > Mauro Carvalho Chehab  
> >   
> >  (maintainer:MEDIA INPUT INFRASTRUCTURE
> > (V4L/DVB))
> > 
> > I believe the Mauro's and Shuah's .mailmap entries are improper and
> > should be changed, but I'm not completely aware of git .mailmap
> > handling and the documentation seems weakly specified.
> > 
> Hmm.. looking at Mauros last .mailmap commit it seems like your patch is
> ok for Mauro. 
> 
> Although  and  are probably
> missing? (@Mauro) 
> 
> 
> $ git shortlog | grep "^Mauro C"
> Mauro Carvalho Chehab (4404):
> $ git log | grep "^Author:.*Mauro Carvalho Chehab" | sort | uniq -c
>   2 Author: Mauro Carvalho Chehab 
> 146 Author: Mauro Carvalho Chehab 
> 645 Author: Mauro Carvalho Chehab 
> 794 Author: Mauro Carvalho Chehab 
>    2015 Author: Mauro Carvalho Chehab 
> 448 Author: Mauro Carvalho Chehab 
> 353 Author: Mauro Carvalho Chehab 
>   1 Author: Mauro Carvalho Chehab 
> 
> 
> 
> Anyway, from a technical viewpoint your patches seem to fix
> the .mailmap entry as the author intended. (See Junio's Email for the
> documantation part) 
> But I would wait for the ack from Mauro and Shuah. 

As far as I understand, a single entry with just their
name and preferred email address would work too because
the name parts are all spelled identically.


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

Re: [PATCH 1/2] submodule documentation: add options to the subcommand

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Aug 2, 2016 at 2:45 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> When reading up on a subcommand of `gi submodule`, it is convenient
>>
>> s/gi /git /;
>
> will fix.
>
> And in the neighboring thread you just pointed out you used to just correct
> spelling fixes like this. I think it really depends on the workflow of the
> contributor. As I do the interdiff of the next version of the series against
> your tree I'll be likely to notice such typos in the content, but not in
> commit messages.

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

What's cooking in git.git (Aug 2016, #01; Tue, 2)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

On the 'master' front, the individual commit count now exceeds 400
since the last major release, which is a good pace.  We may want to
start slowing down once the current crop of topics in 'next' hits
the 'master' and switch our attention to regression hunting.  The
'maint' branch has been accumulating enough material to make it the
next maintenance release v2.9.3.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* ab/gitweb-link-html-escape (2016-08-01) 1 commit
 - gitweb: escape link body in format_ref_marker

 The characters in the label shown for tags/refs for commits shown
 in "gitweb" output are now properly escaped for proper HTML output.

 Will merge to 'next'.


* ew/build-time-pager-tweaks (2016-08-01) 1 commit
 . pager: move pager-specific setup into the build

 The build procedure learned PAGER_ENV knob that lists what default
 environment variable settings to export for popular pagers.  This
 mechanism is used to tweak the default settings to MORE on FreeBSD.

 Waiting for a reroll.


* ib/t3700-add-chmod-x-updates (2016-08-01) 3 commits
 - t3700: add a test_mode_in_index helper function
 - t3700: merge two tests into one
 - t3700: remove unwanted leftover files before running new tests

 The t3700 test about "add --chmod=-x" have been made a bit more
 robust and generally cleaned up.

 Will merge to 'next'.


* jt/format-patch-from-config (2016-08-01) 1 commit
 - format-patch: format.from gives the default for --from

 "git format-patch" learned format.from configuration variable to
 specify the default settings for its "--from" option.


* rs/st-mult (2016-08-01) 1 commit
 - pass constants as first argument to st_mult()

 Micro optimization of st_mult() facility used to check the integer
 overflow coming from multiplication to compute size of memory
 allocation.

 Will merge to 'next'.


* rs/use-strbuf-addstr (2016-08-01) 1 commit
 - use strbuf_addstr() for adding constant strings to a strbuf

 Will merge to 'next'.


* sb/submodule-update-dot-branch (2016-08-01) 7 commits
 - submodule update: allow '.' for branch value
 - submodule--helper: add remote-branch helper
 - submodule-config: keep configured branch around
 - submodule--helper: fix usage string for relative-path
 - submodule update: narrow scope of local variable
 - submodule update: respect depth in subsequent fetches
 - t7406: future proof tests with hard coded depth

 A few updates to "git submodule update".

 Will merge to 'next'.


* jc/hashmap-doc-init (2016-08-02) 1 commit
 - hashmap: clarify that hashmap_entry can safely be discarded

 The API documentation for hashmap was unclear if hashmap_entry
 can be safely discarded without any other consideration.  State
 that it is safe to do so.

--
[Stalled]

* jh/clean-smudge-annex (2016-08-01) 9 commits
 - use smudgeToFile filter in recursive merge
 - use smudgeToFile filter in git am
 - better recovery from failure of smudgeToFile filter
 - warn on unusable smudgeToFile/cleanFromFile config
 - use smudgeToFile in git checkout etc
 - use cleanFromFile in git add
 - add smudgeToFile and cleanFromFile filter configs
 - clarify %f documentation
 - Merge branch 'cc/apply-am' into HEAD
 (this branch uses cc/apply-am.)

 The interface to "clean/smudge" filters require Git to feed the
 whole contents via pipe, which is suboptimal for some applications.
 "cleanFromFile/smudgeToFile" commands are the moral equilvalents
 for these filters but they interact with the files on the
 filesystem directly.

 This is starting to bit-rot, as the topic it is built upon keeps
 getting rerolled.  I _think_ I rebased it correctly, but I would
 not be surprised if I made a mistake.

 Will discard if/when I have to do another rebase, preferring to
 have a fresh reroll directly from the author.


* sb/bisect (2016-04-15) 22 commits
 . SQUASH???
 . bisect: get back halfway shortcut
 . bisect: compute best bisection in compute_relevant_weights()
 . bisect: use a bottom-up traversal to find relevant weights
 . bisect: prepare for different algorithms based on find_all
 . bisect: rename count_distance() to compute_weight()
 . bisect: make total number of commits global
 . bisect: introduce distance_direction()
 . bisect: extract get_distance() function from code duplication
 . bisect: use commit instead of commit list as arguments when appropriate
 . bisect: replace clear_distance() by unique markers
 . bisect: use struct node_data array instead of int array
 . bisect: get rid of recursion in count_distance()
 . bisect: 

Re: [PATCH 1/2] submodule documentation: add options to the subcommand

[Stefan Beller]

On Tue, Aug 2, 2016 at 2:45 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> When reading up on a subcommand of `gi submodule`, it is convenient
>
> s/gi /git /;

will fix.

And in the neighboring thread you just pointed out you used to just correct
spelling fixes like this. I think it really depends on the workflow of the
contributor. As I do the interdiff of the next version of the series against
your tree I'll be likely to notice such typos in the content, but not in
commit messages.

>
>> to have its options nearby and not just at the top of the man page.
>> Add the options to each subcommand.
>>
>> While at it, also document the `--checkout` option for `update`.
>
> I do find the resulting per-subcommand description easier to read
> with this change.
>
> Perhaps we want to go one step further and change the SYNOPSIS so
> that per-subcommand options are not described there at all?
> I.e.e.g.
>
> -'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]...
> +'git submodule' [--quiet] update [...]

will do.

My original intention was to get rid of the duplicates in the OPTIONS
section, where each option has

This option is only valid for  and 

in its description. So I looked at the `stash` man page, which has
the options listed with the subcommands (and also has []
in the SYNOPSIS but also some of the options as well).

I think only the long option lists (i.e. those that are more than one line)
will be collapsed. The short options are ok, specifically when you just
want to know e.g. if foreach supports the recursive option. Then you
can open the man page and have no need to scroll down to the foreach
command.

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

Re: [PATCH 1/2] submodule documentation: add options to the subcommand

[Junio C Hamano]

Stefan Beller  writes:

> When reading up on a subcommand of `gi submodule`, it is convenient

s/gi /git /;

> to have its options nearby and not just at the top of the man page.
> Add the options to each subcommand.
>
> While at it, also document the `--checkout` option for `update`.

I do find the resulting per-subcommand description easier to read
with this change.

Perhaps we want to go one step further and change the SYNOPSIS so
that per-subcommand options are not described there at all?
I.e.e.g.

-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]...
+'git submodule' [--quiet] update [...]


> @@ -231,7 +231,7 @@ As an example, +git submodule foreach \'echo $path 
> {backtick}git
>  rev-parse HEAD{backtick}'+ will show the path and currently checked out
>  commit for each submodule.
>  
> -sync::
> +sync [--recursive] [--] [...]::
>   Synchronizes submodules' remote URL configuration setting
>   to the value specified in .gitmodules. It will only affect those
>   submodules which already have a URL entry in .git/config (that is 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

Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Johannes Schindelin  writes:

>> > There are a couple of places where return values never indicated errors
>> > before, as wie simply died instead of returning.
>> 
>> s/wie/we/;
>
> Right. What can I say, I am surrounded by too many Germans.
>
> I fixed this locally, in case another re-roll should be required. What you
> have in `pu` looks correct to me, though. Let me know if you want me to
> re-submit nevertheless.

I usually do this kind of obvious typofix and consistency fix
without even mentioning them in my review comments to reduce the
noise levels.  But that works better ONLY if the patch authors then
fetch from 'pu' and replace their copies with what I fixed up
already and base their reroll on top by amending and/or building on
top (of course, that also requires my local fix must all be limited
to uncontroversial ones).

So either I should change my workflow and mention any and all
typofixes in my review comments (which consumes the review
bandwidth), or I should force patch authors to do the "fetch from
'pu' and replace" somehow to avoid this kind of back-and-forth.

I am not sure which should be the way to go.

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

Re: [PATCH v5 14/16] merge-recursive: offer an option to retain the output in 'obuf'

[Junio C Hamano]

Johannes Schindelin  writes:

>> But in that case, there would be both messages meant for the
>> standard output and also meant for the standard error, and we need
>> some way to make sure they go to the right channel.
>
> Not necessarily. Let's have a look at our existing code in
> git-rebase.sh:
>
>   output () {
>   case "$verbose" in
>   '')
>   output=$("$@" 2>&1 )
>   status=$?
>   test $status != 0 && printf "%s\n" "$output"
>   return $status
>   ;;
>   *)
>   "$@"
>   ;;
>   esac
>   }
>
> This incredibly well-named function (, my fault: dfa49f3 (Shut "git
> rebase -i" up when no --verbose was given, 2007-07-23)) accumulates all
> output, both stdout and stderr, and shows it only in case of an error.
>
> Crucially, *all* output goes to stdout. No distinction is being made
> between stdout and stderr.

> ...
> This is the existing behavior of rebase -i.
> ...
> As such, it would be a serious mistake to implement that mode and use it
> in the rebase--helper: it would very likely cause regressions in existing
> scripts, probably even my own.

Sounds like we are desperately trying to find an excuse to do a
wrong thing by finding an existing piece of code that did a wrong
thing already.

That leaves a bad taste in my mouth, but as "rebase -i" is meant to
be an "interactive" command, I would imagine that nobody would have
expected to run it as "git rebase -i >/dev/null" in order to view
only the error messages (or vice versa with "2>errs").

So OK then, at least for now.

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

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Eric Wong  writes:

> So v3 will be MORE=FRX, as less was added:
>
> commit 98170c0c3ba86eb1cc975e7848d075bf2abc1ed0
> Author: ps 
> Date:   Mon May 22 10:00:00 2000 +
>
>   bmake glue for less.
>
> and more was nuked:
>
> commit cde9059fa3e4dc7e259c3864d7536252a5c580a0
> Author: ps 
> Date:   Mon May 29 13:31:51 2000 +
>
>   Nuke more from the repository.
>
> And "git branch -r --contains" on both of those commits says
> they showed up in the 5.0 release.  However, further
> investigation says more was even gone by the 4.1.0 release
>
>   git show origin/release/4.1.0:usr.bin/more # non-existent tree
>   git show origin/release/4.0.0:usr.bin/more # tree still exists
>
> But, "git show origin/release/4.0.0:usr.bin/more/option.c"
> reveals more from those days wouldn't handle -R anyways,
> and hopefully nobody is still running 4.0.0...

OK.  And they can always set $MORE on their own to work it around,
or we can do a release-dependent tweak when somebody screams.

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

Re: [PATCH 1/1 v2] pager: move pager-specific setup into the build

[Junio C Hamano]

Jeff King  writes:

> On Mon, Aug 01, 2016 at 04:03:40PM -0700, Junio C Hamano wrote:
>
>> Eric Wong  writes:
>> 
>> > From: Junio C Hamano 
>> >
>> > Allowing PAGER_ENV to be set at build-time allows us to move
>> > pager-specific knowledge out of our build.  Currently, this
>> > allows us to set a better default for FreeBSD where more(1)
>> > is the same binary as less(1).
>> 
>> Thanks for resurrecting, but I am not sure what "a better default"
>> is from the above description and with the patch.  Even though a
>> naive reading of the above (i.e. "less" and "more" are the same)
>> makes me expect that the patch will give the same set of default
>> environment settings to those on FreeBSD, you give LESS=FRX and
>> MORE=-R, i.e. they are configured differently.
>
> I wondered about this, too. They are the same binary, but calling less
> as "more" (or setting LESS_IS_MORE) causes less to behave "like more".

I guessed that, but if that is the case, "more is the same binary"
is irrelevant.  "more" behaves differently from "less" might be, but
what "less" does is much less important than "more needs this default
setting to work pleasantly", which is what is missing.

So I'd say

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  This allows us to
set a better default for FreeBSD more(1), which misbehaves if
MORE environment variable is left empty $in_such_and_such_way,
by defaulting it to -R.

without even mentioning anything about "less" may be a more
understandable justification for a patch that sets MORE only on
FreeBSD.


--
To unsubscribe from this list: send the line "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 bisect for reachable commits only

[Junio C Hamano]

Oleg Taranenko  writes:

> First, assuming the common ancestor is GOOD based on the fact that
> some descendant given as GOOD is pretty bad idea.

What you claim is fundamentally incompatible with the way "bisect"
works as a O(log(n)) operation.  It is likely that your definition
of Good for the purpose of your bug-hunting needs to be rethought if
you want to take advantage of "bisect".

> I have another request to get git bisect more user-friendly, regarding
> rolling back last step or steps, if accidentally 'git bisect bad' or
> 'good' was wrong entered, but I think it worth for another thread.

Are you aware that you can check $GIT_DIR/BISECT_LOG and replay it
to recreate any previous state of the bisection?  That would
probably help.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 13/13] bisect--helper: `bisect_start` shell function partially in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, j, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flag;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for(i = 0; i < argc; i++) {

SP after for.

> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
> + if (!strcmp(argv[i], "--term-good")) {
> + must_write_terms = 1;
> + strbuf_reset(>term_good);
> + strbuf_addstr(>term_good, argv[++i]);
> + break;
> + }
> + if (!strcmp(argv[i], "--term-bad")) {
> + must_write_terms = 1;
> + strbuf_reset(>term_bad);
> + strbuf_addstr(>term_bad, argv[++i]);
> + break;
> + }

The original was not careful, either, but what if the user ends the
command line with "--term-good", without anything after it?

Also the original is prepared to handle --term-good=boa; because
this function can be be called directly from the UI (i.e. "git
bisect start --term-good=boa"), not supporting that form would be
seen as a regression.

> + if (starts_with(argv[i], "--") &&
> + !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
> + string_list_clear(, 0);
> + string_list_clear(, 0);
> + die(_("unrecognised option: '%s'"), argv[i]);
> + }
> + if (get_oid(argv[i], ) || has_double_dash) {

Calling get_oid() alone is insufficient to make sure argv[i] refers
to an existing object that is a committish.  The "^{commit}" suffix
in the original is there for a reason.

> + string_list_clear(, 0);
> + string_list_clear(, 0);

You seem to want the revs list really really clean ;-)

> + die(_("'%s' does not appear to be a valid revision"), 
> argv[i]);
> + }
> + else
> + string_list_append(, oid_to_hex());
> + }
> +
> + for (j = 0; j < revs.nr; j++) {

Why "j", not "i", as clearly the previous loop has finished at this
point?  The only reason why replacing "j" with "i" would make this
function buggy would be if a later part of this function depended on
the value of "i" when the control left the above loop, but if that
were the case (I didn't check carefully), such a precious value that
has long term effect throughout the remainder of the function must
not be kept in an otherwise throw-away loop counter variable "i".

Introduce a new "int pathspec_pos" and set it to "i" immediately
after the "for (i = 0; i < argc; i++) { ... }" loop above, perhaps.

> + struct strbuf state = STRBUF_INIT;
> + /*
> +  * The user ran "git bisect start  ", hence
> +  * did not explicitly specify the terms, but we are already
> +  * starting to set references named with the default terms,
> +  * and won't be able to change afterwards.
> +  */
> + must_write_terms = 1;
> +
> + if (bad_seen)
> + strbuf_addstr(, terms->term_good.buf);
> + else {
> + bad_seen = 1;
> + strbuf_addstr(, terms->term_bad.buf);
> + }
> + string_list_append(, state.buf);
> + strbuf_release();
> + }

How about this instead?

/*
 * that comment block goes here
 */
must_write_terms = !!revs.nr;
for (i = 0; i < revs.nr; i++) {
if (bad_seen)
string_list_append(, terms->term_good.buf);
else
string_list_append(, terms->term_bad.buf);
}

> +
> + /*
> +  * Verify HEAD
> +  */
> + head = resolve_ref_unsafe("HEAD", 0, sha1, );

The last parameter is a set of flag bits, so call it flags.

> + if (!head) {
> + if (get_sha1("HEAD", sha1)) {
> + string_list_clear(, 0);
> + string_list_clear(, 0);
> + die(_("Bad HEAD - I need a HEAD"));

We see many repeated calls to clear these two string lists before
exiting with failure, either by dying or return -1.

I wonder how bad the resulting code would look like if we employed
the standard pattern of having a "fail_return:" label at the end of
the function (after the "return" 

Re: [RFC/PATCH v11 09/13] bisect--helper: `bisect_write` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> Reimplement the `bisect_write` shell function in C and add a
> `bisect-write` subcommand to `git bisect--helper` to call it from
> git-bisect.sh

Up to around this step we've seen these patches well enough and I
think with another reroll or two, they are in good enough shape to
be split out and frozen for 'next'.  We may not be there quite yet,
but I think we are getting pretty close.

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 v3 03/10] pkt-line: add packet_flush_gentle()

[Torsten Bögershausen]

On Sun, Jul 31, 2016 at 11:45:08PM +0200, Lars Schneider wrote:
> 
> > On 31 Jul 2016, at 22:36, Torstem Bögershausen  wrote:
> > 
> > 
> > 
> >> Am 29.07.2016 um 20:37 schrieb larsxschnei...@gmail.com:
> >> 
> >> From: Lars Schneider 
> >> 
> >> packet_flush() would die in case of a write error even though for some 
> >> callers
> >> an error would be acceptable.
> > What happens if there is a write error ?
> > Basically the protocol is out of synch.
> > Lenght information is mixed up with payload, or the other way
> > around.
> > It may be, that the consequences of a write error are acceptable,
> > because a filter is allowed to fail.
> > What is not acceptable is a "broken" protocol.
> > The consequence schould be to close the fd and tear down all
> > resources. connected to it.
> > In our case to terminate the external filter daemon in some way,
> > and to never use this instance again.
> 
> Correct! That is exactly what is happening in kill_protocol2_filter()
> here:

Wait a second.
Is kill the same as shutdown ?
I would expect that
The process terminates itself as soon as it detects EOF.
As there is nothing more read.

Then the next question: The combination of kill & protocol in kill_protocol(),
what does it mean ?
Is it more like a graceful shutdown_protocol() ?

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

Re: [RFC/PATCH v11 12/13] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int bisect_terms(struct bisect_terms *terms, int term_defined)
> +{
> + if (get_terms(terms)) {
> + fprintf(stderr, "no terms defined\n");
> + return -1;
> + }
> + if (!term_defined) {
> + printf("Your current terms are %s for the old state\nand "
> +"%s for the new state.\n", terms->term_good.buf,
> +terms->term_bad.buf);
> + return 0;
> + }

In the original, all of the above messages go through gettext; this
rewrite should do the same.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 11/13] bisect--helper: `bisect_next_check` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int mark_good(const char *refname, const struct object_id *oid,
> +  int flag, void *cb_data)
> +{
> + int *m_good = (int *)cb_data;
> + *m_good = 0;
> + return 0;
> +}

See below.

> +static int bisect_next_check(const struct bisect_terms *terms,
> +  const char *current_term)
> +{
> + int missing_good = 1, missing_bad = 1;

It is somewhat unusual to start with "assume we are OK" and then
"it turns out that we are not".

> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
> + char *good_glob = xstrfmt("%s*", terms->term_good.buf);

The original runs

git for-each-ref "refs/bisect/$TERM_GOOD-*

but this one lacks the final dash.

> + if (ref_exists(bad_ref))
> + missing_bad = 0;
> + free(bad_ref);
> +
> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +  (void *) _good);
> + free(good_glob);

The for-each helper does not return until it iterates over all the
matching refs, but you are only interested in seeing if at least one
exists.  It may make sense to return 1 from mark_good() to terminate
the traversal early.

> + if (!missing_good && !missing_bad)
> + return 0;
> +
> + if (!current_term)
> + return -1;
> +
> + if (missing_good && !missing_bad && current_term &&
> + !strcmp(current_term, terms->term_good.buf)) {
> + char *yesno;
> + /*
> +  * have bad (or new) but not good (or old). We could bisect
> +  * although this is less optimum.
> +  */
> + fprintf(stderr, "Warning: bisecting only with a %s commit\n",
> + terms->term_bad.buf);

In the original, this message goes through gettext.

> + if (!isatty(0))
> + return 0;
> + /*
> +  * TRANSLATORS: Make sure to include [Y] and [n] in your
> +  * translation. The program will only accept English input
> +  * at this point.
> +  */
> + yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
> + if (starts_with(yesno, "N") || starts_with(yesno, "n"))
> + return -1;
> + return 0;
> + }

When the control falls into the above if(){} block, the function
will always return.  It will clarify that this is the end of such a
logical block to have a blank line here.

> + if (!is_empty_or_missing_file(git_path_bisect_start()))
> + return error(_("You need to give me at least one good|old and "
> + "bad|new revision. You can use \"git bisect "
> + "bad|new\" and \"git bisect good|old\" for "
> + "that. \n"));
> + else
> + return error(_("You need to start by \"git bisect start\". "
> + "You then need to give me at least one good|"
> + "old and bad|new revision. You can use \"git "
> + "bisect bad|new\" and \"git bisect good|old\" "
> + " for that.\n"));

The i18n on these two messages seem to be different from the
original, which asks bisect_voc to learn what 'bad' and 'good' are
called and attempts to use these words from the vocabulary.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 10/13] bisect--helper: `check_and_set_terms` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> Reimplement the `check_and_set_terms` shell function in C and add
> `check-and-set-terms` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
>
> Using `--check-and-set-terms` subcommand is a temporary measure to port
> shell function in C so as to use the existing test suite. As more
> functions are ported, this subcommand will be retired and will be called
> by some other methods.

I think "this subcommand will be retired but its implementation will
be called by ..." would clarify the direction.

> + if (!no_term_file &&
> + strcmp(cmd, terms->term_bad.buf) &&
> + strcmp(cmd, terms->term_good.buf))
> + return error(_("Invalid command: you're currently in a "
> + "'%s' '%s' bisect"), terms->term_bad.buf,

This changes a message text, switching from "... good/bad bisect."
to "... 'good' 'bad' bisect".  Intended?

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

Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int bisect_clean_state(void)
> +{
> + int result = 0;
> +
> + /* There may be some refs packed during bisection */
> + struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> + for_each_ref_in("refs/bisect/", mark_for_removal, (void *) 
> _for_removal);
> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
> + result = delete_refs(_for_removal);
> + refs_for_removal.strdup_strings = 1;
> + string_list_clear(_for_removal, 0);
> + remove_path(git_path_bisect_expected_rev());
> + remove_path(git_path_bisect_ancestors_ok());
> + remove_path(git_path_bisect_log());
> + remove_path(git_path_bisect_names());
> + remove_path(git_path_bisect_run());
> + remove_path(git_path_bisect_terms());
> + /* Cleanup head-name if it got left by an old version of git-bisect */
> + remove_path(git_path_head_name());
> +  * Cleanup BISECT_START last to support the --no-checkout option
> +  * introduced in the commit 4796e823a.
> +  */
> + remove_path(git_path_bisect_start());

I can see that refs/files-backend.c misuses it already, but
remove_path() helper is about removing a path in the working tree,
together with any parent directory that becomes empty due to the
removal.  You do not expect $GIT_DIR/ to become an empty directory
after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
if it becomes empty.  It is a wrong helper function to use here.

Also you do not seem to check the error from the function to smudge
the "result" you are returning from this function.

Isn't unlink_or_warn() more correct helper to use here?

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

Re: [RFC/PATCH v11 08/13] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> + for (i = 0; i < rev_nr; i++) {
> + if (!is_expected_rev(revs[i])) {
> + remove_path(git_path_bisect_ancestors_ok());
> + remove_path(git_path_bisect_expected_rev());
> + return 0;

Same comment on the use of this helper function applies here, too.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 07/13] bisect--helper: `bisect_reset` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> + if (!file_exists(git_path_bisect_head())) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + argv_array_pushl(, "checkout", branch.buf, "--", NULL);
> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> + error(_("Could not check out original HEAD '%s'. Try "
> + "'git bisect reset '."), branch.buf);

Somebody seems to have a keen eye.  Looks much better with a space
after "Try" ;-)
--
To unsubscribe from this list: send the line "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] submodule update documentation: don't repeat ourselves

[Stefan Beller]

The documentation for the `git submodule update` command, repeats itself
for each update option, "This is done when  is given, or no
option is given and `submodule..update` is set to .

Avoid these repetitive clauses by stating the command line options take
precedence over configured options.

Also add 'none' to the list of options instead of mentioning it in the
following running text and split the list into two parts, one that is
accessible via the command line and one that is only reachable via the
configuration variables.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1452c31..9b5abaf 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -158,13 +158,13 @@ Update the registered submodules to match what the 
superproject
 expects by cloning missing submodules and updating the working tree of
 the submodules. The "updating" can be done in several ways depending
 on command line options and the value of `submodule..update`
-configuration variable. Supported update procedures are:
+configuration variable. The command line option takes precedence over
+the configuration variable. if neither is given, a checkout is performed.
+update procedures supported both from the command line as well as setting
+`submodule..update`:
 
checkout;; the commit recorded in the superproject will be
-   checked out in the submodule on a detached HEAD. This is
-   done when `--checkout` option is given, or no option is
-   given, and `submodule..update` is unset, or if it is
-   set to 'checkout'.
+   checked out in the submodule on a detached HEAD.
 +
 If `--force` is specified, the submodule will be checked out (using
 `git checkout --force` if appropriate), even if the commit specified
@@ -172,23 +172,21 @@ in the index of the containing repository already matches 
the commit
 checked out in the submodule.
 
rebase;; the current branch of the submodule will be rebased
-   onto the commit recorded in the superproject. This is done
-   when `--rebase` option is given, or no option is given, and
-   `submodule..update` is set to 'rebase'.
+   onto the commit recorded in the superproject.
 
merge;; the commit recorded in the superproject will be merged
-   into the current branch in the submodule. This is done
-   when `--merge` option is given, or no option is given, and
-   `submodule..update` is set to 'merge'.
+   into the current branch in the submodule.
+
+The following procedures are only available via the `submodule..update`
+configuration variable:
 
custom command;; arbitrary shell command that takes a single
argument (the sha1 of the commit recorded in the
-   superproject) is executed. This is done when no option is
-   given, and `submodule..update` has the form of
-   '!command'.
+   superproject) is executed. When `submodule..update`
+   is set to '!command', the remainder after the exclamation mark
+   is the custom command.
 
-When no option is given and `submodule..update` is set to 'none',
-the submodule is not updated.
+   none;; the submodule is not updated.
 
 If the submodule is not yet initialized, and you just want to use the
 setting as stored in .gitmodules, you can automatically initialize the
-- 
2.9.2.525.g1760797

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

[PATCH 1/2] submodule documentation: add options to the subcommand

[Stefan Beller]

When reading up on a subcommand of `gi submodule`, it is convenient
to have its options nearby and not just at the top of the man page.
Add the options to each subcommand.

While at it, also document the `--checkout` option for `update`.

Signed-off-by: Stefan Beller 
---

  This mini series applies to master, there no other patches in flight
  that touch Documentation/git-submodules.txt except
  sb/submodule-default-paths, which we are holding back currently.
  
  Thanks,
  Stefan

 Documentation/git-submodule.txt | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bf3bb37..1452c31 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
+ [--[no-]recommend-shallow] [-f|--force] 
[--checkout|--rebase|--merge]
  [--reference ] [--depth ] [--recursive]
  [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
@@ -62,7 +62,7 @@ if you choose to go that route.
 
 COMMANDS
 
-add::
+add [-b ] [-f|--force] [--name ] [--reference ] 
[--depth ] [--]  []::
Add the given repository as a submodule at the given path
to the changeset to be committed next to the current
project: the current project is termed the "superproject".
@@ -103,7 +103,7 @@ together in the same relative location, and only the
 superproject's URL needs to be provided: git-submodule will correctly
 locate the submodule using the relative URL in .gitmodules.
 
-status::
+status [--cached] [--recursive] [--] [...]::
Show the status of the submodules. This will print the SHA-1 of the
currently checked out commit for each submodule, along with the
submodule path and the output of 'git describe' for the
@@ -120,7 +120,7 @@ submodules with respect to the commit recorded in the index 
or the HEAD,
 linkgit:git-status[1] and linkgit:git-diff[1] will provide that information
 too (and can also report changes to a submodule's work tree).
 
-init::
+init [--] [...]::
Initialize the submodules recorded in the index (which were
added and committed elsewhere) by copying submodule
names and urls from .gitmodules to .git/config.
@@ -135,7 +135,7 @@ init::
the explicit 'init' step if you do not intend to customize
any submodule locations.
 
-deinit::
+deinit [-f|--force] (--all|[--] ...)::
Unregister the given submodules, i.e. remove the whole
`submodule.$name` section from .git/config together with their work
tree. Further calls to `git submodule update`, `git submodule foreach`
@@ -151,7 +151,7 @@ instead of deinit-ing everything, to prevent mistakes.
 If `--force` is specified, the submodule's working tree will
 be removed even if it contains local modifications.
 
-update::
+update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] 
[-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth 
] [--recursive] [--jobs ] [--] [...]::
 +
 --
 Update the registered submodules to match what the superproject
@@ -197,7 +197,7 @@ submodule with the `--init` option.
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
-summary::
+summary [--cached|--files] [(-n|--summary-limit) ] [commit] [--] 
[...]::
Show commit summary between the given commit (defaults to HEAD) and
working tree/index. For a submodule in question, a series of commits
in the submodule between the given super project commit and the
@@ -210,7 +210,7 @@ summary::
 Using the `--submodule=log` option with linkgit:git-diff[1] will provide that
 information too.
 
-foreach::
+foreach [--recursive] ::
Evaluates an arbitrary shell command in each checked out submodule.
The command has access to the variables $name, $path, $sha1 and
$toplevel:
@@ -231,7 +231,7 @@ As an example, +git submodule foreach \'echo $path 
{backtick}git
 rev-parse HEAD{backtick}'+ will show the path and currently checked out
 commit for each submodule.
 
-sync::
+sync [--recursive] [--] [...]::
Synchronizes submodules' remote URL configuration setting
to the value specified in .gitmodules. It will only affect those
submodules which already have a URL entry in .git/config (that is the
-- 
2.9.2.525.g1760797

--
To unsubscribe from this list: send the line "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: get_maintainer.pl and .mailmap entries with more than 2 addresses

[Junio C Hamano]

Joe Perches  writes:

> Hello Florian.
>
> There is at least an oddity with get_maintainer handling of a
> .mailmap entry form.
>
> For instance:
>
> Mauro's .mailmap entry is:
> Mauro Carvalho Chehab   
>    
>  
>
> Is this a valid form?

I do not think so, according to "git shortlog --help" (the canonical
source of the document is Documentation/mailmap.txt, but shortlog
doc includes it).  Here is the relevant bits.

In the simple form, each line in the file consists of the canonical
real name of an author, whitespace, and an email address used in the
commit (enclosed by '<' and '>') to map to the name. For example:
--
Proper Name 
--

The more complex forms are:
--
 
--
which allows mailmap to replace only the email part of a commit, and:
--
Proper Name  
--
which allows mailmap to replace both the name and the email of a
commit matching the specified commit email address, and:
--
Proper Name  Commit Name 
--
which allows mailmap to replace both the name and the email of a
commit matching both the specified commit name and email address.

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

Re: [[PATCH v2] 1/4] patch-ids: stop using a hand-rolled hashmap implementation

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> Oh, we are already safely in Unrelated Tangent Land for a while, I would
>> think. Nothing of what we are discussing in this thread has anything to do
>> with Kevin's patch series,...
>
> Oh, no question about that.  Go back to my review, to which your
> message I am responding to is a reply to.  What you wrote are all
> about things after "This is a tangent, but this made me wonder if it
> is safe to simply free(3) the result...", which pointed out rough
> points in the hashmap API from the API user's point of view and
> suggested a few possible improvement opportunities.

In other words, I'd be happy with a patch like this, outside the
scope of and independent from this series.

When the hashmap_entry structure does acquire references to external
resources (which I wouldn't judge the likelihood of), this paragraph
will become stale, but that is exactly the point at which _clear()
function needs to be added to the API and described here, replacing
this paragraph.

I do not mind having an empty _clear() before that happens, but I do
not think it adds much safety.  A disciplined user of the API may
call that empty _clear() to make her code future-proof, but we know
there are undisciplined developers and reviewers and there will be
codepaths that call _init() without calling the empty _clear(), and
we won't notice it.  Whoever is adding the real need for _clear()
must audit the codebase at that point _anyway_, and that is why I
think having an empty _clear() before would not buy us much.

 Documentation/technical/api-hashmap.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-hashmap.txt 
b/Documentation/technical/api-hashmap.txt
index ad7a5bd..1dcec3d 100644
--- a/Documentation/technical/api-hashmap.txt
+++ b/Documentation/technical/api-hashmap.txt
@@ -104,6 +104,11 @@ If `free_entries` is true, each hashmap_entry in the map 
is freed as well
 `entry` points to the entry to initialize.
 +
 `hash` is the hash code of the entry.
++
+The hashmap_entry structure does not hold references to external resources,
+and it is safe to just discard it once you are done with it (i.e. if
+your structure was allocated with xmalloc(), you can just free(3) it,
+and if it is on stack, you can just let it go out of scope).
 
 `void *hashmap_get(const struct hashmap *map, const void *key, const void 
*keydata)`::
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 02/13] bisect: rewrite `check_term_format` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +/*
> + * Check whether the string `term` belongs to the set of strings
> + * included in the variable arguments.
> + */
> +static int one_of(const char *term, ...)
> +{
> + int res = 0;
> + va_list matches;
> + const char *match;
> +
> + va_start(matches, term);
> + while (!res && (match = va_arg(matches, const char *)))
> + res = !strcmp(term, match);
> + va_end(matches);
> +
> + return res;
> +}

It might be safer to mark this function with LAST_ARG_MUST_BE_NULL,
but because this is static to this function, it may not matter too
much.  Just an observation, not a strong suggestion to change the
patch.

> +static int check_term_format(const char *term, const char *orig_term)
> +{
> + int res;
> + char *new_term = xstrfmt("refs/bisect/%s", term);
> +
> + res = check_refname_format(new_term, 0);
> + free(new_term);

Yup, that looks much more straight-forward than using a one-time-use
strbuf.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 03/13] bisect--helper: `write_terms` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int write_terms(const char *bad, const char *good)
> +{
> + FILE *fp;
> + int res;
> +
> + if (!strcmp(bad, good))
> + return error(_("please use two different terms"));
> +
> + if (check_term_format(bad, "bad") || check_term_format(good, "good"))
> + return -1;
> +
> + fp = fopen(git_path_bisect_terms(), "w");
> + if (!fp)
> + return error_errno(_("could not open the file BISECT_TERMS"));
> +
> + res = fprintf(fp, "%s\n%s\n", bad, good);
> + res |= fclose(fp);
> + return (res < 0) ? -1 : 0;
> +}

If fprintf(3) were a function that returns 0 on success and negative
on error (like fclose(3) is), the pattern to cascade the error
return with "res |= another_call()" is appropriate, but the made me
hiccup a bit while reading it.  It is not wrong per-se and it would
certainly be making it worse if we did something silly like

res = fprintf(...) < 0 ? -1 : 0;
res |= fclose(fp);

so I guess what you have is the most succinct way to do this.

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

Re: git bisect for reachable commits only

[Stefan Beller]

On Tue, Aug 2, 2016 at 3:15 AM, Oleg Taranenko  wrote:
> Guys,
>
> thanks for discussion, I will try to reply in bulk here.
> First, assuming the common ancestor is GOOD based on the fact that
> some descendant given as GOOD is pretty bad idea.
> It may be, but may not be. In the git-flow like workflows new features
> (aka branches) are created from trunk (master/develop/...)
> sporadically,
> but later they will mutual merging. I would say more probably they
> have not common base, then have.

git bisect has the underlying assumption that the BUG is not there initially
and introduced by one specific commit, and continues to be there until B.
With this assumption you can do a binary search, which allows searching
in O(log n), which is optimal for the number of iterations needed.

>
> Second, I don't ask "create a new algorithm to find all transition
> from good/old to bad/new", not nesessary. If programmer feels
> something
> suspicious, he/she can create another bisect session with narrowed commit 
> range.
>
> Third, testing of any specific commit can be very expensive operation.
> In my case - shutdown servers/refresh dbs/clean/rebuild in
> eclipse/running servers/dropping browser cache/running app in
> browser/going through some pages/view UI.
> Some of steps of course are automated, but some not. Anyway I spend
> 5-10 min for every iteration. So knowing what commit is bad or good is
> very valuable, then I'm very interested to hunt the bug-introduced
> commit with minimal count of testing.


As you point out each iteration is a burden to the user, so they should be
kept to a minimum.

>
> Scenario 4 (I will keep my previous mail numbering for possible later 
> reference)
>  z1z2---z3
> / /  \
> Gx1x2/---x3---x4--B
>   \ /   /
>y1--y2--y3--y4
>
> This is the happy straight case with closed DAG (hehe, git for
> scientists) between given G good and B bad commits.
> Ideal bisect will check first the shortest way between G & B:
> x1/x2/x3/x4. Let name first-bug commit we are really hunting H and
> current first-bug candidate as h.
> If h == x1 or x2 -> stop, found
> If h == x3, bisect will try to test y2/y3/y4 path only
> If h == x4, bisect will select shortest path z1/z2 (keeping in mind,
> that x2 is already tested and is good)
>   If h == z1 - found
>   if h == z2 - looking in path y1/y2/y3

* git is agnostic of the workflow, i.e. it doesn't know the notion of
topic branches or such.
* What is the worst case in you strategy?
  (h=y4 means 7 tests by the user IIUC)

Given the assumptions as laid out above such that we are able to
do a binary search, the ideal candidate for scenario 4 is
y4 or z3 as these split the set of commits to be investigated into
2 equally sized sets of ancestors and non-ancestors.

When a specific workflow is given, it may make sense to weight
commits differently. Also some people asked for a strategy that only
checks merge commits first, as there are far fewer merge commits and
these generally are used for introducing a new feature or refactoring.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH v11 01/13] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Junio C Hamano]

Pranit Bauva  writes:

> `--next-all` is meant to be used as a subcommand to support multiple
> "operation mode" though the current implementation does not contain any
> other subcommand along side with `--next-all` but further commits will
> include some more subcommands.

Sounds sensible.

As long as the dispatch happens inside cmd_bisect__helper() itself,
limiting the enum definition local to the function also looks like a
good thing to do (and I do not see a reason why we need the world
outside this function to know about the enum in the future).



> Helped-by: Johannes Schindelin 
> Mentored-by: Lars Schneider 
> Mentored-by: Christian Couder 
> Signed-off-by: Pranit Bauva 
> ---


>  builtin/bisect--helper.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 3324229..8111c91 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
>  
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> - int next_all = 0;
> + enum { NEXT_ALL = 1 } cmdmode = 0;
>   int no_checkout = 0;
>   struct option options[] = {
> - OPT_BOOL(0, "next-all", _all,
> -  N_("perform 'git bisect next'")),
> + OPT_CMDMODE(0, "next-all", ,
> +  N_("perform 'git bisect next'"), NEXT_ALL),
>   OPT_BOOL(0, "no-checkout", _checkout,
>N_("update BISECT_HEAD instead of checking out the 
> current commit")),
>   OPT_END()
> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
> char *prefix)
>   argc = parse_options(argc, argv, prefix, options,
>git_bisect_helper_usage, 0);
>  
> - if (!next_all)
> + if (!cmdmode)
>   usage_with_options(git_bisect_helper_usage, options);
>  
> - /* next-all */
> - return bisect_next_all(prefix, no_checkout);
> + switch (cmdmode) {
> + case NEXT_ALL:
> + return bisect_next_all(prefix, no_checkout);
> + default:
> + die("BUG: unknown subcommand '%d'", cmdmode);
> + }
> + return 0;
>  }
>
> --
> https://github.com/git/git/pull/281
--
To unsubscribe from this list: send the line "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] t7063: work around FreeBSD's lazy mtime update feature

[Junio C Hamano]

Duy Nguyen  writes:

> OK how about this squashed in? The name was taken from fbsd definition
> IN_LAZYMOD.

I am sorry that I didn't spot this possiblity earlier, but do we
need anything conditional?  Either FREEBSD or LAZYMOD prerequisite
tells very little what the "Work around lazy mtime update" is and
where it triggers (I think the conclusion of your investigation was
that the timestamp on the containing directory, but that is ONLY
recorded in the log message, i.e. readers need to run 'git blame'
to find out).

It might be a better approach to have a helper function with
descriptive name and comment early in t7063, e.g.

# On some filesystems (e.g. FreeBSD's ext2 and ufs) this
# and that happens when we do blah, which forces the
# untracked cache code to take the slow path.  A test
# that wants to make sure the fast path works correctly
# should call this helper to make mtime of the containing
# directory in sync with the reality after doing blah and
# before checking the fast path behaviour
test_sync_directory_mtime () {
ls -ld . >/dev/null
}

and then call it at strategic places without any prerequisite.

The helper may turn out to be useful outside the context of 7063
later, at which time we can move it to test-lib-functions, but that
is a separate step.

> Off topic. Since I found this macro defined twice, in ext2 and ufs,
> but not in zfs (found its source!), I assume zfs does not have this
> particular feature (but I didn't test it). Untracked cache may be more
> effecient there.


> -- 8< --
> diff --git a/t/t7063-status-untracked-cache.sh 
> b/t/t7063-status-untracked-cache.sh
> index 08fc586..8bb048a 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -419,7 +419,7 @@ test_expect_success 'create/modify files, some of which 
> are gitignored' '
>   rm base
>  '
>  
> -test_expect_success FREEBSD 'Work around lazy mtime update' '
> +test_expect_success LAZYMOD 'Work around lazy mtime update' '
>   ls -ld . >/dev/null
>  '
>  
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3c730a2..1fc5266 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -962,7 +962,7 @@ case $(uname -s) in
>   test_set_prereq GREP_STRIPS_CR
>   ;;
>  *FreeBSD*)
> - test_set_prereq FREEBSD
> + test_set_prereq LAZYMOD
>   test_set_prereq POSIXPERM
>   test_set_prereq BSLASHPSPEC
>   test_set_prereq EXECKEEPSPID
> -- 8< --
>
> --
> Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

[Junio C Hamano]

Johannes Schindelin  writes:

>> Perhaps hashmap API needs fixing in the longer term not to call this
>> type hashmap_cmp_fn; instead it should lose cmp and say something
>> like hashmap_eq_fn or something.
>
> Maybe.
>
> But to make sure: you do not expect Kevin to do that in the context of
> this here patch series, right?

I think I already answered this in the message you are responding
to.

> Do you want a note in the commit message about this "abuse" of a negative
> return value, or a code comment?

I do not think negative (or non-zero) return is an "abuse" at all.
It is misleading in the context of the function whose name has "cmp"
in it, but that is not the fault of this function, rather, the
breakage is more in the API that calls a function that wants to know
only equality a "cmp".  A in-code comment before the function name
may be appropriate:

/*
 * hashmap API calls hashmap_cmp_fn, but it only wants
 * "does the key match the entry?" with 0 (matches) and
 * non-zero (does not match).
 */
static int patch_id_match(const struct patch_id *ent,
  const struct patch_id *key,
  const void *keydata)
{
...

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

Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

[Junio C Hamano]

Jakub Narębski  writes:

> The problem is that one expects hashmap_cmp_fn() to return ==0 on equality,
> while one would expect for hashmap_eq_fn() to return true (==1) on equality.
> So we would have to rewrite all calling sites.

Yes, and I do not think it is a "problem".  There only are about a
dozen callsites of hashmap_init().

> It would be nice to have a comment about how hashmap uses cmpfn in
> hashmap.h.  

That is the absolute minimum but I think that is already done in the
Documentation/technical/api-hashmap.txt.

> Though... currently our hashmap implementation uses linked
> list (separate chaining) for handling hash collisions (for collision
> resolution). More sophisticated data structures, such as balanced search
> trees,...

But that requires total ordering of the elements registered in a
hashmap.  So far, because cmp_fn that was misnamed was only about
equality, you can safely use a hashmap to store things that do not
have inherent order among them.  Besides, if your hashmap has to
optimize the hash collision resolution part with complex strucure,
your hash function is bad or your hash bucket growing strategy is
suboptimal, or both, which is the first thing you should look at,
not the "now how would we find it in the bucket with too many
things?"

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

Re: [[PATCH v2] 1/4] patch-ids: stop using a hand-rolled hashmap implementation

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Mon, 1 Aug 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > It would be a serious bug if hashmap_entry_init() played games with
>> > references, given its signature (that this function does not have any
>> > access to the hashmap structure, only to the entry itself):
>> >
>> >void hashmap_entry_init(void *entry, unsigned int hash)
>> 
>> I do not think we are on the same page.  The "reference to other
>> resource" I wondered was inside the hashmap_entry structure, IOW,
>> "the entry itself".
>
> Oh, I see now.
>
>> Which is declared to be opaque to the API users,
>
> Actually, not really. We cannot do that in C: we need to define the struct
> in hashmap.h so that its size is known to the users.

What I meant was what Documentation/technical/api-hashmap.txt said.
I know that we cannot embed a true "opaque" structure in C just as
you do.

> That is the reason, I guess, why we have the documentation in
> Documentation/technical/api-hashmap.txt: it would have to talk about your
> hypothetical hashmap_entry_clear() (which would better be named
> *_release() BTW, unless I misunderstood what you want a hypothetical
> future version of that function to do).

I think there is no misunderstanding on your part.  I am following
the conclusion (as I recall) of a discussion on the list not in so
distant past about X_free(x) that frees the resource an instance of
"struct X" holds and also the instance itself, and X_clear(x) that
only frees the resources without freeing the instance (the latter of
which allows x to be on stack, you do X_init() and conclude with
X_clear()).  The name X_clear() is more in line with existing API
functions like string_list_clear(), argv_array_clear().  An oddball
is strbuf_release(), which I think made you make the above comment.

>> I have a slight preference to avoid the lazy "void *", but that is
>> an unrelated tangent.
>
> Oh, we are already safely in Unrelated Tangent Land for a while, I would
> think. Nothing of what we are discussing in this thread has anything to do
> with Kevin's patch series,...

Oh, no question about that.  Go back to my review, to which your
message I am responding to is a reply to.  What you wrote are all
about things after "This is a tangent, but this made me wonder if it
is safe to simply free(3) the result...", which pointed out rough
points in the hashmap API from the API user's point of view and
suggested a few possible improvement opportunities.

>> I am saying that an uneven over-enginnering is bad.
>
> Hmm. I guess that the _init() function could be replaced by an _INIT macro
> a la STRBUF_INIT. Not sure it is really worth the effort, though.

I do not think so, as X_init() and X_INIT() from the point of view
of the API user would not make much difference; as long as the
documentation does not say it is safe to make it go out of scope
without "deinitialize" it, the reader will be left wondering.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

get_maintainer.pl and .mailmap entries with more than 2 addresses

[Joe Perches]

Hello Florian.

There is at least an oddity with get_maintainer handling of a
.mailmap entry form.

For instance:

Mauro's .mailmap entry is:
Mauro Carvalho Chehab   
   
 

Is this a valid form?

get_maintainer output for Mauro is:

$ ./scripts/get_maintainer.pl drivers/media/ -f
Mauro Carvalho Chehab   
   
 (maintainer:MEDIA INPUT INFRASTRUCTURE (V4L/DVB))

I believe the Mauro's and Shuah's .mailmap entries are improper and
should be changed, but I'm not completely aware of git .mailmap
handling and the documentation seems weakly specified.

https://git-scm.com/docs/git-check-mailmap

Maybe get_maintainer.pl needs a change or perhaps this patch
is  appropriate.
---
 .mailmap | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index c0d5704..c7f92ca 100644
--- a/.mailmap
+++ b/.mailmap
@@ -96,7 +96,12 @@ Linus Lüssing  

 Linus Lüssing  
 Mark Brown 
 Matthieu CASTET 
-Mauro Carvalho Chehab   
   
 
+Mauro Carvalho Chehab  
+Mauro Carvalho Chehab  
+Mauro Carvalho Chehab  
+Mauro Carvalho Chehab  
+Mauro Carvalho Chehab  
+Mauro Carvalho Chehab  
 Matt Ranostay  Matthew Ranostay 

 Matt Ranostay  
 Mayuresh Janorkar 
@@ -132,7 +137,10 @@ Santosh Shilimkar 
 Sascha Hauer 
 S.Çağlar Onur 
 Shiraz Hashim  
-Shuah Khan    
 
+Shuah Khan  
+Shuah Khan  
+Shuah Khan  
+Shuah Khan  
 Simon Kelley 
 Stéphane Witzmann 
 Stephen Hemminger 

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

Re: [PATCH v3 7/8] status: update git-status.txt for --porcelain=v2

[Jeff Hostetler]

On 08/02/2016 11:19 AM, Jakub Narębski wrote:

W dniu 01.08.2016 o 17:39, Jeff Hostetler pisze:

On 07/30/2016 01:22 PM, Jakub Narębski wrote:

W dniu 26.07.2016 o 23:11, Jeff Hostetler pisze:

This is a nice change, available because of lack of backward
compatibility with v1.  The porcelain v2 format branch-related
information could be enhanced without risk of breaking parsers,
or having new information put at the end even if it does not fit
there (like in previous iteration).

One thing that can serve as goal for the series is using the
question: would it make __git_ps1() from git-prompt.sh be able
to render fully decorated prompt with all bells and whistles,
and with all combinations of options.  Thus beside upstream
in the future we might want SVN upstream, and/or pushed-to
remote branch (and remote push upstream repository), etc.

But that's for the future (and it is possible for the future)...



Yes, I was hoping to be able to simplify and/or speed up
__git_ps1() with this data.  "Namespacing" the branch data
here.  And then later add the state data (in a merge,
in a rebase, and etc.) in a series of "# state.*" headers.
And so on, until we get everything that __git_ps1() needs.
However, to really make that work, we might want to add
a --no-scan (or minimial scan) option, to just return the
header data, since __git_ps1() doesn't care about the list
of changes.


But __git_ps1() with GIT_PS1_SHOWDIRTYSTATE would want to know
if there are unstaged and staged changes, and with
GIT_PS1_SHOWUNTRACKEDFILES it would want to know if there
are untracked (unknown) files, isn't it?

And GIT_PS1_SHOWSTASHSTATE would want to know if there is
something stashed, but I guess it is outside the remit of
git-status...

Also, __git_ps1() uses colors from git-status, so if it
is switched to use --porcelain=v2, then there should be an
option to turn the color on, isn't it?



All of these are good questions.  But __git_ps1() is outside my
scope right now.  All I was saying above is that by making
the branch details available here and, later, extending V2
output by adding other such headers to the output, we could
try to remove much of the business logic in __git_ps1() and
hopefully reduce it to just formatting and coloring the prompt
using the computed result from status.

But I've only skimmed __git_ps1() (and it's a little dense),
so I'd have to study it more before I could answer all of your
questions.

Thanks,
Jeff




--
To unsubscribe from this list: send the line "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: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

[Stephen Morton]

On 2016-07-25 6:22 PM, Jeff King wrote:

On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote:


I have always assumed the post-receive hook to be executed whenever a commit
is "accepted" by the (gitolite) server. That does not seem to be true any
more.

Generally, yeah, I would expect that to be the case, too.


Since 9658846 is appears that, when a client bails out, the pre-receive hook
continues to run and the commit is written to the repository, but no
post-receive hook is executed. No signal of any kind is received in the
hook, not even a sig pipe when the post- hook is writing to stdout whilst
the client has disconnected.

I see. The problem is that cmd_receive_pack() does this:

 execute_commands(commands, unpack_status, );
 if (pack_lockfile)
 unlink_or_warn(pack_lockfile);
 if (report_status)
 report(commands, unpack_status);
 run_receive_hook(commands, "post-receive", 1);
 run_update_post_hook(commands);

It reports the status to the client, and _then_ runs the post-receive
hook. But if that reporting fails (either because of an error, or if we
just get SIGPIPE because the client hung up), then we don't actually run
the hooks.

Leaving 9658846 out of it entirely, it is always going to be racy
whether we notice that the client hung up during the pre-receive step.
E.g.:

   - your pre-receive might not write any output, so the muxer has
 nothing to write to the other side, and we never notice that the
 connection closed until we write the status out in report()

   - if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not
 a sub-thread. And thus we don't know of a hangup except by checking
 the result of finish_async(), which we never do.

   - the client could hang up just _after_ we've written the pre-receive
 output, but before report() is called, so there's nothing to notice
 until we're in report()

So I think 9658846 just made that race a bit longer, because it means
that a write error in the sideband muxer during the pre-receive hook
means we return an error via finish_async() rather than unceremoniously
calling exit() from a sub-thread. So we have a longer period during
which we might actually finish off execute_commands() but not make it
out of report().

And the real solution is to make cmd_receive_pack() more robust, and try
harder to run the hooks even when the client hangs up or we have some
other reporting error (because getting data back to the user is only one
use of post-receive hooks; they are also used to queue jobs or do
maintenance).

But that's a bit tricky, as it requires report() to ignore SIGPIPE, and
to stop using write_or_die() or any other functions that can exit (some
of which happen at a lower level). Plus if a client does hangup, we
don't want our hook to die with SIGPIPE either, so we'd want to proxy
the data into /dev/null.

-Peff


Sounds tricky. I do think it's important, almost a 'data integrity' 
issue, that IF a commit is received, THEN the post-receive hook must be 
run. Too much mission-critical stuff is based on post-receive hooks.


The alternatives, as I see them --either document that the post-receive 
hook cannot be fully trusted and that all such uses must change to 
asynchronous polling, or otherwise just say that nobody should hit 
Ctrl-C during a push (not even reflexively when their lizard-brain says 
"Woops, no!") and hope that network issues don't cause the same thing-- 
are simply not realistic.


Stephen



--
To unsubscribe from this list: send the line "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] t7063: work around FreeBSD's lazy mtime update feature

[Duy Nguyen]

On Mon, Aug 01, 2016 at 02:04:44PM -0700, Junio C Hamano wrote:
> Duy Nguyen  writes:
> 
> > On Mon, Aug 1, 2016 at 3:37 AM, Torstem Bögershausen  wrote:
> >> the term FREEBSD may be too generic to point out a single feature
> >> in an OS distributution.
> >> Following your investigations, it may even be possible that
> >> other systems adapt this "feature"?
> >>
> >> How about
> >> LAZY_DIR_MTIME_UPDATE
> >> (or similar)
> >
> > This feature was added in 1998, so yes there's a chance it has spread
> > to a few fbsd derivatives (not sure if openbsd or netbsd is close
> > enough and they ever exchange changes). But I'd rather wait for the
> > second OS to expose the same feature before renaming it.
> 
> I think a name based on the observed behaviour ("feature") would be
> more appropriate because I'd be more worried about us finding other
> glitches we see (initially) only on FBSD.  People who need to adjust
> tests that use the same FBSD prereq would have to wonder which
> prereq-skip is due to which glitch.

OK how about this squashed in? The name was taken from fbsd definition
IN_LAZYMOD.

Off topic. Since I found this macro defined twice, in ext2 and ufs,
but not in zfs (found its source!), I assume zfs does not have this
particular feature (but I didn't test it). Untracked cache may be more
effecient there.

-- 8< --
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index 08fc586..8bb048a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -419,7 +419,7 @@ test_expect_success 'create/modify files, some of which are 
gitignored' '
rm base
 '
 
-test_expect_success FREEBSD 'Work around lazy mtime update' '
+test_expect_success LAZYMOD 'Work around lazy mtime update' '
ls -ld . >/dev/null
 '
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 3c730a2..1fc5266 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -962,7 +962,7 @@ case $(uname -s) in
test_set_prereq GREP_STRIPS_CR
;;
 *FreeBSD*)
-   test_set_prereq FREEBSD
+   test_set_prereq LAZYMOD
test_set_prereq POSIXPERM
test_set_prereq BSLASHPSPEC
test_set_prereq EXECKEEPSPID
-- 8< --

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

Re: [PATCH v3 7/8] status: update git-status.txt for --porcelain=v2

[Jakub Narębski]

W dniu 01.08.2016 o 17:39, Jeff Hostetler pisze:
> On 07/30/2016 01:22 PM, Jakub Narębski wrote:
>> W dniu 26.07.2016 o 23:11, Jeff Hostetler pisze:
>>
>> This is a nice change, available because of lack of backward
>> compatibility with v1.  The porcelain v2 format branch-related
>> information could be enhanced without risk of breaking parsers,
>> or having new information put at the end even if it does not fit
>> there (like in previous iteration).
>>
>> One thing that can serve as goal for the series is using the
>> question: would it make __git_ps1() from git-prompt.sh be able
>> to render fully decorated prompt with all bells and whistles,
>> and with all combinations of options.  Thus beside upstream
>> in the future we might want SVN upstream, and/or pushed-to
>> remote branch (and remote push upstream repository), etc.
>>
>> But that's for the future (and it is possible for the future)...
>>
> 
> Yes, I was hoping to be able to simplify and/or speed up
> __git_ps1() with this data.  "Namespacing" the branch data
> here.  And then later add the state data (in a merge,
> in a rebase, and etc.) in a series of "# state.*" headers.
> And so on, until we get everything that __git_ps1() needs.
> However, to really make that work, we might want to add
> a --no-scan (or minimial scan) option, to just return the
> header data, since __git_ps1() doesn't care about the list
> of changes.

But __git_ps1() with GIT_PS1_SHOWDIRTYSTATE would want to know
if there are unstaged and staged changes, and with
GIT_PS1_SHOWUNTRACKEDFILES it would want to know if there
are untracked (unknown) files, isn't it?

And GIT_PS1_SHOWSTASHSTATE would want to know if there is
something stashed, but I guess it is outside the remit of
git-status...
 
Also, __git_ps1() uses colors from git-status, so if it
is switched to use --porcelain=v2, then there should be an
option to turn the color on, isn't it?

-- 
Jakub Narębski


--
To unsubscribe from this list: send the line "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: Un-paged commit messages in git filter-branch's commit-filter?

[Stefan Tauner]

On Mon, 1 Aug 2016 17:36:31 -0400
Jeff King  wrote:

> On Sun, Jul 31, 2016 at 06:39:35PM +0200, Stefan Tauner wrote:
> 
> > > There are some output formats that will wrap lines, but by default,
> > > filter-branch should not be using them (and I could not reproduce the
> > > issue in a simple test). Can you show us what your commit-filter looks
> > > like?  
> > 
> > Thanks for your answer. I have tried to reproduce it in other (newly
> > created) repositories but failed. However, it seems to relate to some
> > kind of persistent paging setting, is that possible?
> > git config -l does not show anything suspicious.
> > 
> > The following commands produce paged output:
> > git show hash
> > git show --pretty=%B
> > git log hash^..hash
> > Commit message in gitk
> > 
> > 
> > These do NOT produce paged output:
> > git patch hash^..hash
> > Commit message in gitg 0.2.7  
> 
> What is "git patch"? An alias for "format-patch?".

Yes, sorry.
And this is the most amazing thing about this behavior... what's so
different between format-patch and log or show --pretty=%B. Shouldn't
these match 100%?

> 
> > This is the script I tried to use to reproduce the problem:
> > 
> > #!/bin/bash
> > export LC_ALL=C
> > input=$(cat)
> > echo "===
> > $input
> > ===" >> /tmp/paging_bug.txt
> > git commit-tree "$@" -m "$input"  
> 
> Can you be more specific about the input you're feeding to git and the
> output you're seeing?
> 
> For instance, if I do:
> 
>   git init
>   echo content >file
>   git add file
>   git commit -m "$(perl -e 'print join(" ", 1..100)')"
> 
> I get a commit message with one long unwrapped line, which I can view
> via git-log, etc.

That's approximately what I did in my tests as well. And like you, when
I do this in a fresh repository, it works like that..

> Now if I try to run filter-branch on that:
> 
>   git filter-branch --commit-filter '
>   input=$(cat)
>   {
>   echo ""
>   echo $input
>   echo ""
>   } >>/tmp/paging_bug.txt
>   git commit-tree "$@" -m "$input"
>   '
> 
> then the commit remains unchanged, and paging_bug shows one long line.

as well as filter-branch. That's what I meant when I wrote I cannot
reproduce it with a new repository (to create a MWE). I wrote the first
mail under the presumption that filter-branch is somehow involved but
apparently it is not the only git command and receives the mangled
input already as the commands stated in the last email show.

> What am I missing?
> 
> (I wondered at first if the extra "cat" and "-m" could be messing up
> whitespace for you, but it should not, as the quoting around "$input"
> should preserve things like newlines. And anyway, the bug in that case
> would be the _opposite_; I'd expect it to stuff everything onto a single
> line rather than breaking lines).

The commit messages I try to process are nothing special really... just
very long and not subject-like (because SVN and not giving too much
thought to them sometimes). The only special thing I can think of is
that they have been processed by git-svn earlier.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
--
To unsubscribe from this list: send the line "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 4/8] status: per-file data collection for --porcelain=v2

[Jeff Hostetler]

From: Jeff Hostetler 

The output of `git status --porcelain` leaves out many details
about the current status that clients might like to have.  This
can force them to be less efficient as they may need to launch
secondary commands (and try to match the logic within git) to
accumulate this extra information.  For example, a GUI IDE might
want the file mode to display the correct icon for a changed
item (without having to stat it afterwards).

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |  3 +++
 wt-status.c  | 63 +++-
 wt-status.h  |  4 
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c3ae2c3..93ce28c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, 
const char *arg, int un
*value = STATUS_FORMAT_PORCELAIN;
else if (!strcmp(arg, "v1") || !strcmp(arg,"1"))
*value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v2") || !strcmp(arg, "2"))
+   *value = STATUS_FORMAT_PORCELAIN_V2;
else
die("unsupported porcelain version '%s'", arg);
 
@@ -1104,6 +1106,7 @@ static struct status_deferred_config {
 static void finalize_deferred_config(struct wt_status *s)
 {
int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+  status_format != STATUS_FORMAT_PORCELAIN_V2 
&&
   !s->null_termination);
 
if (s->null_termination) {
diff --git a/wt-status.c b/wt-status.c
index a9031e4..15d3349 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -434,6 +434,31 @@ static void wt_status_collect_changed_cb(struct 
diff_queue_struct *q,
if (S_ISGITLINK(p->two->mode))
d->new_submodule_commits = !!oidcmp(>one->oid,
>two->oid);
+
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   die("BUG: worktree status add???");
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->mode_index = p->one->mode;
+   oidcpy(>oid_index, >one->oid);
+   /* mode_worktree is zero for a delete. */
+   break;
+
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   case DIFF_STATUS_UNMERGED:
+   d->mode_index = p->one->mode;
+   d->mode_worktree = p->two->mode;
+   oidcpy(>oid_index, >one->oid);
+   break;
+
+   case DIFF_STATUS_UNKNOWN:
+   die("BUG: worktree status unknown???");
+   break;
+   }
+
}
 }
 
@@ -479,12 +504,36 @@ static void wt_status_collect_updated_cb(struct 
diff_queue_struct *q,
if (!d->index_status)
d->index_status = p->status;
switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   /* Leave {mode,oid}_head zero for an add. */
+   d->mode_index = p->two->mode;
+   oidcpy(>oid_index, >two->oid);
+   break;
+   case DIFF_STATUS_DELETED:
+   d->mode_head = p->one->mode;
+   oidcpy(>oid_head, >one->oid);
+   /* Leave {mode,oid}_index zero for a delete. */
+   break;
+   
case DIFF_STATUS_COPIED:
case DIFF_STATUS_RENAMED:
d->head_path = xstrdup(p->one->path);
+   d->score = p->score * 100 / MAX_SCORE;
+   /* fallthru */
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   d->mode_head = p->one->mode;
+   d->mode_index = p->two->mode;
+   oidcpy(>oid_head, >one->oid);
+   oidcpy(>oid_index, >two->oid);
break;
case DIFF_STATUS_UNMERGED:
d->stagemask = unmerged_mask(p->two->path);
+   /*
+* Don't bother setting {mode,oid}_{head,index} since 
the print
+* code will output the stage values directly and not 
use the
+* values in these fields.
+*/
break;
}
}
@@ -565,9 +614,18 @@ static void wt_status_collect_changes_initial(struct 
wt_status *s)
if (ce_stage(ce)) {
d->index_status = 

[PATCH v4 3/8] status: support --porcelain[=]

[Jeff Hostetler]

From: Jeff Hostetler 

Update --porcelain argument to take optional version parameter
to allow multiple porcelain formats to be supported in the future.

The token "v1" is the default value and indicates the traditional
porcelain format.  (The token "1" is an alias for that.)

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt |  7 +--
 builtin/commit.c | 21 ++---
 t/t7060-wtstatus.sh  | 21 +
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index e1e8f57..6b1454b 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,11 +32,14 @@ OPTIONS
 --branch::
Show the branch and tracking info even in short-format.
 
---porcelain::
+--porcelain[=]::
Give the output in an easy-to-parse format for scripts.
This is similar to the short output, but will remain stable
across Git versions and regardless of user configuration. See
below for details.
++
+The version parameter is used to specify the format version.
+This is optional and defaults to the original version 'v1' format.
 
 --long::
Give the output in the long-format. This is the default.
@@ -96,7 +99,7 @@ configuration variable documented in linkgit:git-config[1].
 
 -z::
Terminate entries with NUL, instead of LF.  This implies
-   the `--porcelain` output format if no other format is given.
+   the `--porcelain=v1` output format if no other format is given.
 
 --column[=]::
 --no-column::
diff --git a/builtin/commit.c b/builtin/commit.c
index a792deb..c3ae2c3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -144,6 +144,21 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
+static int opt_parse_porcelain(const struct option *opt, const char *arg, int 
unset)
+{
+   enum wt_status_format *value = (enum wt_status_format *)opt->value;
+   if (unset)
+   *value = STATUS_FORMAT_NONE;
+   else if (!arg)
+   *value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v1") || !strcmp(arg,"1"))
+   *value = STATUS_FORMAT_PORCELAIN;
+   else
+   die("unsupported porcelain version '%s'", arg);
+
+   return 0;
+}
+
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
struct strbuf *buf = opt->value;
@@ -1316,9 +1331,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
N_("show status concisely"), STATUS_FORMAT_SHORT),
OPT_BOOL('b', "branch", _branch,
 N_("show branch information")),
-   OPT_SET_INT(0, "porcelain", _format,
-   N_("machine-readable output"),
-   STATUS_FORMAT_PORCELAIN),
+   { OPTION_CALLBACK, 0, "porcelain", _format,
+ N_("version"), N_("machine-readable output"),
+ PARSE_OPT_OPTARG, opt_parse_porcelain },
OPT_SET_INT(0, "long", _format,
N_("show status in long format (default)"),
STATUS_FORMAT_LONG),
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 44bf1d8..00e0ceb 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -228,4 +228,25 @@ test_expect_success 'status --branch with detached HEAD' '
test_i18ncmp expected actual
 '
 
+## Duplicate the above test and verify --porcelain=v1 arg parsing.
+test_expect_success 'status --porcelain=v1 --branch with detached HEAD' '
+   git reset --hard &&
+   git checkout master^0 &&
+   git status --branch --porcelain=v1 >actual &&
+   cat >expected <<-EOF &&
+   ## HEAD (no branch)
+   ?? .gitconfig
+   ?? actual
+   ?? expect
+   ?? expected
+   ?? mdconflict/
+   EOF
+   test_i18ncmp expected actual
+'
+
+## Verify parser error on invalid --porcelain argument.
+test_expect_success 'status --porcelain=bogus' '
+   test_must_fail git status --porcelain=bogus
+'
+
 test_done
-- 
2.8.0.rc4.17.gac42084.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 v4 1/8] status: rename long-format print routines

[Jeff Hostetler]

From: Jeff Hostetler 

Renamed the various wt_status_print*() routines to be
wt_longstatus_print*() to make it clear that these
routines are only concerned with the normal/long
status output.

This will hopefully reduce confusion as other status
formats are added in the future.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |   4 +-
 wt-status.c  | 110 +++
 wt-status.h  |   2 +-
 3 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1f6dbcd..b80273b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -515,7 +515,7 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
break;
case STATUS_FORMAT_NONE:
case STATUS_FORMAT_LONG:
-   wt_status_print(s);
+   wt_longstatus_print(s);
break;
}
 
@@ -1403,7 +1403,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
case STATUS_FORMAT_LONG:
s.verbose = verbose;
s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_status_print();
+   wt_longstatus_print();
break;
}
return 0;
diff --git a/wt-status.c b/wt-status.c
index de62ab2..b9a58fd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -139,7 +139,7 @@ void wt_status_prepare(struct wt_status *s)
s->display_comment_prefix = 0;
 }
 
-static void wt_status_print_unmerged_header(struct wt_status *s)
+static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 {
int i;
int del_mod_conflict = 0;
@@ -191,7 +191,7 @@ static void wt_status_print_unmerged_header(struct 
wt_status *s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(struct wt_status *s)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -207,9 +207,9 @@ static void wt_status_print_cached_header(struct wt_status 
*s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_dirty_header(struct wt_status *s,
-int has_deleted,
-int has_dirty_submodules)
+static void wt_longstatus_print_dirty_header(struct wt_status *s,
+int has_deleted,
+int has_dirty_submodules)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -226,9 +226,9 @@ static void wt_status_print_dirty_header(struct wt_status 
*s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_other_header(struct wt_status *s,
-const char *what,
-const char *how)
+static void wt_longstatus_print_other_header(struct wt_status *s,
+const char *what,
+const char *how)
 {
const char *c = color(WT_STATUS_HEADER, s);
status_printf_ln(s, c, "%s:", what);
@@ -238,7 +238,7 @@ static void wt_status_print_other_header(struct wt_status 
*s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(struct wt_status *s)
 {
status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -304,8 +304,8 @@ static int maxwidth(const char *(*label)(int), int minval, 
int maxval)
return result;
 }
 
-static void wt_status_print_unmerged_data(struct wt_status *s,
- struct string_list_item *it)
+static void wt_longstatus_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
 {
const char *c = color(WT_STATUS_UNMERGED, s);
struct wt_status_change_data *d = it->util;
@@ -331,9 +331,9 @@ static void wt_status_print_unmerged_data(struct wt_status 
*s,
strbuf_release();
 }
 
-static void wt_status_print_change_data(struct wt_status *s,
-   int change_type,
-   struct string_list_item *it)
+static void wt_longstatus_print_change_data(struct wt_status *s,
+   int change_type,
+   struct string_list_item *it)
 {
struct wt_status_change_data *d = it->util;
const char *c = color(change_type, s);
@@ -378,7 +378,7 @@ static void wt_status_print_change_data(struct wt_status *s,
status = d->worktree_status;
break;
default:
-   die("BUG: unhandled change_type %d in 
wt_status_print_change_data",
+   die("BUG: unhandled 

[PATCH v4 5/8] status: print per-file porcelain v2 status data

[Jeff Hostetler]

From: Jeff Hostetler 

Print per-file information in porcelain v2 format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 wt-status.c | 283 +++-
 1 file changed, 282 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 15d3349..46061d4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1813,6 +1813,287 @@ static void wt_porcelain_print(struct wt_status *s)
wt_shortstatus_print(s);
 }
 
+/*
+ * Convert various submodule status values into a
+ * fixed-length string of characters in the buffer provided.
+ */
+static void wt_porcelain_v2_submodule_state(
+   struct wt_status_change_data *d,
+   char sub[5])
+{
+   if (S_ISGITLINK(d->mode_head) ||
+   S_ISGITLINK(d->mode_index) ||
+   S_ISGITLINK(d->mode_worktree)) {
+   sub[0] = 'S';
+   sub[1] = d->new_submodule_commits ? 'C' : '.';
+   sub[2] = (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED) ? 'M' 
: '.';
+   sub[3] = (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ? 'U' 
: '.';
+   } else {
+   sub[0] = 'N';
+   sub[1] = '.';
+   sub[2] = '.';
+   sub[3] = '.';
+   }
+   sub[4] = 0;
+}
+
+/*
+ * Fix-up changed entries before we print them.
+ */
+static void wt_porcelain_v2_fix_up_changed(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+
+   if (!d->index_status) {
+   /*
+* This entry is unchanged in the index (relative to the head).
+* Therefore, the collect_updated_cb was never called for this
+* entry (during the head-vs-index scan) and so the head column
+* fields were never set.
+*
+* We must have data for the index column (from the
+* index-vs-worktree scan (otherwise, this entry should not be
+* in the list of changes)).
+*
+* Copy index column fields to the head column, so that our
+* output looks complete.
+*/
+   assert(d->mode_head == 0);
+   d->mode_head = d->mode_index;
+   oidcpy(>oid_head, >oid_index);
+   }
+
+   if (!d->worktree_status) {
+   /*
+* This entry is unchanged in the worktree (relative to the 
index).
+* Therefore, the collect_changed_cb was never called for this 
entry
+* (during the index-vs-worktree scan) and so the worktree 
column
+* fields were never set.
+*
+* We must have data for the index column (from the 
head-vs-index
+* scan).
+*
+* Copy the index column fields to the worktree column so that
+* our output looks complete.
+*
+* Note that we only have a mode field in the worktree column
+* because the scan code tries really hard to not have to 
compute it.
+*/
+   assert(d->mode_worktree == 0);
+   d->mode_worktree = d->mode_index;
+   }
+}
+
+/*
+ * Print porcelain v2 info for tracked entries with changes.
+ */
+static void wt_porcelain_v2_print_changed_entry(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+   struct strbuf buf_current = STRBUF_INIT;
+   struct strbuf buf_src = STRBUF_INIT;
+   const char *path_current = NULL;
+   const char *path_src = NULL;
+   char key[3];
+   char submodule_token[5];
+   char sep_char, eol_char;
+
+   wt_porcelain_v2_fix_up_changed(it, s);
+   wt_porcelain_v2_submodule_state(d, submodule_token);
+
+   key[0] = d->index_status ? d->index_status : '.';
+   key[1] = d->worktree_status ? d->worktree_status : '.';
+   key[2] = 0;
+
+   if (s->null_termination) {
+   /*
+* In -z mode, we DO NOT C-Quote pathnames.  Current path is 
ALWAYS first.
+* A single NUL character separates them.
+*/
+   sep_char = '\0';
+   eol_char = '\0';
+   path_current = it->string;
+   path_src = d->head_path;
+   } else {
+   /*
+* Path(s) are C-Quoted if necessary. Current path is ALWAYS 
first.
+* The source path is only present when necessary.
+* A single TAB separates them (because paths can contain spaces
+* which are not escaped and C-Quoting does escape TAB 
characters).
+*/
+   sep_char = '\t';
+   eol_char = '\n';
+   path_current = 

[PATCH v4 6/8] status: print branch info with --porcelain=v2 --branch

[Jeff Hostetler]

From: Jeff Hostetler 

Expand porcelain v2 output to include branch and tracking
branch information.  This includes the commit SHA, the branch,
the upstream branch, and the ahead and behind counts.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |  5 
 wt-status.c  | 90 
 wt-status.h  |  1 +
 3 files changed, 96 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 93ce28c..b1fd2d1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -510,6 +510,8 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   if (!s->is_initial)
+   hashcpy(s->sha1_commit, sha1);
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
@@ -1378,6 +1380,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
fd = hold_locked_index(_lock, 0);
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+   if (!s.is_initial)
+   hashcpy(s.sha1_commit, sha1);
+
s.ignore_submodule_arg = ignore_submodule_arg;
s.status_format = status_format;
s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index 46061d4..592fbd2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1814,6 +1814,92 @@ static void wt_porcelain_print(struct wt_status *s)
 }
 
 /*
+ * Print branch information for porcelain v2 output.  These lines
+ * are printed when the '--branch' parameter is given.
+ *
+ *# branch.oid 
+ *# branch.head 
+ *   [# branch.upstream 
+ *   [# branch.ab + -]]
+ *
+ *   ::= the current commit hash or the the literal
+ *   "(initial)" to indicate an initialized repo
+ *   with no commits.
+ *
+ * ::=  the current branch name or
+ *   "(detached)" literal when detached head or
+ *   "(unknown)" when something is wrong.
+ *
+ * ::= the upstream branch name, when set.
+ *
+ *::= integer ahead value, when upstream set
+ *   and the commit is present (not gone).
+ *
+ *   ::= integer behind value, when upstream set
+ *   and commit is present.
+ *
+ *
+ * The end-of-line is defined by the -z flag.
+ *
+ *  ::= NUL when -z,
+ *   LF when NOT -z.
+ *
+ */
+static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+{
+   struct branch *branch;
+   const char *base;
+   const char *branch_name;
+   struct wt_status_state state;
+   int ab_info, nr_ahead, nr_behind;
+   char eol = s->null_termination ? '\0' : '\n';
+
+   memset(, 0, sizeof(state));
+   wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
+
+   fprintf(s->fp, "# branch.oid %s%c",
+   (s->is_initial ? "(initial)" : 
sha1_to_hex(s->sha1_commit)),
+   eol);
+
+   if (!s->branch)
+   fprintf(s->fp, "# branch.head %s%c", "(unknown)", eol);
+   else {
+   if (!strcmp(s->branch, "HEAD")) {
+   fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
+
+   if (state.rebase_in_progress || 
state.rebase_interactive_in_progress)
+   branch_name = state.onto;
+   else if (state.detached_from)
+   branch_name = state.detached_from;
+   else
+   branch_name = "";
+   } else {
+   branch_name = NULL;
+   skip_prefix(s->branch, "refs/heads/", _name);
+
+   fprintf(s->fp, "# branch.head %s%c", branch_name, eol);
+   }
+
+   /* Lookup stats on the upstream tracking branch, if set. */
+   branch = branch_get(branch_name);
+   base = NULL;
+   ab_info = (stat_tracking_info(branch, _ahead, _behind, 
) == 0);
+   if (base) {
+   base = shorten_unambiguous_ref(base, 0);
+   fprintf(s->fp, "# branch.upstream %s%c", base, eol);
+   free((char *)base);
+
+   if (ab_info)
+   fprintf(s->fp, "# branch.ab +%d -%d%c", 
nr_ahead, nr_behind, eol);
+   }
+   }
+
+   free(state.branch);
+   free(state.onto);
+   free(state.detached_from);
+}
+
+/*
  * Convert various submodule status values into a
  * fixed-length string of characters in the buffer provided.
  */
@@ -2057,6 +2143,7 @@ static void wt_porcelain_v2_print_other(
 /*
  * Print porcelain V2 status.
  *
+ * []
  * []*
  * []*
  * []*
@@ -2069,6 +2156,9 @@ void 

[PATCH v4 7/8] git-status.txt: describe --porcelain=v2 format

[Jeff Hostetler]

From: Jeff Hostetler 

Update status manpage to include information about
porcelain V2 format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt | 123 +--
 1 file changed, 119 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 6b1454b..76e7c92 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -183,12 +183,12 @@ in which case `XY` are `!!`.
 
 If -b is used the short-format status is preceded by a line
 
-## branchname tracking info
+## branchname tracking info
 
-Porcelain Format
-
+Porcelain Format Version 1
+~~
 
-The porcelain format is similar to the short format, but is guaranteed
+Version 1 porcelain format is similar to the short format, but is guaranteed
 not to change in a backwards-incompatible way between Git versions or
 based on user configuration. This makes it ideal for parsing by scripts.
 The description of the short format above also describes the porcelain
@@ -210,6 +210,121 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Porcelain Format Version 2
+~~
+
+Version 2 format adds more detailed information about the state of
+the worktree and changed items.  Version 2 also defines an extensible
+set of easy to parse optional headers.
+
+Header lines start with "#" and are added in response to specific
+command line arguments.  Parsers should ignore headers they
+don't recognize.
+
+### Branch Headers
+
+If `--branch` is given, a series of header lines are printed with
+information about the current branch.
+
+Line Notes
+
+# branch.oid  | (initial)Current commit.
+# branch.head  | (detached)  Current branch.
+# branch.upstream   If upstream is set.
+# branch.ab + -   If upstream is set and
+ the commit is present.
+
+
+### Changed Tracked Entries
+
+Following the headers, a series of lines are printed for tracked
+entries.  One of three different line formats may be used to describe
+an entry depending on the type of change.  Tracked entries are printed
+in an undefined order; parsers should allow for a mixture of the 3
+line types in any order.
+
+Ordinary changed entries have the following format:
+
+1
+
+Renamed or copied entries have the following format:
+
+2 
+
+Field   Meaning
+
+A 2 character field containing the staged and
+unstaged XY values described in the short format,
+with unchanged indicated by a "." rather than
+a space.
+   A 4 character field describing the submodule state.
+"N..." when the entry is not a submodule.
+"S" when the entry is a submodule.
+ is "C" if the commit changed; otherwise ".".
+ is "M" if it has tracked changes; otherwise ".".
+ is "U" if there are untracked changes; otherwise ".".
+The 6 character octal file mode in the HEAD.
+The octal file mode in the index.
+The octal file mode in the worktree.
+The SHA1 value in the HEAD.
+The SHA1 value in the index.
+ The rename or copied percentage score. For example "R100"
+or "C75".
+  The current pathname.
+   When the `-z` option is used, the 2 pathnames are separated
+with a NUL (ASCII 0x00) byte; otherwise, a tab (ASCII 0x09)
+byte separates them.
+   The original path. This is only present when the entry
+has been renamed or copied.
+
+
+Unmerged entries have the following format; the first character is
+a "u" to distinguish from ordinary changed entries.
+
+u  
+
+Field   Meaning
+
+A 2 character field describing the conflict type
+as described in the short format.
+   A 4 character field describing the submodule state
+as described above.
+The octal file mode for stage 1.
+The octal file mode for stage 2.
+The octal file mode for stage 3.
+The octal file mode in the worktree.
+The SHA1 value for stage 1.
+The SHA1 value for stage 2.
+The SHA1 value for stage 3.
+  

[PATCH v4 0/8] status: V2 porcelain status

[Jeff Hostetler]

This patch series adds porcelain V2 format to status.
This provides detailed information about file changes
and about the current branch.

The new output is accessed via:
git status --porcelain=v2 [--branch]

Relative to the V3 patch series, in this V4 patch series
I've updated Documentation/git-status.txt for clarity.

Jeff Hostetler (8):
  status: rename long-format print routines
  status: cleanup API to wt_status_print
  status: support --porcelain[=]
  status: per-file data collection for --porcelain=v2
  status: print per-file porcelain v2 status data
  status: print branch info with --porcelain=v2 --branch
  git-status.txt: describe --porcelain=v2 format
  status: tests for --porcelain=v2

 Documentation/git-status.txt | 130 +-
 builtin/commit.c |  78 +++---
 t/t7060-wtstatus.sh  |  21 ++
 t/t7064-wtstatus-pv2.sh  | 585 +++
 wt-status.c  | 567 -
 wt-status.h  |  19 +-
 6 files changed, 1289 insertions(+), 111 deletions(-)
 create mode 100755 t/t7064-wtstatus-pv2.sh

-- 
2.8.0.rc4.17.gac42084.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 v4 2/8] status: cleanup API to wt_status_print

[Jeff Hostetler]

From: Jeff Hostetler 

Refactor the API between builtin/commit.c and wt-status.[ch].
Hide details of the various wt_*status_print() routines inside
wt-status.c behind a single (new) wt_status_print() routine
and eliminate the switch statements from builtin/commit.c

This will allow us to more easily add new status formats
and isolate that within wt-status.c

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c | 51 +--
 wt-status.c  | 25 ++---
 wt-status.h  | 16 
 3 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b80273b..a792deb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static enum status_format {
-   STATUS_FORMAT_NONE = 0,
-   STATUS_FORMAT_LONG,
-   STATUS_FORMAT_SHORT,
-   STATUS_FORMAT_PORCELAIN,
-
-   STATUS_FORMAT_UNSPECIFIED
-} status_format = STATUS_FORMAT_UNSPECIFIED;
+static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   s->status_format = status_format;
+   s->ignore_submodule_arg = ignore_submodule_arg;
 
wt_status_collect(s);
-
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print(s);
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print(s);
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   wt_longstatus_print(s);
-   break;
-   }
+   wt_status_print(s);
 
return s->commitable;
 }
@@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name)
  * is not in effect here.
  */
 static struct status_deferred_config {
-   enum status_format status_format;
+   enum wt_status_format status_format;
int show_branch;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
@@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
s.ignore_submodule_arg = ignore_submodule_arg;
+   s.status_format = status_format;
+   s.verbose = verbose;
+
wt_status_collect();
 
if (0 <= fd)
@@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
if (s.relative_paths)
s.prefix = prefix;
 
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print();
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print();
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   s.verbose = verbose;
-   s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_longstatus_print();
-   break;
-   }
+   wt_status_print();
return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index b9a58fd..a9031e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
show_bisect_in_progress(s, state, state_color);
 }
 
-void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s)
 {
const char *branch_color = color(WT_STATUS_ONBRANCH, s);
const char *branch_status_color = color(WT_STATUS_HEADER, s);
@@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(struct wt_status *s)
 {
int i;
 
@@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s)
}
 }
 
-void wt_porcelain_print(struct wt_status *s)
+static void wt_porcelain_print(struct wt_status *s)
 {
s->use_color = 0;
s->relative_paths = 0;
@@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s)
s->no_gettext = 1;
wt_shortstatus_print(s);
 }
+
+void wt_status_print(struct wt_status *s)
+{
+   switch 

[PATCH v4 8/8] status: tests for --porcelain=v2

[Jeff Hostetler]

From: Jeff Hostetler 

Unit tests for porcelain v2 status format.

Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 
---
 t/t7064-wtstatus-pv2.sh | 585 
 1 file changed, 585 insertions(+)
 create mode 100755 t/t7064-wtstatus-pv2.sh

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
new file mode 100755
index 000..ff0dd3d
--- /dev/null
+++ b/t/t7064-wtstatus-pv2.sh
@@ -0,0 +1,585 @@
+#!/bin/sh
+
+test_description='git status --porcelain=v2
+
+This test exercises porcelain V2 output for git status.'
+
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   test_tick &&
+   git config --local core.autocrlf false &&
+   echo x >file_x &&
+   echo y >file_y &&
+   echo z >file_z &&
+   mkdir dir1 &&
+   echo a >dir1/file_a &&
+   echo b >dir1/file_b
+'
+
+
+##
+## Confirm output prior to initial commit.
+##
+
+test_expect_success pre_initial_commit_0 '
+   cat >expected <<-EOF &&
+   # branch.oid (initial)
+   # branch.head master
+   ? actual
+   ? dir1/
+   ? expected
+   ? file_x
+   ? file_y
+   ? file_z
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=normal 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success pre_initial_commit_1 '
+   git add file_x file_y file_z dir1 &&
+   SHA_A=`git hash-object -t blob -- dir1/file_a` &&
+   SHA_B=`git hash-object -t blob -- dir1/file_b` &&
+   SHA_X=`git hash-object -t blob -- file_x` &&
+   SHA_Y=`git hash-object -t blob -- file_y` &&
+   SHA_Z=`git hash-object -t blob -- file_z` &&
+   SHA_ZERO= &&
+
+   cat >expected <<-EOF &&
+   # branch.oid (initial)
+   # branch.head master
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+## Try -z on the above
+test_expect_success pre_initial_commit_2 '
+   cat >expected.lf <<-EOF &&
+   # branch.oid (initial)
+   # branch.head master
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_A dir1/file_a
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_B dir1/file_b
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_X file_x
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Y file_y
+   1 A. N... 00 100644 100644 $SHA_ZERO $SHA_Z file_z
+   ? actual
+   ? expected
+   EOF
+   perl -pe y/\\012/\\000/ expected &&
+   rm expected.lf &&
+
+   git status -z --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+##
+## Create first commit. Confirm commit sha in new track header.
+## Then make some changes on top of it.
+##
+
+test_expect_success initial_commit_0 '
+   git commit -m initial &&
+   H0=`git rev-parse HEAD` &&
+   cat >expected <<-EOF &&
+   # branch.oid $H0
+   # branch.head master
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_1 '
+   echo x >>file_x &&
+   SHA_X1=`git hash-object -t blob -- file_x` &&
+   rm file_z &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected <<-EOF &&
+   # branch.oid $H0
+   # branch.head master
+   1 .M N... 100644 100644 100644 $SHA_X $SHA_X file_x
+   1 .D N... 100644 100644 00 $SHA_Z $SHA_Z file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_2 '
+   git add file_x &&
+   git rm file_z &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected <<-EOF &&
+   # branch.oid $H0
+   # branch.head master
+   1 M. N... 100644 100644 100644 $SHA_X $SHA_X1 file_x
+   1 D. N... 100644 00 00 $SHA_Z $SHA_ZERO file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_3 '
+ 

Re: [[PATCH v2] 1/4] patch-ids: stop using a hand-rolled hashmap implementation

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > It would be a serious bug if hashmap_entry_init() played games with
> > references, given its signature (that this function does not have any
> > access to the hashmap structure, only to the entry itself):
> >
> > void hashmap_entry_init(void *entry, unsigned int hash)
> 
> I do not think we are on the same page.  The "reference to other
> resource" I wondered was inside the hashmap_entry structure, IOW,
> "the entry itself".

Oh, I see now.

> Which is declared to be opaque to the API users,

Actually, not really. We cannot do that in C: we need to define the struct
in hashmap.h so that its size is known to the users.

> so whoever defined that API cannot blame me for not checking its
> definition to see that it only has "unsigned int hash" and no allocated
> memory or open file descriptor in it that needs freeing.

That is the reason, I guess, why we have the documentation in
Documentation/technical/api-hashmap.txt: it would have to talk about your
hypothetical hashmap_entry_clear() (which would better be named
*_release() BTW, unless I misunderstood what you want a hypothetical
future version of that function to do).

And quite frankly, unless we *have* to, I would rather try to avoid
introducing that function as much as possible, as it would make using the
hashmap API even more finicky than it already is.

> By the way, the first parameter of the function being "void *" is
> merely to help lazy API users, who have their own structure that
> embeds the hashmap_entry as its first element, as API documentation
> tells them to do, e.g.
> 
>   struct foo {
>   struct hashmap_entry e;
> ... other "foo" specific fields come here ...
>   } foo;
> 
> and because of the lazy "void *", they do not have to do this:
> 
>   hashmap_entry_init(>e, ...);
> 
> which would be required if the first parameter were "struct
> hashmap_entry *", but they can just do this:
> 
>   hashmap_entry_init(, ...);

Yes, I know that. It is the common way to simulate subclassing in C, for
lack of a more compile-safe construct.

> I have a slight preference to avoid the lazy "void *", but that is
> an unrelated tangent.

Oh, we are already safely in Unrelated Tangent Land for a while, I would
think. Nothing of what we are discussing in this thread has anything to do
with Kevin's patch series, which is about trying to use resources more
sensibly when using the revision machinery's --cherry-pick option.

And since we are already there, I'll offer an opinion in favor of `void
*`: doing the >e dance could quite possibly suggest that `e` is a
field just like any other field (and does not necessarily *need* to be the
first).

But again, this has nothing to do with the patch series we are discussing
here.

> >> The fact that hashmap_entry_init() is there but there is no
> >> corresponding hashmap_entry_clear() hints that there is nothing to be
> >> worried about and I can see from the implementation of
> >> hashmap_entry_init() that no extra resource is held inside, but an
> >> API user should not have to guess.  We may want to do one of the two
> >> things:
> >> 
> >>  * document that an embedded hashmap_entry does not hold any
> >>resource that need to be released and it is safe to free the user
> >>structure that embeds one; or
> >> 
> >>  * implement hashmap_entry_clear() that currently is a no-op.
> >
> > Urgh. The only reason we have hashmap_entry_init() is that we *may* want
> > to extend `struct hashmap_entry` at some point. That is *already*
> > over-engineered because that point in time seems quite unlikely to arrive,
> > like, ever.
> 
> I am saying that an uneven over-enginnering is bad.

Hmm. I guess that the _init() function could be replaced by an _INIT macro
a la STRBUF_INIT. Not sure it is really worth the effort, though.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "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] blame: drop strdup of string literal

[Jeff King]

On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote:

> On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:
> 
> > > +   struct string_list range_list = STRING_LIST_INIT_NODUP;
> > 
> > Related to this series, there's an additional "fix" which ought to be
> > made, probably as a separate patch. In particular, in cmd_blame():
> > 
> > if (lno && !range_list.nr)
> > string_list_append(_list, xstrdup("1"));
> > 
> > which supplies a default range ("line 1 through end of file") if -L
> > was not specified. I used xstrdup() on the literal "1" in 58dbfa2
> > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with
> > parse_opt_string_list() which was unconditionally xstrdup'ing the
> > argument (but no longer does as of patch 1/3 of this series).
> 
> Yeah, I'd agree that this is a minor bug both before and after the
> series due to the leak. Want to roll a patch on top?

Here it is, just to tie up a loose end. I marked you as the author since
the hard part was noticing the issue and explaining the history, which
you already did above.

-- >8 --
From: Eric Sunshine 
Subject: [PATCH] blame: drop strdup of string literal

This strdup was added as part of 58dbfa2 (blame: accept
multiple -L ranges, 2013-08-06) to be consistent with
parse_opt_string_list(), which appends to the same list.

But as of 7a7a517 (parse_opt_string_list: stop allocating
new strings, 2016-06-13), we should stop using strdup (to
match parse_opt_string_list, and for all the reasons
described in that commit; namely that it does nothing useful
and causes us to leak the memory).

Signed-off-by: Jeff King 
---
 builtin/blame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index ab66cde..29bd479 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
lno = prepare_lines();
 
if (lno && !range_list.nr)
-   string_list_append(_list, xstrdup("1"));
+   string_list_append(_list, "1");
 
anchor = 1;
range_set_init(, range_list.nr);
-- 
2.9.2.670.g42e63de

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

Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Fri, 29 Jul 2016, Junio C Hamano wrote:
> >
> >> Kevin Willford  writes:
> >> 
> >> >  static int patch_id_cmp(struct patch_id *a,
> >> >  struct patch_id *b,
> >> > -void *keydata)
> >> > +struct diff_options *opt)
> >> >  {
> >> > +if (is_null_sha1(a->patch_id) &&
> >> > +commit_patch_id(a->commit, opt, a->patch_id, 0))
> >> > +return error("Could not get patch ID for %s",
> >> > +oid_to_hex(>commit->object.oid));
> >> > +if (is_null_sha1(b->patch_id) &&
> >> > +commit_patch_id(b->commit, opt, b->patch_id, 0))
> >> > +return error("Could not get patch ID for %s",
> >> > +oid_to_hex(>commit->object.oid));
> >> >  return hashcmp(a->patch_id, b->patch_id);
> >> >  }
> >> 
> >> These error returns initially looks slightly iffy in that in general
> >> the caller of any_cmp_fn() wants to know how a/b compares, but by
> >> returning error(), it always says "a is smaller than b".
> >
> > I am to blame, as this is my design.
> >
> > And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
> > and >0 for comparisons, when hashmaps try to avoid any order.
> 
> Perhaps hashmap API needs fixing in the longer term not to call this
> type hashmap_cmp_fn; instead it should lose cmp and say something
> like hashmap_eq_fn or something.

Maybe.

But to make sure: you do not expect Kevin to do that in the context of
this here patch series, right?

Ciao,
Dscho
--
To unsubscribe from this list: send the line "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] gitweb: escape link body in format_ref_marker

[Andreas Brauchli]

On Mon, Aug 1, 2016 at 9:54 PM, Junio C Hamano  wrote:
> Jakub Narębski  writes:
>
>> Good catch!
>>
>> Acked-by: Jakub Narębski 
>
> Sigh; the contents may be good but the patch is unusable as-is
> because of heavy whitespace damage.
>
> I'll fix it up.  Thanks, both.
My apologies for that, it seems that gmail doesn't do tabs.
I resubmitted the PR as #283 on GitHub along with Jakub's Ack and
fixed the typo he pointed out in the commit message

https://github.com/git/git/pull/283

Kind regards
andreas

>
>>> ---
>>>  gitweb/gitweb.perl | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 2fddf75..33d701d 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2090,7 +2090,7 @@ sub format_ref_marker {
>>> -href => href(
>>> action=>$dest_action,
>>> hash=>$dest
>>> -   )}, $name);
>>> +   )}, esc_html($name));
>>>
>>> $markers .= " >> class=\"".esc_attr($class)."\" title=\"".esc_attr($ref)."\">" .
>>> $link . "";
>>>
--
To unsubscribe from this list: send the line "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 bisect for reachable commits only

[Oleg Taranenko]

Guys,

thanks for discussion, I will try to reply in bulk here.
First, assuming the common ancestor is GOOD based on the fact that
some descendant given as GOOD is pretty bad idea.
It may be, but may not be. In the git-flow like workflows new features
(aka branches) are created from trunk (master/develop/...)
sporadically,
but later they will mutual merging. I would say more probably they
have not common base, then have.

Second, I don't ask "create a new algorithm to find all transition
from good/old to bad/new", not nesessary. If programmer feels
something
suspicious, he/she can create another bisect session with narrowed commit range.

Third, testing of any specific commit can be very expensive operation.
In my case - shutdown servers/refresh dbs/clean/rebuild in
eclipse/running servers/dropping browser cache/running app in
browser/going through some pages/view UI.
Some of steps of course are automated, but some not. Anyway I spend
5-10 min for every iteration. So knowing what commit is bad or good is
very valuable, then I'm very interested to hunt the bug-introduced
commit with minimal count of testing.

Scenario 4 (I will keep my previous mail numbering for possible later reference)
 z1z2---z3
/ /  \
Gx1x2/---x3---x4--B
  \ /   /
   y1--y2--y3--y4

This is the happy straight case with closed DAG (hehe, git for
scientists) between given G good and B bad commits.
Ideal bisect will check first the shortest way between G & B:
x1/x2/x3/x4. Let name first-bug commit we are really hunting H and
current first-bug candidate as h.
If h == x1 or x2 -> stop, found
If h == x3, bisect will try to test y2/y3/y4 path only
If h == x4, bisect will select shortest path z1/z2 (keeping in mind,
that x2 is already tested and is good)
  If h == z1 - found
  if h == z2 - looking in path y1/y2/y3

Scenario 5.
  v1---v2
 /  \
w1--/---w2---w3-w4--w5
   /   /   / \
  /   /   /z1z2---z3  \
 /   /   // /  \   \
C3--C2--C1--G--x1x2/---x3---x4--x5--x6--B
\ /   /
 y1--y2--y3--y4

Unhappy case, we have side branches which may introduce bug behaviour,
we need to look it through to figure out why it was done, what problem
was solved for that and so on.
Let looking in shortest path x1-x6. If h == x1..x4 - happy use case of
scenario 4. If discover that h == x5, we are forgetting about z/y
paths, but first we looking for nearest common commit (C1). As far as
we agree that currently is not clear when the new feature was
introduced we need to explicit check commit C1 whether it contains a
feature we are hunting bug up.
if C1 is good then pretty possible bad transition was happend in w2-w5
commits. Else (C1 is bad) assume that there is no transition from good
to bad, then assume H == x5 (stop)
if C1 is good and h == w4/w5 => stop,
  else if h == w3, new roundtrip, forgetting about w1 commit(not
interesting), testing C2, if bad - stop H == w3, if good, v1/v2
commits are to test.
  else if h == w2, forgetting C2 testing, just testing C3. If bad,
stop, H == w3, if good, w1 to test.

Using this approach we can safe working with ever octopus merging
(personally I'm not using, but why not)


Scenario 6.
z1---z2---z3
   //   \
C1--G--x1x2/--x3 |
 \  \ / \|
  \  y1--y2--y3--y4--y5--y6--B
   \  \ /|
\  w1--w2-w3 |
 \  /
  v1--v2

Important note. Before start every side circuit based on common
ancestor user should be explicitly warned, that this is not just
ordinal intermediate bisect commit testing, but possible new round
trips with new commit/steps counts
For example, if current shortest path is x1-x6, bisect should say
about only 6 commits (3 after bisect), not calculating commits in
other paths.
Reaching node decision, bisect will stay and prompt for testing new
common ancestor with clear instructions what happens, if it will be
good or bad, (new unchecked commits and new left bisect steps, in case
good and stop or switch to other path in case of octopus).

I have another request to get git bisect more user-friendly, regarding
rolling back last step or steps, if accidentally 'git bisect bad' or
'good' was wrong entered, but I think it worth for another thread.


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

Re: [[PATCH v2] 4/4] rebase: avoid computing unnecessary patch IDs

[Jakub Narębski]

W dniu 01.08.2016 o 22:11, Junio C Hamano pisze:
> Johannes Schindelin  writes:
>> On Fri, 29 Jul 2016, Junio C Hamano wrote:

>>> These error returns initially looks slightly iffy in that in general
>>> the caller of any_cmp_fn() wants to know how a/b compares, but by
>>> returning error(), it always says "a is smaller than b".
>>
>> I am to blame, as this is my design.
>>
>> And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
>> and >0 for comparisons, when hashmaps try to avoid any order.
> 
> Perhaps hashmap API needs fixing in the longer term not to call this
> type hashmap_cmp_fn; instead it should lose cmp and say something
> like hashmap_eq_fn or something.

The problem is that one expects hashmap_cmp_fn() to return ==0 on equality,
while one would expect for hashmap_eq_fn() to return true (==1) on equality.
So we would have to rewrite all calling sites.

It would be nice to have a comment about how hashmap uses cmpfn in
hashmap.h.  


Though... currently our hashmap implementation uses linked
list (separate chaining) for handling hash collisions (for collision
resolution). More sophisticated data structures, such as balanced search
trees, or splay trees, are worth considering only if the load factor is
large, or if the hash distribution is likely to be very non-uniform,
or if one must guarantee good performance even in a worst-case scenario.
So almost certainly comparing function (which is needed for trees)
won't be needed.

Best,
-- 
Jakub Narębski

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

Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Subject: Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors
> 
> s/merge_/merge-/; for this one alone.

I see that de8946de1694a8cf311daab7b2c416d76cb04d23 still shows it with an
underscore instead of a dash, while you fixed my other tyops in this patch
series.

I tend to think that the underscore is correct: this change is not so much
about the builtin (which is written with a dash) but about the function
(written with an underscore, used by more than just merge-recursive, e.g.
cherry-pick).

> > There are a couple of places where return values never indicated errors
> > before, as wie simply died instead of returning.
> 
> s/wie/we/;

Right. What can I say, I am surrounded by too many Germans.

I fixed this locally, in case another re-roll should be required. What you
have in `pu` looks correct to me, though. Let me know if you want me to
re-submit nevertheless.

BTW I should have said this earlier: I run all my rebases, all my merges,
all my cherry-picks using a Git version with these patches for months (of
course, the patches have changed, but not in the most critical parts I was
concerned about, the parts where die() calls were replaced). If I would
have found any regression, I would have notified you immediately, of
course.

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

Re: [PATCH v6 05/16] Prepare the builtins for a libified merge_recursive()

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Previously, callers of merge_trees() or merge_recursive() expected that
> > code to die() with an error message. This used to be okay because we
> > called those commands from scripts, and had a chance to print out a
> > message in case the command failed fatally (read: with exit code 128).
> >
> > As scripting incurs its own set of problems (portability, speed,
> > idiosynchracies of different shells, limited data structures leading to
> 
> I think I typofixed this when I queued the previous one on 'pu'
> already, but s/synch/sync/; 

Whoops. Fixed locally.

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

Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Lars Schneider]

> On 02 Aug 2016, at 07:53, Johannes Sixt  wrote:
> 
> Am 01.08.2016 um 13:14 schrieb Lars Schneider:
> >> On 30 Jul 2016, at 11:50, Johannes Sixt  wrote:
> >> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:
> >>> static struct child_to_clean *children_to_clean;
> >>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
> >>> {
> >>>   while (children_to_clean) {
> >>>   struct child_to_clean *p = children_to_clean;
> >>> + if (p->clean_on_exit_handler)
> >>> + p->clean_on_exit_handler(p->pid);
> >>
> >> This summons demons. cleanup_children() is invoked from a signal
> >> handler. In this case, it can call only async-signal-safe functions.
> >> It does not look like the handler that you are going to install
> >> later will take note of this caveat!
> >>
> >>>   children_to_clean = p->next;
> >>>   kill(p->pid, sig);
> >>>   if (!in_signal)
> >>
> >> The condition that we see here in the context protects free(p)
> >> (which is not async-signal-safe). Perhaps the invocation of the new
> >> callback should be skipped in the same manner when this is called
> >> from a signal handler? 507d7804 (pager: don't use unsafe functions
> >> in signal handlers) may be worth a look.
> >
> > Thanks a lot of pointing this out to me!
> >
> > Do I get it right that after the signal "SIGTERM" I can do a cleanup
> > and don't need to worry about any function calls but if I get any
> > other signal then I can only perform async-signal-safe calls?
> 
> No. SIGTERM is not special.
> 
> Perhaps you were misled by the SIGTERM mentioned in 
> cleanup_children_on_exit()? This function is invoked on regular exit (not 
> from a signal). SIGTERM is used in this case to terminate children that are 
> still lingering around.

Yes, that was my source of confusion. Thanks for the clarification!

> 
> > If this is correct, then the following solution would work great:
> >
> > if (!in_signal && p->clean_on_exit_handler)
> > p->clean_on_exit_handler(p->pid);
> 
> This should work nevertheless because in_signal is set when the function is 
> invoked from a signal handler (of any signal that is caught) via 
> cleanup_children_on_signal().

Right. Thank you!

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

Re: [PATCH v3 06/10] run-command: add clean_on_exit_handler

[Johannes Sixt]

Am 01.08.2016 um 13:14 schrieb Lars Schneider:
>> On 30 Jul 2016, at 11:50, Johannes Sixt  wrote:
>> Am 30.07.2016 um 01:37 schrieb larsxschnei...@gmail.com:
>>> static struct child_to_clean *children_to_clean;
>>> @@ -30,6 +31,8 @@ static void cleanup_children(int sig, int in_signal)
>>> {
>>>while (children_to_clean) {
>>>struct child_to_clean *p = children_to_clean;
>>> +  if (p->clean_on_exit_handler)
>>> +  p->clean_on_exit_handler(p->pid);
>>
>> This summons demons. cleanup_children() is invoked from a signal
>> handler. In this case, it can call only async-signal-safe functions.
>> It does not look like the handler that you are going to install
>> later will take note of this caveat!
>>
>>>children_to_clean = p->next;
>>>kill(p->pid, sig);
>>>if (!in_signal)
>>
>> The condition that we see here in the context protects free(p)
>> (which is not async-signal-safe). Perhaps the invocation of the new
>> callback should be skipped in the same manner when this is called
>> from a signal handler? 507d7804 (pager: don't use unsafe functions
>> in signal handlers) may be worth a look.
>
> Thanks a lot of pointing this out to me!
>
> Do I get it right that after the signal "SIGTERM" I can do a cleanup
> and don't need to worry about any function calls but if I get any
> other signal then I can only perform async-signal-safe calls?

No. SIGTERM is not special.

Perhaps you were misled by the SIGTERM mentioned in 
cleanup_children_on_exit()? This function is invoked on regular exit 
(not from a signal). SIGTERM is used in this case to terminate children 
that are still lingering around.


> If this is correct, then the following solution would work great:
>
>if (!in_signal && p->clean_on_exit_handler)
>p->clean_on_exit_handler(p->pid);

This should work nevertheless because in_signal is set when the function 
is invoked from a signal handler (of any signal that is caught) via 
cleanup_children_on_signal().


-- Hannes

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