Re: [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

[Eddy Petrișor]

2018-04-19 2:53 GMT+03:00 Stefan Beller :
> Hi Eddy,
>
> all the following patches 3-9 are touching the test as added in patch
> 2, which would go best with this patch.
> Could you squash all commits into one?

Yes,

I did not have time yesterday to put all changes into a single commit
with an associated note (because note managment seems to be a huge
pain), so I preferred small commits.

But I wanted to get your feedback on something, I'll reply in thread
arm where you actually suspected the problem.

> There are a couple ways to do it:
>
>   git reset --soft
>   git commit -a --reuse-commit-message=<...>
>
> or using
>
> git rebase --interactive origin/master
> # and then marking all but the first as "fixup"

I am aware of git rebase -i and use it regularly, that's why patches
3-9 have the 'fixup' prefix.

> I think the end result looks good, but that is best reviewed as one
> piece instead of 9 patches.


-- 
Eddy Petrișor

Re: Git issue report

[Junio C Hamano]

"xfswi...@163.com"  writes:

> Hi,
>   I met a issue when using git.
> I cannot delete the file by the commond 'git rm'.
> The file name is a little diff from common file.
> I accidentally named the file "filename\r", display such as filename^M. Then 
> I commit the file by 'git add .'.
> After I find this mistake, I remove the file, then try to use commond "git 
> rm" to delete the file, but failed.
>
> My git version is 1.7.9.5.

The following works fine with v1.7.9.5 (I have separate installs of
various versions of Git and use "rungit $version" to choose from
them, so just read "rungit v1.7.9.5" below as if it is "git").

$ rungit v1.7.9.5 init
$ N=$(printf "filename\015")
$ echo >"$N"
$ /bin/ls fil* | od -cx
000   f   i   l   e   n   a   m   e  \r  \n
   6966656c616e656d0a0d
012
$ rungit v1.7.9.5 add .
$ rungit v1.7.9.5 ls-files
"filename\r"
$ rm filename*
$ rungit v1.7.9.5 rm filename*
$ rungit v1.7.9.5 ls-files 
$ /bin/ls
$ exit

and I do not think of any reason why we would have broken it since.

FYI, you do not have to do a separate "rm filename*" in the above
sequence; "git rm filename*" would remove it from both the working
tree and from the index.  I did it in the above illustration in two
separate steps only because you said you removed and then did "git
rm" and I wanted to emulate it.

> Is this issue reported?

I do not recall hearing about it, but you must have looked for one
hard already and I haven't, so...

> If this issue is solved, could you tell me which version I should get.

It appears to me that such an issue did not exist in the first
place.

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Linus Torvalds  writes:

> Maybe something like the attached patch? Then I get:
> ...
> [torvalds@i7 linux]$ time git gc --prune=npw
> fatal: Failed to parse prune expiry value npw
>
> real0m0.004s
> user0m0.002s
> sys 0m0.002s
>
> and you could smush it into your commit (if you want my sign-off, take it)
>
>   Linus
>
>  builtin/gc.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3e67124ea..a4b20aaaf 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -354,6 +354,7 @@ int cmd_gc(int argc, const char **argv, const char 
> *prefix)
>   const char *name;
>   pid_t pid;
>   int daemonized = 0;
> + timestamp_t dummy;
>  
>   struct option builtin_gc_options[] = {
>   OPT__QUIET(, N_("suppress progress reporting")),
> @@ -392,6 +393,9 @@ int cmd_gc(int argc, const char **argv, const char 
> *prefix)
>   if (argc > 0)
>   usage_with_options(builtin_gc_usage, builtin_gc_options);
>  
> + if (parse_expiry_date(prune_expire, ))
> + die(_("Failed to parse prune expiry value %s"), prune_expire);
> +

At this point prune_expire could be NULL, so the if() needs a bit
tightening, but otherwise it looks good.

Here is the final one (at least for today).

-- >8 --
Subject: [PATCH] parseopt: handle malformed --expire arguments nicer

A few commands that parse --expire= command line option behave
silly when given nonsense input.  For example

$ git prune --no-expire
Segmentation falut
$ git prune --expire=npw; echo $?
129

Both come from parse_opt_expiry_date_cb().

The former is because the function is not prepared to see arg==NULL
(for "--no-expire", it is a norm; "--expire" at the end of the
command line could be made to pass NULL, if it is told that the
argument is optional, but we don't so we do not have to worry about
that case).

The latter is because it does not check the value returned from  the
underlying parse_expiry_date().

This seems to be a recent regression introduced while we attempted
to avoid spewing the entire usage message when given a correct
option but with an invalid value at 3bb0923f ("parse-options: do not
show usage upon invalid option value", 2018-03-22).  Before that, we
didn't fail silently but showed a full usage help (which arguably is
not all that better).

Also catch this error early when "git gc --prune=" is
misspelled by doing a dummy parsing before the main body of "gc"
that is time consuming even begins.  Otherwise, we'd spend time to
pack objects and then later have "git prune" first notice the error.
Aborting "gc" in the middle that way is not harmful but is ugly and
can be avoided.

Helped-by: Linus Torvalds 
Signed-off-by: Junio C Hamano 
---
 builtin/gc.c   |  4 
 parse-options-cb.c |  6 +-
 t/t5304-prune.sh   | 10 ++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3c5eae0edf..858aa444e1 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -353,6 +353,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
const char *name;
pid_t pid;
int daemonized = 0;
+   timestamp_t dummy;
 
struct option builtin_gc_options[] = {
OPT__QUIET(, N_("suppress progress reporting")),
@@ -388,6 +389,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
if (argc > 0)
usage_with_options(builtin_gc_usage, builtin_gc_options);
 
+   if (prune_expire && parse_expiry_date(prune_expire, ))
+   die(_("Failed to parse prune expiry value %s"), prune_expire);
+
if (aggressive) {
argv_array_push(, "-f");
if (aggressive_depth > 0)
diff --git a/parse-options-cb.c b/parse-options-cb.c
index c6679cb2cd..872627eafe 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const 
char *arg,
 int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
 int unset)
 {
-   return parse_expiry_date(arg, (timestamp_t *)opt->value);
+   if (unset)
+   arg = "never";
+   if (parse_expiry_date(arg, (timestamp_t *)opt->value))
+   die("malformed expiration date '%s'", arg);
+   return 0;
 }
 
 int parse_opt_color_flag_cb(const struct option *opt, const char *arg,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 6694c19a1e..af69cdc112 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -320,4 +320,14 @@ test_expect_success 'prune: handle HEAD reflog in multiple 
worktrees' '
test_cmp expected actual
 '
 
+test_expect_success 'prune: handle expire option correctly' '
+   test_must_fail git prune --expire 2>error &&
+   test_i18ngrep "requires a value" error &&
+
+   

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> We have a -s ours, but not a -s theirs. This is a WIP patch to implement
> that. It works, but I haven't dealt with this part of the internal API
> before, comments most welcome.
>
> The purpose of this is that I'm working with a rollout tool that is
> capable of doing hotfixes on top of old commits on "master".
>
> It does this by cherry-picking a commit from origin/master, and then
> merges it with origin/master & pushes it back, before finally reset
> --hard to the cherry-pick & rolling out.
>
> The reason it's doing this is to maintain the guarantee that all rolled
> out commits are reachable from "master", and to handle the more general
> case where original work is made during a hotfix, we don't want to then
> do a subsequent "normal" rollout and miss the fix.

This question has nothing to do with your "-s theirs" but let me see
if I got the above correctly.  Suppose you have a deployed branch
(say, "prod"), all developments happen on "master" elsewhere that
can be seen as "origin/master", so you may have a few fixes that is
not yet in "prod" you would want to cherry-pick from origin/master.

$ git checkout prod
$ git cherry-pick origin/master~2
$ git cherry-pick origin/master

Let's say that "master" had a fix at HEAD~2, HEAD~1 is a feature
enhancement that is not yet ready for "prod", and HEAD is another
fix.  Up to this point you successfully back-ported the fixes to
"prod".

Then you do merge the tip into "master", i.e.

$ git checkout origin/master && git merge -s ours prod
$ git push origin HEAD:master
$ git checkout prod

to make sure that the "master" at the source of truth knows that
it already has what our "prod" with these two cherry-picks have.

Is that what is going on here?

I am just wondering what would and should happen to the non-fix
commit in the middle in the above example.  Perhaps your workflow
automatically does the right thing to it, perhaps not.


[Footnote]

Obviously you can do this the other way around if you had "-s
theirs", i.e. instead of the last two lines from the above sequence,
you could do

$ git merge -s nth -X 2 origin/master
$ git push origin HEAD:master
$ git reset --hard HEAD@{1}

but it is not all that interesting (at least to me) either way, as a
larger issue with the above I'd imagine people would see is that
even temporarily you would expose "master" material in that working
tree you usually have "prod" checkout.  That would irritate those
who consider that "push to deploy" aka "live site is actually a
working tree" is sensible more than the lack of "-s theirs" I would
think.

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Questions:
>
>  1. Should I be calling read-tree here with run_command_v_opt(), or is
> there some internal API I should be using?

The internal is unpack_trees(), which is usabe as a library-ish API
reasonably cleanly and easily.  For a project large enough where the
perforce can become an issue, the overhead to spawn read-tree as an
external process would be dwarfed by the cost of real processing of
merging multiple trees into an in-core index, but it involves two
extra writing and reading the index file back and forth compared to
the approach to use unpack_trees() to do everything in core.  As
long as the "now make sure that the contents of the index file is
that of the tree of the N-th parent" code is cleanly isolated in the
implementation, it is probably better to refrain from premature
optimization.

>  2. Currently merge-ours is just a no-op since we take the current HEAD,
> but maybe it would be cleaner to implement it in terms of this
> thing, also to get immediate test coverage for all the -s ours
> stuff. We'd be reading the tree redundantly into the index, but
> maybe it's worth it for the coverage...

I doubt that it would be a sensible approach.  If anything, even if
we lived in an alternate universe where "-s ours" weren't invented
and "-s become-nth-tree -W $N" feature gets first introduced, I
would imagine that we would introduce a code to special case "-s
ours" (aka "take the current HEAD") as an obvious optimization for
the common and trivial case, essentially splitting the "unification"
you are suggesting here.

>  3. Is there a better name for this than -s theirs? Maybe `-s nth -X N`?

I tend to agree that "-s thiers -X N" is horrible; "-s ours -X N"
would slightly be a better choice as it does not have to introduce a
new option.  "-s nth -X N" does not sound all that bad.

"Does this feature make sense?" was not among the questions listed,
and I am not ready to answer to it yet anyway, so ...

Re: [PATCH v6 00/15] rebase -i: offer to recreate commit topology

[Sergey Organov]

Johannes Schindelin  writes:

> Hi Phillip,
>
> On Fri, 13 Apr 2018, Phillip Wood wrote:
>
>> On 12/04/18 23:02, Johannes Schindelin wrote:
>> > 
>> > [...]
>> > 
>> > So: the order of the 3-way merges does matter.
>> >
>> > [...]
>> 
>> Those conflicts certainly look intimidating (and the ones in your later
>> reply with the N way merge example still look quite complicated). One
>> option would be just to stop and have the user resolve the conflicts
>> after each conflicting 3-way merge rather than at the end of all the
>> merges. There are some downsides: there would need to be a way to
>> explain to the user that this is an intermediate step (and what that
>> step was); the code would have to do some book keeping to know where it
>> had got to; and it would stop and prompt the user to resolve conflicts
>> more often which could be annoying but hopefully they'd be clearer to
>> resolve because they weren't nested.
>
> I thought about that. But as I pointed out: the order of the merges *does*
> matter. Otherwise we force the user to resolve conflicts that they
> *already* resolved during this rebase...

How it's relevant to what Phillip suggested? How the order of taking 2
steps, A and B, affects an ability to stop after the first step? It's
still either "A,stop,B" or "B,stop,A", depending on the chosen order.

What's the _actual_ problem here, if any?

-- Sergey

Re: man page for "git remote set-url" seems confusing/contradictory

[Martin Ågren]

On 18 April 2018 at 22:56, Todd Zullinger  wrote:
> Tangentially (and I don't know if it's worth fixing), while
> poking around the documentation which includes urls.txt I
> noticed that git-clone.txt refers readers to the "URLS
> section below" when the name of the section is "GIT URLS".
>
> I doubt any readers would be confused, but it would be
> consistent with the other files which include urls.txt to
> use "GIT URLS" as the referenced section name.
>
> -- >& --
> Subject: [PATCH] doc/clone: update caption for GIT URLS cross-reference
>
> The description of the  argument directs readers to "See the
> URLS section below".  When generating HTML this becomes a link to the
> "GIT URLS" section.  When reading the man page in a terminal, the
> caption is slightly misleading.  Use "GIT URLS" as the caption to avoid
> an confusion.

s/an/any/?

>
> The man page produced by asciidoc doesn't include hyperlinks.  The
> description of the  argument simply
>

Abandoned first attempt at log message? ;-)

> Signed-off-by: Todd Zullinger 
> ---
>  Documentation/git-clone.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 42ca7b5095..b844b9957c 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -260,7 +260,7 @@ or `--mirror` is given)
>
>  ::
> The (possibly remote) repository to clone from.  See the
> -   <> section below for more information on specifying
> +   <> section below for more information on specifying
> repositories.

Indeed. Matches urls.txt and the others who refer there.

Martin

Re: [PATCH 1/2] commit: fix --short and --porcelain

[Samuel Lijin]

On Wed, Apr 18, 2018 at 8:55 PM, Samuel Lijin  wrote:
> Thanks for the quick review!
>
> On Wed, Apr 18, 2018 at 11:38 AM, Martin Ågren  wrote:
>> Hi Samuel,
>>
>> Welcome back. :-)
>>
>> On 18 April 2018 at 05:06, Samuel Lijin  wrote:
>>> Make invoking `git commit` with `--short` or `--porcelain` return status
>>> code zero when there is something to commit.
>>>
>>> Mark the commitable flag in the wt_status object in the call to
>>> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
>>> and simplify the logic in the latter function to take advantage of the
>>> logic shifted to the former.
>>
>> The subject is sort of vague about what is being fixed. Maybe "commit:
>> fix return code of ...", or "wt-status: set `commitable` when
>> collecting, not when printing". Or something... I can't come up with
>> something brilliant off the top of my head.
>>
>> I did not understand the first paragraph until I had read the second and
>> peaked at the code. Maybe tell the story the other way around? Something
>> like this:
>>
>>   Mark the `commitable` flag in the wt_status object in
>>   `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
>>   and simplify the logic in the latter function to take advantage of the
>>   logic shifted to the former.
>>
>>   This means that callers do need to actually use the printer function
>>   to collect the `commitable` flag -- it is sufficient to call
>>   `wt_status_collect()`.
>>
>>   As a result, invoking `git commit` with `--short` or `--porcelain`
>>   results in return status code zero when there is something to commit.
>>   This fixes two bugs documented in our test suite.
>
> That definitely works better. Will fix when I reroll.
>
>>>  t/t7501-commit.sh |  4 ++--
>>>  wt-status.c   | 39 +++
>>>  2 files changed, 29 insertions(+), 14 deletions(-)
>>
>> I tried to find somewhere in the documentation where this bug was
>> described (git-commit.txt or git-status.txt), but failed. So there
>> should be nothing to update there.
>>
>>> +static void wt_status_mark_commitable(struct wt_status *s) {
>>> +   int i;
>>> +
>>> +   for (i = 0; i < s->change.nr; i++) {
>>> +   struct wt_status_change_data *d = (s->change.items[i]).util;
>>> +
>>> +   if (d->index_status && d->index_status != 
>>> DIFF_STATUS_UNMERGED) {
>>> +   s->commitable = 1;
>>> +   return;
>>> +   }
>>> +   }
>>> +}
>>
>> This helper does exactly what the old code did inside
>> `wt_longstatus_print_updated()` with regards to `commitable`. Ok.
>>
>> This function does not reset `commitable` to 0, so reusing a `struct
>> wt_status` won't necessarily work out. I have not thought about whether
>> such a caller would be horribly broken for other reasons...
>>
>>>  void wt_status_collect(struct wt_status *s)
>>>  {
>>> wt_status_collect_changes_worktree(s);
>>> @@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
>>> wt_status_collect_changes_initial(s);
>>> else
>>> wt_status_collect_changes_index(s);
>>> +
>>> wt_status_collect_untracked(s);
>>> +
>>> +   wt_status_mark_commitable(s);
>>>  }
>>
>> So whenever we `..._collect()`, `commitable` is set for us. This is the
>> only caller of the new helper, so in order to be able to trust
>> `commitable`, one needs to call `wt_status_collect()`. Seems a
>> reasonable assumption to make that the caller will remember to do so
>> before printing. (And all current users do, so we're not regressing in
>> some user.)
>>
>>>  static void wt_longstatus_print_unmerged(struct wt_status *s)
>>> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct 
>>> wt_status *s)
>>>
>>>  static void wt_longstatus_print_updated(struct wt_status *s)
>>>  {
>>> -   int shown_header = 0;
>>> -   int i;
>>> +   if (!s->commitable) {
>>> +   return;
>>> +   }
>>
>> Regarding my comment above: If you forget to `..._collect()` first, this
>> function is a no-op.
>>
>>> +
>>> +   wt_longstatus_print_cached_header(s);
>>>
>>> +   int i;
>>
>> You should leave this variable declaration at the top of the function.
>>
>>> for (i = 0; i < s->change.nr; i++) {
>>> struct wt_status_change_data *d;
>>> struct string_list_item *it;
>>> it = &(s->change.items[i]);
>>> d = it->util;
>>> -   if (!d->index_status ||
>>> -   d->index_status == DIFF_STATUS_UNMERGED)
>>> -   continue;
>>> -   if (!shown_header) {
>>> -   wt_longstatus_print_cached_header(s);
>>> -   s->commitable = 1;
>>> -   shown_header = 1;
>>> +   if (d->index_status &&
>>> +   

Git issue report

[xfswi...@163.com]

Hi,
  I met a issue when using git.
I cannot delete the file by the commond 'git rm'.
The file name is a little diff from common file.
I accidentally named the file "filename\r", display such as filename^M. Then I 
commit the file by 'git add .'.
After I find this mistake, I remove the file, then try to use commond "git rm" 
to delete the file, but failed.

My git version is 1.7.9.5.
Is this issue reported?
If this issue is solved, could you tell me which version I should get.

Thanks!
Xiaofeng Chen
04/19/2018

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Linus Torvalds  writes:

> Yes, I get that nice "malformed expiration date 'npw'" error, but I
> get it after 64 seconds has passed.

Ah, that timing aspect of the issue didn't occur to me.  The patch
indeed is a reasonable workaround.

Thanks.

Re: [PATCH v9 2/2] builtin/config.c: support `--type=` as preferred alias for `--type`

[Taylor Blau]

On Thu, Apr 19, 2018 at 11:47:50AM +0900, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > diff --git a/builtin/config.c b/builtin/config.c
> > index 92fb8d56b1..bd7a8d0ce7 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -61,6 +61,58 @@ static int show_origin;
> >  #define TYPE_PATH  4
> >  #define TYPE_EXPIRY_DATE   5
> >
> > +#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
> > +   { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
> > +   PARSE_OPT_NONEG, option_parse_type, (i) }
> > +
> > +static struct option builtin_config_options[];
> > +
> > +static int option_parse_type(const struct option *opt, const char *arg,
> > +int unset)
> > +{
>
> Declare all local variables here.  We do not accept decl-after-statement.

My apologies, I will read Documentation/CodingGuidelines carefully. I
have generated the following patch locally:

diff --git a/builtin/config.c b/builtin/config.c
index bd7a8d0ce7..2f91ef15a4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -70,6 +70,9 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 int unset)
 {
+   int new_type;
+   int *to_type;
+
if (unset) {
*((int *) opt->value) = 0;
return 0;
@@ -79,7 +82,7 @@ static int option_parse_type(const struct option *opt, const 
char *arg,
 * To support '--' style flags, begin with new_type equal to
 * opt->defval.
 */
-   int new_type = opt->defval;
+   new_type = opt->defval;
if (!new_type) {
if (!strcmp(arg, "bool"))
new_type = TYPE_BOOL;
@@ -95,7 +98,7 @@ static int option_parse_type(const struct option *opt, const 
char *arg,
die(_("unrecognized --type argument, %s"), arg);
}

-   int *to_type = opt->value;
+   *to_type = opt->value;
if (*to_type && *to_type != new_type) {
/*
 * Complain when there is a new type not equal to the old type.

---

And would be happy to apply it locally myself and send it to you via a
re-roll. You are also free to apply it yourself if it would be easier. I
do not have a preference one way or another.


Thanks,
Taylor

Re: [PATCH v9 2/2] builtin/config.c: support `--type=` as preferred alias for `--type`

[Junio C Hamano]

Taylor Blau  writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index 92fb8d56b1..bd7a8d0ce7 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -61,6 +61,58 @@ static int show_origin;
>  #define TYPE_PATH4
>  #define TYPE_EXPIRY_DATE 5
>  
> +#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
> + { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
> + PARSE_OPT_NONEG, option_parse_type, (i) }
> +
> +static struct option builtin_config_options[];
> +
> +static int option_parse_type(const struct option *opt, const char *arg,
> +  int unset)
> +{

Declare all local variables here.  We do not accept decl-after-statement.

> + if (unset) {
> + *((int *) opt->value) = 0;
> + return 0;
> + }
> +
> + /*
> +  * To support '--' style flags, begin with new_type equal to
> +  * opt->defval.
> +  */
> + int new_type = opt->defval;

Like this one and ...

> + if (!new_type) {
> + if (!strcmp(arg, "bool"))
> + new_type = TYPE_BOOL;
> + else if (!strcmp(arg, "int"))
> + new_type = TYPE_INT;
> + else if (!strcmp(arg, "bool-or-int"))
> + new_type = TYPE_BOOL_OR_INT;
> + else if (!strcmp(arg, "path"))
> + new_type = TYPE_PATH;
> + else if (!strcmp(arg, "expiry-date"))
> + new_type = TYPE_EXPIRY_DATE;
> + else
> + die(_("unrecognized --type argument, %s"), arg);
> + }
> +
> + int *to_type = opt->value;

... this one.

> + if (*to_type && *to_type != new_type) {
> + /*
> +  * Complain when there is a new type not equal to the old type.
> +  * This allows for combinations like '--int --type=int' and
> +  * '--type=int --type=int', but disallows ones like '--type=bool
> +  * --int' and '--type=bool
> +  * --type=int'.
> +  */
> + error("only one type at a time.");
> + usage_with_options(builtin_config_usage,
> + builtin_config_options);
> + }
> + *to_type = new_type;
> +
> + return 0;
> +}
> +

Re: Silly "git gc" UI issue.

[Linus Torvalds]

On Wed, Apr 18, 2018 at 7:16 PM, Junio C Hamano  wrote:
> A few commands that parse --expire= command line option
> behaves silly when given nonsense input.  For example

So this patch definitely improves on the error message.

But look at what happens for the kernel:

[torvalds@i7 linux]$ time git gc --prune=npw
Counting objects: 6006319, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (912166/912166), done.
Writing objects: 100% (6006319/6006319), done.
Total 6006319 (delta 5050577), reused 6006319 (delta 5050577)
fatal: malformed expiration date 'npw'
error: failed to run prune

real1m4.376s
user0m59.963s
sys 0m5.182s



Yes, I get that nice "malformed expiration date 'npw'" error, but I
get it after 64 seconds has passed.

So i think builtin/gc.c should use this same parse_expiry_date()
parse_opt_expiry_date_cb() thing for its timestamp parsing.

It does actually seem to do that for the gc_log_expire value that it
loads from the config file.

Maybe something like the attached patch? Then I get:

[torvalds@i7 linux]$ time git gc --prune=npw
fatal: Failed to parse prune expiry value npw

real0m0.004s
user0m0.002s
sys 0m0.002s

and you could smush it into your commit (if you want my sign-off, take it)

  Linus
 builtin/gc.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124ea..a4b20aaaf 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -354,6 +354,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	const char *name;
 	pid_t pid;
 	int daemonized = 0;
+	timestamp_t dummy;
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(, N_("suppress progress reporting")),
@@ -392,6 +393,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (argc > 0)
 		usage_with_options(builtin_gc_usage, builtin_gc_options);
 
+	if (parse_expiry_date(prune_expire, ))
+		die(_("Failed to parse prune expiry value %s"), prune_expire);
+
 	if (aggressive) {
 		argv_array_push(, "-f");
 		if (aggressive_depth > 0)

[PATCH] send-email: allow re-editing of message

[Drew DeVault]

When shown the email summary, an opportunity is presented for the user
to edit the email as if they had specified --annotate. This also permits
them to edit it multiple times.

Signed-off-by: Drew DeVault 
Reviewed-by: Simon Ser 
---
 git-send-email.perl | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2fa7818ca..14f2e8ae4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1330,9 +1330,14 @@ sub file_name_is_absolute {
return File::Spec::Functions::file_name_is_absolute($path);
 }
 
-# Returns 1 if the message was sent, and 0 otherwise.
-# In actuality, the whole program dies when there
-# is an error sending a message.
+# Prepares the email, then asks the user what to do.
+#
+# If the user chooses to send the email, it's sent and 1 is returned.
+# If the user chooses not to send the email, 0 is returned.
+# If the user decides they want to make further edits, -1 is returned and the
+# caller is expected to call send_message again after the edits are performed.
+#
+# If an error occurs sending the email, this just dies.
 
 sub send_message {
my @recipients = unique_email_list(@to);
@@ -1404,15 +1409,17 @@ Message-Id: $message_id
 
 EOF
}
-   # TRANSLATORS: Make sure to include [y] [n] [q] [a] in your
+   # TRANSLATORS: Make sure to include [y] [n] [e] [q] [a] in your
# translation. The program will only accept English input
# at this point.
-   $_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
-valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
+   $_ = ask(__("Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): 
"),
+valid_re => qr/^(?:yes|y|no|n|edit|e|quit|q|all|a)/i,
 default => $ask_default);
die __("Send this email reply required") unless defined $_;
if (/^n/i) {
return 0;
+   } elsif (/^e/i) {
+   return -1;
} elsif (/^q/i) {
cleanup_compose_files();
exit(0);
@@ -1552,7 +1559,9 @@ $references = $initial_in_reply_to || '';
 $subject = $initial_subject;
 $message_num = 0;
 
-foreach my $t (@files) {
+sub process_file {
+   my ($t) = @_;
+
open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
 
my $author = undef;
@@ -1755,6 +1764,10 @@ foreach my $t (@files) {
}
 
my $message_was_sent = send_message();
+   if ($message_was_sent == -1) {
+   do_edit($t);
+   return 0;
+   }
 
# set up for the next message
if ($thread && $message_was_sent &&
@@ -1776,6 +1789,14 @@ foreach my $t (@files) {
undef $auth;
sleep($relogin_delay) if defined $relogin_delay;
}
+
+   return 1;
+}
+
+foreach my $t (@files) {
+   while (!process_file($t)) {
+   # This space deliberately left blank
+   }
 }
 
 # Execute a command (e.g. $to_cmd) to get a list of email addresses
-- 
2.17.0

Re: Silly "git gc" UI issue.

[Linus Torvalds]

On Wed, Apr 18, 2018 at 6:52 PM, Junio C Hamano  wrote:
>
> Regardless of your originai "git gc" issue, we should make "prune"
> say something on this error.  And when we do so, I would think that
> error message will come before the final "error: failed to run
> prune".

So to me, the real failure is the fact that it spent a a lot of time
packing my repository before it then failed the prune at the end.

I don't actually mind the quality of the error message too much -
although it could be improved.

I mind the "oh, goddamnit, you just spent over a minute of CPU time on
my fairly high-end desktop, and _then_ you decided to tell me that I'm
a moron and couldn't type 'now' correctly".

So to me, the big deal would be that builtin/gc.c should validate the
date *before* it starts, instead of doing all that work, and then
executing "git prune" with invalid arguments..

Linus

Re: Silly "git gc" UI issue.

[Junio C Hamano]

A few commands that parse --expire= command line option
behaves silly when given nonsense input.  For example

$ git prune --no-expire
Segmentation falut
$ git prune --expire=npw; echo $?
129

Both come from parse_opt_expiry_date_cb().

The former is because the function is not prepared to see arg==NULL
(for "--no-expire", it is a norm; "--expire" at the end of the
command line could be made to pass NULL, if it is told that the
argument is optional, but we don't so we do not have to worry about
that case).

The latter is because it does not check the value returned from  the
underlying parse_expiry_date().  

This seems to be a recent regression introduced while we attempted
to avoid spewing the entire usage message when given a correct
option but with an invalid value at 3bb0923f ("parse-options: do not
show usage upon invalid option value", 2018-03-22).

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

 * I do not expect this to be the final version (not just it lacks
   tests, but I haven't even run existing tests with the change
   yet), but I think I diagnosed the root cause correctly, at least.

 parse-options-cb.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index c6679cb2cd..872627eafe 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -38,7 +38,11 @@ int parse_opt_approxidate_cb(const struct option *opt, const 
char *arg,
 int parse_opt_expiry_date_cb(const struct option *opt, const char *arg,
 int unset)
 {
-   return parse_expiry_date(arg, (timestamp_t *)opt->value);
+   if (unset)
+   arg = "never";
+   if (parse_expiry_date(arg, (timestamp_t *)opt->value))
+   die("malformed expiration date '%s'", arg);
+   return 0;
 }
 
 int parse_opt_color_flag_cb(const struct option *opt, const char *arg,

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Junio C Hamano  writes:

> It turns out that prune silently goes away given a bad expiry
>
> $ git prune --expire=nyah ; echo $?
> 129
>
> Regardless of your originai "git gc" issue, we should make "prune"
> say something on this error.  And when we do so, I would think that
> error message will come before the final "error: failed to run
> prune".
>
> Or perhaps we do so and then squelch "error: failed to run prune",
> trusting that a corrected "git prune" will always say something when
> it fails.

It turns out that

$ git worktree prune --expire=nyah

shares the same issue.  I'll take a look at OPT_EXPIRY_DATE() thing.

Re: Silly "git gc" UI issue.

[Junio C Hamano]

Linus Torvalds  writes:

> You get this:
>
>git gc --prune=npw
>
> Yeah, that "npw" should be "now", which is where the klutz thing comes in.
>
> It turns out that git reacts ridiculously badly to this.

$ git gc --prune=npw
Counting objects: 10, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (10/10), done.
Total 10 (delta 2), reused 10 (delta 2)
error: failed to run prune

It turns out that prune silently goes away given a bad expiry

$ git prune --expire=nyah ; echo $?
129

Regardless of your originai "git gc" issue, we should make "prune"
say something on this error.  And when we do so, I would think that
error message will come before the final "error: failed to run
prune".

Or perhaps we do so and then squelch "error: failed to run prune",
trusting that a corrected "git prune" will always say something when
it fails.

Silly "git gc" UI issue.

[Linus Torvalds]

Ok, this is ridiculous, but I've done it several times, so I thought
I'd finally mention it to somebody on the git list that may care:

  "My name is Linus, and I'm a klutz".

what does that have to do with anything?

Now, imagine you're a klutz. Imagine you want to clean up your .git
directory. Combine those things, and what do you get?

You get this:

   git gc --prune=npw

Yeah, that "npw" should be "now", which is where the klutz thing comes in.

It turns out that git reacts ridiculously badly to this.

I'm just assuming that everybody else is scarily competent if I'm the
first to have reported this.

  Linus

Re: [PATCH 0/4] doc: cleaning up instances of \--

[brian m. carlson]

On Tue, Apr 17, 2018 at 09:15:25PM +0200, Martin Ågren wrote:
> This is a patch series to convert \-- to -- in our documentation. The
> first patch is a reiteration of 1c262bb7b2 (doc: convert \--option to
> --option, 2015-05-13) to fix some instances that have appeared since.
> The other three patches deal with standalone "\--" which we can't
> always turn into "--" since it can be rendered as an em dash.

This series looked sane to me as well.  I appreciate the work to keep
our documentation working in both AsciiDoc and Asciidoctor.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v3 0/9] Compute and consume generation numbers

[Jakub Narebski]

Derrick Stolee  writes:

> -- >8 --
>
> This is the one of several "small" patches that follow the serialized
> Git commit graph patch (ds/commit-graph) and lazy-loading trees
> (ds/lazy-load-trees).
>
> As described in Documentation/technical/commit-graph.txt, the generation
> number of a commit is one more than the maximum generation number among
> its parents (trivially, a commit with no parents has generation number
> one). This section is expanded to describe the interaction with special
> generation numbers GENERATION_NUMBER_INFINITY (commits not in the commit-graph
> file) and *_ZERO (commits in a commit-graph file written before generation
> numbers were implemented).
>
> This series makes the computation of generation numbers part of the
> commit-graph write process.
>
> Finally, generation numbers are used to order commits in the priority
> queue in paint_down_to_common(). This allows a short-circuit mechanism
> to improve performance of `git branch --contains`.
>
> Further, use generation numbers for 'git tag --contains', providing a
> significant speedup (at least 95% for some cases).
>
> A more substantial refactoring of revision.c is required before making
> 'git log --graph' use generation numbers effectively.
>
> This patch series is build on ds/lazy-load-trees.
>
> Derrick Stolee (9):
>   commit: add generation number to struct commmit

Nice and short patch. Looks good to me.

>   commit-graph: compute generation numbers

Another quite easy to understand patch. LGTM.

>   commit: use generations in paint_down_to_common()

Nice and short patch; minor typo in comment in code.
Otherwise it looks good to me.

>   commit-graph.txt: update design document

I see that diagram got removed in this version; maybe it could be
replaced with relationship table?

Anyway, it looks good to me.

>   ref-filter: use generation number for --contains

A question: how performance looks like after the change if commit-graph
is not available?

>   commit: use generation numbers for in_merge_bases()

Possible typo in the commit message, and stylistic inconsistence in
in_merge_bases() - though actually more clear than existing code.

Short, simple, and gives good performance improvenebts.

>   commit: add short-circuit to paint_down_to_common()

Looks good to me; ignore [mostly] what I have written in response to the
patch in question.

>   commit-graph: always load commit-graph information

Looks all right; question: parameter or one more global variable.

>   merge: check config before loading commits

This looks good to me.

>
>  Documentation/technical/commit-graph.txt | 30 +--
>  alloc.c  |  1 +
>  builtin/merge.c  |  5 +-
>  commit-graph.c   | 99 +++-
>  commit-graph.h   |  8 ++
>  commit.c | 54 +++--
>  commit.h |  7 +-
>  object.c |  2 +-
>  ref-filter.c | 23 +-
>  sha1_file.c  |  2 +-
>  t/t5318-commit-graph.sh  |  9 +++
>  11 files changed, 199 insertions(+), 41 deletions(-)
>
>
> base-commit: 7b8a21dba1bce44d64bd86427d3d92437adc4707

Re: [PATCH v3 8/9] commit-graph: always load commit-graph information

[Jakub Narebski]

Derrick Stolee  writes:

> Most code paths load commits using lookup_commit() and then
> parse_commit(). In some cases, including some branch lookups, the commit
> is parsed using parse_object_buffer() which side-steps parse_commit() in
> favor of parse_commit_buffer().
>
> With generation numbers in the commit-graph, we need to ensure that any
> commit that exists in the commit-graph file has its generation number
> loaded.

All right, that is nice explanation of the why behind this change.

>
> Create new load_commit_graph_info() method to fill in the information
> for a commit that exists only in the commit-graph file. Call it from
> parse_commit_buffer() after loading the other commit information from
> the given buffer. Only fill this information when specified by the
> 'check_graph' parameter. This avoids duplicate work when we already
> checked the graph in parse_commit_gently() or when simply checking the
> buffer contents in check_commit().

Couldn't this 'check_graph' parameter be a global variable similar to
the 'commit_graph' variable?  Maybe I am not understanding it.

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 51 --
>  commit-graph.h |  8 
>  commit.c   |  7 +--
>  commit.h   |  2 +-
>  object.c   |  2 +-
>  sha1_file.c|  2 +-
>  6 files changed, 49 insertions(+), 23 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 688d5b1801..21e853c21a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -245,13 +245,19 @@ static struct commit_list **insert_parent_or_die(struct 
> commit_graph *g,
>   return _list_insert(c, pptr)->next;
>  }
>  
> +static void fill_commit_graph_info(struct commit *item, struct commit_graph 
> *g, uint32_t pos)
> +{
> + const unsigned char *commit_data = g->chunk_commit_data + 
> GRAPH_DATA_WIDTH * pos;
> + item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
> +}
> +
>  static int fill_commit_in_graph(struct commit *item, struct commit_graph *g, 
> uint32_t pos)
>  {
>   uint32_t edge_value;
>   uint32_t *parent_data_ptr;
>   uint64_t date_low, date_high;
>   struct commit_list **pptr;
> - const unsigned char *commit_data = g->chunk_commit_data + (g->hash_len 
> + 16) * pos;
> + const unsigned char *commit_data = g->chunk_commit_data + 
> GRAPH_DATA_WIDTH * pos;

I'm probably wrong, but isn't it unrelated change?

>  
>   item->object.parsed = 1;
>   item->graph_pos = pos;
> @@ -292,31 +298,40 @@ static int fill_commit_in_graph(struct commit *item, 
> struct commit_graph *g, uin
>   return 1;
>  }
>  
> +static int find_commit_in_graph(struct commit *item, struct commit_graph *g, 
> uint32_t *pos)
> +{
> + if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
> + *pos = item->graph_pos;
> + return 1;
> + } else {
> + return bsearch_graph(commit_graph, &(item->object.oid), pos);
> + }
> +}

All right (after the fix).

> +
>  int parse_commit_in_graph(struct commit *item)
>  {
> + uint32_t pos;
> +
> + if (item->object.parsed)
> + return 0;
>   if (!core_commit_graph)
>   return 0;
> - if (item->object.parsed)
> - return 1;

Hmmm... previously the function returned 1 if item->object.parsed, now
it returns 0 for this situation.  I don't understand this change.

> -
>   prepare_commit_graph();
> - if (commit_graph) {
> - uint32_t pos;
> - int found;
> - if (item->graph_pos != COMMIT_NOT_FROM_GRAPH) {
> - pos = item->graph_pos;
> - found = 1;
> - } else {
> - found = bsearch_graph(commit_graph, 
> &(item->object.oid), );
> - }
> -
> - if (found)
> - return fill_commit_in_graph(item, commit_graph, pos);
> - }
> -
> + if (commit_graph && find_commit_in_graph(item, commit_graph, ))
> + return fill_commit_in_graph(item, commit_graph, pos);

Nice refactoring.

>   return 0;
>  }
>  
> +void load_commit_graph_info(struct commit *item)
> +{
> + uint32_t pos;
> + if (!core_commit_graph)
> + return;
> + prepare_commit_graph();
> + if (commit_graph && find_commit_in_graph(item, commit_graph, ))
> + fill_commit_graph_info(item, commit_graph, pos);
> +}

And the reason for the refactoring.

> +
>  static struct tree *load_tree_for_commit(struct commit_graph *g, struct 
> commit *c)
>  {
>   struct object_id oid;
> diff --git a/commit-graph.h b/commit-graph.h
> index 260a468e73..96cccb10f3 100644
> --- a/commit-graph.h
> +++ b/commit-graph.h
> @@ -17,6 +17,14 @@ char *get_commit_graph_filename(const char *obj_dir);
>   */
>  int parse_commit_in_graph(struct commit *item);
>  
> +/*
> + * It is possible that we loaded commit contents from the commit 

Re: [RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

[Stefan Beller]

Hi Eddy,

all the following patches 3-9 are touching the test as added in patch
2, which would go best with this patch.
Could you squash all commits into one?

There are a couple ways to do it:

  git reset --soft
  git commit -a --reuse-commit-message=<...>

or using

git rebase --interactive origin/master
# and then marking all but the first as "fixup"

I think the end result looks good, but that is best reviewed as one
piece instead of 9 patches.

Thanks,
Stefan

Re: [RFC WIP PATCH] merge: implement -s theirs -X N

[Stefan Beller]

On Wed, Apr 18, 2018 at 3:48 PM, Ævar Arnfjörð Bjarmason
 wrote:
> We have a -s ours, but not a -s theirs. This is a WIP patch to implement
> that. It works, but I haven't dealt with this part of the internal API
> before, comments most welcome.

I hope reference pointers are welcome, too.
https://public-inbox.org/git/xmqqzi9iazrp@gitster.mtv.corp.google.com/

Re: [PATCH v3 7/9] commit: add short-circuit to paint_down_to_common()

[Jakub Narebski]

Derrick Stolee  writes:

> When running 'git branch --contains', the in_merge_bases_many()
> method calls paint_down_to_common() to discover if a specific
> commit is reachable from a set of branches. Commits with lower
> generation number are not needed to correctly answer the
> containment query of in_merge_bases_many().

Right. This description is not entirely clear to me, but I don't have a
better proposal. Good enough, I guess.

>
> Add a new parameter, min_generation, to paint_down_to_common() that
> prevents walking commits with generation number strictly less than
> min_generation. If 0 is given, then there is no functional change.

Is it new parameter really needed, i.e. do you really need to change the
signature of this function?  See below for details.

>
> For in_merge_bases_many(), we can pass commit->generation as the
> cutoff,...

This is the only callsite that uses min_generation with non-zero value,
and it uses commit->generation to fill it... while commit itself is one
of exiting parameters.

> [...], and this saves time during 'git branch --contains' queries
> that would otherwise walk "around" the commit we are inspecting.

If I understand the code properly, what happens is that we can now
short-circuit if all commits that are left are lower than the target
commit.

This is because max-order priority queue is used: if the commit with
maximum generation number is below generation number of target commit,
then target commit is not reachable from any commit in the priority
queue (all of which has generation number less or equal than the commit
at head of queue, i.e. all are same level or deeper); compare what I
have written in [1]

[1]: https://public-inbox.org/git/866052dkju@gmail.com/

Do I have that right?  If so, it looks all right to me.

>
> For a copy of the Linux repository, where HEAD is checked out at
> v4.13~100, we get the following performance improvement for
> 'git branch --contains' over the previous commit:
>
> Before: 0.21s
> After:  0.13s
> Rel %: -38%

Nice.

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index bceb79c419..a70f120878 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -805,11 +805,14 @@ static int queue_has_nonstale(struct prio_queue *queue)
>  }
>  
>  /* all input commits in one and twos[] must have been parsed! */
> -static struct commit_list *paint_down_to_common(struct commit *one, int n, 
> struct commit **twos)
> +static struct commit_list *paint_down_to_common(struct commit *one, int n,
> + struct commit **twos,
> + int min_generation)
>  {
>   struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
>   struct commit_list *result = NULL;
>   int i;
> + uint32_t last_gen = GENERATION_NUMBER_INFINITY;

Do we really need to change the signature of paint_down_to_common(), or
would it be enough to create a local variable min_generation set
initially to one->generation.

 +  uint32_t min_generation = one->generation;
 +  uint32_t last_gen = GENERATION_NUMBER_INFINITY;

>  
>   one->object.flags |= PARENT1;
>   if (!n) {
> @@ -828,6 +831,13 @@ static struct commit_list *paint_down_to_common(struct 
> commit *one, int n, struc
>   struct commit_list *parents;
>   int flags;
>  
> + if (commit->generation > last_gen)
> + BUG("bad generation skip");
> + last_gen = commit->generation;
> +
> + if (commit->generation < min_generation)
> + break;
> +

I think, after looking at the whole post-image code, that it is all
right.

>   flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
>   if (flags == (PARENT1 | PARENT2)) {
>   if (!(commit->object.flags & RESULT)) {
> @@ -876,7 +886,7 @@ static struct commit_list *merge_bases_many(struct commit 
> *one, int n, struct co
>   return NULL;
>   }
>  
> - list = paint_down_to_common(one, n, twos);
> + list = paint_down_to_common(one, n, twos, 0);
>  
>   while (list) {
>   struct commit *commit = pop_commit();
> @@ -943,7 +953,7 @@ static int remove_redundant(struct commit **array, int 
> cnt)
>   filled_index[filled] = j;
>   work[filled++] = array[j];
>   }
> - common = paint_down_to_common(array[i], filled, work);
> + common = paint_down_to_common(array[i], filled, work, 0);
>   if (array[i]->object.flags & PARENT2)
>   redundant[i] = 1;
>   for (j = 0; j < filled; j++)
> @@ -1067,7 +1077,7 @@ int in_merge_bases_many(struct commit *commit, int 
> nr_reference, struct commit *
>   if (commit->generation 

[RFC WIP PATCH] merge: implement -s theirs -X N

[Ævar Arnfjörð Bjarmason]

We have a -s ours, but not a -s theirs. This is a WIP patch to implement
that. It works, but I haven't dealt with this part of the internal API
before, comments most welcome.

The purpose of this is that I'm working with a rollout tool that is
capable of doing hotfixes on top of old commits on "master".

It does this by cherry-picking a commit from origin/master, and then
merges it with origin/master & pushes it back, before finally reset
--hard to the cherry-pick & rolling out.

The reason it's doing this is to maintain the guarantee that all rolled
out commits are reachable from "master", and to handle the more general
case where original work is made during a hotfix, we don't want to then
do a subsequent "normal" rollout and miss the fix.

In cases where I detect (by looking at patch-id's) that the only commits
that are being hotfixed are those cherry-picked from upstream, I want to
fully resolve the merge in favor of @{u}, i.e. end up with the same tree
SHA-1. This is the inverse of -s ours, but we have no -s theirs, this
implements that.

The `-s recursive -X theirs strategy` won't do, because that will just
resolve conflicts in favor of @{u}, but will screw up the well-known
cases where a merge produces bad results with no conflicts, due to edge
cases where patches apply cleanly but produce broken programs.

So this can be used as `-s theirs -X 2 @{u}` to implement that. The `-s
ours` strategy is then equivalent ot `-s theirs -X 1` (1st commit), and
we can do any value of `-X N` for octopus merges.

As noted `-s theirs` should be the same as supplying it with `-X 2`, but
I haven't implemented that yet.

Questions:

 1. Should I be calling read-tree here with run_command_v_opt(), or is
there some internal API I should be using?

 2. Currently merge-ours is just a no-op since we take the current HEAD,
but maybe it would be cleaner to implement it in terms of this
thing, also to get immediate test coverage for all the -s ours
stuff. We'd be reading the tree redundantly into the index, but
maybe it's worth it for the coverage...

 3. Is there a better name for this than -s theirs? Maybe `-s nth -X N`?

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..65d62b191f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -93,6 +93,7 @@
 /git-merge-recursive
 /git-merge-resolve
 /git-merge-subtree
+/git-merge-theirs
 /git-mergetool
 /git-mergetool--lib
 /git-mktag
diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 4a58aad4b8..a84482aafc 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -117,6 +117,12 @@ ours::
branches.  Note that this is different from the -Xours option to
the 'recursive' merge strategy.

+theirs::
+   This resolves any number of heads, but the resulting tree of
+   the merge is always that of the Nth branch head specified via
+   `-X n`. If it's not specified it'll default ot `-X 2`,
+   supplying `-X 1` makes this equivalent to the `ours` strategy.
+
 subtree::
This is a modified recursive strategy. When merging trees A and
B, if B corresponds to a subtree of A, B is first adjusted to
diff --git a/Makefile b/Makefile
index f181687250..00ded0c37c 100644
--- a/Makefile
+++ b/Makefile
@@ -998,6 +998,7 @@ BUILTIN_OBJS += builtin/merge-file.o
 BUILTIN_OBJS += builtin/merge-index.o
 BUILTIN_OBJS += builtin/merge-ours.o
 BUILTIN_OBJS += builtin/merge-recursive.o
+BUILTIN_OBJS += builtin/merge-theirs.o
 BUILTIN_OBJS += builtin/merge-tree.o
 BUILTIN_OBJS += builtin/mktag.o
 BUILTIN_OBJS += builtin/mktree.o
@@ -2815,6 +2816,7 @@ check-docs::
case "$$v" in \
git-merge-octopus | git-merge-ours | git-merge-recursive | \
git-merge-resolve | git-merge-subtree | \
+   git-merge-theirs \
git-fsck-objects | git-init-db | \
git-remote-* | git-stage | \
git-?*--?* ) continue ;; \
diff --git a/builtin.h b/builtin.h
index 42378f3aa4..a48eaf3a67 100644
--- a/builtin.h
+++ b/builtin.h
@@ -187,6 +187,7 @@ extern int cmd_merge_index(int argc, const char **argv, 
const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char 
*prefix);
+extern int cmd_merge_theirs(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_mktag(int argc, const char **argv, const char *prefix);
 extern int cmd_mktree(int argc, const char **argv, const char *prefix);
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index c84c6e05e9..da5ba0b370 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -18,7 +18,6 @@ int cmd_merge_ours(int argc, const char **argv, const char 
*prefix)
 {
if (argc == 2 && 

[RFC PATCH v4 6/9] fixup:t7406:don't call init after add, is redundant

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c5b412c46..32995e272 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -275,8 +275,7 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
git config -f .gitmodules submodule."submodl2b2".branch b2 &&
git add .gitmodules &&
test_tick &&
-   git commit -m "add l2 module with branch b2 in l1 module in branch b1" 
&&
-   git submodule init submodl2b2 &&
+   git commit -m "add l2 (on b2) in l1 (on b1)" &&
git rev-parse --verify HEAD >../expectl1 &&
git checkout master &&
cd ../super5 &&
-- 
2.16.2

[RFC PATCH v4 4/9] fixup:t7404:use 'git -C' instead of cd .. && git

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fb370991..974f949df 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -288,10 +288,7 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
git commit -m "add l1 module with branch b1 in super5" &&
git submodule init submodl1b1 &&
git clone super5 super &&
-   (
-   cd super &&
-   git submodule update --recursive --init
-   ) &&
+   git -C super submodule update --recursive --init &&
(
cd submodl1b1 &&
git rev-parse --verify HEAD >../../actuall1 &&
-- 
2.16.2

[RFC PATCH v4 5/9] fixup:t7406:cleanup chained submodules after test is done

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 974f949df..c5b412c46 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -298,7 +298,9 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
cd submodl2b2 &&
git rev-parse --verify HEAD >../../../actuall2 &&
test_cmp ../../../expectl2 ../../../actuall2
-   )
+   ) &&
+   test_when_finished "rm submodl2b2" &&
+   test_when_finished "rm submodl1b1"
 '
 
 test_expect_success 'local config should override .gitmodules branch' '
-- 
2.16.2

[RFC PATCH v4 8/9] fixup:t7406:use super_w instead of the existing super

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 18328be73..f44872143 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -286,10 +286,11 @@ test_expect_success 'submodule update --remote 
--recursive --init should fetch m
test_tick &&
git commit -m "add l1 module with branch b1 in super5" &&
git submodule init submodl1b1 &&
-   git clone super5 super &&
-   git -C super submodule update --recursive --init &&
+   cd .. &&
+   git clone super5 super_w &&
+   git -C super_w submodule update --recursive --init &&
(
-   cd submodl1b1 &&
+   cd super_w/submodl1b1 &&
git rev-parse --verify HEAD >../../actuall1 &&
test_cmp ../../expectl1 ../../actuall1
) &&
@@ -298,6 +299,7 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
git rev-parse --verify HEAD >../../../actuall2 &&
test_cmp ../../../expectl2 ../../../actuall2
) &&
+   test_when_finished "rm super_w" &&
test_when_finished "rm submodl2b2" &&
test_when_finished "rm submodl1b1"
 '
-- 
2.16.2

[RFC PATCH v4 2/9] t7406: add test for non-default branch in submodule

[Eddy Petrișor]

From: Eddy Petrișor 

If a submodule uses a non-default branch and the branch info is versioned, on
submodule update --recursive --init the correct branch should be checked out.

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 54 +
 1 file changed, 54 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 6f083c4d6..7b65f1dd1 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -259,6 +259,60 @@ test_expect_success 'submodule update --remote should 
fetch upstream changes wit
)
 '
 
+test_expect_success 'submodule update --remote --recursive --init should fetch 
module branch from .gitmodules' '
+   git clone . super5 &&
+   git clone super5 submodl2b2 &&
+   git clone super5 submodl1b1 &&
+   cd submodl2b2 &&
+   echo linel2b2 > l2b2 &&
+   git checkout -b b2 &&
+   git add l2b2 &&
+   test_tick &&
+   git commit -m "commit on b2 branch in l2" &&
+   git rev-parse --verify HEAD >../expectl2 &&
+   git checkout master &&
+   cd ../submodl1b1 &&
+   git checkout -b b1 &&
+   echo linel1b1 > l1b1 &&
+   git add l1b1 &&
+   test_tick &&
+   git commit -m "commit on b1 branch in l1" &&
+   git submodule add ../submodl2b2 submodl2b2 &&
+   git config -f .gitmodules submodule."submodl2b2".branch b2 &&
+   git add .gitmodules &&
+   test_tick &&
+   git commit -m "add l2 module with branch b2 in l1 module in branch b1" 
&&
+   git submodule init submodl2b2 &&
+   git rev-parse --verify HEAD >../expectl1 &&
+   git checkout master &&
+   cd ../super5 &&
+   echo super_with_2_chained_modules > super5 &&
+   git add super5 &&
+   test_tick &&
+   git commit -m "commit on default branch in super5" &&
+   git submodule add ../submodl1b1 submodl1b1 &&
+   git config -f .gitmodules submodule."submodl1b1".branch b1 &&
+   git add .gitmodules &&
+   test_tick &&
+   git commit -m "add l1 module with branch b1 in super5" &&
+   git submodule init submodl1b1 &&
+   git clone super5 super &&
+   (
+   cd super &&
+   git submodule update --recursive --init
+   ) &&
+   (
+   cd submodl1b1 &&
+   git rev-parse --verify HEAD >../../actuall1 &&
+   test_cmp ../../expectl1 ../../actuall1
+   ) &&
+   (
+   cd submodl2b2 &&
+   git rev-parse --verify HEAD >../../../actuall2 &&
+   test_cmp ../../../expectl2 ../../../actuall2
+   )
+'
+
 test_expect_success 'local config should override .gitmodules branch' '
(cd submodule &&
 git checkout test-branch &&
-- 
2.16.2

[RFC PATCH v4 9/9] fixup:t7406:change branches in submodules after the link is done

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f44872143..68c25ce0f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -267,17 +267,16 @@ test_expect_success 'submodule update --remote 
--recursive --init should fetch m
git checkout -b b2 &&
test_commit "l2_on_b2" &&
git rev-parse --verify HEAD >../expectl2 &&
-   git checkout master &&
cd ../submodl1b1 &&
git checkout -b b1 &&
test_commit "l1_on_b1" &&
git submodule add ../submodl2b2 submodl2b2 &&
git config -f .gitmodules submodule."submodl2b2".branch b2 &&
git add .gitmodules &&
+   git -C ../submodl2b2 checkout master &&
test_tick &&
git commit -m "add l2 (on b2) in l1 (on b1)" &&
git rev-parse --verify HEAD >../expectl1 &&
-   git checkout master &&
cd ../super5 &&
test_commit super5_commit_before_2_chained_modules_on_default_branch &&
git submodule add ../submodl1b1 submodl1b1 &&
@@ -285,6 +284,7 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
git add .gitmodules &&
test_tick &&
git commit -m "add l1 module with branch b1 in super5" &&
+   git -C ../submodl1b1 checkout master &&
git submodule init submodl1b1 &&
cd .. &&
git clone super5 super_w &&
-- 
2.16.2

[RFC PATCH v4 1/9] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

[Eddy Petrișor]

From: Eddy Petrișor 

There are projects such as llvm/clang which use several repositories, and they
might be forked for providing support for various features such as adding Redox
awareness to the toolchain. This typically means the superproject will use
another branch than master, occasionally even use an old commit from that
non-master branch.

Combined with the fact that when incorporating such a hierachy of repositories
usually the user is interested in just the exact commit specified in the
submodule info, it follows that a desireable usecase is to be also able to
provide '--depth 1' or at least have a shallow clone to avoid waiting for ages
for the clone operation to finish.

In theory, this should be straightforward since the git protocol allows
fetching an arbitary commit, but, in practice, some servers do not permit
fetch-by-sha1.

Git submodule seems to be very stubborn and cloning master, although the
wrapper script and the gitmodules-helper could work together to clone directly
the branch specified in the .gitmodules file, if specified.

Signed-off-by: Eddy Petrișor 
---
 git-submodule.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 24914963c..65e3af08b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -589,8 +589,10 @@ cmd_update()
branch=$(git submodule--helper remote-branch "$sm_path")
if test -z "$nofetch"
then
+   # non-default branch refspec
+   br_refspec=$(git submodule-helper remote-branch 
$sm_path)
# Fetch remote before determining tracking $sha1
-   fetch_in_submodule "$sm_path" $depth ||
+   fetch_in_submodule "$sm_path" $depth 
$br_refspec ||
die "$(eval_gettext "Unable to fetch in 
submodule path '\$sm_path'")"
fi
remote_name=$(sanitize_submodule_env; cd "$sm_path" && 
get_default_remote)
-- 
2.16.2

[RFC PATCH v4 3/9] fixup:t7406: use test_commit instead of echo/add/commit as suggested by Stefan Beller

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 15 +++
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7b65f1dd1..7fb370991 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -264,19 +264,13 @@ test_expect_success 'submodule update --remote 
--recursive --init should fetch m
git clone super5 submodl2b2 &&
git clone super5 submodl1b1 &&
cd submodl2b2 &&
-   echo linel2b2 > l2b2 &&
git checkout -b b2 &&
-   git add l2b2 &&
-   test_tick &&
-   git commit -m "commit on b2 branch in l2" &&
+   test_commit "l2_on_b2" &&
git rev-parse --verify HEAD >../expectl2 &&
git checkout master &&
cd ../submodl1b1 &&
git checkout -b b1 &&
-   echo linel1b1 > l1b1 &&
-   git add l1b1 &&
-   test_tick &&
-   git commit -m "commit on b1 branch in l1" &&
+   test_commit "l1_on_b1" &&
git submodule add ../submodl2b2 submodl2b2 &&
git config -f .gitmodules submodule."submodl2b2".branch b2 &&
git add .gitmodules &&
@@ -286,10 +280,7 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
git rev-parse --verify HEAD >../expectl1 &&
git checkout master &&
cd ../super5 &&
-   echo super_with_2_chained_modules > super5 &&
-   git add super5 &&
-   test_tick &&
-   git commit -m "commit on default branch in super5" &&
+   test_commit super5_with_2_chained_modules_on_default_branch &&
git submodule add ../submodl1b1 submodl1b1 &&
git config -f .gitmodules submodule."submodl1b1".branch b1 &&
git add .gitmodules &&
-- 
2.16.2

[RFC PATCH v4 7/9] fixup:t7406:supr5 commit is done before submodules chaining

[Eddy Petrișor]

From: Eddy Petrișor 

Signed-off-by: Eddy Petrișor 
---
 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 32995e272..18328be73 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -279,7 +279,7 @@ test_expect_success 'submodule update --remote --recursive 
--init should fetch m
git rev-parse --verify HEAD >../expectl1 &&
git checkout master &&
cd ../super5 &&
-   test_commit super5_with_2_chained_modules_on_default_branch &&
+   test_commit super5_commit_before_2_chained_modules_on_default_branch &&
git submodule add ../submodl1b1 submodl1b1 &&
git config -f .gitmodules submodule."submodl1b1".branch b1 &&
git add .gitmodules &&
-- 
2.16.2

Re: [PATCH v3 6/9] commit: use generation numbers for in_merge_bases()

[Jakub Narebski]

Derrick Stolee  writes:

> The containment algorithm for 'git branch --contains' is different
> from that for 'git tag --contains' in that it uses is_descendant_of()
> instead of contains_tag_algo(). The expensive portion of the branch
> algorithm is computing merge bases.
>
> When a commit-graph file exists with generation numbers computed,
> we can avoid this merge-base calculation when the target commit has
> a larger generation number than the target commits.

You have "target" twice in above paragraph; one of those should probably
be something else.

>
> Performance tests were run on a copy of the Linux repository where
> HEAD is contained in v4.13 but no earlier tag. Also, all tags were
> copied to branches and 'git branch --contains' was tested:
>
> Before: 60.0s
> After:   0.4s
> Rel %: -99.3%

Nice...

>
> Reported-by: Jeff King 
> Signed-off-by: Derrick Stolee 
> ---
>  commit.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)

...especially for so small changes.

>
> diff --git a/commit.c b/commit.c
> index a44899c733..bceb79c419 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1053,12 +1053,19 @@ int in_merge_bases_many(struct commit *commit, int 
> nr_reference, struct commit *
>  {
>   struct commit_list *bases;
>   int ret = 0, i;
> + uint32_t min_generation = GENERATION_NUMBER_INFINITY;
>  
>   if (parse_commit(commit))
>   return ret;
> - for (i = 0; i < nr_reference; i++)
> + for (i = 0; i < nr_reference; i++) {
>   if (parse_commit(reference[i]))
>   return ret;
> + if (min_generation > reference[i]->generation)
> + min_generation = reference[i]->generation;
> + }
> +
> + if (commit->generation > min_generation)
> + return 0;

Why not use "return ret;" instead of "return 0;", like the rest of the
code [cryptically] does, that is:

  + if (commit->generation > min_generation)
  + return ret;

>  
>   bases = paint_down_to_common(commit, nr_reference, reference);
>   if (commit->object.flags & PARENT2)

Otherwise, it looks good to me.

Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll

[Junio C Hamano]

Johannes Schindelin  writes:

> Unless I am misunderstanding violently what you say, that is, in which
> case I would like to ask for a clarification why this patch (which does
> not change a thing unless NO_POLL is defined!) must be rejected, and while
> at it, I would like to ask you how introducing a layer of indirection with
> a full new function that is at least moderately misleading (as it would be
> named xpoll() despite your desire that it should do things that poll()
> does *not* do) would be preferable to this here patch that changes but a
> few lines to introduce a regular heartbeat check for platforms that

Our xwrite() and other xfoo() are to "fix" undesirable aspect of the
underlying pure POSIX API to make it more suitable for our codebase.
When pure POSIX poll() that requires the implementing or emulating
platform pays attention to the children being waited on is not
appropriate for the codepath we are using (i.e. the place where the
patch is touching), it would be in line to introduce a "fixed" API
that allows us to pass that information, so that we can build on top
of that abstraction that is *not* pure POSIX abstraction, no?  After
all, you are the one who constantly whine that Git is implemented on
POSIX API and it is inconvenient for other platforms.

Re: [PATCH 2/2] daemon: graceful shutdown of client connection

[Johannes Schindelin]

Hi Kim,

On Sun, 15 Apr 2018, Kim Gybels wrote:

> On (13/04/18 15:03), Johannes Schindelin wrote:
> > I wonder whether you found a reliable way to trigger this? It would be
> > nice to have a regression test for this.
> 
> On my system, it reproduced reliably using Oleg's example [1], below is
> my bash version of it.

Okay.

> Script to generate repository with some history:
> 
>   $ cat example.sh
>   #!/bin/bash
>   
>   git init example
>   cd example
>   
>   git --help > foo.txt
>   
>   for i in $(seq 1 12); do
>   cat foo.txt foo.txt > bar.txt
>   mv bar.txt foo.txt
>   git add foo.txt
>   git commit -m v$i
>   done

Okay, so this sets up a minimal repository with a moderate size, inflating
foo.txt from the initial Git help text by factor 2**12 = 4096. The help
text is around 2kB, so we end up with an ~8MB large file in the end that
grew exponentially.

>   $ ./example.sh
>   Initialized empty Git repository in C:/git/bug/example/.git/
>   [master (root-commit) 2e44b4a] v1
>1 file changed, 84 insertions(+)
>create mode 100644 foo.txt
>   [master 9791332] v2
>1 file changed, 84 insertions(+)
>   [master 524e672] v3
>1 file changed, 168 insertions(+)
>   [master afec6ef] v4
>1 file changed, 336 insertions(+)
>   [master 1bcd9cc] v5
>1 file changed, 672 insertions(+)
>   [master 2f38a8e] v6
>1 file changed, 1344 insertions(+)
>   [master 33382fe] v7
>1 file changed, 2688 insertions(+)
>   [master 6c2cbd6] v8
>1 file changed, 5376 insertions(+)
>   [master 8d0770f] v9
>1 file changed, 10752 insertions(+)
>   [master 517d650] v10
>1 file changed, 21504 insertions(+)
>   [master 9e12406] v11
>1 file changed, 43008 insertions(+)
>   [master 4c4f600] v12
>1 file changed, 86016 insertions(+)
> 
> Server side:
> 
>   $ git daemon --verbose --reuseaddr --base-path=$(pwd) --export-all
>   [4760] Ready to rumble
>   [696] Connection from 127.0.0.1:2054
>   [696] unable to set SO_KEEPALIVE on socket: No such file or directory
>   [696] Extended attribute "host": 127.0.0.1
>   [696] Request upload-pack for '/example'

I guess apart from the generated repo size (which would make this an
expensive test in and of itself), the fact that we have to run the daemon
(and that lib-git-daemon.sh requires the PIPE prerequisite which we
disabled on Windows) makes it hard to turn this into a regular regression
test in t/.

Although we *could* of course introduce a test that does not use
lib-git-daemon.sh and is hidden behind the EXPENSIVE prerequisite or some
such.

BTW I just tested this, and it indeed fixes the problem, so I am eager to
ship it to the Git for Windows users. Let's see whether we can convince
Junio that your quite small and easy-to-review patches are not too much of
a maintenance burden, and the fact that they fix a long-standing bug
should help.

BTW I had to apply this to build with DEVELOPER=1:

diff --git a/daemon.c b/daemon.c
index 97fadd62d10..1cc901e9739 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1162,13 +1162,13 @@ static int service_loop(struct socketlist
*socklist)
signal(SIGCHLD, child_handler);
 
for (;;) {
-   int i;
+   int i, ret;
 
check_dead_children();
 #ifdef NO_POLL
poll_timeout = live_children ? 100 : -1;
 #endif
-   int ret = poll(pfd, socklist->nr, poll_timeout);
+   ret = poll(pfd, socklist->nr, poll_timeout);
if  (ret == 0) {
continue;
} else if (ret < 0) {

Would you mind squashing that in?

Thanks,
Dscho

Re: [PATCH v3 1/1] perl: fix installing modules from contrib

[Junio C Hamano]

Christian Hesse  writes:

> Commit 20d2a30f (Makefile: replace perl/Makefile.PL with simple make rules)
> removed a target that allowed Makefiles from contrib/ to get the correct
> install path. This introduces a new target for main Makefile and fixes
> installation for Mediawiki module.
>
> v2: Pass prefix as that can have influence as well, add single quotes
> for _SQ variant.
> v3: Rename target, add to .PHONY.
>
> Signed-off-by: Christian Hesse 
> ---

Thanks for rerolling.  I should have made it a bit more clear that
the say-* thing was merely a personal preference "I would be writing
it that way if I were doing it", not a "You should write it this way
when working on this project".  I think .PHONY is still a good idea
to have, even for only its documentation value (it is unlikely that
anybody would create a file "perllibdir").

Let me queue this on top of the v2 queued in 'next' as an
incremental update.

Thanks.

-- >8 --
From: Christian Hesse 
SUbject: Makefile: mark perllibdir as a .PHONY target

This target should be marked as .PHONY, just like other targets that
exist only for their side effects that do not create filesystem
entities with the same name.

Signed-off-by: Christian Hesse 
Signed-off-by: Junio C Hamano 
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 75b9ad3b48..b284eb20aa 100644
--- a/Makefile
+++ b/Makefile
@@ -1973,6 +1973,7 @@ GIT-PERL-DEFINES: FORCE
echo "$$FLAGS" >$@; \
fi
 
+.PHONY: perllibdir
 perllibdir:
@echo '$(perllibdir_SQ)'
 

[PATCH v9 2/2] builtin/config.c: support `--type=` as preferred alias for `--type`

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values can be interpreted as that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid squatting on this option too soon, so that
`git config` can support `--color` (in the traditional sense) in the
future, if that is desired.

In this patch, we support `--type=` in
addition to `--int`, `--bool`, and etc. This allows the aforementioned
upcoming patch to support querying a color value with a default via
`--type=color --default=...`, without squandering `--color`.

We retain the historic behavior of complaining when multiple,
legacy-style `--` flags are given, as well as extend this to
conflicting new-style `--type=` flags. `--int --type=int` (and its
commutative pair) does not complain, but `--bool --type=int` (and its
commutative pair) does.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 71 
 builtin/config.c | 63 +---
 t/t1300-repo-config.sh   | 58 +++--
 3 files changed, 152 insertions(+), 40 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..b759009c75 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
-'git config' [] [type] --add name value
-'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
-'git config' [] [type] [-z|--null] --get-urlmatch name URL
+'git config' [] [--type=] [--show-origin] [-z|--null] name 
[value [value_regex]]
+'git config' [] [--type=] --add name value
+'git config' [] [--type=] --replace-all name value 
[value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] --get 
name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
--get-all name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
[--name-only] --get-regexp name_regex [value_regex]
+'git config' [] [--type=] [-z|--null] --get-urlmatch name 
URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given .  If no
+`--type=` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <>.
 --list::
List all variables set in config file, along with their values.
 
---bool::
-   'git config' will ensure that the output is "true" or "false"
+--type ::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in ``'s
+  canonical form.
++
+Valid ``'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value 

[PATCH v9 0/2] builtin/config.c: support `--type=` as preferred alias for `--type`

[Taylor Blau]

Hi,

Attached is my final re-roll of the series to add `--type=` as a
supported replacement for `--`.

Since the last time, I have changed the following:

  * OPT_CALLBACK_VALUE no longer takes a function pointer, and instead
assumes option_parse_type().

  * No longer rely on 'type' as a global from within
option_parse_type(), instead pass it through opt->value. (NB: it is
never NULL, and thus always safe to *(opt->value).)

Thanks to all who reviewed this series in its many iterations. I am very
happy with how it turned out, and was fortunate to receive feedback and
eventual consensus on the interface.


Thanks,
Taylor

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: support `--type=` as preferred alias for
`--type`

 Documentation/git-config.txt |  71 +---
 builtin/config.c | 102 +--
 t/t1300-repo-config.sh   |  63 ++
 3 files changed, 177 insertions(+), 59 deletions(-)

Inter-diff (since v8):

diff --git a/builtin/config.c b/builtin/config.c
index 7184c09582..bd7a8d0ce7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,9 +61,9 @@ static int show_origin;
 #define TYPE_PATH  4
 #define TYPE_EXPIRY_DATE   5

-#define OPT_CALLBACK_VALUE(s, l, h, f, i) \
-   { OPTION_CALLBACK, (s), (l), NULL, NULL, (h), PARSE_OPT_NOARG | \
-   PARSE_OPT_NONEG, (f), (i) }
+#define OPT_CALLBACK_VALUE(s, l, v, h, i) \
+   { OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
+   PARSE_OPT_NONEG, option_parse_type, (i) }

 static struct option builtin_config_options[];

@@ -71,7 +71,7 @@ static int option_parse_type(const struct option *opt, const 
char *arg,
 int unset)
 {
if (unset) {
-   type = 0;
+   *((int *) opt->value) = 0;
return 0;
}

@@ -95,7 +95,8 @@ static int option_parse_type(const struct option *opt, const 
char *arg,
die(_("unrecognized --type argument, %s"), arg);
}

-   if (type != 0 && type != new_type) {
+   int *to_type = opt->value;
+   if (*to_type && *to_type != new_type) {
/*
 * Complain when there is a new type not equal to the old type.
 * This allows for combinations like '--int --type=int' and
@@ -107,7 +108,7 @@ static int option_parse_type(const struct option *opt, 
const char *arg,
usage_with_options(builtin_config_usage,
builtin_config_options);
}
-   type = new_type;
+   *to_type = new_type;

return 0;
 }
@@ -135,12 +136,12 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "get-color", , N_("find the color configured: slot 
[default]"), ACTION_GET_COLOR),
OPT_BIT(0, "get-colorbool", , N_("find the color setting: slot 
[stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
-   OPT_CALLBACK('t', "type", NULL, "", N_("value is given this type"), 
option_parse_type),
-   OPT_CALLBACK_VALUE(0, "bool", N_("value is \"true\" or \"false\""), 
option_parse_type, TYPE_BOOL),
-   OPT_CALLBACK_VALUE(0, "int", N_("value is decimal number"), 
option_parse_type, TYPE_INT),
-   OPT_CALLBACK_VALUE(0, "bool-or-int", N_("value is --bool or --int"), 
option_parse_type, TYPE_BOOL_OR_INT),
-   OPT_CALLBACK_VALUE(0, "path", N_("value is a path (file or directory 
name)"), option_parse_type, TYPE_PATH),
-   OPT_CALLBACK_VALUE(0, "expiry-date", N_("value is an expiry date"), 
option_parse_type, TYPE_EXPIRY_DATE),
+   OPT_CALLBACK('t', "type", , "", N_("value is given this type"), 
option_parse_type),
+   OPT_CALLBACK_VALUE(0, "bool", , N_("value is \"true\" or 
\"false\""), TYPE_BOOL),
+   OPT_CALLBACK_VALUE(0, "int", , N_("value is decimal number"), 
TYPE_INT),
+   OPT_CALLBACK_VALUE(0, "bool-or-int", , N_("value is --bool or 
--int"), TYPE_BOOL_OR_INT),
+   OPT_CALLBACK_VALUE(0, "path", , N_("value is a path (file or 
directory name)"), TYPE_PATH),
+   OPT_CALLBACK_VALUE(0, "expiry-date", , N_("value is an expiry 
date"), TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),


--
2.17.0

[PATCH v9 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau 
---
 builtin/config.c   | 49 +++---
 t/t1300-repo-config.sh | 11 ++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd628..92fb8d56b1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL  1
+#define TYPE_INT   2
+#define TYPE_BOOL_OR_INT   3
+#define TYPE_PATH  4
+#define TYPE_EXPIRY_DATE   5
 
 static struct option builtin_config_options[] = {
OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "get-color", , N_("find the color configured: slot 
[default]"), ACTION_GET_COLOR),
OPT_BIT(0, "get-colorbool", , N_("find the color setting: slot 
[stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
-   OPT_BIT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
-   OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT),
-   OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
-   OPT_BIT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
-   OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
+   OPT_SET_INT(0, "bool", , N_("value is \"true\" or \"false\""), 
TYPE_BOOL),
+   OPT_SET_INT(0, "int", , N_("value is decimal number"), TYPE_INT),
+   OPT_SET_INT(0, "bool-or-int", , N_("value is --bool or --int"), 
TYPE_BOOL_OR_INT),
+   OPT_SET_INT(0, "path", , N_("value is a path (file or directory 
name)"), TYPE_PATH),
+   OPT_SET_INT(0, "expiry-date", , N_("value is an expiry date"), 
TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (show_keys)
strbuf_addch(buf, key_delim);
 
-   if (types == TYPE_INT)
+   if (type == TYPE_INT)
strbuf_addf(buf, "%"PRId64,
git_config_int64(key_, value_ ? value_ : 
""));
-   else if (types == TYPE_BOOL)
+   else if (type == TYPE_BOOL)
strbuf_addstr(buf, git_config_bool(key_, value_) ?
  "true" : "false");
-   else if (types == TYPE_BOOL_OR_INT) {
+   else if (type == TYPE_BOOL_OR_INT) {
int is_bool, v;
v = git_config_bool_or_int(key_, value_, _bool);
if (is_bool)
strbuf_addstr(buf, v ? "true" : "false");
else
strbuf_addf(buf, "%d", v);
-   } else if (types == TYPE_PATH) {
+   } else if (type == TYPE_PATH) {
const char *v;
if (git_config_pathname(, key_, value_) < 0)
return -1;
strbuf_addstr(buf, v);
free((char 

Re: [PATCH v3 0/2] fsexcludes: Add programmatic way to exclude files

[Junio C Hamano]

Ben Peart  writes:

> I found a bug with how this patch series deals with untracked
> files. I'm going to retract this patch until I have time to create a
> new test case to demonstrate the bug and come up with a good fix.
>
> Ben

Thanks.

Re: [PATCH] submodule--helper: don't print null in 'submodule status'

[Junio C Hamano]

Stefan Beller  writes:

> Hi Nguyễn,
>
> On Wed, Apr 18, 2018 at 7:53 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> The function compute_rev_name() can return NULL sometimes (e.g. right
>> after 'submodule init'). The current code makes 'submodule status'
>> print this:
>>
>>  19d97bf5af05312267c2e874ee6bcf584d9e9681 sha1collisiondetection ((null))
>>
>> This ugly 'null' adds no value to the user using this command. More
>> importantly printf() on some platform can't handle NULL as a string
>> and will crash instead of printing '(null)'.
>>
>> Check for this and skip printing this part (the alternative is
>> printing '(n/a)' or something but I think that is just noise).
>
> This patch restores the behavior from before a9f8a37584 (submodule:
> port submodule subcommand 'status' from shell to C, 2017-10-06),
> so this is the right way to go instead of the alternatives you considered.
>
> Thanks!
>
> Reviewed-by: Stefan Beller 

Excellent.  Thanks, both.

Will queue.

>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  builtin/submodule--helper.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index a404df3ea4..4dc7d7d29f 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -596,8 +596,12 @@ static void print_status(unsigned int flags, char 
>> state, const char *path,
>>
>> printf("%c%s %s", state, oid_to_hex(oid), displaypath);
>>
>> -   if (state == ' ' || state == '+')
>> -   printf(" (%s)", compute_rev_name(path, oid_to_hex(oid)));
>> +   if (state == ' ' || state == '+') {
>> +   const char *name = compute_rev_name(path, oid_to_hex(oid));
>> +
>> +   if (name)
>> +   printf(" (%s)", name);
>> +   }
>>
>> printf("\n");
>>  }
>> --
>> 2.17.0.367.g5dd2e386c3
>>

Re: [PATCH] git-send-email: Cc more people

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> But IMO this patch is really lacking a few things before being ready:
>
> 1. You have no tests for this. See t/t9001-send-email.sh for examples,
> ...
> 2. Just a few lines down from your quoted hunk we have this:
> ... code about $supress_cc{} ...
>Your change should at least describe why those aren't being updated,
>but probably we should add some other command-line option for
>ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
>--[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
>a historical alias for --[no-]wildcard-by-cc=signed-off.
> 3. Ditto all the documentation in "man git-send-email" about
> ...

Thanks, I agree that 2. (the lack of suppression) is a showstopper.
I'd further say that these new CC-sources should be disabled by
default and made opt-in to avoid surprising existing users.

One thing we also need to be very careful about is that some of the
fields may not even have an e-mail address.  We can expect that
S-o-b and Cc would be of form "human readable name "
by their nature, but it is perfectly fine to write only human
readable name without address on random lines like "suggeted-by" and
"helped-by".  There needs a way for the end-user to avoid using data
found on such lines as if they are valid e-mail addresses.

Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll

[Johannes Schindelin]

Hi Kim,

On Sun, 15 Apr 2018, Kim Gybels wrote:

> On (13/04/18 14:36), Johannes Schindelin wrote:
> > > The poll provided in compat/poll.c is not interrupted by receiving
> > > SIGCHLD. Use a timeout for cleaning up dead children in a timely
> > > manner.
> > 
> > Maybe say "When using this poll emulation, use a timeout ..."?
> 
> I will rewrite the commit message when I reroll the patch. Calling the
> poll "uninterruptible" might be wrong as well, although the poll
> doesn't return with EINTR when a child process terminates, it might
> still be interruptible in other ways. On a related note, the handler
> for SIGCHLD is simply not called in Git-for-Windows' daemon.

Right. There is no signal infrastructure on Windows that is an exact
equivalent of what Junio desires.

> > > @@ -1161,8 +1162,13 @@ static int service_loop(struct socketlist 
> > > *socklist)
> > >   int i;
> > >  
> > >   check_dead_children();
> > > -
> > > - if (poll(pfd, socklist->nr, -1) < 0) {
> > > +#ifdef NO_POLL
> > > + poll_timeout = live_children ? 100 : -1;
> > > +#endif
> > > + int ret = poll(pfd, socklist->nr, poll_timeout);
> > > + if  (ret == 0) {
> > > + continue;
> > > + } else if (ret < 0) {
> > 
> > I would find it a bit easier on the eyes if this did not use curlies, and
> > dropped the unnecessary `else` (`continue` will take care of that):
> > 
> > if (!ret)
> > continue;
> > if (ret < 0)
> > [...]
> 
> Funny, that's how I would normally write it, if I wasn't so focused on
> trying to follow the coding quidelines. While I'm at it, I will also
> fix that sneaky double space after the if.

:-)

> Is it ok to add the timeout for all platforms using the poll
> emulation, since I only tested for Windows?

>From my reading of the patch, it changes only one thing, and only in the
case that the developer asked to build with NO_POLL (which means that the
platform does not have a native poll()): instead of waiting indefinitely,
the poll() call is interrupted in regular intervals to give
reap_dead_children() a chance to clean up.

And that's all it does.

So it is a simply heartbeat for platforms that require it, and that
heartbeat would not even hurt any platform that would *not* require it.

In short: from my point of view, it is fine to add the timeout for all
NO_POLL platforms, even if it was only tested on Windows.

Of course, we *do* know that there is one other user of NO_POLL: the
NonStop platform.

Randall, would you mind testing these two patches on NonStop?

Thanks,
Johannes

Re: [PATCH 0/3] completion: improvements for git stash

[Thomas Gummerer]

On 04/18, Junio C Hamano wrote:
> Thomas Gummerer  writes:
> 
> > In this series we stop completing the 'git stash save' subcommand in
> > git-completion.bash.  The command has been deprecated for a while,...
> 
> Anything deprecated in Oct 2017 is still too young in Git's
> timescale, but this is the right thing to do in the longer term.

Fair enough.  I vaguely remembered this thread [1], that tried to add
'rm' back to the autocompletion after it was removed originally to try
and start the deprecation process.  Reading the thread again now
though it seems what you outline below to just not show "save" when
"git stash " or "git stash s" is typed makes more sense as a
step now.

I also notice that we never seemed to have taken any of the
suggestions made there (either adding 'rm' back to the completion
options, or going further in the deprecation process), though that's a
different topic :)

> Regarding [2/3], I think 
> 
>  - It is perfectly fine to stop offering "save" among the choices
>when "git stash " is given, so that we AVOID actively
>suggesting "save" to those who do not know (or do not want to
>learn) about it.  Instead we would knudge them towards "push".
>After all, that is what "deprecation" is all about.
> 
>  - It also is OK to limit the offering to "show" against "git stash
>s", even though the user _could_ have meant "save" than the
>above case with totally empty subcommand name.
> 
>  - It is questionable to stop offering "save" to "git stash
>sav" it is clear that the user wants to say "save" in this
>case, as there is no other subcommand the user could have meant.
> 
>  - It is outright hostile to the end user if we stop completing "git
>stash save --q" with "--quiet".  At that point, we _know_
>that the user wants "save", and getting in the way of those who
>know what they are using does not feel quite right.
> 
> Of course, being more accomodating to existing users by taking the
> last two points above seriously would raise a separate issue of when
> we stop doing so, and if we should start doing something else.  If
> there is a way to implement a "reluctant completion" that gives
> "save" as a completion choice while giving a deprecation warning to
> let the user know of the plan for removal of "save", that would be
> good, for example.

Thanks for the suggestions, I'll take a closer look at what could be
done, and will send a re-roll.

> Thanks.

[1]: 
01020160a0004473-277c3d7c-4e3b-4c50-9d44-4a106f37f1d9-000...@eu-west-1.amazonses.com

Re: [RFC PATCH] ident: don't cache default date

[Junio C Hamano]

Johannes Sixt  writes:

> While I like the basic theme of your patch, I think we should fix this
> case in a much simpler way, namely, use the infrastructure that was
> introduced for git-am.

Yup.  reset_ident_date() was introduced by 4d9c7e6f ("am: reset
cached ident date for each patch", 2016-08-01) and the commit
explains very well why it is a good idea to have both the caching
and also the strategic resetting it introduces.

Thanks, all.

> I've shamelessly lifted the commit message from your patch.
>
>  8< 
> Subject: [PATCH] sequencer: reset the committer date before commits
>
> Now that the sequencer commits without forking when the commit message
> isn't edited all the commits that are picked have the same committer
> date. If a commit is reworded it's committer date will be a later time
> as it is created by running an separate instance of 'git commit'.  If
> the reworded commit is follow by further picks, those later commits
> will have an earlier committer date than the reworded one. This is
> caused by git caching the default date used when GIT_COMMITTER_DATE is
> not set. Reset the cached date before a commit is generated
> in-process.
>
> Signed-off-by: Johannes Sixt 
> ---
>  sequencer.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index f9d1001dee..f0bac903a0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char 
> *author,
>   goto out;
>   }
>  
> + reset_ident_date();
> +
>   if (commit_tree_extended(msg->buf, msg->len, , parents,
>oid, author, opts->gpg_sign, extra)) {
>   res = error(_("failed to write commit object"));

Re: [PATCH 1/2] daemon: use timeout for uninterruptible poll

[Johannes Schindelin]

Hi Junio,

On Mon, 16 Apr 2018, Junio C Hamano wrote:

> Kim Gybels  writes:
> 
> > The poll provided in compat/poll.c is not interrupted by receiving
> > SIGCHLD. Use a timeout for cleaning up dead children in a timely manner.
> 
> I think you identified the problem and diagnosed it correctly, but I
> find that the change proposed here introduces a severe layering
> violation.  The code is still calling what is called poll(), which
> should not have such a broken semantics.

While I have sympathy for your desire to apply pure POSIX functionality,
the reality is that we do not have this luxury. Not if we want to support
Git on the still most prevalent development platform: Windows. On Windows,
you simply do not have that poll() that you are looking for.

In particular, there is no signal handling of the type you seem to want to
require.

As to the layering violation you mention, first a HN quote, just to loosen
the mood, and to at least partially ease the blow delivered by your mail:

There is no such thing as a layering violation. You should be
immediately suspicious of anyone who claims that there are such
things.

;-)

Seriously again. If you care to have a look at the patch, you will see
that the loop (which will now benefit from Kim's timeout on platforms
without POSIX signal handling) *already* contains that call to
reap_dead_children().

In other words, you scolded Kim for something that this patch did not
introduce, but which was already there.

Unless I am misunderstanding violently what you say, that is, in which
case I would like to ask for a clarification why this patch (which does
not change a thing unless NO_POLL is defined!) must be rejected, and while
at it, I would like to ask you how introducing a layer of indirection with
a full new function that is at least moderately misleading (as it would be
named xpoll() despite your desire that it should do things that poll()
does *not* do) would be preferable to this here patch that changes but a
few lines to introduce a regular heartbeat check for platforms that
require it?

Thank you,
Dscho

[PATCH v4] bisect: create 'bisect_flags' parameter in find_bisection()

[Harald Nordgren]

Make it possible to implement bisecting only on first parents or on
merge commits by passing flags to find_bisection(), instead of just
a 'find_all' boolean.

Signed-off-by: Harald Nordgren 
---

Notes:
Use unsigned type and cache flag value

 bisect.c   | 15 ++-
 bisect.h   |  6 --
 builtin/rev-list.c |  6 +++---
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/bisect.c b/bisect.c
index a579b50884..4eafc8262b 100644
--- a/bisect.c
+++ b/bisect.c
@@ -254,9 +254,10 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
  */
 static struct commit_list *do_find_bisection(struct commit_list *list,
 int nr, int *weights,
-int find_all)
+unsigned bisect_flags)
 {
int n, counted;
+   unsigned find_all = bisect_flags & BISECT_FIND_ALL;
struct commit_list *p;
 
counted = 0;
@@ -365,7 +366,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-   int *all, int find_all)
+   int *all, unsigned bisect_flags)
 {
int nr, on_list;
struct commit_list *list, *p, *best, *next, *last;
@@ -400,9 +401,9 @@ void find_bisection(struct commit_list **commit_list, int 
*reaches,
weights = xcalloc(on_list, sizeof(*weights));
 
/* Do the real work of finding bisection commit. */
-   best = do_find_bisection(list, nr, weights, find_all);
+   best = do_find_bisection(list, nr, weights, bisect_flags);
if (best) {
-   if (!find_all) {
+   if (!(bisect_flags & BISECT_FIND_ALL)) {
list->item = best->item;
free_commit_list(list->next);
best = list;
@@ -943,6 +944,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct rev_info revs;
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
+   unsigned bisect_flags = 0;
struct object_id *bisect_rev;
char *steps_msg;
 
@@ -957,7 +959,10 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
bisect_common();
 
-   find_bisection(, , , !!skipped_revs.nr);
+   if (skipped_revs.nr)
+   bisect_flags |= BISECT_FIND_ALL;
+
+   find_bisection(, , , bisect_flags);
revs.commits = managed_skipped(revs.commits, );
 
if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index a5d9248a47..1d40a33ad2 100644
--- a/bisect.h
+++ b/bisect.h
@@ -1,15 +1,17 @@
 #ifndef BISECT_H
 #define BISECT_H
 
+#define BISECT_FIND_ALL(1u<<0)
+
 /*
  * Find bisection. If something is found, `reaches` will be the number of
  * commits that the best commit reaches. `all` will be the count of
  * non-SAMETREE commits. If nothing is found, `list` will be NULL.
  * Otherwise, it will be either all non-SAMETREE commits or the single
- * best commit, as chosen by `find_all`.
+ * best commit, as chosen by flag `BISECT_FIND_ALL`.
  */
 extern void find_bisection(struct commit_list **list, int *reaches, int *all,
-  int find_all);
+  unsigned bisect_flags);
 
 extern struct commit_list *filter_skipped(struct commit_list *list,
  struct commit_list **tried,
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index fadd3ec14c..8752f5bbed 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -360,8 +360,8 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
int i;
int bisect_list = 0;
int bisect_show_vars = 0;
-   int bisect_find_all = 0;
int use_bitmap_index = 0;
+   unsigned bisect_flags = 0;
const char *show_progress = NULL;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -426,7 +426,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
}
if (!strcmp(arg, "--bisect-all")) {
bisect_list = 1;
-   bisect_find_all = 1;
+   bisect_flags |= BISECT_FIND_ALL;
info.flags |= BISECT_SHOW_ALL;
revs.show_decorations = 1;
continue;
@@ -538,7 +538,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
if (bisect_list) {
int reaches, all;
 
-   find_bisection(, , , bisect_find_all);
+   find_bisection(, , , bisect_flags);
 
if (bisect_show_vars)
return show_bisect_vars(, reaches, all);
-- 
2.14.3 (Apple Git-98)

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Junio C Hamano]

Elijah Newren  writes:

> On Mon, Apr 16, 2018 at 11:07 PM, Junio C Hamano  wrote:
>> * en/rename-directory-detection-reboot (2018-04-16) 32 commits
>>  ...
>>  - directory rename detection: testcases to avoid taking detection too far
>>  - directory rename detection: directory splitting testcases
>>  - directory rename detection: basic testcases
>>
>>  Reboot of an attempt to detect wholesale directory renames and use
>>  it while merging.
>
>
> Thanks for rebasing/cherry-picking the series and applying my newest
> changes.  It looks like a couple of your squashed fixes in the rebase
> of the old commits ...

Thanks for rebooting the effort.  The above wasn't a serious attempt
to re-queue the series to be polished for 'next'---to me, this
branch was primarily to keep track of your interim 29+fractional
patches until "the real thing", possibly rebased to newer codebase,
gets submitted.

> Also, the newest patches can still be fooled by a specially crafted
> rename/add conflict, but I believe with a small restructure I can both
> simplify the code and cover all the cases.

That's nice.  Very much looking forward to the updates.

Thanks.

Re: [PATCH v3 5/9] ref-filter: use generation number for --contains

[Jakub Narebski]

Here I can offer only the cursory examination, as I don't know this area
of code in question.

Derrick Stolee  writes:

> A commit A can reach a commit B only if the generation number of A
> is larger than the generation number of B. This condition allows
> significantly short-circuiting commit-graph walks.
>
> Use generation number for 'git tag --contains' queries.
>
> On a copy of the Linux repository where HEAD is containd in v4.13
> but no earlier tag, the command 'git tag --contains HEAD' had the
> following peformance improvement:
>
> Before: 0.81s
> After:  0.04s
> Rel %:  -95%

A question: what is the performance after if the "commit-graph" feature
is disabled, or there is no commit-graph file?  Is there performance
regression in this case, or is the difference negligible?

>
> Helped-by: Jeff King 
> Signed-off-by: Derrick Stolee 
> ---
>  ref-filter.c | 23 +++
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index cffd8bf3ce..e2fea6d635 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1587,7 +1587,8 @@ static int in_commit_list(const struct commit_list 
> *want, struct commit *c)
>  /*
>   * Test whether the candidate or one of its parents is contained in the list.
 ^

Sidenote: when examining the code after the change, I have noticed that
the above part of commit header for the comtains_test() function is no
longer entirely correct, as the function only checks the candidate
commit, and in no place it access its parents.

But that is not your problem.

>   * Do not recurse to find out, though, but return -1 if inconclusive.
>   */
>  static enum contains_result contains_test(struct commit *candidate,
> const struct commit_list *want,
> -   struct contains_cache *cache)
> +   struct contains_cache *cache,
> +   uint32_t cutoff)
>  {
>   enum contains_result *cached = contains_cache_at(cache, candidate);
>  
> @@ -1603,6 +1604,10 @@ static enum contains_result contains_test(struct 
> commit *candidate,
>  
>   /* Otherwise, we don't know; prepare to recurse */
>   parse_commit_or_die(candidate);
> +
> + if (candidate->generation < cutoff)
> + return CONTAINS_NO;
> +

Looks good to me.

The only [minor] question may be whether to define separate type for
generation numbers, and whether to future proof the tests - though the
latter would be almost certainly overengineering, and the former
probablt too.

>   return CONTAINS_UNKNOWN;
>  }
>  
> @@ -1618,8 +1623,18 @@ static enum contains_result contains_tag_algo(struct 
> commit *candidate,
> struct contains_cache *cache)
>  {
>   struct contains_stack contains_stack = { 0, 0, NULL };
> - enum contains_result result = contains_test(candidate, want, cache);
> + enum contains_result result;
> + uint32_t cutoff = GENERATION_NUMBER_INFINITY;
> + const struct commit_list *p;
> +
> + for (p = want; p; p = p->next) {
> + struct commit *c = p->item;
> + parse_commit_or_die(c);
> + if (c->generation < cutoff)
> + cutoff = c->generation;
> + }

Sholdn't the above be made conditional on the ability to get generation
numbers from the commit-graph file (feature is turned on and file
exists)?  Otherwise here after the change contains_tag_algo() now parses
each commit in 'want', which I think was not done previously.

With commit-graph file parsing is [probably] cheap.  Without it, not
necessary.

But I might be worrying about nothing.

>  
> + result = contains_test(candidate, want, cache, cutoff);

Other than the question about possible performace regression if
commit-graph data is not available, it looks good to me.

>   if (result != CONTAINS_UNKNOWN)
>   return result;
>  
> @@ -1637,7 +1652,7 @@ static enum contains_result contains_tag_algo(struct 
> commit *candidate,
>* If we just popped the stack, parents->item has been marked,
>* therefore contains_test will return a meaningful yes/no.
>*/
> - else switch (contains_test(parents->item, want, cache)) {
> + else switch (contains_test(parents->item, want, cache, cutoff)) 
> {
>   case CONTAINS_YES:
>   *contains_cache_at(cache, commit) = CONTAINS_YES;
>   contains_stack.nr--;
> @@ -1651,7 +1666,7 @@ static enum contains_result contains_tag_algo(struct 
> commit *candidate,
>   }
>   }
>   free(contains_stack.contains_stack);
> - return contains_test(candidate, want, cache);
> + return contains_test(candidate, want, cache, cutoff);

Simple change. It 

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Junio C Hamano]

Christian Hesse  writes:

> Junio C Hamano  on Tue, 2018/04/17 15:07:
>> * ab/simplify-perl-makefile (2018-04-11) 1 commit
>>   (merged to 'next' on 2018-04-17 at 4448756934)
>>  + perl: fix installing modules from contrib
>> 
>>  Recent simplification of build procedure forgot a bit of tweak to
>>  the build procedure of contrib/mw-to-git/
>> 
>>  Will merge to 'master'.
>
> Looks like cooking is v2 of the patch, while we were at v3, no?

If that is the case, unless v3 was obviously crappy and was
discarded, which sounds unlikely, it is more plausible that
I must have missed v3 altogether, isn't it?

I'll stop merging this down while I hunt for that v3 version.  As
the above is already in 'next', we may have to ask the v3 patch to
be rewritten to be an incremental update to it, though.

Thanks for reminding me.  Very much appreciated.

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Junio C Hamano]

Jonathan Tan  writes:

>> I considered this done a long time ago,
>>
>> "All 6 patches look good to me, thanks.
>>  Reviewed-by: Jonathan Tan "
>>
>> https://public-inbox.org/git/20180328161727.af10f596dffc8e01205c4...@google.com/
>
> To add to this, I sent this in reply to version 3 of this patch set,
> after Stefan addressed my comments. Most of my in-depth comments were
> in reply to version 1 of this patch, which are the grandchild replies
> to [1].
>
> [1] https://public-inbox.org/git/20180327213918.77851-1-sbel...@google.com/

Thanks for helping out.  Very much appreciated.

Re: man page for "git remote set-url" seems confusing/contradictory

[Todd Zullinger]

Junio C Hamano wrote:
> Jacob Keller  writes:
> 
>> Maybe something like:
>>
>> "Note that the push URL and the fetch URL, even though they can be set
>> differently, are expected to refer to the same repository. For
>> example, the fetch URL might use an unauthenticated transport, while
>> the push URL generally requires authentication" Then follow this bit
>> with the mention of multiple remotes.
>>
>> (I think "repository" conveys the meaning better than "place".
>> Technically, the URLs can be completely different as long as they end
>> up contacting the same real server repository.)
> 
> Sounds sensible.  Let's see if there is any further input and then
> somebody pleaes wrap this up in a final patch ;-)

A pointer to the "GIT URLS" section in git-fetch(1) might
also be useful?  That section is provided via urls.txt and
urls-remotes.txt and is included the git-clone, git-fetch,
git-pull, and git-push man pages.

We could also include urls-remotes.txt in git-remote, though
that seems like a lot of text to add to yet another man
page.  Maybe a giturls.txt could be created and referenced
(as a further enhancement if someone is inclined).

Tangentially (and I don't know if it's worth fixing), while
poking around the documentation which includes urls.txt I
noticed that git-clone.txt refers readers to the "URLS
section below" when the name of the section is "GIT URLS".

I doubt any readers would be confused, but it would be
consistent with the other files which include urls.txt to
use "GIT URLS" as the referenced section name.

-- >& --
Subject: [PATCH] doc/clone: update caption for GIT URLS cross-reference

The description of the  argument directs readers to "See the
URLS section below".  When generating HTML this becomes a link to the
"GIT URLS" section.  When reading the man page in a terminal, the
caption is slightly misleading.  Use "GIT URLS" as the caption to avoid
an confusion.

The man page produced by asciidoc doesn't include hyperlinks.  The
description of the  argument simply

Signed-off-by: Todd Zullinger 
---
 Documentation/git-clone.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 42ca7b5095..b844b9957c 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -260,7 +260,7 @@ or `--mirror` is given)
 
 ::
The (possibly remote) repository to clone from.  See the
-   <> section below for more information on specifying
+   <> section below for more information on specifying
repositories.
 
 ::
-- 
2.17.0

-- 
Todd
~~
The ultimate result of shielding men from the effects of folly is to
fill the world with fools.
-- Herbert Spencer

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Junio C Hamano]

Derrick Stolee  writes:

> I'm sorry that my second message was terse. My response to v1 [1] was
>
>> I looked through these patches and only found one set of whitespace
>> > 
> errors. Compiles and tests fine on my machine. > > Reviewed-by:
> Derrick Stolee  So, I pulled the code, went
> through it patch-by-patch, and saw that the transformations were made
> using the established pattern. The second review was to chime in that
> my v1 comments had been addressed. Thanks, -Stolee
> [1]
> https://public-inbox.org/git/6c319100-df47-3b8d-8661-24e4643ad...@gmail.com/

Thanks.

Re: [PATCH v3] bisect: create 'bisect_flags' parameter in find_bisection()

[Johannes Schindelin]

Hi Harald,

On Sun, 15 Apr 2018, Harald Nordgren wrote:

> Make it possible to implement bisecting only on first parents or on
> merge commits by passing flags to find_bisection(), instead of just
> a 'find_all' boolean.
> 
> Signed-off-by: Harald Nordgren 

Very nice. Just two little suggestions:

> diff --git a/bisect.c b/bisect.c
> index a579b50884..19dac7491d 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -254,7 +254,7 @@ static struct commit_list *best_bisection_sorted(struct 
> commit_list *list, int n
>   */
>  static struct commit_list *do_find_bisection(struct commit_list *list,
>int nr, int *weights,
> -  int find_all)
> +  unsigned int bisect_flags)

For flags, we frequently seem to use the type `unsigned` (which is the
same as `unsigned int`, just shorter).

>  {
>   int n, counted;
>   struct commit_list *p;
> @@ -313,7 +313,7 @@ static struct commit_list *do_find_bisection(struct 
> commit_list *list,
>   clear_distance(list);
>  
>   /* Does it happen to be at exactly half-way? */
> - if (!find_all && halfway(p, nr))
> + if (!(bisect_flags & BISECT_FIND_ALL) && halfway(p, nr))

Rather than expanding the short & sweet `find_all` to `(bisect_flags &
BISECT_FIND_ALL)` in three places in this function, I would rather just
define this local variable in the beginning:

unsigned int find_all = bisect_flags & BISECT_FIND_ALL;

This would also reduce the size of the diff.

All in all, very nice!

Ciao,
Johannes

P.S.: I fully agree with Junio and eagerly await what comes next ;-)

Re: [PATCH v4] git{,-blame}.el: remove old bitrotting Emacs code

[Todd Zullinger]

Thanks for working on this cruft cleanup Ævar and to
Jonathan & Junio for asking questions about how to improve
this transition for packagers & users.

Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
>>> On the other hand, the 6-lines of e-lisp you wrote for git.el
>>> replacement is something the packagers could have written for their
>>> users, so (1) if we really want to go extra mile without trusting
>>> that distro packagers are less competent than us in helping their
>>> users, we'd be better off to leave Makefile in, or (2) if we trust
>>> packagers and leave possible end-user confusion as their problem
>>> (not ours), then we can just remove as your previous round did.
>>>
>>> And from that point of view, I find this round slightly odd.
>>
>> I think the way it is makes sense. In Debian debian/git-el.install just
>> does:
>> ...
>> RedHat does use contrib/emacs/Makefile:
>> ...
>> But they can either just do their own byte compilation as they surely do
>> for other elisp packages,...
>
> In short, Debian happens to be OK, but RedHat folks need to do work
> and cannot use what we ship out of the box, *IF* they care about end
> user experience.

I don't think it's a big deal for the Fedora/Red Hat
packages to adjust and manually install the stub git-el.

Anyone doing automated rebuilds from the current Fedora
git.spec will notice the make failure and can then check the
relese notes to find out about the change, I imagine.

> That was exactly why I felt it was "odd" (iow, "uneven").  We bother
> to give a stub git.el; we do not bother to make sure it would keep
> being installed if the packagers do not bother to update their
> procedure.

I wonder if leaving the Makefile in place would prevent
packages from even noticing the change?  It might still be a
good plan to help ease the transition.  I don't know if
that's as important for something in contrib/ or not.

> If we do not change anything other than making *.el into stubs, then
> it is a lot more likely that end user experience on *any* distro
> that have been shipping contrib/emacs/ stuff will by default
> (i.e. if the packagers do not do anything to adjust) be what we
> design here---upon loading they'd see (error) triggering that nudge
> them towards modern and maintained alternatives.  If we do more than
> that, e.g. remove Makefile, then some distros need to adjust, or
> their build would be broken.
> 
> I suspect that the set of people Cc'ed on the thread are a lot more
> familiar than I am with how distro packagers prefer us to deliber,
> so I'll stop speculating at this point.

I should note that I'm not an emacs user, so I likely lack a
good understanding of how people use the current
git{,-blame}.el files.  I could simply be doing it wrong in
the steps I took to test this.

With the fedora packaging, we've shipped a git-init.el that
adds autoload entries for git-status and git-blame-mode
(since adding the emacs files in 2007).  That allows users
to make use of those features without adding anything to
their local emacs configuration.

If I open emacs with a current fedora packaging, I can issue
M-x git-status or M-x git-blame-mode.  After applying this
patch and removing the git-init.el, that no longer works
but rather than the detailed warning message, I just get a
transient "no matches" in the emacs status line.

However, if I add "(require 'git)" to ~/.emacs, then I get
the "error: git.el no longer ships with git" message in the
warnings buffer.

Does this mean that only users who have manually loaded
git.el will see the warning?  If so, is there a preferred
method to have the warning appear when calling the commands
previously provided, to give a better user experience?

-- 
Todd
~~
Faith, n. Belief without evidence in what is told by one who speaks
without knowledge, of things without parallel.
-- Ambrose Bierce, "The Devil's Dictionary"

Re: [PATCH] git-send-email: Cc more people

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 18 2018, Matthew Wilcox wrote:

> From: Matthew Wilcox 
>
> Several of my colleagues (and myself) have expressed surprise and
> annoyance that git-send-email doesn't automatically pick up people who
> are listed in patches as Reported-by: or Reviewed-by: or ... many other
> tags that would seem (to us) to indicate that person might be interested.
> This patch to git-send-email tries to pick up all Foo-by: tags.
>
> Signed-off-by: Matthew Wilcox 
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2fa7818ca..926815329 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>   # Now parse the message body
>   while(<$fh>) {
>   $message .=  $_;
> - if (/^(Signed-off-by|Cc): (.*)/i) {
> + if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>   chomp;
>   my ($what, $c) = ($1, $2);
>   # strip garbage for the address we'll use:

I like this direction, I've actually been meaning to take this further
and try to parse out SHA1s in the commit message, look those up, and add
their authors to CC one of these days.

But IMO this patch is really lacking a few things before being ready:

1. You have no tests for this. See t/t9001-send-email.sh for examples,
   i.e. stuff like

   (body) Adding cc: C O Mitter  from line
   'Signed-off-by: C O Mitter '

   Should have corresponding tests for "Reviewed-by" "Seen-by"
   etc. These are easy to add, just edit the raw messages and test that
   for the output about adding CCs.

2. Just a few lines down from your quoted hunk we have this:

# strip garbage for the address we'll use:
$c = strip_garbage_one_address($c);
# sanitize a bit more to decide whether to suppress the address:
my $sc = sanitize_address($c);
if ($sc eq $sender) {
next if ($suppress_cc{'self'});
} else {
next if $suppress_cc{'sob'} and $what =~ /Signed-off-by/i;
next if $suppress_cc{'bodycc'} and $what =~ /Cc/i;
}
push @cc, $c;
printf(__("(body) Adding cc: %s from line '%s'\n"),
$c, $_) unless $quiet;

   So before we just supported Signed-off-by as a special case, but now
   your patch adds WHAT-EVER-by without updating the the corresponding
   --[no-]signed-off-by-cc command-line options.

   Your change should at least describe why those aren't being updated,
   but probably we should add some other command-line option for
   ignoring these wildcards, e.g. --[no-]wildcard-by-cc=reviewed
   --[no-]wildcard-by-cc=seen etc, and we can make --[no-]signed-off-by
   a historical alias for --[no-]wildcard-by-cc=signed-off.

3. Ditto all the documentation in "man git-send-email" about
   "signed-off-by", "sob" etc, and the "signedoffbycc" variable
   documented both there and in "man git-config".

Style comment: First time I've seen someone write a charclass as
[A-Z-a-z] and mean it, usually it's usually it's [A-Za-z-] to clarify
that the "-" isn't a range. Makes sense (to me) to have ranges first &
stray chars last.

Re: [PATCH v2 1/1] completion: load completion file for external subcommand

[SZEDER Gábor]

On Tue, Apr 10, 2018 at 10:28 PM, Florian Gamböck  wrote:
> Adding external subcommands to Git is as easy as to put an executable
> file git-foo into PATH. Packaging such subcommands for a Linux
> distribution can be achieved by unpacking the executable into /usr/bin
> of the user's system. Adding system-wide completion scripts for new
> subcommands, however, can be a bit tricky.
>
> Since bash-completion started to use dynamical loading of completion
> scripts since v1.90 (preview of v2.0),

I believe the main bash-completion repository can be found at:

  https://github.com/scop/bash-completion.git

This repository still contains the branch 'dynamic-loading'; for the
record it points to 3b029892f6f9db3b7210a7f66d636be3e5ec5fa2.

Two commits on that branch are worth mentioning:

  20c05b43 (Load completions in separate files dynamically, get rid of
have()., 2011-10-12)
  5baebf81 (Add _xfunc for loading and calling functions on demand,
use it in apt-get, cvsps, rsync, and sshfs., 2011-10-13)

> it is no longer sufficient to
> drop a completion script of a subcommand into the standard completions
> path, /usr/share/bash-completion/completions, since this script will not
> be loaded if called as a git subcommand.
>
> For example, look at https://bugs.gentoo.org/544722. To give a short
> summary: The popular git-flow subcommand provides a completion script,
> which gets installed as /usr/share/bash-completion/completions/git-flow.
>
> If you now type into a Bash shell:
>
> git flow 
>
> You will not get any completions, because bash-completion only loads
> completions for git and git has no idea that git-flow is defined in
> another file. You have to load this script manually or trigger the
> dynamic loader with:
>
> git-flow  # Please notice the dash instead of whitespace
>
> This will not complete anything either, because it only defines a Bash
> function, without generating completions. But now the correct completion
> script has been loaded and the first command can use the completions.
>
> So, the goal is now to teach the git completion script to consider the
> possibility of external completion scripts for subcommands, but of
> course without breaking current workflows.
>
> I think the easiest method is to use a function that is defined by
> bash-completion v2.0+, namely __load_completion.

This is wrong, __load_completion() was introduced in cad3abfc
(__load_completion: New function, use in _completion_loader and
_xfunc, 2015-07-15), and the first release tag containg it is '2.2'
from 2016-03-03.

The release tags '1.90' and '2.0' are from 2011-11-03 and 2012-06-17,
respectively.  This leaves a couple of years long hole where
completions were already loaded dynamically but there was no
__load_completion() function.

Would it be possible to use _xfunc() instead to plug that hole?  It
seems the be tricky, because that function not only sources but also
_calls_ the completion function.

> It will take care of
> loading the correct script if present. Afterwards, the git completion
> script behaves as usual.
>
> This way we can leverage bash-completion's dynamic loading for git
> subcommands and make it easier for developers to distribute custom
> completion scripts.
>
> Signed-off-by: Florian Gamböck 
> ---
>  contrib/completion/git-completion.bash | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index b09c8a236..09a820990 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3096,12 +3096,22 @@ __git_main ()
> fi
>
> local completion_func="_git_${command//-/_}"
> +   if ! declare -f $completion_func >/dev/null 2>/dev/null &&
> +   declare -f __load_completion >/dev/null 2>/dev/null
> +   then
> +   __load_completion "git-$command"
> +   fi
> declare -f $completion_func >/dev/null 2>/dev/null && 
> $completion_func && return
>
> local expansion=$(__git_aliased_command "$command")
> if [ -n "$expansion" ]; then
> words[1]=$expansion
> completion_func="_git_${expansion//-/_}"
> +   if ! declare -f $completion_func >/dev/null 2>/dev/null &&
> +   declare -f __load_completion >/dev/null 2>/dev/null
> +   then
> +   __load_completion "git-$expansion"
> +   fi
> declare -f $completion_func >/dev/null 2>/dev/null && 
> $completion_func
> fi
>  }
> --
> 2.16.1
>

Re: [PATCH v3 4/9] commit-graph.txt: update design document

[Jakub Narebski]

Derrick Stolee  writes:

> We now calculate generation numbers in the commit-graph file and use
> them in paint_down_to_common().

All right.

>
> Expand the section on generation numbers to discuss how the three
> special generation numbers GENERATION_NUMBER_INFINITY, _ZERO, and
> _MAX interact with other generation numbers.

Very good.

>
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/technical/commit-graph.txt | 30 +++-
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/commit-graph.txt 
> b/Documentation/technical/commit-graph.txt
> index 0550c6d0dc..d9f2713efa 100644
> --- a/Documentation/technical/commit-graph.txt
> +++ b/Documentation/technical/commit-graph.txt
> @@ -77,6 +77,29 @@ in the commit graph. We can treat these commits as having 
> "infinite"
>  generation number and walk until reaching commits with known generation
>  number.
>
> +We use the macro GENERATION_NUMBER_INFINITY = 0x to mark commits not
> +in the commit-graph file. If a commit-graph file was written by a version
> +of Git that did not compute generation numbers, then those commits will
> +have generation number represented by the macro GENERATION_NUMBER_ZERO = 0.

I have to wonder if there would be any relesed Git that do not compute
generation numbers...

On the other hand in case the user-visible view of the project history
changes, be it because shallow clone is shortened or deepened, or grafts
file is edited, or a commit object is replaced with another with
different parents - we can still use "commit-graph" data, just pretend
that generation numbers (which are invalid in altered history) are all
zero.  (I'll write about this idea in comments to later series.)

On the other hand with GENERATION_NUMBER_ZERO these series of patches
are self-contained and bisectable.

> +
> +Since the commit-graph file is closed under reachability, we can guarantee
> +the following weaker condition on all commits:

I have had to look up the contents of the whole file, but it turns out
that it is all right: "weaker condition" refers to earlier "N <= M".

Minor sidenote: if one would be extremly pedantic, one could say that
previous condition is incorrect, because it doesn't state explicitely
that commit A != commit B. ;-)

> +
> +If A and B are commits with generation numbers N amd M, respectively,
> +and N < M, then A cannot reach B.
> +
> +Note how the strict inequality differs from the inequality when we have
> +fully-computed generation numbers. Using strict inequality may result in
> +walking a few extra commits, but the simplicity in dealing with commits
> +with generation number *_INFINITY or *_ZERO is valuable.
> +
> +We use the macro GENERATION_NUMBER_MAX = 0x3FFF to for commits whose
> +generation numbers are computed to be at least this value. We limit at
> +this value since it is the largest value that can be stored in the
> +commit-graph file using the 30 bits available to generation numbers. This
> +presents another case where a commit can have generation number equal to
> +that of a parent.

I wonder if something like the table I have proposed in v2 version of
this patch [1] would make it easier or harder to understand.

[1]: https://public-inbox.org/git/86a7u7mnzi@gmail.com/

Something like the following:

 |  gen(B)
 |
gen(A)   | _INFINITY | _MAX | larger   | smaller  | _ZERO
-+---+--+--+--+
_INFINITY| = | >| >| >| >
_MAX | < N   | =| >| >| >
larger   | < N   | < N  | = n  | >| >
smaller  | < N   | < N  | < N  | = n  | >
_ZERO| < N   | < N  | < N  | < N  | =

Here "n" and "N" denotes stronger condition, and "N" denotes weaker
condition.  We have _INFINITY > _MAX > larger > smaller > _ZERO.

> +
>  Design Details
>  --
>
> @@ -98,17 +121,12 @@ Future Work
>  - The 'commit-graph' subcommand does not have a "verify" mode that is
>necessary for integration with fsck.
>
> -- The file format includes room for precomputed generation numbers. These
> -  are not currently computed, so all generation numbers will be marked as
> -  0 (or "uncomputed"). A later patch will include this calculation.
> -
>  - After computing and storing generation numbers, we must make graph
>walks aware of generation numbers to gain the performance benefits they
>enable. This will mostly be accomplished by swapping a commit-date-ordered
>priority queue with one ordered by generation number. The following
> -  operations are important candidates:
> +  operation is an important candidate:
>
> -- paint_down_to_common()
>  - 'log --topo-order'
>
>  - Currently, parse_commit_gently() requires filling in the root tree

Looks good.

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Elijah Newren]

On Mon, Apr 16, 2018 at 11:07 PM, Junio C Hamano  wrote:
> * en/rename-directory-detection-reboot (2018-04-16) 32 commits
>  - merge-recursive: fix check for skipability of working tree updates
>  - merge-recursive: Fix was_tracked() to quit lying with some renamed paths
>  - t6046: testcases checking whether updates can be skipped in a merge
>  - merge-recursive: improve output precision around skipping updates
>  - merge-recursive: avoid spurious rename/rename conflict from dir renames
>  - directory rename detection: new testcases showcasing a pair of bugs
>  - merge-recursive: fix remaining directory rename + dirty overwrite cases
>  - merge-recursive: fix overwriting dirty files involved in renames
>  - merge-recursive: avoid clobbering untracked files with directory renames
>  - merge-recursive: apply necessary modifications for directory renames
>  - merge-recursive: when comparing files, don't include trees
>  - merge-recursive: check for file level conflicts then get new name
>  - merge-recursive: add computation of collisions due to dir rename & merging
>  - merge-recursive: check for directory level conflicts
>  - merge-recursive: add get_directory_renames()
>  - merge-recursive: make a helper function for cleanup for handle_renames
>  - merge-recursive: split out code for determining diff_filepairs
>  - merge-recursive: make !o->detect_rename codepath more obvious
>  - merge-recursive: fix leaks of allocated renames and diff_filepairs
>  - merge-recursive: introduce new functions to handle rename logic
>  - merge-recursive: move the get_renames() function
>  - directory rename detection: tests for handling overwriting dirty files
>  - directory rename detection: tests for handling overwriting untracked files
>  - directory rename detection: miscellaneous testcases to complete coverage
>  - directory rename detection: testcases exploring possibly suboptimal merges
>  - directory rename detection: more involved edge/corner testcases
>  - directory rename detection: testcases checking which side did the rename
>  - directory rename detection: files/directories in the way of some renames
>  - directory rename detection: partially renamed directory testcase/discussion
>  - directory rename detection: testcases to avoid taking detection too far
>  - directory rename detection: directory splitting testcases
>  - directory rename detection: basic testcases
>
>  Reboot of an attempt to detect wholesale directory renames and use
>  it while merging.


Thanks for rebasing/cherry-picking the series and applying my newest
changes.  It looks like a couple of your squashed fixes in the rebase
of the old commits (designed to deal with compilation errors from the
tree entry functions having been converted to object_id) went into the
wrong commits, breaking bisectability.  When I send out my next round,
I'll only replace the top four commits, but I'll add in a few commits
that can be squashed to fix the bisectability.

Also, the newest patches can still be fooled by a specially crafted
rename/add conflict, but I believe with a small restructure I can both
simplify the code and cover all the cases.

Re: [RFC 01/10] submodule: add 'core.submodulesFile' to override the '.gitmodules' path

[Stefan Beller]

Hi Antonio,

>>
>> Good point! I wonder if the cleaner solution would be to just
>> tell git to use HEAD:.gitmodules and not check out the file?
>> then you would not need to come up with a namespace for names
>> of the .gitmodules files and scatter them into the worktree as well?
>>
>
> Any solution which:
>
>   1. prevents the gitmodules file to be checked out
>   2. but still tracks it in the git repository
>
> OR
>
>   1. allows to set the gitmoudles file under some namespace
>
> would work for vcsh I guess.

I personally would tend to rather go for supporting your first solution
(prevent .gitmodules from checked-out, load from sparse HEAD),
but I do not have strong arguments or feeling about this dimension.
I am fine with a namespaced .gtimodules solution, too.

Both solutions can be implemented by either:

A) adding the code where it is (like your patch, e.g. using

> -   value=$(git config -f .gitmodules submodule."$name"."$option")
> +   gitmodules_file=$(git config core.submodulesfile)
> +   : ${gitmodules_file:=.gitmodules}
> +   value=$(git config -f "$gitmodules_file" 
> submodule."$name"."$option")

B) adding a helper, which is a layer of indirection
to load the relevant configuration.

And when it comes to this dimension, I'd strongly favor B over A.
Having this indirection helper in place enables to add more options
later easily as only one place needs to be touched.
(These other options could include the other solution as presented above,
or the idea with the special ref as mentioned in an earlier email)


>> > Can you give an example from the user point of view of such a
>> > "config-from-gitmodules" command?
>> >
>>
>> git submodule config  
>>
>> as an 'alias' for
>>
>>gitmodules_file=$(git config core.submodulesfile)
>>: ${gitmodules_file:=.gitmodules}
>>value=$(git config -f "$gitmodules_file"
>> submodule."$name"."$option")
>>
>> The helper would figure out which config file to load form
>> (.gitmodules in tree, HEAD:.gitmodules, your new proposed gitmodules file,
>> .git/config... or the special ref) and then return the  for 
>>
>> So maybe:
>>
>> $ git clone https://gerrit.googlesource.com/gerrit && cd gerrit
>> # ^ My goto-repo with submodules
>>
>> $ git submodule config "plugins/hooks" URL
>> ../plugins/hooks
>>
>>
>
> I may look into such supporting changes once you decide the approach to
> take for the bigger problem.

I think once we have the helper in place you can implement the solution
to the bigger problem as you like?

There are a few pros and cons for namespaced .gitmodules and
non-checked-out sparse HEAD .gitmodules:

How do you modify the .gitmodules config?

In the namespaced solution, you can tell users to edit that
file manually or use "git config -f $new_location" to manipulate
that file.

In the sparse solution editing becomes a little bit trickier, as you
need to edit a file in the index (or HEAD).

If you have the special ref, you could just checkout the
special ref in another worktree and make changes and
commit there


How do you change the setup?

In case of a sparse gitmodules file, you can just check it out
(make it non-sparse) or vice versa.

In case of a namespaced gitmodules file, you'd change the
config setting and have to move the file to the new location.
as git config is just about configuring, the user is left alone
with moving the file, or would we have a helper for that?
("git submodule relocate-gitmodules" or such)?

If you have the special ref, you could just checkout the
special ref in another worktree and make changes and
commit there.

I hope this helps instead of confusing more,

Thanks,
Stefan

Re: [PATCH 1/2] commit: fix --short and --porcelain

[Martin Ågren]

Hi Samuel,

Welcome back. :-)

On 18 April 2018 at 05:06, Samuel Lijin  wrote:
> Make invoking `git commit` with `--short` or `--porcelain` return status
> code zero when there is something to commit.
>
> Mark the commitable flag in the wt_status object in the call to
> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> and simplify the logic in the latter function to take advantage of the
> logic shifted to the former.

The subject is sort of vague about what is being fixed. Maybe "commit:
fix return code of ...", or "wt-status: set `commitable` when
collecting, not when printing". Or something... I can't come up with
something brilliant off the top of my head.

I did not understand the first paragraph until I had read the second and
peaked at the code. Maybe tell the story the other way around? Something
like this:

  Mark the `commitable` flag in the wt_status object in
  `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
  and simplify the logic in the latter function to take advantage of the
  logic shifted to the former.

  This means that callers do need to actually use the printer function
  to collect the `commitable` flag -- it is sufficient to call
  `wt_status_collect()`.

  As a result, invoking `git commit` with `--short` or `--porcelain`
  results in return status code zero when there is something to commit.
  This fixes two bugs documented in our test suite.

>  t/t7501-commit.sh |  4 ++--
>  wt-status.c   | 39 +++
>  2 files changed, 29 insertions(+), 14 deletions(-)

I tried to find somewhere in the documentation where this bug was
described (git-commit.txt or git-status.txt), but failed. So there
should be nothing to update there.

> +static void wt_status_mark_commitable(struct wt_status *s) {
> +   int i;
> +
> +   for (i = 0; i < s->change.nr; i++) {
> +   struct wt_status_change_data *d = (s->change.items[i]).util;
> +
> +   if (d->index_status && d->index_status != 
> DIFF_STATUS_UNMERGED) {
> +   s->commitable = 1;
> +   return;
> +   }
> +   }
> +}

This helper does exactly what the old code did inside
`wt_longstatus_print_updated()` with regards to `commitable`. Ok.

This function does not reset `commitable` to 0, so reusing a `struct
wt_status` won't necessarily work out. I have not thought about whether
such a caller would be horribly broken for other reasons...

>  void wt_status_collect(struct wt_status *s)
>  {
> wt_status_collect_changes_worktree(s);
> @@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
> wt_status_collect_changes_initial(s);
> else
> wt_status_collect_changes_index(s);
> +
> wt_status_collect_untracked(s);
> +
> +   wt_status_mark_commitable(s);
>  }

So whenever we `..._collect()`, `commitable` is set for us. This is the
only caller of the new helper, so in order to be able to trust
`commitable`, one needs to call `wt_status_collect()`. Seems a
reasonable assumption to make that the caller will remember to do so
before printing. (And all current users do, so we're not regressing in
some user.)

>  static void wt_longstatus_print_unmerged(struct wt_status *s)
> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct 
> wt_status *s)
>
>  static void wt_longstatus_print_updated(struct wt_status *s)
>  {
> -   int shown_header = 0;
> -   int i;
> +   if (!s->commitable) {
> +   return;
> +   }

Regarding my comment above: If you forget to `..._collect()` first, this
function is a no-op.

> +
> +   wt_longstatus_print_cached_header(s);
>
> +   int i;

You should leave this variable declaration at the top of the function.

> for (i = 0; i < s->change.nr; i++) {
> struct wt_status_change_data *d;
> struct string_list_item *it;
> it = &(s->change.items[i]);
> d = it->util;
> -   if (!d->index_status ||
> -   d->index_status == DIFF_STATUS_UNMERGED)
> -   continue;
> -   if (!shown_header) {
> -   wt_longstatus_print_cached_header(s);
> -   s->commitable = 1;
> -   shown_header = 1;
> +   if (d->index_status &&
> +   d->index_status != DIFF_STATUS_UNMERGED) {
> +   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, 
> it);
> }
> -   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
> }
> -   if (shown_header)
> -   wt_longstatus_print_trailer(s);
> +
> +   wt_longstatus_print_trailer(s);
>  }

This rewrite matches the original logic, assuming we can trust
`commitable`. The result is a function called `print()` which does not
modify the struct it is given 

Re: [RFC PATCH] ident: don't cache default date

[Johannes Sixt]

Am 18.04.2018 um 19:47 schrieb Phillip Wood:
> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Apr 18 2018, Phillip Wood wrote:
>>> From: Phillip Wood 
>>> as it is created by running an separate instance of 'git commit'.  If
>>> the reworded commit is follow by further picks, those later commits
>>> will have an earlier committer date than the reworded one. This is
>>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>>> not set. Fix this by not caching the date.
>>>
>>> Users expect commits to have the same author and committer dates when
>>> the don't explicitly set them. As the date is now updated each time
>>> git_author_info() or git_committer_info() is run it is possible to end
>>> up with different author and committer dates. Fix this for
>>> 'commit-tree', 'notes' and 'merge' by using a single date in
>>> commit_tree_extended() and passing it explicitly to the new functions
>>> git_author_info_with_date() and git_committer_info_with_date() when
>>> neither the author date nor the committer date are explicitly
>>> set. 'commit' always passes the author date to commit_tree_extended()
>>> and relied on the date caching to have the same committer and author
>>> dates when neither was specified. Fix this by setting
>>> GIT_COMMITTER_DATE to be the same as the author date passed to
>>> commit_tree_extended().
>>>
>>> Signed-off-by: Phillip Wood 
>>> Reported-by: Johannes Sixt 
>>> ---
>>>
>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>>> builtin/commit.c will break someone's hook script. Maybe it would be
>>> better to add a committer parameter to commit_tree() and
>>> commit_tree_extended().

While I like the basic theme of your patch, I think we should fix this
case in a much simpler way, namely, use the infrastructure that was
introduced for git-am.

I've shamelessly lifted the commit message from your patch.

 8< 
Subject: [PATCH] sequencer: reset the committer date before commits

Now that the sequencer commits without forking when the commit message
isn't edited all the commits that are picked have the same committer
date. If a commit is reworded it's committer date will be a later time
as it is created by running an separate instance of 'git commit'.  If
the reworded commit is follow by further picks, those later commits
will have an earlier committer date than the reworded one. This is
caused by git caching the default date used when GIT_COMMITTER_DATE is
not set. Reset the cached date before a commit is generated
in-process.

Signed-off-by: Johannes Sixt 
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index f9d1001dee..f0bac903a0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
goto out;
}
 
+   reset_ident_date();
+
if (commit_tree_extended(msg->buf, msg->len, , parents,
 oid, author, opts->gpg_sign, extra)) {
res = error(_("failed to write commit object"));
-- 
2.17.0.69.g0c1d01d9b6

Re: [PATCH v6 05/15] sequencer: introduce the `merge` command

[Phillip Wood]

On 14/04/18 01:51, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Fri, 13 Apr 2018, Phillip Wood wrote:
> 
>> On 13/04/18 11:12, Phillip Wood wrote:
>>> @@ -3030,7 +3029,8 @@ static int pick_commits(struct todo_list *todo_list, 
>>> struct replay_opts *opts)
>>> return error(_("unknown command %d"), item->command);
>>>  
>>> if (res < 0 && (item->command == TODO_LABEL ||
>>> -   item->command == TODO_RESET)) {
>>> +   item->command == TODO_RESET ||
>>> +   item->command == TODO_MERGE)) {
>>
>> Unfortunately it's not as simple as that - we only want to reschedule if
>> merge_recursive() fails, not if run_git_commit() does.
> 
> Correct. How about introducing a flag `reschedule` that is passed to
> do_label(), do_reset() and do_merge()?

That would work (I was thinking about using return codes but having a
parameter is a better idea). Do you want me to re-roll the fixups or are
you happy to make the changes in your next version?

> 
> Seeing as do_reset() and do_merge() already have a replay_opts parameter,
> we could add a field `needs_rescheduling` and pass the replay_opts also to
> do_label().

I'm slightly wary of putting state in an options structure but maybe it
doesn't matter.

Best Wishes

Phillip

> Ciao,
> Dscho
> 

Re: [PATCH v6 04/15] sequencer: introduce new commands to reset the revision

[Phillip Wood]

On 15/04/18 18:17, Philip Oakley wrote:
> From: "Phillip Wood" 
> : Friday, April 13, 2018 11:03 AM
>> If a label or reset command fails it is likely to be due to a
>> typo. Rescheduling the command would make it easier for the user to fix
>> the problem as they can just run 'git rebase --edit-todo'. 
> 
> Is this worth noting in the command documentation? "If the label or
> reset command fails then fix
> the problem by runnning 'git rebase --edit-todo'." ?
> 
> Just a thought.

Yes that's a good idea, thanks

>> It also
>> ensures that the problem has actually been fixed when the rebase
>> continues. I think you could do it like this
>>
> 
> -- 
> Philip
> (also @dunelm, 73-79..)
That's a bit before me (94-00) were you there when they were building
the hill colleges and some of the science site?

Best Wishes

Phillip

Re: [RFC PATCH] ident: don't cache default date

[Phillip Wood]

Hi Ævar, thanks for your comments

On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Apr 18 2018, Phillip Wood wrote:
> 
>> From: Phillip Wood 
>>
>> Now that the sequencer commits without forking when the commit message
>> isn't edited all the commits that are picked have the same committer
>> date. If a commit is reworded it's committer date will be a later time
> 
> s/it's/its/

Thanks I'll change it

>> as it is created by running an separate instance of 'git commit'.  If
>> the reworded commit is follow by further picks, those later commits
>> will have an earlier committer date than the reworded one. This is
>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>> not set. Fix this by not caching the date.
>>
>> Users expect commits to have the same author and committer dates when
>> the don't explicitly set them. As the date is now updated each time
>> git_author_info() or git_committer_info() is run it is possible to end
>> up with different author and committer dates. Fix this for
>> 'commit-tree', 'notes' and 'merge' by using a single date in
>> commit_tree_extended() and passing it explicitly to the new functions
>> git_author_info_with_date() and git_committer_info_with_date() when
>> neither the author date nor the committer date are explicitly
>> set. 'commit' always passes the author date to commit_tree_extended()
>> and relied on the date caching to have the same committer and author
>> dates when neither was specified. Fix this by setting
>> GIT_COMMITTER_DATE to be the same as the author date passed to
>> commit_tree_extended().
>>
>> Signed-off-by: Phillip Wood 
>> Reported-by: Johannes Sixt 
>> ---
>>
>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>> builtin/commit.c will break someone's hook script. Maybe it would be
>> better to add a committer parameter to commit_tree() and
>> commit_tree_extended().
>>
>> This needs some tests. I think we could test that the sequencer gives
>> each commit a new date with 'git rebase -i --exec="sleep 2"
>> --force-rebase @~2' and test the committer dates of the rebased
>> commits. I'm not sure if test -gt can cope with numbers that big
>> though, maybe we'll have to be content with test !=. For 'git commit'
>> I think doing GIT_EDITOR='sleep 2; echo message >"$1"' and checking
>> the committer date and author date will work as the author date is set
>> before the user edits the commit message. I'm not sure how to test
>> callers of commit_tree() though (that's commit-tree, notes and merge)
>> as I've not been able to come up with anything that will guarantee the
>> author and committer dates are different if the code in
>> commit_tree_extended() that ensures they are the same gets broken.
> 
> The behavior you're describing where we end up with later commits in the
> rebase with earlier CommitDate's definitely sounds like a minor
> regression, and yeah we should have tests for this.
> 
> My mental model of this is that we shouldn't be trying at all to adjust
> the CommitDate in a sequence to be the same, and just make it be
> whatever time() returns, except in the case where a date is passed
> explicitly.
> 
> My cursory reading of this RFC patch is that it's definitely an
> improvement because we don't end up with things out of order, but is
> there any reason for why we should be trying to aim to keep this
> "consistent" within a single "git rebase" command by default, as opposed
> to always just writing the current CommitDate at the time we make the
> commit, that sounds like the most intuitive thing to me, and I can't see
> any downsides with that.

It's not trying to keep the date "consistent" within a single rebase
command, each commit created by the sequencer gets a date stamp with the
current time when the commit is created. What it is doing is keeping the
author and committer dates the same for commit/commit-tree/merge/notes
when the user does not specify either of them. While I don't think it
particularly matters that they match (so long as the committer date is
later than the author date), it is what git does at the moment and
someone maybe relying on the current behavior.

Best Wishes

Phillip

> 
> It leaks info about how long it took someone to do the rebase, but so
> what?
> 

Re: [PATCH] submodule--helper: don't print null in 'submodule status'

[Stefan Beller]

Hi Nguyễn,

On Wed, Apr 18, 2018 at 7:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> The function compute_rev_name() can return NULL sometimes (e.g. right
> after 'submodule init'). The current code makes 'submodule status'
> print this:
>
>  19d97bf5af05312267c2e874ee6bcf584d9e9681 sha1collisiondetection ((null))
>
> This ugly 'null' adds no value to the user using this command. More
> importantly printf() on some platform can't handle NULL as a string
> and will crash instead of printing '(null)'.
>
> Check for this and skip printing this part (the alternative is
> printing '(n/a)' or something but I think that is just noise).

This patch restores the behavior from before a9f8a37584 (submodule:
port submodule subcommand 'status' from shell to C, 2017-10-06),
so this is the right way to go instead of the alternatives you considered.

Thanks!

Reviewed-by: Stefan Beller 

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/submodule--helper.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index a404df3ea4..4dc7d7d29f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -596,8 +596,12 @@ static void print_status(unsigned int flags, char state, 
> const char *path,
>
> printf("%c%s %s", state, oid_to_hex(oid), displaypath);
>
> -   if (state == ' ' || state == '+')
> -   printf(" (%s)", compute_rev_name(path, oid_to_hex(oid)));
> +   if (state == ' ' || state == '+') {
> +   const char *name = compute_rev_name(path, oid_to_hex(oid));
> +
> +   if (name)
> +   printf(" (%s)", name);
> +   }
>
> printf("\n");
>  }
> --
> 2.17.0.367.g5dd2e386c3
>

Re: [PATCH] git-send-email: Cc more people

[Mathieu Desnoyers]

- On Apr 18, 2018, at 10:33 AM, rostedt rost...@goodmis.org wrote:

> On Wed, 18 Apr 2018 07:05:03 -0700
> Matthew Wilcox  wrote:
> 
>> From: Matthew Wilcox 
>> 
>> Several of my colleagues (and myself) have expressed surprise and
>> annoyance that git-send-email doesn't automatically pick up people who
>> are listed in patches as Reported-by: or Reviewed-by: or ... many other
>> tags that would seem (to us) to indicate that person might be interested.
>> This patch to git-send-email tries to pick up all Foo-by: tags.
> 
> Acked-by: Steven Rostedt (VMware) 
> 
> Note, this is one of the reasons I still use quilt to send my email.
> I've modified my quilt scripts to do what Matthew does here below.

Acked-by: Mathieu Desnoyers 

I find it really surprising and unexpected that people listed as
"Reviewed-by" don't end up being CC'd.

Thanks,

Mathieu

> 
> -- Steve
> 
> 
>> 
>> Signed-off-by: Matthew Wilcox 
>> 
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2fa7818ca..926815329 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>>  # Now parse the message body
>>  while(<$fh>) {
>>  $message .=  $_;
>> -if (/^(Signed-off-by|Cc): (.*)/i) {
>> +if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>>  chomp;
>>  my ($what, $c) = ($1, $2);
> > # strip garbage for the address we'll use:

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[PATCH 0/2] Fix --short and --porcelain options for commit

[Samuel Lijin]

Hi all - I last contributed about a year ago and I've finally found the
time to start contributing again, and hopefully I'll stick around this
time. Figured I'd start with something small :)

Samuel Lijin (2):
  commit: fix --short and --porcelain
  wt-status: const-ify all printf helper methods

 t/t7501-commit.sh |  4 ++--
 wt-status.c   | 57 +++
 wt-status.h   |  4 ++--
 3 files changed, 40 insertions(+), 25 deletions(-)

-- 
2.16.2

[PATCH 2/2] wt-status: const-ify all printf helper methods

[Samuel Lijin]

Change the method signatures of all printf helper methods to take a
`const struct wt_status *` rather than a `struct wt_status *`.

Signed-off-by: Samuel Lijin 
---
 wt-status.c | 18 +-
 wt-status.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 26b0a6221..55d29bc09 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NIL,/* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
const char *c = "";
if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char 
*color,
const char *fmt, va_list ap, const char *trail)
 {
struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, 
const char *color,
strbuf_release();
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
const char *fmt, ...)
 {
va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
const char *fmt, ...)
 {
va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
   const char *fmt, ...)
 {
va_list ap;
@@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct 
wt_status *s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct 
wt_status *s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct 
wt_status *s,
strbuf_release();
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
int change_type,
struct string_list_item *it)
 {
@@ -768,7 +768,7 @@ static void wt_longstatus_print_unmerged(struct wt_status 
*s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
if (!s->commitable) {
return;
diff --git a/wt-status.h b/wt-status.h
index 430770b85..83a1f7c00 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -135,9 +135,9 @@ int wt_status_check_bisect(const struct worktree *wt,
   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, 
...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char 
*fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, 
...);
+void status_printf(const struct wt_status *s, const char *color, const char 
*fmt, ...);
 
 /* The following functions expect that the caller took care of reading the 
index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.16.2

[PATCH 1/2] commit: fix --short and --porcelain

[Samuel Lijin]

Make invoking `git commit` with `--short` or `--porcelain` return status
code zero when there is something to commit.

Mark the commitable flag in the wt_status object in the call to
`wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
and simplify the logic in the latter function to take advantage of the
logic shifted to the former.

Signed-off-by: Samuel Lijin 
---
 t/t7501-commit.sh |  4 ++--
 wt-status.c   | 39 +++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..85a8217fd 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,12 @@ test_expect_success '--dry-run with stuff to commit returns 
ok' '
git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --porcelain
 '
diff --git a/wt-status.c b/wt-status.c
index 50815e5fa..26b0a6221 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,19 @@ static void wt_status_collect_untracked(struct wt_status 
*s)
s->untracked_in_ms = (getnanotime() - t_begin) / 100;
 }
 
+static void wt_status_mark_commitable(struct wt_status *s) {
+   int i;
+
+   for (i = 0; i < s->change.nr; i++) {
+   struct wt_status_change_data *d = (s->change.items[i]).util;
+
+   if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) 
{
+   s->commitable = 1;
+   return;
+   }
+   }
+}
+
 void wt_status_collect(struct wt_status *s)
 {
wt_status_collect_changes_worktree(s);
@@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
wt_status_collect_changes_initial(s);
else
wt_status_collect_changes_index(s);
+
wt_status_collect_untracked(s);
+
+   wt_status_mark_commitable(s);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status 
*s)
 
 static void wt_longstatus_print_updated(struct wt_status *s)
 {
-   int shown_header = 0;
-   int i;
+   if (!s->commitable) {
+   return;
+   }
+
+   wt_longstatus_print_cached_header(s);
 
+   int i;
for (i = 0; i < s->change.nr; i++) {
struct wt_status_change_data *d;
struct string_list_item *it;
it = &(s->change.items[i]);
d = it->util;
-   if (!d->index_status ||
-   d->index_status == DIFF_STATUS_UNMERGED)
-   continue;
-   if (!shown_header) {
-   wt_longstatus_print_cached_header(s);
-   s->commitable = 1;
-   shown_header = 1;
+   if (d->index_status &&
+   d->index_status != DIFF_STATUS_UNMERGED) {
+   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, 
it);
}
-   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
}
-   if (shown_header)
-   wt_longstatus_print_trailer(s);
+
+   wt_longstatus_print_trailer(s);
 }
 
 /*
-- 
2.16.2

Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary

[Duy Nguyen]

On Wed, Apr 18, 2018 at 12:47 AM, Philip Oakley  wrote:
>>> > Is that something I should add to my todo to add a 'guide' category >
>>> > etc.?
>>>
>>> I added it too [1]. Not sure if you want anything more on top though.
>
>
> What I've seen is looking good - I've not had as much time as I'd like..
>
> I'm not sure of the status of the git/generate-cmdlist.sh though. Should
> that also be updated, or did I miss that?

Yes it's updated by other patches in the same thread.
-- 
Duy

Re: [PATCH v3 0/2] fsexcludes: Add programmatic way to exclude files

[Ben Peart]

I found a bug with how this patch series deals with untracked files. 
I'm going to retract this patch until I have time to create a new test 
case to demonstrate the bug and come up with a good fix.


Ben

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Christian Hesse]

Junio C Hamano  on Tue, 2018/04/17 15:07:
> * ab/simplify-perl-makefile (2018-04-11) 1 commit
>   (merged to 'next' on 2018-04-17 at 4448756934)
>  + perl: fix installing modules from contrib
> 
>  Recent simplification of build procedure forgot a bit of tweak to
>  the build procedure of contrib/mw-to-git/
> 
>  Will merge to 'master'.

Looks like cooking is v2 of the patch, while we were at v3, no?
-- 
Best regards,
Chris


pgpMlnERUFLLz.pgp
Description: OpenPGP digital signature

Re: [PATCH/RFC] completion: complete all possible -no-

[Duy Nguyen]

On Wed, Apr 18, 2018 at 5:43 AM, Junio C Hamano  wrote:
> So, the earlier mention of "clone --no-checkout" sounded about not
> losing this historical practice, but (desirabilty of magic number 4
> aside) this "show first handful of --no-foo" feature is not about
> historical practice but is forward looking, in the sense that you do
> not mark "important" negated options in the source, which would be a
> way to handle the histrical "clone --no-checkout", but let the
> machinery mechanically choose among --no-foo (with the stupid choice
> criterion "first four are shown").

Well you kinda mark important in the source too. --no-checkout for
exampled is declared as OPT_BOOL(0, "no-checkout"... and parse-options
code has to add the double-negative form --checkout back [1].

The "first four" is chosen after carefully examining all commands and
observing that none of them have more than 4 "important" --no-. But
yes it is questionable and I should be able to do better to separate
the favorable --no- from the other extra and most-of-the-time-useless
--no- options.

> That allows other commands to
> have many --no-foo form without overwhelming the choices, but I am
> not sure if it is much better than a possible alternative of only
> showing --no-foo for more "important" foo's when show_gitcomp() is
> asked to list all of things. It would certainly be a more involved
> solution, that might require an update to the way how the choices
> are precomputed (you'd end up having to keep a separate "use this
> list when completing '--no-'" in addition to the normal list).

I did think about this alternative and was still undecided. Suppose
that you have less than 4 "important" --no- options, showing some
extra ones to me does not really hurt anything and if we could show
more options (within the same screen space) we should. But on the
other hand maintaining this magic number could be a maintenance
nightmare... Yeah I think I'm shifting towards no magic number now.

[1] These double negative options will _always_ show up,  there is no
easy way to hide them because they don't start with --no-. But we
don't have a lot of options starting with "no-" so it's probably fine.
-- 
Duy

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Thandesha VK]

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?
--> Correct.

Does that happen with the 17.2 version of p4?
-->Correct.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way)
-->Sure.

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.
-->Most of other places are already doing key check in the hash. Looks
like this line was missed out.

On Wed, Apr 18, 2018 at 4:08 AM, Luke Diamand  wrote:
> On 17 April 2018 at 20:12, Thandesha VK  wrote:
>> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
>> the size value even with latest version of p4 (17.2). In that case, we
>> have to regenerate the digest for file save it - It mean something is
>> wrong with the file in perforce.
>
> Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.
>
> We are just talking about the output from "p4 print" and the
> "fileSize" key, right?
>
> Does that happen with the 17.2 version of p4?
>
>> Regarding, sys.stdout.write v/s print, I see script using both of them
>> without a common pattern. I can change it to whatever is more
>> appropriate.
>
> print() probably makes more sense; can we try to use the function form
> so that we don't deliberately make the path to python3 harder (albeit
> in a very tiny way).
>
>>
>> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey  wrote:
>>> Does a missing "fileSize" actually mean that there is something wrong with 
>>> the file?
>>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>>> (which I attribute to our rather ancient (2007.2) Perforce server)
>>> I'm not an expert in Perforce, so don't know for sure.
>
> My 2015 version of p4d reports a fileSize.
>
>>>
>>> However, `p4 -G sizes` works fine even with our p4 server.
>>> Should we then go one step further and use `p4 -G sizes` to obtain the 
>>> "fileSize" when it's not returned by `p4 -G print`?
>>> Or is it an overkill for a simple verbose print out?
>
> If your server isn't reporting "fileSize" then there are a few other
> places where I would expect git-p4 to also fail.
>
> If we're going to support this very ancient version of p4d, then
> gracefully handling a missing fileSize will be useful.
>
>>>
>>> Also, please, find one comment inline below.
>>>
>>> Thank you,
>>> Andrey
>>>
>>> From: Thandesha VK 
 Sounds good. How about an enhanced version of fix from both of us.
 This will let us know that something is not right with the file but
 will not bark

 $ git diff
 diff --git a/git-p4.py b/git-p4.py
 index 7bb9cadc6..df901976f 100755
 --- a/git-p4.py
 +++ b/git-p4.py
 @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
  relPath = self.stripRepoPath(file['depotFile'], 
 self.branchPrefixes)
  relPath = self.encodeWithUTF8(relPath)
  if verbose:
 -size = int(self.stream_file['fileSize'])
 +if 'fileSize' not in self.stream_file:
 +   print "WARN: File size from perforce unknown. Please 
 verify by p4 sizes %s" %(file['depotFile'])
>>> For whatever reason, the code below uses sys.stdout.write() instead of 
>>> print().
>>> Should it be used here for consistency as well?
>>>
 +   size = "-1"
 +else:
 +   size = self.stream_file['fileSize']
 +size = int(size)
  sys.stdout.write('\r%s --> %s (%i MB)\n' %
 (file['depotFile'], relPath, size/1024/1024))
  sys.stdout.flush()


 On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  
 wrote:
> Sure, I totally agree.
> Sorry, I just wasn't clear enough in my previous email.
> I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
> "fileSize" is not available,
> while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
> known.
> In other words,
>  * if "fileSize" is known:
>  ** both yours and mine patches don't change existing behavior;
>  * if "fileSize" is not known:
>  ** your patch makes streamOneP4File() not print anything;
>  ** my patch makes streamOneP4File() print "%s --> %s".
>
> Hope, I'm clearer this time.
>
> Thank you,
> Andrey
>
> From: Thandesha VK 
>> *I* think keeping the filesize info is better with --verbose option as
>> that gives some clue about the file we are working on. What do you
>> think?
>> Script has similar checks of key existence at other places where it is
>> looking for fileSize.
>>
>> On Tue, Apr 17, 2018 at 9:22 AM, 

[PATCH] submodule--helper: don't print null in 'submodule status'

[Nguyễn Thái Ngọc Duy]

The function compute_rev_name() can return NULL sometimes (e.g. right
after 'submodule init'). The current code makes 'submodule status'
print this:

 19d97bf5af05312267c2e874ee6bcf584d9e9681 sha1collisiondetection ((null))

This ugly 'null' adds no value to the user using this command. More
importantly printf() on some platform can't handle NULL as a string
and will crash instead of printing '(null)'.

Check for this and skip printing this part (the alternative is
printing '(n/a)' or something but I think that is just noise).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/submodule--helper.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a404df3ea4..4dc7d7d29f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -596,8 +596,12 @@ static void print_status(unsigned int flags, char state, 
const char *path,
 
printf("%c%s %s", state, oid_to_hex(oid), displaypath);
 
-   if (state == ' ' || state == '+')
-   printf(" (%s)", compute_rev_name(path, oid_to_hex(oid)));
+   if (state == ' ' || state == '+') {
+   const char *name = compute_rev_name(path, oid_to_hex(oid));
+
+   if (name)
+   printf(" (%s)", name);
+   }
 
printf("\n");
 }
-- 
2.17.0.367.g5dd2e386c3

FROM I.M.F

[IMF UK]

imf london.pdf
Description: Adobe PDF document

Re: [PATCH v3 3/9] commit: use generations in paint_down_to_common()

[Derrick Stolee]

On 4/18/2018 10:31 AM, Jakub Narebski wrote:

Derrick Stolee  writes:


Define compare_commits_by_gen_then_commit_date(), which uses generation
numbers as a primary comparison and commit date to break ties (or as a
comparison when both commits do not have computed generation numbers).

Since the commit-graph file is closed under reachability, we know that
all commits in the file have generation at most GENERATION_NUMBER_MAX
which is less than GENERATION_NUMBER_INFINITY.

This change does not affect the number of commits that are walked during
the execution of paint_down_to_common(), only the order that those
commits are inspected. In the case that commit dates violate topological
order (i.e. a parent is "newer" than a child), the previous code could
walk a commit twice: if a commit is reached with the PARENT1 bit, but
later is re-visited with the PARENT2 bit, then that PARENT2 bit must be
propagated to its parents. Using generation numbers avoids this extra
effort, even if it is somewhat rare.

Does it mean that it gives no measureable performance improvements for
typical test cases?


Not in this commit. When we add the `min_generation` parameter in a 
later commit, we do get a significant performance boost (when we can 
supply a non-zero value to `min_generation`).


This step of using generation numbers for the priority is important for 
that commit, but on its own has limited value outside of the clock-skew 
case mentioned above.





Signed-off-by: Derrick Stolee 
---
  commit.c | 20 +++-
  commit.h |  1 +
  2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 711f674c18..a44899c733 100644
--- a/commit.c
+++ b/commit.c
@@ -640,6 +640,24 @@ static int compare_commits_by_author_date(const void *a_, 
const void *b_,
return 0;
  }
  
+int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused)

+{
+   const struct commit *a = a_, *b = b_;
+
+   /* newer commits first */
+   if (a->generation < b->generation)
+   return 1;
+   else if (a->generation > b->generation)
+   return -1;
+
+   /* use date as a heuristic when generataions are equal */

Very minor typo in above comment:

s/generataions/generations/


Good catch!




+   if (a->date < b->date)
+   return 1;
+   else if (a->date > b->date)
+   return -1;
+   return 0;
+}
+
  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
*unused)
  {
const struct commit *a = a_, *b = b_;
@@ -789,7 +807,7 @@ static int queue_has_nonstale(struct prio_queue *queue)
  /* all input commits in one and twos[] must have been parsed! */
  static struct commit_list *paint_down_to_common(struct commit *one, int n, 
struct commit **twos)
  {
-   struct prio_queue queue = { compare_commits_by_commit_date };
+   struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
struct commit_list *result = NULL;
int i;
  
diff --git a/commit.h b/commit.h

index aac3b8c56f..64436ff44e 100644
--- a/commit.h
+++ b/commit.h
@@ -341,6 +341,7 @@ extern int remove_signature(struct strbuf *buf);
  extern int check_commit_signature(const struct commit *commit, struct 
signature_check *sigc);
  
  int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);

+int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
void *unused);
  
  LAST_ARG_MUST_BE_NULL

  extern int run_commit_hook(int editor_is_used, const char *index_file, const 
char *name, ...);

Re: [PATCH] git-send-email: Cc more people

[Steven Rostedt]

On Wed, 18 Apr 2018 07:05:03 -0700
Matthew Wilcox  wrote:

> From: Matthew Wilcox 
> 
> Several of my colleagues (and myself) have expressed surprise and
> annoyance that git-send-email doesn't automatically pick up people who
> are listed in patches as Reported-by: or Reviewed-by: or ... many other
> tags that would seem (to us) to indicate that person might be interested.
> This patch to git-send-email tries to pick up all Foo-by: tags.

Acked-by: Steven Rostedt (VMware) 

Note, this is one of the reasons I still use quilt to send my email.
I've modified my quilt scripts to do what Matthew does here below.

-- Steve


> 
> Signed-off-by: Matthew Wilcox 
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2fa7818ca..926815329 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1665,7 +1665,7 @@ foreach my $t (@files) {
>   # Now parse the message body
>   while(<$fh>) {
>   $message .=  $_;
> - if (/^(Signed-off-by|Cc): (.*)/i) {
> + if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
>   chomp;
>   my ($what, $c) = ($1, $2);
>   # strip garbage for the address we'll use:

Re: [PATCH v3 3/9] commit: use generations in paint_down_to_common()

[Jakub Narebski]

Derrick Stolee  writes:

> Define compare_commits_by_gen_then_commit_date(), which uses generation
> numbers as a primary comparison and commit date to break ties (or as a
> comparison when both commits do not have computed generation numbers).
>
> Since the commit-graph file is closed under reachability, we know that
> all commits in the file have generation at most GENERATION_NUMBER_MAX
> which is less than GENERATION_NUMBER_INFINITY.
>
> This change does not affect the number of commits that are walked during
> the execution of paint_down_to_common(), only the order that those
> commits are inspected. In the case that commit dates violate topological
> order (i.e. a parent is "newer" than a child), the previous code could
> walk a commit twice: if a commit is reached with the PARENT1 bit, but
> later is re-visited with the PARENT2 bit, then that PARENT2 bit must be
> propagated to its parents. Using generation numbers avoids this extra
> effort, even if it is somewhat rare.

Does it mean that it gives no measureable performance improvements for
typical test cases?

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit.c | 20 +++-
>  commit.h |  1 +
>  2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/commit.c b/commit.c
> index 711f674c18..a44899c733 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -640,6 +640,24 @@ static int compare_commits_by_author_date(const void 
> *a_, const void *b_,
>   return 0;
>  }
>  
> +int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
> void *unused)
> +{
> + const struct commit *a = a_, *b = b_;
> +
> + /* newer commits first */
> + if (a->generation < b->generation)
> + return 1;
> + else if (a->generation > b->generation)
> + return -1;
> +
> + /* use date as a heuristic when generataions are equal */

Very minor typo in above comment:

s/generataions/generations/

> + if (a->date < b->date)
> + return 1;
> + else if (a->date > b->date)
> + return -1;
> + return 0;
> +}
> +
>  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
> *unused)
>  {
>   const struct commit *a = a_, *b = b_;
> @@ -789,7 +807,7 @@ static int queue_has_nonstale(struct prio_queue *queue)
>  /* all input commits in one and twos[] must have been parsed! */
>  static struct commit_list *paint_down_to_common(struct commit *one, int n, 
> struct commit **twos)
>  {
> - struct prio_queue queue = { compare_commits_by_commit_date };
> + struct prio_queue queue = { compare_commits_by_gen_then_commit_date };
>   struct commit_list *result = NULL;
>   int i;
>  
> diff --git a/commit.h b/commit.h
> index aac3b8c56f..64436ff44e 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -341,6 +341,7 @@ extern int remove_signature(struct strbuf *buf);
>  extern int check_commit_signature(const struct commit *commit, struct 
> signature_check *sigc);
>  
>  int compare_commits_by_commit_date(const void *a_, const void *b_, void 
> *unused);
> +int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, 
> void *unused);
>  
>  LAST_ARG_MUST_BE_NULL
>  extern int run_commit_hook(int editor_is_used, const char *index_file, const 
> char *name, ...);

[PATCH] git-send-email: Cc more people

[Matthew Wilcox]

From: Matthew Wilcox 

Several of my colleagues (and myself) have expressed surprise and
annoyance that git-send-email doesn't automatically pick up people who
are listed in patches as Reported-by: or Reviewed-by: or ... many other
tags that would seem (to us) to indicate that person might be interested.
This patch to git-send-email tries to pick up all Foo-by: tags.

Signed-off-by: Matthew Wilcox 

diff --git a/git-send-email.perl b/git-send-email.perl
index 2fa7818ca..926815329 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1665,7 +1665,7 @@ foreach my $t (@files) {
# Now parse the message body
while(<$fh>) {
$message .=  $_;
-   if (/^(Signed-off-by|Cc): (.*)/i) {
+   if (/^([A-Z-a-z]*-by|Cc): (.*)/i) {
chomp;
my ($what, $c) = ($1, $2);
# strip garbage for the address we'll use:

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Jonathan Tan]

On Tue, Apr 17, 2018 at 11:05 AM, Stefan Beller  wrote:
>> * sb/submodule-move-nested (2018-03-29) 6 commits
>>  - submodule: fixup nested submodules after moving the submodule
>>  - submodule-config: remove submodule_from_cache
>>  - submodule-config: add repository argument to submodule_from_{name, path}
>>  - submodule-config: allow submodule_free to handle arbitrary repositories
>>  - grep: remove "repo" arg from non-supporting funcs
>>  - submodule.h: drop declaration of connect_work_tree_and_git_dir
>>
>>  Moving a submodule that itself has submodule in it with "git mv"
>>  forgot to make necessary adjustment to the nested sub-submodules;
>>  now the codepath learned to recurse into the submodules.
>>
>>  What's the doneness of this thing?
>
> I considered this done a long time ago,
>
> "All 6 patches look good to me, thanks.
>  Reviewed-by: Jonathan Tan "
>
> https://public-inbox.org/git/20180328161727.af10f596dffc8e01205c4...@google.com/

To add to this, I sent this in reply to version 3 of this patch set,
after Stefan addressed my comments. Most of my in-depth comments were
in reply to version 1 of this patch, which are the grandchild replies
to [1].

[1] https://public-inbox.org/git/20180327213918.77851-1-sbel...@google.com/

Re: What's cooking in git.git (Apr 2018, #02; Tue, 17)

[Derrick Stolee]

On 4/17/2018 9:04 PM, Junio C Hamano wrote:

Stefan Beller  writes:


  What's the doneness of this thing?  I didn't recall seeing any
  response, especially ones that demonstrated the reviewer carefully
  read and thought about the issues surrounding the code.  Not that I
  spotted any problems in these patches myself, though.

Stolee and Brandon provided a "quick LGTM" type of review
https://public-inbox.org/git/20180409232536.gb102...@google.com/
https://public-inbox.org/git/9ddfee7e-025a-79c9-8d6b-700c65a14...@gmail.com/

Yup.  Giving positive reviews is harder than giving constructive
criticism.  Much harder.

As readers cannot tell from a "quick LGTM" between "I didn't read it
but it did not smell foul" and "I read it thoroughly, understood how
the solution works, it was presented well, and agree with the design
and implementation---there is nothing to add", the reviewers need to
come up with some way to express that it is the latter case rather
than the former.

I would not claim that I've perfected my technique to do so, but
when responding to such a "good" series, I rephrase the main idea in
the series in my own words to show that I as a reviewer read the
series well enough to be able to do so, perhaps with comparison with
possible alternatives I could think of and dicussion to argue that
the solution presented in the series is better, in an attempt to
demonstrate that I am qualified to say "this one is good" with good
enough understanding of both the issue the series addresses and the
solution in the series.


I'm sorry that my second message was terse. My response to v1 [1] was

> I looked through these patches and only found one set of whitespace > 
errors. Compiles and tests fine on my machine. > > Reviewed-by: Derrick 
Stolee  So, I pulled the code, went through it 
patch-by-patch, and saw that the transformations were made using the 
established pattern. The second review was to chime in that my v1 
comments had been addressed. Thanks, -Stolee
[1] 
https://public-inbox.org/git/6c319100-df47-3b8d-8661-24e4643ad...@gmail.com/

Re: [PATCH 01/11] t9902-completion: add tests demonstrating issues with quoted pathnames

[Johannes Schindelin]

Hi Gábor,

On Tue, 17 Apr 2018, SZEDER Gábor wrote:

> Completion functions see all words on the command line verbatim,
> including any backslash-escapes, single and double quotes that might
> be there.  Furthermore, git commands quote pathnames if they contain
> certain special characters.  All these create various issues when
> doing git-aware path completion.
> 
> Add a couple of failing tests to demonstrate these issues.
> 
> Later patches in this series will discuss these issues in detail as
> they fix them.
> 
> Signed-off-by: SZEDER Gábor 
> ---
> 
> Notes:
> Do any more new tests need FUNNYNAMES* prereq?

Yes.

>  t/t9902-completion.sh | 91 +++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index b7f5b1e632..ff2e4a8f5f 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1427,6 +1427,97 @@ test_expect_success 'complete files' '
>   test_completion "git add mom" "momified"
>  '
>  
> +# The next tests only care about how the completion script deals with
> +# unusual characters in path names.  By defining a custom completion
> +# function to list untracked files they won't be influenced by future
> +# changes of the completion functions of real git commands, and we
> +# don't have to bother with adding files to the index in these tests.
> +_git_test_path_comp ()
> +{
> + __git_complete_index_file --others
> +}
> +
> +test_expect_failure 'complete files - escaped characters on cmdline' '
> + test_when_finished "rm -rf \"New|Dir\"" &&
> + mkdir "New|Dir" &&
> + >"New|Dir/New" &&
> +
> + test_completion "git test-path-comp N" \
> + "New|Dir" &&# Bash will turn this into "New\|Dir/"
> + test_completion "git test-path-comp New\\|D" \
> + "New|Dir" &&
> + test_completion "git test-path-comp New\\|Dir/N" \
> + "New|Dir/New" && # Bash will turn this into
> + # "New\|Dir/New\ "
> + test_completion "git test-path-comp New\\|Dir/New\\" \
> + "New|Dir/New"
> +'

This fails with:

2018-04-18T11:12:55.0436371Z expecting success: 
2018-04-18T11:12:55.0436665Ztest_when_finished "rm -rf \"New|Dir\"" &&
2018-04-18T11:12:55.0436799Zmkdir "New|Dir" &&
2018-04-18T11:12:55.0436904Z>"New|Dir/New" &&
2018-04-18T11:12:55.0436972Z 
2018-04-18T11:12:55.0437158Ztest_completion "git test-path-comp N" \
2018-04-18T11:12:55.0437296Z"New|Dir" &&# Bash will 
turn this into "New\|Dir/"
2018-04-18T11:12:55.0437413Ztest_completion "git test-path-comp New\\|D" \
2018-04-18T11:12:55.0437522Z"New|Dir" &&
2018-04-18T11:12:55.0437629Ztest_completion "git test-path-comp 
New\\|Dir/N" \
2018-04-18T11:12:55.0437767Z"New|Dir/New" && # Bash 
will turn this into
2018-04-18T11:12:55.0438040Z# 
"New\|Dir/New\ "
2018-04-18T11:12:55.0438152Ztest_completion "git test-path-comp 
New\\|Dir/New\\" \
2018-04-18T11:12:55.0438504Z"New|Dir/New"
2018-04-18T11:12:55.0438742Z 
2018-04-18T11:12:55.0590984Z ++ test_when_finished 'rm -rf "New|Dir"'
2018-04-18T11:12:55.0591722Z ++ test 0 = 0
2018-04-18T11:12:55.0592001Z ++ test_cleanup='{ rm -rf "New|Dir"
2018-04-18T11:12:55.0592290Z} && (exit "$eval_ret"); eval_ret=$?; :'
2018-04-18T11:12:55.0592472Z ++ mkdir 'New|Dir'
2018-04-18T11:12:55.0717255Z ++ test_completion 'git test-path-comp N' 'New|Dir'
2018-04-18T11:12:55.0717680Z ++ test 2 -gt 1
2018-04-18T11:12:55.0718062Z ++ printf '%s\n' 'New|Dir'
2018-04-18T11:12:55.0718275Z ++ run_completion 'git test-path-comp N'
2018-04-18T11:12:55.0718447Z ++ local -a COMPREPLY _words
2018-04-18T11:12:55.0718631Z ++ local _cword
2018-04-18T11:12:55.0718806Z ++ _words=($1)
2018-04-18T11:12:55.0718965Z ++ test N = ' '
2018-04-18T11:12:55.0719124Z ++ ((  _cword = 3 - 1  ))
2018-04-18T11:12:55.0719286Z ++ __git_wrap__git_main
2018-04-18T11:12:55.0719467Z ++ __git_func_wrap __git_main
2018-04-18T11:12:55.0719633Z ++ local cur words cword prev
2018-04-18T11:12:55.0719801Z ++ _get_comp_words_by_ref -n =: cur words cword 
prev
2018-04-18T11:12:55.0720074Z ++ '[' 6 -gt 0 ']'
2018-04-18T11:12:55.0720239Z ++ case "$1" in
2018-04-18T11:12:55.0720406Z ++ shift
2018-04-18T11:12:55.0720584Z ++ '[' 5 -gt 0 ']'
2018-04-18T11:12:55.0720742Z ++ case "$1" in
2018-04-18T11:12:55.0720899Z ++ shift
2018-04-18T11:12:55.0721054Z ++ '[' 4 -gt 0 ']'
2018-04-18T11:12:55.0721240Z ++ case "$1" in
2018-04-18T11:12:55.0721392Z ++ cur=N
2018-04-18T11:12:55.0721547Z ++ shift
2018-04-18T11:12:55.0721717Z ++ '[' 3 -gt 0 ']'
2018-04-18T11:12:55.0721879Z ++ case "$1" in
2018-04-18T11:12:55.0722040Z ++ words=("${_words[@]}")
2018-04-18T11:12:55.0722201Z ++ shift
2018-04-18T11:12:55.0722396Z ++ '[' 2 -gt 0 ']'
2018-04-18T11:12:55.0722931Z ++ case 

[ANNOUNCE] Git Rev News edition 38

[Christian Couder]

Hi everyone,

The 38th edition of Git Rev News is now published:

  https://git.github.io/rev_news/2018/04/18/edition-38/

Thanks a lot to all the contributors: Jiang Xin, Jacob Keller, Luca
Milanesio, Sergey Organov and Kaartic Sivaraam!

Enjoy,
Christian, Jakub, Markus and Gabriel.

Re: [RFC 01/10] submodule: add 'core.submodulesFile' to override the '.gitmodules' path

[Antonio Ospite]

On Mon, 16 Apr 2018 14:22:35 -0700
Stefan Beller  wrote:

> On Mon, Apr 16, 2018 at 9:37 AM, Antonio Ospite  wrote:
> > On Thu, 12 Apr 2018 16:50:03 -0700
> > Stefan Beller  wrote:
> >
> >> Hi Antonio,
> >>
> >> On Thu, Apr 12, 2018 at 3:20 PM, Antonio Ospite  wrote:
> >> > When multiple repositories with detached work-trees take turns using the
> >> > same directory as their work-tree, and more than one of them want to use
> >> > submodules, there will be conflicts about the '.gitmodules' file.
> >>
> >> unlike other files which would not conflict?
> >> There might be file names such as LICENSE, Readme.md etc,
> >> which are common enough that they would produce conflicts as well?
> >> I find this argument on its own rather weak. ("Just delete everything in
> >> the working dir before using it with another repository"). I might be
> >> missing a crucial bit here?
> >>
> >
> > All the vcsh repositories _share_ the same work-tree; they may control
> > it taking turns but, in general, all files are meant to be checked out
> > at all times as the basic use case is: *distinct* sets of config files.
> >
> > Maybe saying that the repositories "take turns" is confusing.
> > It's an unnecessary information, so I will omit that part form the
> > commit message.
> 
> So they all have the same workdir, do they track the same set of files
> or do they track a disjoint set of files, and ignoring the other repositories
> files via the ignore mechanism?
>

To recap,

vcsh[1] sets $HOME as the work-tree of multiple repositories to track
different sets of dotfiles in distinct repositories, while still having
the files directly available in $HOME. Each repository can ignore
untracked files via the ignore mechanism (namely core.excludesFile).

[1] https://github.com/RichiH/vcsh

For all this to work well, the sets of the tracked files would also need
to be disjoint, and usually they "practically" are, once a few
exceptions are taken care of.

Common intersecting items like LICENSE and README can be handled via
sparse-checkout to have "disjoint checkouts" and this solves most of
the problems, but the same mechanism cannot be used for .gitmodules as
it needs to be checked out.

And the problem cannot be worked around like done with .gitignore
(using core.excludesFile instead) because .gitmodules is unique and
hardcoded.

> This sounds like an interesting setup. I never though of that as something
> useful (in either configuration).
>

Give vcsh a try maybe.

[...]
> > However I guess that my point here is that the gitmodules file is
> > something that influences git behavior so it should not be on the user's
> > shoulder to manage conflicts for it, and most importantly it needs to
> > be checked out for git to access it, doesn't it?
> 
> Good point! I wonder if the cleaner solution would be to just
> tell git to use HEAD:.gitmodules and not check out the file?
> then you would not need to come up with a namespace for names
> of the .gitmodules files and scatter them into the worktree as well?
>

Any solution which:

  1. prevents the gitmodules file to be checked out
  2. but still tracks it in the git repository

OR
  
  1. allows to set the gitmoudles file under some namespace

would work for vcsh I guess.

> 
> >> > -   value=$(git config -f .gitmodules 
> >> > submodule."$name"."$option")
> >> > +   gitmodules_file=$(git config core.submodulesfile)
> >> > +   : ${gitmodules_file:=.gitmodules}
> >> > +   value=$(git config -f "$gitmodules_file" 
> >> > submodule."$name"."$option")
> >>
> >> I wonder if it would be cheaper to write a special config lookup now, e.g.
> >> in builtin/submodule--helper.c we could have a "config-from-gitmodules"
> >> subcommand that is looking up the modules file and then running the config
> >> on that file.
> >>
> >
> > Can you give an example from the user point of view of such a
> > "config-from-gitmodules" command?
> >
> 
> git submodule config  
> 
> as an 'alias' for
> 
>gitmodules_file=$(git config core.submodulesfile)
>: ${gitmodules_file:=.gitmodules}
>value=$(git config -f "$gitmodules_file"
> submodule."$name"."$option")
> 
> The helper would figure out which config file to load form
> (.gitmodules in tree, HEAD:.gitmodules, your new proposed gitmodules file,
> .git/config... or the special ref) and then return the  for 
> 
> So maybe:
> 
> $ git clone https://gerrit.googlesource.com/gerrit && cd gerrit
> # ^ My goto-repo with submodules
> 
> $ git submodule config "plugins/hooks" URL
> ../plugins/hooks
> 
>

I may look into such supporting changes once you decide the approach to
take for the bigger problem.

Thank you,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See 

Re: [RFC PATCH] ident: don't cache default date

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 18 2018, Phillip Wood wrote:

> From: Phillip Wood 
>
> Now that the sequencer commits without forking when the commit message
> isn't edited all the commits that are picked have the same committer
> date. If a commit is reworded it's committer date will be a later time

s/it's/its/

> as it is created by running an separate instance of 'git commit'.  If
> the reworded commit is follow by further picks, those later commits
> will have an earlier committer date than the reworded one. This is
> caused by git caching the default date used when GIT_COMMITTER_DATE is
> not set. Fix this by not caching the date.
>
> Users expect commits to have the same author and committer dates when
> the don't explicitly set them. As the date is now updated each time
> git_author_info() or git_committer_info() is run it is possible to end
> up with different author and committer dates. Fix this for
> 'commit-tree', 'notes' and 'merge' by using a single date in
> commit_tree_extended() and passing it explicitly to the new functions
> git_author_info_with_date() and git_committer_info_with_date() when
> neither the author date nor the committer date are explicitly
> set. 'commit' always passes the author date to commit_tree_extended()
> and relied on the date caching to have the same committer and author
> dates when neither was specified. Fix this by setting
> GIT_COMMITTER_DATE to be the same as the author date passed to
> commit_tree_extended().
>
> Signed-off-by: Phillip Wood 
> Reported-by: Johannes Sixt 
> ---
>
> I'm slightly nervous that setting GIT_COMMITTER_DATE in
> builtin/commit.c will break someone's hook script. Maybe it would be
> better to add a committer parameter to commit_tree() and
> commit_tree_extended().
>
> This needs some tests. I think we could test that the sequencer gives
> each commit a new date with 'git rebase -i --exec="sleep 2"
> --force-rebase @~2' and test the committer dates of the rebased
> commits. I'm not sure if test -gt can cope with numbers that big
> though, maybe we'll have to be content with test !=. For 'git commit'
> I think doing GIT_EDITOR='sleep 2; echo message >"$1"' and checking
> the committer date and author date will work as the author date is set
> before the user edits the commit message. I'm not sure how to test
> callers of commit_tree() though (that's commit-tree, notes and merge)
> as I've not been able to come up with anything that will guarantee the
> author and committer dates are different if the code in
> commit_tree_extended() that ensures they are the same gets broken.

The behavior you're describing where we end up with later commits in the
rebase with earlier CommitDate's definitely sounds like a minor
regression, and yeah we should have tests for this.

My mental model of this is that we shouldn't be trying at all to adjust
the CommitDate in a sequence to be the same, and just make it be
whatever time() returns, except in the case where a date is passed
explicitly.

My cursory reading of this RFC patch is that it's definitely an
improvement because we don't end up with things out of order, but is
there any reason for why we should be trying to aim to keep this
"consistent" within a single "git rebase" command by default, as opposed
to always just writing the current CommitDate at the time we make the
commit, that sounds like the most intuitive thing to me, and I can't see
any downsides with that.

It leaks info about how long it took someone to do the rebase, but so
what?

Re: [BUG] git p4 clone fails when p4 sizes does not return 'fileSize' key

[Luke Diamand]

On 17 April 2018 at 20:12, Thandesha VK  wrote:
> I have few cases where even p4 -G sizes (or p4 sizes) is not returning
> the size value even with latest version of p4 (17.2). In that case, we
> have to regenerate the digest for file save it - It mean something is
> wrong with the file in perforce.

Just to be clear - git-p4 does not use the p4 "sizes" command anywhere AFAIK.

We are just talking about the output from "p4 print" and the
"fileSize" key, right?

Does that happen with the 17.2 version of p4?

> Regarding, sys.stdout.write v/s print, I see script using both of them
> without a common pattern. I can change it to whatever is more
> appropriate.

print() probably makes more sense; can we try to use the function form
so that we don't deliberately make the path to python3 harder (albeit
in a very tiny way).

>
> On Tue, Apr 17, 2018 at 11:47 AM, Mazo, Andrey  wrote:
>> Does a missing "fileSize" actually mean that there is something wrong with 
>> the file?
>> Because, for me, `p4 -G print` doesn't print "fileSize" for _any_ file.
>> (which I attribute to our rather ancient (2007.2) Perforce server)
>> I'm not an expert in Perforce, so don't know for sure.

My 2015 version of p4d reports a fileSize.

>>
>> However, `p4 -G sizes` works fine even with our p4 server.
>> Should we then go one step further and use `p4 -G sizes` to obtain the 
>> "fileSize" when it's not returned by `p4 -G print`?
>> Or is it an overkill for a simple verbose print out?

If your server isn't reporting "fileSize" then there are a few other
places where I would expect git-p4 to also fail.

If we're going to support this very ancient version of p4d, then
gracefully handling a missing fileSize will be useful.

>>
>> Also, please, find one comment inline below.
>>
>> Thank you,
>> Andrey
>>
>> From: Thandesha VK 
>>> Sounds good. How about an enhanced version of fix from both of us.
>>> This will let us know that something is not right with the file but
>>> will not bark
>>>
>>> $ git diff
>>> diff --git a/git-p4.py b/git-p4.py
>>> index 7bb9cadc6..df901976f 100755
>>> --- a/git-p4.py
>>> +++ b/git-p4.py
>>> @@ -2566,7 +2566,12 @@ class P4Sync(Command, P4UserMap):
>>>  relPath = self.stripRepoPath(file['depotFile'], 
>>> self.branchPrefixes)
>>>  relPath = self.encodeWithUTF8(relPath)
>>>  if verbose:
>>> -size = int(self.stream_file['fileSize'])
>>> +if 'fileSize' not in self.stream_file:
>>> +   print "WARN: File size from perforce unknown. Please verify 
>>> by p4 sizes %s" %(file['depotFile'])
>> For whatever reason, the code below uses sys.stdout.write() instead of 
>> print().
>> Should it be used here for consistency as well?
>>
>>> +   size = "-1"
>>> +else:
>>> +   size = self.stream_file['fileSize']
>>> +size = int(size)
>>>  sys.stdout.write('\r%s --> %s (%i MB)\n' %
>>> (file['depotFile'], relPath, size/1024/1024))
>>>  sys.stdout.flush()
>>>
>>>
>>> On Tue, Apr 17, 2018 at 10:33 AM, Mazo, Andrey  wrote:
 Sure, I totally agree.
 Sorry, I just wasn't clear enough in my previous email.
 I meant that your patch suppresses "%s --> %s (%i MB)" line in case 
 "fileSize" is not available,
 while my patch suppresses just "(%i MB)" portion if the "fileSize" is not 
 known.
 In other words,
  * if "fileSize" is known:
  ** both yours and mine patches don't change existing behavior;
  * if "fileSize" is not known:
  ** your patch makes streamOneP4File() not print anything;
  ** my patch makes streamOneP4File() print "%s --> %s".

 Hope, I'm clearer this time.

 Thank you,
 Andrey

 From: Thandesha VK 
> *I* think keeping the filesize info is better with --verbose option as
> that gives some clue about the file we are working on. What do you
> think?
> Script has similar checks of key existence at other places where it is
> looking for fileSize.
>
> On Tue, Apr 17, 2018 at 9:22 AM, Andrey Mazo  wrote:
>> Huh, I actually have a slightly different fix for the same issue.
>> It doesn't suppress the corresponding verbose output completely, but 
>> just removes the size information from it.
>>
>> Also, I'd mention that the workaround is trivial -- simply omit the 
>> "--verbose" option.
>>
>> Andrey Mazo (1):
>>   git-p4: fix `sync --verbose` traceback due to 'fileSize'
>>
>>  git-p4.py | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>

Thanks
Luke

[RFC PATCH] ident: don't cache default date

[Phillip Wood]

From: Phillip Wood 

Now that the sequencer commits without forking when the commit message
isn't edited all the commits that are picked have the same committer
date. If a commit is reworded it's committer date will be a later time
as it is created by running an separate instance of 'git commit'.  If
the reworded commit is follow by further picks, those later commits
will have an earlier committer date than the reworded one. This is
caused by git caching the default date used when GIT_COMMITTER_DATE is
not set. Fix this by not caching the date.

Users expect commits to have the same author and committer dates when
the don't explicitly set them. As the date is now updated each time
git_author_info() or git_committer_info() is run it is possible to end
up with different author and committer dates. Fix this for
'commit-tree', 'notes' and 'merge' by using a single date in
commit_tree_extended() and passing it explicitly to the new functions
git_author_info_with_date() and git_committer_info_with_date() when
neither the author date nor the committer date are explicitly
set. 'commit' always passes the author date to commit_tree_extended()
and relied on the date caching to have the same committer and author
dates when neither was specified. Fix this by setting
GIT_COMMITTER_DATE to be the same as the author date passed to
commit_tree_extended().

Signed-off-by: Phillip Wood 
Reported-by: Johannes Sixt 
---

I'm slightly nervous that setting GIT_COMMITTER_DATE in
builtin/commit.c will break someone's hook script. Maybe it would be
better to add a committer parameter to commit_tree() and
commit_tree_extended().

This needs some tests. I think we could test that the sequencer gives
each commit a new date with 'git rebase -i --exec="sleep 2"
--force-rebase @~2' and test the committer dates of the rebased
commits. I'm not sure if test -gt can cope with numbers that big
though, maybe we'll have to be content with test !=. For 'git commit'
I think doing GIT_EDITOR='sleep 2; echo message >"$1"' and checking
the committer date and author date will work as the author date is set
before the user edits the commit message. I'm not sure how to test
callers of commit_tree() though (that's commit-tree, notes and merge)
as I've not been able to come up with anything that will guarantee the
author and committer dates are different if the code in
commit_tree_extended() that ensures they are the same gets broken.

 builtin/commit.c |  8 
 cache.h  |  2 ++
 commit.c | 22 +++---
 ident.c  | 24 ++--
 4 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab0..4ad8317f88 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -584,6 +584,14 @@ static void determine_author_info(struct strbuf 
*author_ident)
export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0);
export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0);
export_one("GIT_AUTHOR_DATE", author.date_begin, author.tz_end, '@');
+   /*
+* Ensure the author and committer dates are identical if nither is
+* explicitly set
+*/
+   if ((!date || !*date) && (!getenv("GIT_COMMITTER_DATE")
+ || !*getenv("GIT_COMMITTER_DATE")))
+   export_one("GIT_COMMITTER_DATE", author.date_begin,
+  author.tz_end, '@');
free(name);
free(email);
free(date);
diff --git a/cache.h b/cache.h
index a61b2d3f0d..9cde499507 100644
--- a/cache.h
+++ b/cache.h
@@ -1484,7 +1484,9 @@ int date_overflows(timestamp_t date);
 #define IDENT_NO_DATE 2
 #define IDENT_NO_NAME 4
 extern const char *git_author_info(int);
+extern const char *git_author_info_with_date(int, const char*);
 extern const char *git_committer_info(int);
+extern const char *git_committer_info_with_date(int, const char*);
 extern const char *fmt_ident(const char *name, const char *email, const char 
*date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_name(void);
diff --git a/commit.c b/commit.c
index 00c99c7272..457cc8b71b 100644
--- a/commit.c
+++ b/commit.c
@@ -1513,6 +1513,7 @@ int commit_tree_extended(const char *msg, size_t msg_len,
 const char *author, const char *sign_commit,
 struct commit_extra_header *extra)
 {
+   struct strbuf date_buf = STRBUF_INIT;
int result;
int encoding_is_utf8;
struct strbuf buffer;
@@ -1540,10 +1541,25 @@ int commit_tree_extended(const char *msg, size_t 
msg_len,
}
 
/* Person/date information */
-   if (!author)
-   author = git_author_info(IDENT_STRICT);
+   if (!author) {
+   char *author_date = xstrdup_or_null(getenv("GIT_AUTHOR_DATE"));
+

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Phillip Wood]

On 16/04/18 06:56, Johannes Sixt wrote:
> 
> Am 15.04.2018 um 23:35 schrieb Junio C Hamano:
>> Ah, do you mean we have an internal sequence like this, when "rebase
>> --continue" wants to conclude an edit/reword?
> 
> Yes, it's only 'reword' that is affected, because then subsequent picks
> are processed by the original process.
> 
>>   - we figure out the committer ident, which grabs a timestamp and
>>     cache it;
>>
>>   - we spawn "commit" to conclude the stopped step, letting it record
>>     its beginning time (which is a bit older than the above) or its
>>     ending time (which is much older due to human typing speed);
> 
> Younger in both cases, of course. According to my tests, we seem to pick
> the beginning time, because the first 'reword'ed commit typically has
> the same timestamp as the preceding picks. Later 'reword'ed commits have
> noticably younger timestamps.
> 
>>   - subsequent "picks" are made in the same process, and share the
>>     timestamp we grabbed in the first step, which is older than the
>>     second one.
>>
>> I guess we'd want a mechanism to tell ident.c layer "discard the
>> cached one, as we are no longer in the same automated sequence", and
>> use that whenever we spawn an editor (or otherwise go interactive).
> 
> Frankly, I think that this caching is overengineered (or prematurly
> optimized). If the design requires that different callers of datestamp()
> must see the same time, then the design is broken. In a fixed design,
> there would be a single call of datestamp() in advance, and then the
> timestamp, which then obviously is a very important piece of data, would
> be passed along as required.

I'm inclined to agree, though it creates complications if we're going to
keep giving commits the same author and committer dates when neither is
explicitly specified.

Best Wishes

Phillip

> 
> -- Hannes

getur þú svarað mér

[Katie Higgins]

Re: [PATCH 0/3] completion: improvements for git stash

[Junio C Hamano]

Thomas Gummerer  writes:

> In this series we stop completing the 'git stash save' subcommand in
> git-completion.bash.  The command has been deprecated for a while,...

Anything deprecated in Oct 2017 is still too young in Git's
timescale, but this is the right thing to do in the longer term.

Regarding [2/3], I think 

 - It is perfectly fine to stop offering "save" among the choices
   when "git stash " is given, so that we AVOID actively
   suggesting "save" to those who do not know (or do not want to
   learn) about it.  Instead we would knudge them towards "push".
   After all, that is what "deprecation" is all about.

 - It also is OK to limit the offering to "show" against "git stash
   s", even though the user _could_ have meant "save" than the
   above case with totally empty subcommand name.

 - It is questionable to stop offering "save" to "git stash
   sav" it is clear that the user wants to say "save" in this
   case, as there is no other subcommand the user could have meant.

 - It is outright hostile to the end user if we stop completing "git
   stash save --q" with "--quiet".  At that point, we _know_
   that the user wants "save", and getting in the way of those who
   know what they are using does not feel quite right.

Of course, being more accomodating to existing users by taking the
last two points above seriously would raise a separate issue of when
we stop doing so, and if we should start doing something else.  If
there is a way to implement a "reluctant completion" that gives
"save" as a completion choice while giving a deprecation warning to
let the user know of the plan for removal of "save", that would be
good, for example.

Thanks.

I am waiting to hear from you soon,

[Mabel Raymond]

From
The international Investigation Financial Crime  Unit

Attention Beneficiary
We need your urgent information .

I am writing to inform you that I am Staff In the international
investigation financial crime  unit and  I have discovered through our
network E-mail  system This Week  that your  Inheritance Claim  Fund
$6.5m usd have been trapped by Some Bank Officers and  who specialized
in collecting money from You.

There fore I would appreciate to inform you that there  
is hope for you to recover all what you have lost . Your  Inheritance
Fund is $6.5m usd  Have Been Removed From That Bank And  Deposited To
Security Financier Company This Morning In Burkina Faso.

You should send to us your complete  information to enable This Office
Submit Them   on your behalf  To The Security And Finance Company To
Enable The Finance Company Proceed Transferring Your Inheritance Fund
$6.5m usd to your country!  This will be completed within the  next
few  days .

1.Your complete name

2.Your Mobile phone number

3. Your nationality

 I am waiting to hear from you soon,

Thank you  .

Mrs Raymond  Mabel

Re: [PATCH] send-email: avoid duplicate In-Reply-To/References

[Stefan Agner]

On 18.04.2018 02:54, Eric Wong wrote:
> Stefan Agner  wrote:
>> This addresses the issue reported here:
>> https://public-inbox.org/git/997160314bbafb3088a401f1c09cc...@agner.ch/
> 
> Thanks for bringing this up.
> 
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1642,10 +1642,15 @@ foreach my $t (@files) {
>>  elsif (/^Content-Transfer-Encoding: (.*)/i) {
>>  $xfer_encoding = $1 if not defined 
>> $xfer_encoding;
>>  }
>> +elsif (/^In-Reply-To: (.*)/i) {
>> +$in_reply_to = $1;
>> +}
>> +elsif (/^References: (.*)/i) {
>> +$references = $1;
>> +}
> 
> References: can span multiple lines with --thread=deep in format-patch
> (technically any header can be, but References: is common)

I think that is ok because we do
# First unfold multiline header fields

...

A quick test with 3 patches in --thread=deep mode looks good:
In-Reply-To:
<87d48c04aae0594ebea7567827d08979ad346380.1524034203.git.ste...@agner.ch>
References:
<06ea66574abfb2dd66adee9996e5fb66903b95a3.1524034203.git.ste...@agner.ch>
<87d48c04aae0594ebea7567827d08979ad346380.1524034203.git.ste...@agner.ch>

--
Stefan

Re: [PATCH/RFC 0/5] Keep all info in command-list.txt in git binary

[Philip Oakley]

From: "Philip Oakley"  : Tuesday, April 17, 2018 11:47
PM

From: "Duy Nguyen"  : Tuesday, April 17, 2018 5:48 PM

On Tue, Apr 17, 2018 at 06:24:41PM +0200, Duy Nguyen wrote:

On Sun, Apr 15, 2018 at 11:21 PM, Philip Oakley 
wrote:
> From: "Duy Nguyen"  : Saturday, April 14, 2018 4:44
> PM
>
>> On Thu, Apr 12, 2018 at 12:06 AM, Philip Oakley
>> 
>> wrote:
>>>
>>> I'm only just catching up, but does/can this series also capture the
>>> non-command guides that are available in git so that the 'git
>>> help -g'
>>> can
>>> begin to list them all?
>>
>>
>> It currently does not. But I don't see why it should not. This should
>> allow git.txt to list all the guides too, for people who skip "git
>> help" and go hard core mode with "man git". Thanks for bringing this
>> up.
>> --
>> Duy
>>
> Is that something I should add to my todo to add a 'guide' category
> etc.?

I added it too [1]. Not sure if you want anything more on top though.


What I've seen is looking good - I've not had as much time as I'd like..

I'm not sure of the status of the git/generate-cmdlist.sh though. Should
that also be updated, or did I miss that?
--
Philip


I may be miss-remembering the order that the `git help` determines the list
of commands and guides. There was at least one place where the list of
commands was generated programatically that I may be confused with (I've not
had time to delve into the code :-(
--






The "anything more" that at least I had in mind was something like
this. Though I'm not sure if it's a good thing to replace a hand
crafted section with an automatedly generated one. This patch on top
combines the "SEE ALSO" and "FURTHER DOCUMENT" into one with most of
documents/guides are extracted from command-list.txt

-- 8< --
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 6232143cb9..3e0ecd2e11 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -292,6 +292,7 @@ doc.dep : $(docdep_prereqs) $(wildcard *.txt)
build-docdep.perl

cmds_txt = cmds-ancillaryinterrogators.txt \
 cmds-ancillarymanipulators.txt \
+ cmds-guide.txt \
 cmds-mainporcelain.txt \
 cmds-plumbinginterrogators.txt \
 cmds-plumbingmanipulators.txt \
diff --git a/Documentation/cmd-list.perl b/Documentation/cmd-list.perl
index 5aa73cfe45..e158bd9b96 100755
--- a/Documentation/cmd-list.perl
+++ b/Documentation/cmd-list.perl
@@ -54,6 +54,7 @@ for (sort <>) {

for my $cat (qw(ancillaryinterrogators
 ancillarymanipulators
+ guide
 mainporcelain
 plumbinginterrogators
 plumbingmanipulators
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..d60d2ae0c7 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -808,29 +808,6 @@ The index is also capable of storing multiple
entries (called "stages")
for a given pathname.  These stages are used to hold the various
unmerged version of a file when a merge is in progress.

-FURTHER DOCUMENTATION
--
-
-See the references in the "description" section to get started
-using Git.  The following is probably more detail than necessary
-for a first-time user.
-
-The link:user-manual.html#git-concepts[Git concepts chapter of the
-user-manual] and linkgit:gitcore-tutorial[7] both provide
-introductions to the underlying Git architecture.
-
-See linkgit:gitworkflows[7] for an overview of recommended workflows.
-
-See also the link:howto-index.html[howto] documents for some useful
-examples.
-
-The internals are documented in the
-link:technical/api-index.html[Git API documentation].
-
-Users migrating from CVS may also want to
-read linkgit:gitcvs-migration[7].
-
-
Authors
---
Git was started by Linus Torvalds, and is currently maintained by Junio
@@ -854,11 +831,16 @@ the Git Security mailing list
.

SEE ALSO

-linkgit:gittutorial[7], linkgit:gittutorial-2[7],
-linkgit:giteveryday[7], linkgit:gitcvs-migration[7],
-linkgit:gitglossary[7], linkgit:gitcore-tutorial[7],
-linkgit:gitcli[7], link:user-manual.html[The Git User's Manual],
-linkgit:gitworkflows[7]
+
+See the references in the "description" section to get started
+using Git.  The following is probably more detail than necessary
+for a first-time user.
+
+include::cmds-guide.txt[]
+
+See also the link:howto-index.html[howto] documents for some useful
+examples. The internals are documented in the
+link:technical/api-index.html[Git API documentation].

GIT
---
diff --git a/command-list.txt b/command-list.txt
index 1835f1a928..f26b8acd52 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -150,10 +150,14 @@ git-whatchanged ancillaryinterrogators
git-worktreemainporcelain
git-write-tree  plumbingmanipulators
gitattributes   guide
+gitcvs-migrationguide
+gitcli  guide
+gitcore-tutorial