Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[Johannes Sixt]

Am 05.08.2016 um 07:36 schrieb Johannes Sixt:

Am 05.08.2016 um 00:39 schrieb Junio C Hamano:

@@ -955,12 +955,10 @@ void **nedpindependent_comalloc(nedpool *p,
size_t elems, size_t *sizes, void **
  */
 char *strdup(const char *s1)
 {
-char *s2 = 0;
-if (s1) {
-size_t len = strlen(s1) + 1;
-s2 = malloc(len);
+size_t len = strlen(s1) + 1;
+s2 = malloc(len);
+if (s1)


It does not make sense to check s1 for NULL when it was passed to
strlen() earlier; strlen() does not accept NULL, either...


Oh! This is a typo. You meant to check s2 for NULL.

And the declaration for s2 should remain, of course.




 memcpy(s2, s1, len);
-}
 return s2;
 }
 #endif


-- Hannes

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

Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[Johannes Sixt]

Am 05.08.2016 um 00:39 schrieb Junio C Hamano:

@@ -955,12 +955,10 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems, 
size_t *sizes, void **
  */
 char *strdup(const char *s1)
 {
-   char *s2 = 0;
-   if (s1) {
-   size_t len = strlen(s1) + 1;
-   s2 = malloc(len);
+   size_t len = strlen(s1) + 1;
+   s2 = malloc(len);
+   if (s1)


It does not make sense to check s1 for NULL when it was passed to 
strlen() earlier; strlen() does not accept NULL, either...



memcpy(s2, s1, len);
-   }
return s2;
 }
 #endif


-- Hannes

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

Re: storing cover letter of a patch series?

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 11:39:49AM -0700, Junio C Hamano wrote:
> The problem with "empty commit trick" is that it is a commit whose
> sole purpose is to describe the series, and its presence makes it
> clear where the series ends, but the topology does not tell where
> the series begins, so it is an unsatisifactory half-measure.

Actually, when using topic branches the series always ends at head, so
it's better to keep the empty commit where series begins.

This was actually suggested by Philip Oakley on this thread
but I'm not sure it was noticed as it was part of a bigger email.

It also maps much better to git am uses - you apply patch 0/N first to
create the empty commit, then the rest of the patches.

This does mean you need to use git rebase to edit that cover
commit, but maybe that is not a big deal, and git rebase could
learn --cover to find and edit that.
-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 02:03:48PM -0700, Jacob Keller wrote:
> On Thu, Sep 10, 2015 at 1:09 PM, Philip Oakley  wrote:
> > From: "Jacob Keller" 
> >>
> >> On Thu, Sep 10, 2015 at 11:44 AM, Junio C Hamano 
> >> wrote:
> >>>
> >>> Jacob Keller  writes:
> >>>
>  I hadn't thought of separating the cover letter from git-send-email.
>  That would be suitable for me.
> >>>
> >>>
> >>> Yeah, I said this number of times over time, and I said it once
> >>> recently in another thread, but I think it was a mistake to allow
> >>> git-send-email to drive format-patch.  It may appear that it will
> >>> make things convenient in the perfect world where no user makes
> >>> mistakes, but people are not perfect in real life.  Expecting them
> >>> to be is being naive.
> >>>
> >>
> >> Yep. I didn't even know cover-letter was an option of format-patch
> >> only thought it was in send-email.
> >>
> > Actually, the one feature I'd like (I think) is to be able to join together
> > the empty commit mechanism and the cover letter mechanism within format
> > patch so that:
> >
> > * the empty commit message would detected and automatically become the [0/N]
> > in the patch series (without need to say --cover-letter)
> >
> > * the cover letter would still have some 'template' markings to say "***
> > insert what's changed here***" or smilar (with option to exclude them).
> >
> > That way, when starting a series / branch, the first item would be to add
> > the explanatory 'empty commit' that states the requirements of what one
> > hopes to achieve (a key cover letter content), which is then followed by
> > commits that move toward that goal.
> >
> > The series can then be rebased as the user develops the code, and that cover
> > note can be edited as required during the rebase.
> >
> > When it comes time to show it to the list, the format patch will *know* from
> > the empty commit that it is the [0/N] cover letter and (perhaps -option) add
> > the appropriate markers ready for editing.

And perhaps git am could learn an option to apply 0/N
as a cover commit.

> > The user edits the cover letter with the extra 'what's changed' / interdiff
> > / whatever, and sends. sendmail barfs if the user hasn't edited the markers.
> >
> > This could also work with the sendmail patch formating (though I've never
> > used that workflow) as now the cover letter becomes automatic for the
> > upstream.
> >
> > Philip
> 
> If there was a way to store this empty commit message tagged as "cover
> letter" that could work well, though generally I prefer the
> non-fast-forward merges as this shows you where the series ended *and*
> began. It's somewhat confusing to newer users.. and this doesn't get
> rebased very well either.
> 
> Some way to indicate a particular "empty" commit is actually a cover
> letter seems easy enough. This seems like the way that I was thinking.

Start the subject with "cover! "?
I have a patch that teaches git-rebase to keep empty commits
where the subject has a given prefix, that might be helpful there.


> Using "edit description" of git-branch seems also to be pretty
> effective for this, even if it doesn't get shared across remotes. (not
> really a necessary feature for what I do).
> 
> But having some way to indicate "cover letter" which gets used as the
> beginning of a log message when doing a particular "merge
> --tip-as-cover" or something like Junio suggested above seems like the
> nicest approach.
> 
> Regards,
> Jake
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Mike Hommey]

On Thu, Aug 04, 2016 at 04:32:02PM -0700, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > On Thu, Aug 04, 2016 at 03:28:55PM -0700, Junio C Hamano wrote:
> >> * mh/connect (2016-06-06) 10 commits
> >>  - connect: [host:port] is legacy for ssh
> >>  ...
> >>  - connect: document why we sometimes call get_port after get_host_and_port
> >> 
> >>  Rewrite Git-URL parsing routine (hopefully) without changing any
> >>  behaviour.
> >> 
> >>  It has been two months without any support.  We may want to discard
> >>  this.
> >
> > What kind of support are you expecting?
> 
> The only rationale I recall you justifying this series was that this
> makes the resulting code easier to read, but I do not recall other
> people agreeing with you, and I do not particularly see the
> resulting code easier to follow.

FWIW, IIRC, Torsten agreed it did.

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

Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Eric Wong]

Junio C Hamano  wrote:
> [Graduated to "master"]

> * ew/http-walker (2016-07-18) 4 commits
>   (merged to 'next' on 2016-07-18 at a430a97)
>  + list: avoid incompatibility with *BSD sys/queue.h
>   (merged to 'next' on 2016-07-13 at 8585c03)
>  + http-walker: reduce O(n) ops with doubly-linked list

Yay!  This finally introduces the Linux kernel linked list
into git.  I'm not sure if it's worth the effort to introduce
cleanup commits to start using it in places where we already
have doubly-linked list implementations:

(+Cc Nicolas and Lukas)
* sha1_file.c delta_base_cache_lru is open codes this
* builtin/pack-redundant.c could probably be adapted, too
 ... any more?

And there may be other places where we have performance problems
walking singly-linked lists and would be better off on a
doubly-linked one (or even just readability ones).



>  cf. 
>  cf. 

Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Junio C Hamano]

"Philip Oakley"  writes:

>> Updates in 4/8 ("give headings") is reported to break formatting?
>> cf. <57913c97.1030...@xiplink.com>
>
> Just to say I haven't forgotten.

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

Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Junio C Hamano]

Mike Hommey  writes:

> On Thu, Aug 04, 2016 at 03:28:55PM -0700, Junio C Hamano wrote:
>> * mh/connect (2016-06-06) 10 commits
>>  - connect: [host:port] is legacy for ssh
>>  ...
>>  - connect: document why we sometimes call get_port after get_host_and_port
>> 
>>  Rewrite Git-URL parsing routine (hopefully) without changing any
>>  behaviour.
>> 
>>  It has been two months without any support.  We may want to discard
>>  this.
>
> What kind of support are you expecting?

The only rationale I recall you justifying this series was that this
makes the resulting code easier to read, but I do not recall other
people agreeing with you, and I do not particularly see the
resulting code easier to follow.

> FWIW, I have WIP cleaning up the code further, tha obviously depends on
> this series.

As this is not even in 'next', your cleaning-up does not have to
depend on it.  It can be part of a reroll, of course.

By the way, "discarding" is not equal to "rejecting".  The latter is
"it is a bad thing to do, don't even come back with a new version".
It is just "This hasn't made any progress, and it is not ready for
'next', either. Keeping it in 'pu' is eating my time without giving
much benefit to the project at this point".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git bisect for reachable commits only

[Oleg Taranenko]

On Tue, Aug 2, 2016 at 11:00 PM, Junio C Hamano  wrote:
> Oleg Taranenko  writes:
>
>> First, assuming the common ancestor is GOOD based on the fact that
>> some descendant given as GOOD is pretty bad idea.
>
> What you claim is fundamentally incompatible with the way "bisect"
> works as a O(log(n)) operation.  It is likely that your definition
> of Good for the purpose of your bug-hunting needs to be rethought if
> you want to take advantage of "bisect".

Without context it sounds a bit silly, agree. Context was, maybe not
explicit stated, based on previous discussion:

If we looking in direct path G..B, of course bisect should show its
power O(log(n));
BUT, assuming that any predecessor (G~1/G~2...)...is good if this
commit G~n has direct path to B,  but not via G, (as git does now) is
wrong.
This is my concern. Ever G~1 may have not feature we are looking for,
then we must treated it as BAD in current git bisect session. To be
sure we require additional evidence and just opening a new bisect
roundrips in case G~n is GOOD.
If G~n confirmed as BAD, we need to stop looking in this path (no need
to find transition BAG -> BAD) and switch to another possible common
ancestor (or next octopus parent)
In merge-based development (opposite to rebased one) this can happen very easy


>> I have another request to get git bisect more user-friendly, regarding
>> rolling back last step or steps, if accidentally 'git bisect bad' or
>> 'good' was wrong entered, but I think it worth for another thread.
>
> Are you aware that you can check $GIT_DIR/BISECT_LOG and replay it
> to recreate any previous state of the bisection?  That would
> probably help.

git bisect log / replay is not convenient. First we need to find place
where to keep log file (not forget its name), then edit it. If I'm not
sure how many steps I did a mistake the troubles doubles,
What are obstacles to implement git bisect back ?
or git bisect back --steps=2
I don't think it will be significant change in code.

It would be a great help especially if hunting in multiply logically
loose-coupled repos. Say searching bug in application, possible caused
elusive changes on several dependent libraries.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Philip Oakley]

From: "Junio C Hamano" 



* po/range-doc (2016-07-20) 8 commits
- doc: revisions - clarify reachability examples
- doc: revisions - define `reachable`
- doc: gitrevisions - clarify 'latter case' is revision walk
- doc: gitrevisions - use 'reachable' in page description
- doc: give headings for the two and three dot notations
- doc: show the actual left, right, and boundary marks
- doc: revisions - name the left and right sides
- doc: use 'symmetric difference' consistently

Clarify various ways to specify the "revision ranges" in the
documentation.

Updates in 4/8 ("give headings") is reported to break formatting?
cf. <57913c97.1030...@xiplink.com>




Just to say I haven't forgotten.

The format breakage was analysed by Jeff (12 July 2016 23:12) and looks to 
be internal to the man viewer.


There's still a suggestion surrounding one of the headings to sort (which 
has gone around in circles;-).


Also I now think I have a sensible idea for the ^@/! examples (i.e. use 
the Loeliger illustration) - that'd make if 9 in the series (was originally 
2).


Plus ensure all comments across V1-4 have been attended to (including yours 
of 12 July 2016 18:04).


--
Philip 


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

Re: What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Mike Hommey]

On Thu, Aug 04, 2016 at 03:28:55PM -0700, Junio C Hamano wrote:
> * mh/connect (2016-06-06) 10 commits
>  - connect: [host:port] is legacy for ssh
>  - connect: move ssh command line preparation to a separate function
>  - connect: actively reject git:// urls with a user part
>  - connect: change the --diag-url output to separate user and host
>  - connect: make parse_connect_url() return the user part of the url as a 
> separate value
>  - connect: group CONNECT_DIAG_URL handling code
>  - connect: make parse_connect_url() return separated host and port
>  - connect: re-derive a host:port string from the separate host and port 
> variables
>  - connect: call get_host_and_port() earlier
>  - connect: document why we sometimes call get_port after get_host_and_port
> 
>  Rewrite Git-URL parsing routine (hopefully) without changing any
>  behaviour.
> 
>  It has been two months without any support.  We may want to discard
>  this.

What kind of support are you expecting?

FWIW, I have WIP cleaning up the code further, tha obviously depends on
this series.

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

Re: [ANNOUNCE] git-series: track changes to a patch series over time

[Josh Triplett]

On Wed, Aug 03, 2016 at 08:12:02PM +0100, Richard Ipsum wrote:
> On Thu, Jul 28, 2016 at 11:40:55PM -0700, Josh Triplett wrote:
> > I'd welcome any feedback, whether on the interface and workflow, the
> > internals and collaboration, ideas on presenting diffs of patch series,
> > or anything else.
> 
> One other nice thing I've noticed about this tool is the
> way series behave like regular git branches: I specify the name
> of the series and from then on all other commands act on that
> series until told otherwise.

Thanks; I spent a while thinking about that part of the workflow.  I
save the current series as a symbolic ref SHEAD, and everything operates
on SHEAD.  (I should probably add support for running things like "git
series log" or "git series format" on a different series, because right
now "until told otherwise" doesn't include a way to tell it otherwise.)

One fun detail that took a couple of iterations to get right: I keep
separate "staged" and "working" versions per-series, so even with
outstanding changes to the cover letter, base, or series, you can always
detach or checkout another series without losing anything.  If you
switch back, all your staged and unstaged changes will remain staged and
unstaged where you left them.  That solves the "checkout a different
series with modifications to the current series" case.

> git-appraise looks as though it might also have this behaviour.
> I think it's a nice way to do it, since you don't generally
> perform more than one review simultaneously. So I may well
> use this idea in git-candidate if it's okay. :)

By all means.  For a review tool like git-candidate, it seems like you'd
want even more contextual information, to make it easier to specify
things like "comment on file F line L".  For instance, what if you
spawned the diff to review in an editor, with plenty of extra context
and a file extension that'll cause most editors to recognize it as a
patch (and specifically a git-candidate patch to allow specialized
editor modes), and told people to add their comments after the line they
applied to?  When the editor exits successfully, you can scan the file,
detect the added lines, and save those as comments.  You could figure
out the appropriate line by looking for the diff hunk headers and
counting line numbers.

If you use a format-patch diff that includes the headers and commit
message, you could also support commenting on those in the same way.
Does the notedb format support commenting on those?

> I haven't found time to use the tool to do any serious review
> yet, but I'll try and post some more feedback when I do.

Thanks!

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

Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[Junio C Hamano]

Let's try it this way.  How about this as a replacement?

-- >8 --
From: Johannes Schindelin 
Date: Thu, 4 Aug 2016 18:07:08 +0200
Subject: [PATCH] nedmalloc: work around overzealous GCC 6 warning

With GCC 6, the strdup() function is declared with the "nonnull"
attribute, stating that it is not allowed to pass a NULL value as
parameter.

In nedmalloc()'s reimplementation of strdup(), Postel's Law is heeded
and NULL parameters are handled gracefully. GCC 6 complains about that
now because it thinks that NULL cannot be passed to strdup() anyway.

Because the callers in this project of strdup() must be prepared to
call any implementation of strdup() supplied by the platform, so it
is pointless to pretend that it is OK to call it with NULL.

Remove the conditional based on NULL-ness of the input; this
squelches the warning.

See https://gcc.gnu.org/gcc-6/porting_to.html for details.

Signed-off-by: Johannes Schindelin 
Helped-by: René Scharfe 
Signed-off-by: Junio C Hamano 
---
 compat/nedmalloc/nedmalloc.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index 677d1b2..88cd78c 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -955,12 +955,10 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems, 
size_t *sizes, void **
  */
 char *strdup(const char *s1)
 {
-   char *s2 = 0;
-   if (s1) {
-   size_t len = strlen(s1) + 1;
-   s2 = malloc(len);
+   size_t len = strlen(s1) + 1;
+   s2 = malloc(len);
+   if (s1)
memcpy(s2, s1, len);
-   }
return s2;
 }
 #endif
-- 
2.9.2-766-gd7972a8

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

Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[Junio C Hamano]

René Scharfe  writes:

> This version of strdup() is only compiled if nedmalloc is used instead
> of the system allocator.  That means we can't rely on strdup() being
> able to take NULL -- some (most?) platforms won't like it.  Removing
> the NULL check would be a more general and overall easier way out, no?

The callers of this version must be prepared to call a version of
strdup() that does not accept NULL, so in a sense, passing NULL to
this function is already an error in the context of this project.

That sounds like a good rationale to take this more straight-forward
approach.

> But it should check the result of malloc() before copying.
> ---
>  compat/nedmalloc/nedmalloc.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
> index a0a16eb..cc18f0c 100644
> --- a/compat/nedmalloc/nedmalloc.c
> +++ b/compat/nedmalloc/nedmalloc.c
> @@ -955,12 +955,10 @@ void **nedpindependent_comalloc(nedpool *p,
> size_t elems, size_t *sizes, void **
>   */
>  char *strdup(const char *s1)
>  {
> - char *s2 = 0;
> - if (s1) {
> - size_t len = strlen(s1) + 1;
> - s2 = malloc(len);
> + size_t len = strlen(s1) + 1;
> + char *s2 = malloc(len);
> + if (s2)
>   memcpy(s2, s1, len);
> - }
>   return s2;
>  }
>  #endif
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What's cooking in git.git (Aug 2016, #02; Thu, 4)

[Junio C Hamano]

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

Many topics in "Cooking" section that haven't seen much activity
recently have been moved to "Stalled" status and marked as "Will
discard".  This is unfortunate but with way many more people wanting
to throw random new topics than too few people who are able/willing
to review them, it was inevitable.

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

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

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

--
[Graduated to "master"]

* da/subtree-2.9-regression (2016-07-26) 2 commits
  (merged to 'next' on 2016-07-26 at 9d71562)
 + subtree: fix "git subtree split --rejoin"
 + t7900-subtree.sh: fix quoting and broken && chains
 (this branch is used by da/subtree-modernize.)

 "git merge" in Git v2.9 was taught to forbid merging an unrelated
 lines of history by default, but that is exactly the kind of thing
 the "--rejoin" mode of "git subtree" (in contrib/) wants to do.
 "git subtree" has been taught to use the "--allow-unrelated-histories"
 option to override the default.


* ew/http-walker (2016-07-18) 4 commits
  (merged to 'next' on 2016-07-18 at a430a97)
 + list: avoid incompatibility with *BSD sys/queue.h
  (merged to 'next' on 2016-07-13 at 8585c03)
 + http-walker: reduce O(n) ops with doubly-linked list
 + http: avoid disconnecting on 404s for loose objects
 + http-walker: remove unused parameter from fetch_object

 Dumb http transport on the client side has been optimized.


* jc/grep-commandline-vs-configuration (2016-07-25) 1 commit
  (merged to 'next' on 2016-07-28 at dd53273)
 + grep: further simplify setting the pattern type

 "git -c grep.patternType=extended log --basic-regexp" misbehaved
 because the internal API to access the grep machinery was not
 designed well.


* jk/diff-do-not-reuse-wtf-needs-cleaning (2016-07-22) 1 commit
  (merged to 'next' on 2016-07-28 at e3c5190)
 + diff: do not reuse worktree files that need "clean" conversion

 There is an optimization used in "git diff $treeA $treeB" to borrow
 an already checked-out copy in the working tree when it is known to
 be the same as the blob being compared, expecting that open/mmap of
 such a file is faster than reading it from the object store, which
 involves inflating and applying delta.  This however kicked in even
 when the checked-out copy needs to go through the convert-to-git
 conversion (including the clean filter), which defeats the whole
 point of the optimization.  The optimization has been disabled when
 the conversion is necessary.


* jk/git-jump (2016-07-22) 3 commits
  (merged to 'next' on 2016-07-28 at 9ef9398)
 + contrib/git-jump: fix typo in README
 + contrib/git-jump: add whitespace-checking mode
 + contrib/git-jump: fix greedy regex when matching hunks

 "git jump" script (in contrib/) has been updated a bit.


* jk/parse-options-concat (2016-07-06) 1 commit
  (merged to 'next' on 2016-07-28 at 219bc3a)
 + parse_options: allocate a new array when concatenating

 Users of the parse_options_concat() API function need to allocate
 extra slots in advance and fill them with OPT_END() when they want
 to decide the set of supported options dynamically, which makes the
 code error-prone and hard to read.  This has been corrected by tweaking
 the API to allocate and return a new copy of "struct option" array.


* jk/push-progress (2016-07-20) 12 commits
  (merged to 'next' on 2016-07-28 at 39598fb)
 + receive-pack: send keepalives during quiet periods
 + receive-pack: turn on connectivity progress
 + receive-pack: relay connectivity errors to sideband
 + receive-pack: turn on index-pack resolving progress
 + index-pack: add flag for showing delta-resolution progress
 + clone: use a real progress meter for connectivity check
 + check_connected: add progress flag
 + check_connected: relay errors to alternate descriptor
 + check_everything_connected: use a struct with named options
 + check_everything_connected: convert to argv_array
 + rev-list: add optional progress reporting
 + check_everything_connected: always pass --quiet to rev-list

 "git push" and "git clone" learned to give better progress meters
 to the end user who is waiting on the terminal.


* jt/fetch-large-handshake-window-on-http (2016-07-19) 1 commit
  (merged to 'next' on 

Re: git grep -P is multiline for negative lookahead/behind

[Junio C Hamano]

On Thu, Aug 4, 2016 at 11:54 AM, Michael Giuffrida
 wrote:
> On Mon, Aug 1, 2016 at 2:35 PM, Junio C Hamano  wrote:
>> I do not think "git grep" was designed to do multi-line anything,
>> with or without lookahead.  If you imagine that the implementation
>> attempts its matches line-by-line, does that explain the observed
>> symptom?
>
> No. If it worked line-by-line, it would produce more results. It is
> not producing the expected matches because it *is* considering the
> previous line in negative lookbehind, when I don't want or expect it
> to. Thus it throws out results that should match.

If that is the case I do not know what is going on; perhaps
somebody more familiar with the pcre codepath can help.

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

Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[René Scharfe]

Am 04.08.2016 um 18:07 schrieb Johannes Schindelin:

With GCC 6, the strdup() function is declared with the "nonnull"
attribute, stating that it is not allowed to pass a NULL value as
parameter.

In nedmalloc()'s reimplementation of strdup(), Postel's Law is heeded
and NULL parameters are handled gracefully. GCC 6 complains about that
now because it thinks that NULL cannot be passed to strdup() anyway.

Let's just shut up GCC >= 6 in that case and go on with our lives.


This version of strdup() is only compiled if nedmalloc is used instead
of the system allocator.  That means we can't rely on strdup() being
able to take NULL -- some (most?) platforms won't like it.  Removing
the NULL check would be a more general and overall easier way out, no?

But it should check the result of malloc() before copying.
---
 compat/nedmalloc/nedmalloc.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index a0a16eb..cc18f0c 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -955,12 +955,10 @@ void **nedpindependent_comalloc(nedpool *p, size_t 
elems, size_t *sizes, void **

  */
 char *strdup(const char *s1)
 {
-   char *s2 = 0;
-   if (s1) {
-   size_t len = strlen(s1) + 1;
-   s2 = malloc(len);
+   size_t len = strlen(s1) + 1;
+   char *s2 = malloc(len);
+   if (s2)
memcpy(s2, s1, len);
-   }
return s2;
 }
 #endif
--
2.9.2

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

Re: [PATCH 3/7] trace: use warning() for printing trace errors

[Junio C Hamano]

Jeff King  writes:

> I wondered if that would then let us drop set_warn_routine(), but it
> looks like there are other warning() calls it cares about. So that would
> invalidate the last paragraph here, though I still think converting the
> trace errors to warning() is a reasonable thing to do.

Yes.  That is why tonight's pushout will have this series in 'jch'
(that is a point on a linear history between 'master' and 'pu') and
tentatively ejects cc/apply-am topic out of 'pu', expecting it to be
rerolled.

Thanks.

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

Re: [PATCH 6/7] trace: disable key after write error

[Jeff King]

On Thu, Aug 04, 2016 at 01:45:11PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > If we get a write error writing to a trace descriptor, the
> > error isn't likely to go away if we keep writing. Instead,
> > you'll just get the same error over and over. E.g., try:
> >
> >   GIT_TRACE_PACKET=42 git ls-remote >/dev/null
> >
> > You don't really need to see:
> >
> >   warning: unable to write trace for GIT_TRACE_PACKET: Bad file descriptor
> >
> > hundreds of times. We could fallback to tracing to stderr,
> > as we do in the error code-path for open(), but there's not
> > much point. If the user fed us a bogus descriptor, they're
> > probably better off fixing their invocation. And if they
> > didn't, and we saw a transient error (e.g., ENOSPC writing
> > to a file), it probably doesn't help anybody to have half of
> > the trace in a file, and half on stderr.
> 
> Yes, I think I like this better than "we cannot open the named file,
> so let's trace into standard error stream" that is done in the code
> in the context of [3/7].  We should do the same over there.

Yeah, I was tempted to strip that out, too. I'll look into preparing a
patch on top.

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

Re: [PATCH 3/7] trace: use warning() for printing trace errors

[Jeff King]

On Thu, Aug 04, 2016 at 01:41:02PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Right now we just fprintf() straight to stderr, which can
> > make the output hard to distinguish. It would be helpful to
> > give it one of our usual prefixes like "error:", "warning:",
> > etc.
> >
> > It doesn't make sense to use error() here, as the trace code
> > is "optional" debugging code. If something goes wrong, we
> > should warn the user, but saying "error" implies the actual
> > git operation had a problem. So warning() is the only sane
> > choice.
> >
> > Note that this does end up calling warn_routine() to do the
> > formatting. So in theory, somebody who tries to trace from
> > their warn_routine() could cause a loop. But nobody does
> > this, and in fact nobody in the history of git has ever
> > replaced the default warn_builtin (there isn't even a
> > set_warn_routine function!).
> 
> I think the last bit is about to change; cf. 545f13c0 (usage: add
> set_warn_routine(), 2016-07-30) on cc/apply-am topic.

Thanks, I meant to check this series against "pu" to make sure there are
no new callers for write_or_whine_pipe(), but forgot to.

It looks like that same topic does add one new caller, and switches the
"fprintf" inside it to use warning().

IMHO the call added by 19a73ac (builtin/apply: make try_create_file()
return -1 on error, 2016-07-30) should just be a regular:

  if (write_in_full(...) < 0)
error(...);

We don't care about the weird pipe handling there (we know we're writing
to a file we just created), and the way the error message is passed in
just makes things weird. Plus it seems more like an error() than a
warning (e.g., we call error() immediately below when close() fails!).
But 8fab3c6 (write_or_die: use warning() instead of fprintf(stderr,
...), 2016-07-30)  makes it an unconditional warning (that commit, btw,
has a bug in that it retains the trailing newline of the message, even
though warning() will add one itself).

So I'd suggest that series drop the call write_or_whine_pipe() and drop
8fab3c6 entirely.

I wondered if that would then let us drop set_warn_routine(), but it
looks like there are other warning() calls it cares about. So that would
invalidate the last paragraph here, though I still think converting the
trace errors to warning() is a reasonable thing to do.

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

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Jeff King  writes:

> Like you, I have occasionally been bitten by Junio doing a fixup, and
> then I end up re-rolling, and lose that fixup.

... which usually is caught when I receive the reroll, as I try to
apply to the same base and compare the result with the previous
round.

> But I think such fixups are a calculated risk. Sometimes they save a lot
> of time, both for the maintainer and the contributor, when they manage
> to prevent another round-trip of the patch series to the list.

Yes.

> IOW, if the flow is something like:
>
>   1. Contributor sends patches. People review.
>
>   2. Minor fixups noticed by maintainer, fixed while applying.

This includes different kinds of things:

a) Trivially correct fixes given in other people's review.

b) Minor fixups by the maintainer, to code.

c) Minor fixups by the maintainer, to proposed log message.

d) "apply --whitespace=fix" whose result I do not even actively
   keep track of.

>   3. Only one small fixup needed from review. Contributor sends
>  squashable patch. Maintainer squashes.
>
> then I think that is a net win over sending the whole series again, for
> the contributor (who does not bother sending), reviewers (who really
> only need to look at the interdiff, which is what that squash is in the
> first place), and the maintainer (who can squash just as easily as
> re-applying the whole series).

> And that is the flip side. If the flow above does not happen, then step
> 2 just becomes a pain.

I think I can

 * stop taking 2-a).  This is less work for me, but some
   contributors are leaky and can lose obviously good suggestions,
   so I am not sure if that is an overall win for the quality of the
   end product;

 * do a separate "SQUASH???" commit and send them out for 2-b).
   This is a lot more work for a large series, but about the same
   amount of work (except for "remembering to send them out" part)
   for a small-ish topic.  A contributor needs to realize that I
   deal with _all_ the incoming topics, not just from topics from
   one contributor, and small additional work tends to add up.

to reduce #2.  Essentially, doing these two are about moving more
responsibility of keeping track of good suggestions in the review
discussion to the contributor, but a bad thing is that it does not
mean that the maintainer can stop keeping track of them.  We need a
way to make sure leaky contributors do not repeat the same issue in
their reroll that has already been pointed out and whose solutions
presented in the previous review.  Fetching from 'pu' and compare
has been one way to do so (that is why I publish 'pu' in the first
place, not to "build on", but to "view"), but apparently not many
contributors are comfortable with that idea.

There is no good way to reduce 2-c) and 2-d), but on the other hand,
there is no strong need for a special channel to propagate these
back.  2-c) can be a regular review comment (but again that risks
"the leaky contributor" problem) and 2-d) will happen automatically
when applying the rerolled version.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GSOC Update] Week 113

[Pranit Bauva]

= SUMMARY ===
My public git.git is available here[1]. I regularly keep pushing my work so
anyone interested can track me there. Feel free to participate in the
discussions going on PRs with my mentors. Your comments are valuable.


 INTRODUCTION  ===
The purpose of this project is to convert the git-bisect utility which partly
exists in the form of shell scripts to C code so as to make it more portable.
I plan to do this by converting each function to C and then calling it from
git-bisect.sh so as to use the existing test suite to test the function which
is converted.

Mentors:
Christian Couder 
Lars Schneider 


=== Updates ==
Things which were done in this week:

 * I have converted bisect_start() but there is a bug which I am still working
   on which conflicts with the `--term-bad` and `--term-good` of
   `--bisect-terms` subcommand. A RFC has been sent to the list[2]. Junio
   provided some reviews on it. A resend can be expected soonish.

== NEXT STEPS 
Things which would be done in the coming week:

 * Resend all patches according to Junio's review.

 * bisect_next() has become a top priority because it would then help
   converting bisect_auto_next() and then it can be called by other important
   functions like bisect_start().

 * Following that I will convert bisect_auto_start()

 * Then bisect_replay().

 My Patches (GSoC project only) ==

 * My current work is sent out to the mailing list here[2] which contains
   the whole conversion. The latter 2 patches still need a lot of
   development.

[1]: https://github.com/pranitbauva1997/git
[2]: http://www.spinics.net/lists/git/msg282332.html

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

Re: [PATCH 6/7] trace: disable key after write error

[Junio C Hamano]

Jeff King  writes:

> If we get a write error writing to a trace descriptor, the
> error isn't likely to go away if we keep writing. Instead,
> you'll just get the same error over and over. E.g., try:
>
>   GIT_TRACE_PACKET=42 git ls-remote >/dev/null
>
> You don't really need to see:
>
>   warning: unable to write trace for GIT_TRACE_PACKET: Bad file descriptor
>
> hundreds of times. We could fallback to tracing to stderr,
> as we do in the error code-path for open(), but there's not
> much point. If the user fed us a bogus descriptor, they're
> probably better off fixing their invocation. And if they
> didn't, and we saw a transient error (e.g., ENOSPC writing
> to a file), it probably doesn't help anybody to have half of
> the trace in a file, and half on stderr.

Yes, I think I like this better than "we cannot open the named file,
so let's trace into standard error stream" that is done in the code
in the context of [3/7].  We should do the same over there.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/7] trace: cosmetic fixes for error messages

[Junio C Hamano]

Jeff King  writes:

> I think it would be nicer to still to print:
>
>  warning: first line
>  warning: second line
>
> etc. We do that for "advice:", but not the rest of the vreportf
> functions. It might be nice to do that, but we'd have to go back to
> printing into a buffer (since we can't break up the incoming format
> string that we feed to fprintf).

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

Re: [PATCH 3/7] trace: use warning() for printing trace errors

[Junio C Hamano]

Jeff King  writes:

> Right now we just fprintf() straight to stderr, which can
> make the output hard to distinguish. It would be helpful to
> give it one of our usual prefixes like "error:", "warning:",
> etc.
>
> It doesn't make sense to use error() here, as the trace code
> is "optional" debugging code. If something goes wrong, we
> should warn the user, but saying "error" implies the actual
> git operation had a problem. So warning() is the only sane
> choice.
>
> Note that this does end up calling warn_routine() to do the
> formatting. So in theory, somebody who tries to trace from
> their warn_routine() could cause a loop. But nobody does
> this, and in fact nobody in the history of git has ever
> replaced the default warn_builtin (there isn't even a
> set_warn_routine function!).

I think the last bit is about to change; cf. 545f13c0 (usage: add
set_warn_routine(), 2016-07-30) on cc/apply-am topic.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Eric Wong]

Stefan Beller  wrote:
> On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin
>  wrote:
> > I guess I have no really good idea yet, either, how to retain the ease of
> > access of sending mails to the list, yet somehow keep a strong tie with
> > the original data stored in Git.
> 
> Does it have to be email? Transmitting text could be solved
> differently as well.

I've thought a lot about this over the years still think email
is the least bad.

Anti-spam tools for other messaging systems are far behind,
proprietary, or non-existent.  bugzilla.kernel.org has been hit
hard lately and I see plenty of bug-tracker-to-mail spam as a
result from projects that use web-based bug trackers.

And email spam filtering isn't even that great
(and I think it needs to be better for IPv6 and .onion adoption
 since much of it is still IPv4-oriented blacklisting).

I guess a blockchain (*coin) implementation might work (like
hashcash is used for email anti-spam), but the ones I've glanced
at all look like a bigger waste of electricity than email
filters.


Of course, centralized systems are unacceptable to me;
and with that I'll never claim any network service I run
will be reliable :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG?] --boundary inconsistent with path limiting

[Junio C Hamano]

Jeff King  writes:

> But now if I limit to "a.t", I get no output at all:
>
>   $ git log --format='%m %s' --boundary a..c -- a.t
>
> whereas I would have expected "- a" to show the boundary.
>
> Is this a bug, or are my expectations wrong?

In a range a..c, there is nothing that touches the path, so there is
no positive outcome.  As boundaries are essentially the parents of
the "last" positive outcome, I would not be surprised if I see an
empty output in that scenario.

But to be honest, I do not think anybody cared between the
distinction between a bug and intended behaviour in this case.

The boundary started as a debugging aid for the traversal machinery
and not as a serious feature to support end-user workflow.  In its
early days, I do not think we even showed _all_ boundaries (instead
we showed only ones that we have already parsed, or something like
that).  I think we added code to do a bit more work when asked to
show boundaries to show boundary commits that the traditional
"primarily for debugging" logic wouldn't have shown later, losing
its value as a debugging aid (because it no longer showed precisely
where the traversal machinery stopped digging).



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

Re: [BUG?] --boundary inconsistent with path limiting

[Jeff King]

On Thu, Aug 04, 2016 at 03:40:43PM -0400, Jeff King wrote:

> That makes sense to me. We omit "c" because it doesn't touch "b.t", and
> obviously include "b", which does. We _do_ include the boundary commit,
> even though it doesn't touch the path, which makes sense to me. It
> remains a boundary whether it touched the path or not, and without it,
> we get no boundary at all.
> 
> But now if I limit to "a.t", I get no output at all:
> 
>   $ git log --format='%m %s' --boundary a..c -- a.t
> 
> whereas I would have expected "- a" to show the boundary.
> 
> Is this a bug, or are my expectations wrong?

So I suppose it depends how you define "boundary" commits. In
get_revision_internal(), I see this comment:

/*
 * boundary commits are the commits that are parents of the
 * ones we got from get_revision_1() but they themselves are
 * not returned from get_revision_1().  Before returning
 * 'c', we need to mark its parents that they could be boundaries.
 */

By that definition, obviously if we do not have any commits to show,
then we have no boundary commits. I don't think this definition is
anywhere in the user-facing documentation, though.

It still seems weird to me, and I wonder if we should show all
UNINTERESTING commits as boundaries in the case that we haven't produced
any positive commits at all. But perhaps there is a case where that
would not be desirable.

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

Re: [PATCH 0/8] Better heuristics make prettier diffs

[Jeff King]

On Thu, Aug 04, 2016 at 12:54:51PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Not that you probably need more random cases of C code, but I happened
> > to be looking at a diff in git.git today, b333d0d6, which is another
> > regression for the compaction heuristic.
> 
> Wow, that one is _really_ bad.  Does it have something to do with
> the removal being at the very end of the file?

I think so. If it were:

  func1() {
 ... unique stuff ...
 ... shared ending ...
  }

  func2() {
 ... more unique stuff ...
 ... shared ending ...
  }

  unrelated_func() {
  }

and we dropped func2, then I think the blank line between func2() and
unrelated_func() would cause the compaction heuristic to stop shifting.

OTOH, if it were:

  func2() {
 ...
  }
  unrelated_func() {
  }

with no newline, you get a similar badly-shifted diff (which is not
surprising, as we were given no syntactic hint that "func2" is a
separate unit from "unrelated_func").

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

Re: [PATCH 0/8] Better heuristics make prettier diffs

[Junio C Hamano]

Jeff King  writes:

> Not that you probably need more random cases of C code, but I happened
> to be looking at a diff in git.git today, b333d0d6, which is another
> regression for the compaction heuristic.

Wow, that one is _really_ bad.  Does it have something to do with
the removal being at the very end of the file?

> The indent heuristic here gets it right.

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

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Junio C Hamano]

Michael Haggerty  writes:

I agree with Peff about "comment on the voodoo upfront".

> +#define START_OF_FILE_BONUS 9
> +#define END_OF_FILE_BONUS 46
> +#define TOTAL_BLANK_WEIGHT 4
> +#define PRE_BLANK_WEIGHT 16
> +#define RELATIVE_INDENT_BONUS -1
> +#define RELATIVE_INDENT_HAS_BLANK_BONUS 15
> +#define RELATIVE_OUTDENT_BONUS -19
> +#define RELATIVE_OUTDENT_HAS_BLANK_BONUS 2

When I read up to here, I thought "Heh, isn't the opposite of INdent
DEdent?" and then saw this:

> +#define RELATIVE_DEDENT_BONUS -63
> +#define RELATIVE_DEDENT_HAS_BLANK_BONUS 50

It turns out that you mean by OUTdent a line that indents further
(if I am reading the code correctly).  Is that obvious to everybody?

> + /* Bonuses based on the location of blank lines: */
> +bonus += TOTAL_BLANK_WEIGHT * total_blanks;
> + bonus += PRE_BLANK_WEIGHT * m->pre_blank;

This and ...

> +} else if (indent > m->pre_indent) {
> + /*
> +  * The line is indented more than its predecessor. Score it 
> based
> +  * on the larger indent:
> +  */
> + score = indent;
> + bonus += RELATIVE_INDENT_BONUS;
> + bonus += RELATIVE_INDENT_HAS_BLANK_BONUS * any_blanks;
> + } else if (indent < m->pre_indent) {

... this seems to be indented correctly even after getting quoted,
which in turn means most of the lines in the added code share
indent-with-non-tab badness.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] t7408: merge short tests, factor out testing method

[Stefan Beller]

Tests consisting of one line each can be consolidated to have fewer tests
to run as well as fewer lines of code.

When having just a few git commands, do not create a new shell but
use the -C flag in Git to execute in the correct directory.

Signed-off-by: Stefan Beller 
---
 t/t7408-submodule-reference.sh | 50 +++---
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index afcc629..1416cbd 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -10,6 +10,16 @@ base_dir=$(pwd)
 
 U=$base_dir/UPLOAD_LOG
 
+test_alternate_usage()
+{
+   alternates_file=$1
+   working_dir=$2
+   test_line_count = 1 $alternates_file &&
+   echo "0 objects, 0 kilobytes" >expect &&
+   git -C $working_dir count-objects >current &&
+   diff expect current
+}
+
 test_expect_success 'preparing first repository' '
test_create_repo A &&
(
@@ -42,44 +52,20 @@ test_expect_success 'preparing superproject' '
)
 '
 
-test_expect_success 'submodule add --reference' '
+test_expect_success 'submodule add --reference uses alternates' '
(
cd super &&
git submodule add --reference ../B "file://$base_dir/A" sub &&
git commit -m B-super-added
-   )
-'
-
-test_expect_success 'after add: existence of info/alternates' '
-   test_line_count = 1 super/.git/modules/sub/objects/info/alternates
-'
-
-test_expect_success 'that reference gets used with add' '
-   (
-   cd super/sub &&
-   echo "0 objects, 0 kilobytes" > expected &&
-   git count-objects > current &&
-   diff expected current
-   )
-'
-
-test_expect_success 'cloning superproject' '
-   git clone super super-clone
-'
-
-test_expect_success 'update with reference' '
-   cd super-clone && git submodule update --init --reference ../B
-'
-
-test_expect_success 'after update: existence of info/alternates' '
-   test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
+   ) &&
+   test_alternate_usage super/.git/modules/sub/objects/info/alternates 
super/sub
 '
 
-test_expect_success 'that reference gets used with update' '
-   cd super-clone/sub &&
-   echo "0 objects, 0 kilobytes" > expected &&
-   git count-objects > current &&
-   diff expected current
+test_expect_success 'updating superproject keeps alternates' '
+   test_when_finished "rm -rf super-clone" &&
+   git clone super super-clone &&
+   git -C super-clone submodule update --init --reference ../B &&
+   test_alternate_usage 
super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
 test_done
-- 
2.9.2.572.g9d9644e.dirty

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

[PATCH 1/6] t7408: modernize style

[Stefan Beller]

No functional change intended. This commit only changes formatting
to the style we recently use, e.g. starting the body of a test with a
single quote on the same line as the header, and then having the test
indented in the following lines.

Whenever we change directories, we do that in subshells.

Signed-off-by: Stefan Beller 
---
 t/t7408-submodule-reference.sh | 138 +
 1 file changed, 71 insertions(+), 67 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index eaea19b..afcc629 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -10,72 +10,76 @@ base_dir=$(pwd)
 
 U=$base_dir/UPLOAD_LOG
 
-test_expect_success 'preparing first repository' \
-'test_create_repo A && cd A &&
-echo first > file1 &&
-git add file1 &&
-git commit -m A-initial'
-
-cd "$base_dir"
-
-test_expect_success 'preparing second repository' \
-'git clone A B && cd B &&
-echo second > file2 &&
-git add file2 &&
-git commit -m B-addition &&
-git repack -a -d &&
-git prune'
-
-cd "$base_dir"
-
-test_expect_success 'preparing superproject' \
-'test_create_repo super && cd super &&
-echo file > file &&
-git add file &&
-git commit -m B-super-initial'
-
-cd "$base_dir"
-
-test_expect_success 'submodule add --reference' \
-'cd super && git submodule add --reference ../B "file://$base_dir/A" sub &&
-git commit -m B-super-added'
-
-cd "$base_dir"
-
-test_expect_success 'after add: existence of info/alternates' \
-'test_line_count = 1 super/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with add' \
-'cd super/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
-
-test_expect_success 'cloning superproject' \
-'git clone super super-clone'
-
-cd "$base_dir"
-
-test_expect_success 'update with reference' \
-'cd super-clone && git submodule update --init --reference ../B'
-
-cd "$base_dir"
-
-test_expect_success 'after update: existence of info/alternates' \
-'test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with update' \
-'cd super-clone/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
+test_expect_success 'preparing first repository' '
+   test_create_repo A &&
+   (
+   cd A &&
+   echo first >file1 &&
+   git add file1 &&
+   git commit -m A-initial
+   )
+'
+
+test_expect_success 'preparing second repository' '
+   git clone A B &&
+   (
+   cd B &&
+   echo second >file2 &&
+   git add file2 &&
+   git commit -m B-addition &&
+   git repack -a -d &&
+   git prune
+   )
+'
+
+test_expect_success 'preparing superproject' '
+   test_create_repo super &&
+   (
+   cd super &&
+   echo file >file &&
+   git add file &&
+   git commit -m B-super-initial
+   )
+'
+
+test_expect_success 'submodule add --reference' '
+   (
+   cd super &&
+   git submodule add --reference ../B "file://$base_dir/A" sub &&
+   git commit -m B-super-added
+   )
+'
+
+test_expect_success 'after add: existence of info/alternates' '
+   test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with add' '
+   (
+   cd super/sub &&
+   echo "0 objects, 0 kilobytes" > expected &&
+   git count-objects > current &&
+   diff expected current
+   )
+'
+
+test_expect_success 'cloning superproject' '
+   git clone super super-clone
+'
+
+test_expect_success 'update with reference' '
+   cd super-clone && git submodule update --init --reference ../B
+'
+
+test_expect_success 'after update: existence of info/alternates' '
+   test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with update' '
+   cd super-clone/sub &&
+   echo "0 objects, 0 kilobytes" > expected &&
+   git count-objects > current &&
+   diff expected current
+'
 
 test_done
-- 
2.9.2.572.g9d9644e.dirty

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

[PATCH 3/6] submodule--helper module-clone: allow multiple references

[Stefan Beller]

Allow users to pass in multiple references, just as clone accepts multiple
references as well.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b22352b..896a3ec 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -442,7 +442,7 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
 }
 
 static int clone_submodule(const char *path, const char *gitdir, const char 
*url,
-  const char *depth, const char *reference, int quiet)
+  const char *depth, struct string_list *reference, 
int quiet)
 {
struct child_process cp;
child_process_init();
@@ -453,8 +453,11 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
argv_array_push(, "--quiet");
if (depth && *depth)
argv_array_pushl(, "--depth", depth, NULL);
-   if (reference && *reference)
-   argv_array_pushl(, "--reference", reference, NULL);
+   if (reference->nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, reference)
+   argv_array_pushl(, "--reference", item->string, 
NULL);
+   }
if (gitdir && *gitdir)
argv_array_pushl(, "--separate-git-dir", gitdir, NULL);
 
@@ -470,13 +473,13 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
 
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-   const char *name = NULL, *url = NULL;
-   const char *reference = NULL, *depth = NULL;
+   const char *name = NULL, *url = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
char *p, *path = NULL, *sm_gitdir;
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
+   struct string_list reference = STRING_LIST_INIT_NODUP;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -491,8 +494,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "url", ,
   N_("string"),
   N_("url where to clone the submodule from")),
-   OPT_STRING(0, "reference", ,
-  N_("string"),
+   OPT_STRING_LIST(0, "reference", ,
+  N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", ,
   N_("string"),
@@ -528,7 +531,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
die(_("could not create directory '%s'"), sm_gitdir);
-   if (clone_submodule(path, sm_gitdir, url, depth, reference, 
quiet))
+   if (clone_submodule(path, sm_gitdir, url, depth, , 
quiet))
die(_("clone of '%s' into submodule path '%s' failed"),
url, path);
} else {
-- 
2.9.2.572.g9d9644e.dirty

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

[PATCH 5/6] submodule update: add super-reference flag

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 14 +-
 git-submodule.sh| 10 ++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b6f297b..707f201 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,6 +584,7 @@ struct submodule_update_clone {
int quiet;
int recommend_shallow;
struct string_list references;
+   struct string_list superreferences;
const char *depth;
const char *recursive_prefix;
const char *prefix;
@@ -600,7 +601,7 @@ struct submodule_update_clone {
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
-   NULL, NULL, NULL, \
+   STRING_LIST_INIT_DUP, NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -715,6 +716,15 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
for_each_string_list_item(item, >references)
argv_array_pushl(>args, "--reference", 
item->string, NULL);
}
+   if (suc->superreferences.nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, >superreferences) {
+   strbuf_reset();
+   argv_array_pushf(>args, "--reference=%s/%s",
+relative_path(item->string, 
suc->prefix, ),
+sub->path);
+   }
+   }
if (suc->depth)
argv_array_push(>args, suc->depth);
 
@@ -835,6 +845,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
   N_("rebase, merge, checkout or none")),
OPT_STRING_LIST(0, "reference", , N_("repo"),
   N_("reference repository")),
+   OPT_STRING_LIST(0, "super-reference", , 
N_("repo"),
+  N_("superproject of a reference repository")),
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
diff --git a/git-submodule.sh b/git-submodule.sh
index 526ea5d..99d45c8 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -34,6 +34,7 @@ command=
 branch=
 force=
 reference=
+superreference=
 cached=
 recursive=
 init=
@@ -520,6 +521,14 @@ cmd_update()
--reference=*)
reference="$1"
;;
+   --super-reference)
+   case "$2" in '') usage ;; esac
+   superreference="--super-reference=$2"
+   shift
+   ;;
+   --super-reference=*)
+   superreference="$1"
+   ;;
-m|--merge)
update="merge"
;;
@@ -576,6 +585,7 @@ cmd_update()
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
${reference:+"$reference"} \
+   ${superreference:+"$superreference"} \
${depth:+--depth "$depth"} \
${recommend_shallow:+"$recommend_shallow"} \
${jobs:+$jobs} \
-- 
2.9.2.572.g9d9644e.dirty

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

[PATCH 4/6] submodule--helper update-clone: allow multiple references

[Stefan Beller]

Allow the user to pass in multiple references to update_clone.
Currently this is only internal API, but once the shell script is
replaced by a C version, this is needed.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 14 +-
 git-submodule.sh|  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 896a3ec..b6f297b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -583,7 +583,7 @@ struct submodule_update_clone {
/* configuration parameters which are passed on to the children */
int quiet;
int recommend_shallow;
-   const char *reference;
+   struct string_list references;
const char *depth;
const char *recursive_prefix;
const char *prefix;
@@ -599,7 +599,8 @@ struct submodule_update_clone {
int failed_clones_nr, failed_clones_alloc;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
+   NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -709,8 +710,11 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
argv_array_pushl(>args, "--url", url, NULL);
-   if (suc->reference)
-   argv_array_push(>args, suc->reference);
+   if (suc->references.nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, >references)
+   argv_array_pushl(>args, "--reference", 
item->string, NULL);
+   }
if (suc->depth)
argv_array_push(>args, suc->depth);
 
@@ -829,7 +833,7 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "update", ,
   N_("string"),
   N_("rebase, merge, checkout or none")),
-   OPT_STRING(0, "reference", , N_("repo"),
+   OPT_STRING_LIST(0, "reference", , N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
diff --git a/git-submodule.sh b/git-submodule.sh
index 2b23ce6..526ea5d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -575,7 +575,7 @@ cmd_update()
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
-   ${reference:+--reference "$reference"} \
+   ${reference:+"$reference"} \
${depth:+--depth "$depth"} \
${recommend_shallow:+"$recommend_shallow"} \
${jobs:+$jobs} \
-- 
2.9.2.572.g9d9644e.dirty

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

[PATCH 6/6] clone: reference flag is used for submodules as well

[Stefan Beller]

When giving a --reference while also giving --recurse, the alternates
for the submodules are assumed to be in the superproject as well.

In case they are not, we error out when cloning the submodule.
However the update command succeeds as usual (with no alternates).

Signed-off-by: Stefan Beller 
---
 builtin/clone.c| 22 ++
 t/t7408-submodule-reference.sh | 31 ++-
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..f586df5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,6 +51,7 @@ static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
 static struct string_list option_reference = STRING_LIST_INIT_NODUP;
+static struct string_list superreferences = STRING_LIST_INIT_DUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -280,10 +281,11 @@ static void strip_trailing_slashes(char *dir)
*end = '\0';
 }
 
-static int add_one_reference(struct string_list_item *item, void *cb_data)
+static int add_one_reference(struct string_list_item *item, void *cb_dir)
 {
char *ref_git;
const char *repo;
+   const char *dir = cb_dir;
struct strbuf alternate = STRBUF_INIT;
 
/* Beware: read_gitfile(), real_path() and mkpath() return static 
buffer */
@@ -316,6 +318,13 @@ static int add_one_reference(struct string_list_item 
*item, void *cb_data)
if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
die(_("reference repository '%s' is grafted"), item->string);
 
+   if (option_recursive) {
+   struct strbuf sb = STRBUF_INIT;
+   char *rel = xstrdup(relative_path(item->string, dir, ));
+   string_list_append(, rel);
+   strbuf_reset();
+   }
+
strbuf_addf(, "%s/objects", ref_git);
add_to_alternates_file(alternate.buf);
strbuf_release();
@@ -323,9 +332,9 @@ static int add_one_reference(struct string_list_item *item, 
void *cb_data)
return 0;
 }
 
-static void setup_reference(void)
+static void setup_reference(char *dir)
 {
-   for_each_string_list(_reference, add_one_reference, NULL);
+   for_each_string_list(_reference, add_one_reference, dir);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -736,6 +745,7 @@ static int checkout(void)
 
if (!err && option_recursive) {
struct argv_array args = ARGV_ARRAY_INIT;
+   static struct string_list_item *item;
argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
 
if (option_shallow_submodules == 1)
@@ -744,6 +754,10 @@ static int checkout(void)
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
+   if (superreferences.nr)
+   for_each_string_list_item(item, )
+   argv_array_pushf(, "--super-reference=%s", 
item->string);
+
err = run_command_v_opt(args.argv, RUN_GIT_CMD);
argv_array_clear();
}
@@ -978,7 +992,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
strbuf_reset();
 
if (option_reference.nr)
-   setup_reference();
+   setup_reference(dir);
 
fetch_pattern = value.buf;
refspec = parse_fetch_refspec(1, _pattern);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1416cbd..2652cfe 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -56,7 +56,8 @@ test_expect_success 'submodule add --reference uses 
alternates' '
(
cd super &&
git submodule add --reference ../B "file://$base_dir/A" sub &&
-   git commit -m B-super-added
+   git commit -m B-super-added &&
+   git repack -ad
) &&
test_alternate_usage super/.git/modules/sub/objects/info/alternates 
super/sub
 '
@@ -68,4 +69,32 @@ test_expect_success 'updating superproject keeps alternates' 
'
test_alternate_usage 
super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
+test_expect_success 'submodules use alternates when cloning a superproject' '
+   test_when_finished "rm -rf super-clone" &&
+   git clone --reference super --recursive super super-clone &&
+   (
+   cd super-clone &&
+   # test superproject has alternates setup correctly
+   test_alternate_usage .git/objects/info/alternates . &&
+   # test submodule has correct setup
+   test_alternate_usage .git/modules/sub/objects/info/alternates 
sub
+   )
+'
+
+test_expect_success 'cloning superproject, missing submodule alternates' '
+   test_when_finished "rm -rf super-clone" 

[PATCH 0/6] git clone: Marry --recursive and --reference

[Stefan Beller]

 Currently when cloning a superproject with --recursive and --reference
 only the superproject learns about its alternates. The submodules are
 cloned independently, which may incur lots of network costs.
 
 Assume that the reference repository has the submodules at the same
 paths as the to-be-cloned submodule and try to setup alternates from
 there.
 
 Some submodules in the referenced superproject may not be there, 
 (they are just not initialized/cloned/checked out), which yields
 an error for now. In future work we may want to soften the alternate
 check and not die in the clone when one of the given alternates doesn't
 exist.
 
 patch 1,2 are modernizing style of t7408, 
 patches 3,4 are not strictly necessary, but I think it is a good thing
 to not leave the submodule related C code in a crippled state (i.e.
 allowing only one reference). The shell code would also need this update,
 but it looked ugly to me, so I postpone it until more of the submodule code
 is written in C. 
 
 Thanks,
 Stefan 

Stefan Beller (6):
  t7408: modernize style
  t7408: merge short tests, factor out testing method
  submodule--helper module-clone: allow multiple references
  submodule--helper update-clone: allow multiple references
  submodule update: add super-reference flag
  clone: reference flag is used for submodules as well

 builtin/clone.c|  22 --
 builtin/submodule--helper.c|  45 
 git-submodule.sh   |  12 +++-
 t/t7408-submodule-reference.sh | 153 +++--
 4 files changed, 147 insertions(+), 85 deletions(-)

-- 
2.9.2.572.g9d9644e.dirty

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

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Junio C Hamano]

Stefan Beller  writes:

> I have just reread the scoring function and I think you could pull out the
> `score=indent` assignment (it is always assigned except for indent <0)
>
> if (indent == -1)
>score = 0;
> else
>score = indent;
> ... lots of bonus computation below, which in its current 
> implementation
> have lots of "score = indent;" lines as well.

Yup.  If each part in this if/else if/... cascade independently sets
complete information (i.e. both "bonus" and "score") necessary for
the final result, then I do not mind the same "score = indent" in
many of them (these case happen to get the same score), but that is
not what we have in this code (i.e. "bonus" has a shared component
that is not affected by thie if/else if/ cascade), so setting score
to indent upfront and reset it to 0 only on a blank line would make
sense.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Junio C Hamano]

Michael Haggerty  writes:

>>> +   }
>>> +   /*
>>> +* We have reached the end of the line without finding any non-space
>>> +* characters; i.e., the whole line consists of trailing spaces, 
>>> which we
>>> +* are not interested in.
>>> +*/
>>> +   return -1;

Not related to Jacob's review, but "the whole line consists of
trailing spaces" made me read it twice; while it is technically
correct, "the whole line consists of spaces", or even "this is a
blank line", would read a lot more easily, at least for me.

> I was implicitly assuming that such lines would have text somewhere
> after those 200 spaces (or 25 TABs or whatever). But you're right, the
> line could consist only of whitespace. Unfortunately, the only way to
> distinguish these two cases is to read the rest of the line, which is
> exactly what we *don't* want to do.

Hmm, why is it exactly what we don't want to do?  Is it a
performance concern?  In other words, is it because this function is
called many times to measure the same line multiple times?  After
all, somebody in this file is already scanning each and every line
to see where it ends to split the input into records, so perhaps a
"right" (if the "theoretical correctness" of the return value from
this function mattered, which you wave-away below) optimization
could be to precompute it while the lines are broken into records
and store it in the "rec" structure?

> But I think it doesn't matter anyway. Such "text" will likely never be
> read by a human, so it's not a big deal if the slider position is not
> picked perfectly. And remember, this whole saga is just to improve the
> aesthetics of the diff. The diff is *correct* (e.g., in the sense of
> applicable) regardless of where we position the sliders.

A better argument may be "if the user is truly reading a diff output
for such an unusual "text", it is likely that she has a very wide
display and/or running less -S, and treating such an overindented line
as if it were a blank line would give a result that is more consistent
to what appears on her display", perhaps?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG?] --boundary inconsistent with path limiting

[Jeff King]

Let's say I have a simple repo with three paths:

git init -q repo
cd repo
for i in a b c
do
echo content >$i.t
git add $i.t
git commit -qm $i &&
git tag $i
done

If I ask for the top 2 commits, with the third as a boundary, I get the
expected output:

  $ git log --format='%m %s' --boundary a..c
  > c
  > b
  - a

If I limit the path to "b.t", I get:

  $ git log --format='%m %s' --boundary a..c -- b.t
  > b
  - a

That makes sense to me. We omit "c" because it doesn't touch "b.t", and
obviously include "b", which does. We _do_ include the boundary commit,
even though it doesn't touch the path, which makes sense to me. It
remains a boundary whether it touched the path or not, and without it,
we get no boundary at all.

But now if I limit to "a.t", I get no output at all:

  $ git log --format='%m %s' --boundary a..c -- a.t

whereas I would have expected "- a" to show the boundary.

Is this a bug, or are my expectations wrong?

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

Re: git grep -P is multiline for negative lookahead/behind

[Michael Giuffrida]

On Mon, Aug 1, 2016 at 2:35 PM, Junio C Hamano  wrote:
> Michael Giuffrida  writes:
>
>> Is this expected behavior, and if so, why/where is this documented?
>
> I do not think "git grep" was designed to do multi-line anything,
> with or without lookahead.  If you imagine that the implementation
> attempts its matches line-by-line, does that explain the observed
> symptom?

No. If it worked line-by-line, it would produce more results. It is
not producing the expected matches because it *is* considering the
previous line in negative lookbehind, when I don't want or expect it
to. Thus it throws out results that should match.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/8] xdl_change_compact(): keep track of the earliest end

[Junio C Hamano]

Michael Haggerty  writes:

> This makes it easier to detect whether shifting is possible, and will
> also make the next change easier.

I can see the code keeping track of earliest_end but the above does
not make it clear what the new "continue" is about.

... easier to detect whether shifting is possible (in which case we
can skip the shifting), and will also make ...

perhaps.

> Signed-off-by: Michael Haggerty 
> ---
>  xdiff/xdiffi.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index 66129db..34f021a 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> @@ -414,7 +414,8 @@ static int recs_match(xrecord_t **recs, long ixs, long 
> ix, long flags)
>  }
>  
>  int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
> - long start, end, io, end_matching_other, groupsize, nrec = xdf->nrec;
> + long start, end, earliest_end, end_matching_other;
> + long io, groupsize, nrec = xdf->nrec;
>   char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
>   unsigned int blank_lines;
>   xrecord_t **recs = xdf->recs;
> @@ -516,6 +517,8 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
> long flags) {
>   end_matching_other = -1;
>   }
>  
> + earliest_end = end;
> +
>   /*
>* Now shift the group forward as long as the first line
>* of the current change group is equal to the line 
> after
> @@ -547,6 +550,9 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, 
> long flags) {
>   }
>   } while (groupsize != end - start);
>  
> + if (end == earliest_end)
> + continue; /* no shifting is possible */
> +
>   if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
>   /*
>* Compaction heuristic: if a group can be moved back 
> and
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] is_blank_line: take a single xrecord_t as argument

[Junio C Hamano]

Michael Haggerty  writes:

> There is no reason for it to take an array and index as argument, as it
> only accesses a single element of the array.

Yup, I think I am partly guilty.  The result looks much nicer.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/8] xdl_change_compact(): fix compaction heuristic to adjust io

[Junio C Hamano]

Michael Haggerty  writes:

> The code branch used for the compaction heuristic incorrectly forgot to
> keep io in sync while the group was shifted. I think that could have
> led to reading past the end of the rchgo array.

I had to read the first sentence three times as "incorrectly forgot"
was a bit strange thing to say (as if there is a situation where
'forgetting to do' is the correct thing to do, but in that case we
would phrase it to stress that not doing is a deliberate choice,
e.g. 'refraining from doing').  Perhaps s/incorrectly // is the
simplest readability improvement?

> Signed-off-by: Michael Haggerty 
> ---
> I didn't actually try to verify the presence of a bug, because it
> seems like more work than worthwhile. But here is my reasoning:
>
> If io is not decremented correctly during one iteration of the outer
> `while` loop, then it will loose sync with the `end` counter. In
> particular it will be too large.
>
> Suppose that the next iterations of the outer `while` loop (i.e.,
> processing the next block of add/delete lines) don't have any sliders.
> Then the `io` counter would be incremented by the number of
> non-changed lines in xdf, which is the same as the number of
> non-changed lines in xdfo that *should have* followed the group that
> experienced the malfunction. But since `io` was too large at the end
> of that iteration, it will be incremented past the end of the
> xdfo->rchg array, and will try to read that memory illegally.

I agree with Peff that these should be in the log message.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] xdl_change_compact(): rename some local variables for clarity

[Junio C Hamano]

Jeff King  writes:

> On Thu, Aug 04, 2016 at 12:00:29AM +0200, Michael Haggerty wrote:
>
>> * ix -> i
>> * ixo -> io
>> * ixs -> start
>> * grpsiz -> groupsize
>
> After your change, I immediately understand three of them. But what is
> "io"?

I had the same reaction.

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

Re: [PATCH v6 16/16] merge-recursive: flush output buffer even when erroring out

[Junio C Hamano]

Johannes Schindelin  writes:

> Ever since 66a155b (Enable output buffering in merge-recursive.,
> 2007-01-14), we had a problem: When the merge failed in a fatal way, all
> regular output was swallowed because we called die() and did not get a
> chance to drain the output buffers.

OK.  Even though I really wanted to see somebody else review this
series as well, I finished reading it through one more time before
that happened, which is unfortunate because I think this is ready to
start cooking in 'next' even though I no longer have much faith in
my eyes alone after staring at this series so many times---you start
missing details.

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

Re: [PATCH v6 15/16] Ensure that the output buffer is released after calling merge_trees()

[Junio C Hamano]

Johannes Schindelin  writes:

> The recursive merge machinery accumulates its output in an output
> buffer, to be flushed at the end of merge_recursive(). At this point,
> we forgot to release the output buffer.
> ...
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ec50932..9e527de 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2078,6 +2078,8 @@ int merge_recursive(struct merge_options *o,
>   commit_list_insert(h2, &(*result)->parents->next);
>   }
>   flush_output(o);
> + if (!o->call_depth && o->buffer_output < 2)
> + strbuf_release(>obuf);

OK, with !o->call_depth the change makes sense to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 08/16] merge-recursive: allow write_tree_from_memory() to error out

[Junio C Hamano]

Johannes Schindelin  writes:

> It is possible that a tree cannot be written (think: disk full). We
> will want to give the caller a chance to clean up instead of letting
> the program die() in such a case.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  merge-recursive.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 2be1e17..1f86338 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1888,8 +1888,8 @@ int merge_trees(struct merge_options *o,
>   else
>   clean = 1;
>  
> - if (o->call_depth)
> - *result = write_tree_from_memory(o);
> + if (o->call_depth && !(*result = write_tree_from_memory(o)))
> + return -1;

I'll let it pass, but we avoid assignment in a conditional part for
a good reason: it can become unreadable pretty quickly.  Writing it
in a long-hand, e.g.

if (o->call_depth) {
*result = write_tree_from_memory(o);
if (!*result)
return -1;
}

future-proofs against the "o->call_depth" condition part and
"write_tree_from_memory(o)" operation part becoming longer, possibly
needing multiple statements.

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

Re: [PATCH v6 07/16] merge-recursive: avoid returning a wholesale struct

[Junio C Hamano]

Johannes Schindelin  writes:

> It is technically allowed, as per C89, for functions' return type to
> be complete structs (i.e. *not* just pointers to structs).
>
> However, it was just an oversight of this developer when converting
> Python code to C code in 6d297f8 (Status update on merge-recursive in
> C, 2006-07-08) which introduced such a return type.
>
> Besides, by converting this construct to pass in the struct, we can now
> start returning a value that can indicate errors in future patches. This
> will help the current effort to libify merge-recursive.c.

I do not think returning a small struct by value is unconditionally
a bad thing, but I do agree with you that this change makes the 
resulting code much easier to read, especially once this starts
returning errors.

Good.

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

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Jeff King]

On Thu, Aug 04, 2016 at 05:29:52PM +0200, Johannes Schindelin wrote:

> So my idea was to introduce a new --reword= option to `git commit`
> that would commit an empty patch and let the user edit the commit message,
> later replacing the original one with the new one. This is not *quite* as
> nice as I want it, because it makes the changes unobvious. On the other
> hand, I would not find a series of sed commands more obvious, in
> particular because that limits you in the ways of sed. And, you know,
> regexp. I like them, but I know many people cannot really work with them.

I don't have a real opinion on this. I probably wouldn't use it, but I
have no problem with it existing. I think it's somewhat orthogonal to
the idea of _transmitting_ those reword operations to somebody else.

> > That pushes work onto the submitter, but saves work from the reviewers,
> > who can quickly say "something like this..." without having to worry
> > about making a full change, formatting it as a diff, etc.
> > 
> > I do think that's the right time-tradeoff to be making, as we have more
> > submitters than reviewers.
> 
> I agree that it is the right trade-off. TBH I was shocked when I learned
> how much effort Junio puts into applying my patches. I do not want that. I
> want my branch to reflect pretty precisely (modulo sign-off, of course)
> what is going to be integrated into Git's source code.

Like you, I have occasionally been bitten by Junio doing a fixup, and
then I end up re-rolling, and lose that fixup (or have to deal with
porting it forward with awkward tools).

But I think such fixups are a calculated risk. Sometimes they save a lot
of time, both for the maintainer and the contributor, when they manage
to prevent another round-trip of the patch series to the list.

IOW, if the flow is something like:

  1. Contributor sends patches. People review.

  2. Minor fixups noticed by maintainer, fixed while applying.

  3. Only one small fixup needed from review. Contributor sends
 squashable patch. Maintainer squashes.

then I think that is a net win over sending the whole series again, for
the contributor (who does not bother sending), reviewers (who really
only need to look at the interdiff, which is what that squash is in the
first place), and the maintainer (who can squash just as easily as
re-applying the whole series).

It does mean the "final" version of the series is never on the list. It
has to be pieced together from the squash (and sometimes step 2 is not
even mentioned on-list).

So I think it is really a judgement call for step (3) on what is a
"small" fixup, and whether it is easier for everybody to look at the
squash interdiff and say "yep, that's right", versus re-reviewing the
whole series.

> I'd much prefer to resubmit a cleaned-up version, even if it was just the
> commit subjects, and be certain that `pu` and my branch are on the same
> page.
> 
> Instead, Junio puts in a tremendous amount of work, and it does not help
> anybody, because the local branches *still* do not have his fixups, and as
> a consequence subsequent iterations of the patch series will have to be
> fixed up *again*.

And that is the flip side. If the flow above does not happen, then step
2 just becomes a pain.

I don't have a silver bullet or anything. I'm mostly just musing.

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

Re: Problem with two copies of same branch diverging

[Junio C Hamano]

On Thu, Aug 4, 2016 at 10:50 AM, Ed Greenberg  wrote:
> On 08/04/2016 01:28 PM, Junio C Hamano wrote:
>>
>> ... indicates that you are not pushing to update the remote
>> repository correctly.  Once you get that part working correctly,
>> after you push at the end of the session, you should be able to do
>> "git reset" at the other side to tell Git to notice that the updated
>> working tree files that were transferred behind its back are now in
>> sync with what is supposed to be checked out.
>
> If this is the case, why do my fresh clones contain the most recent commit?

Exactly. Why does your "push" not result in "git log" to show the
most recent commit? Once you solve that, "git reset" would do
the right thing, I would think, just like a fresh clone shows the
latest.  The thing is, that I didn't quite find out what your "push"
is doing wrong in your original message.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] pager: move pager-specific setup into the build

[Jeff King]

On Thu, Aug 04, 2016 at 11:34:10AM +, Eric Wong wrote:

> > > --- a/config.mak.uname
> > > +++ b/config.mak.uname
> > > @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
> > >   HAVE_PATHS_H = YesPlease
> > >   GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
> > >   HAVE_BSD_SYSCTL = YesPlease
> > > + PAGER_ENV = LESS=FRX LV=-c MORE=FRX
> > >  endif
> > 
> > Is it worth setting up PAGER_ENV's default values before including
> > config.mak.*, and then using "+=" here? That avoids this line getting
> > out of sync with the usual defaults.
> 
> Good point, but it makes ordering problematic for folks
> who want to override it config.mak or command-line.

I'm not sure it changes much for them. Their "=" in config.mak, etc,
would override our default, and anything on the command line overrides
all of the in-Makefile stuff anyway. The only difference would be if
they use "+=" in config.mak, but there I think it would be an
improvement.

I'm OK to leave it as-is until somebody actually cares, though.

> > I know you said you don't like string parsing in C. Here is a patch (on
> > top of yours) that converts the parsing to shell, and generates a
> > pre-built array-of-struct (this is similar to the big series I posted
> > long ago, but just touching this one spot, not invading the whole
> > Makefile). Feel free to ignore it as over-engineered, but I thought I'd
> > throw it out there in case it appeals.
> 
> Yeah, but I'd rather not introduce more complexity into the
> build process, either (unless it's a performance-sensitive part,
> which this is not).  Also, while my original 2/2 to make it
> configurable at runtime was discarded, I wouldn't rule out
> somebody making a compelling case for it and it would be
> an easier change from the parse-at-runtime code.

Yeah, I had similar thoughts while writing it.

Your v4 patch looks fine to me.

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

Re: [PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[Junio C Hamano]

Johannes Schindelin  writes:

> With GCC 6, the strdup() function is declared with the "nonnull"
> attribute, stating that it is not allowed to pass a NULL value as
> parameter.
>
> In nedmalloc()'s reimplementation of strdup(), Postel's Law is heeded
> and NULL parameters are handled gracefully. GCC 6 complains about that
> now because it thinks that NULL cannot be passed to strdup() anyway.
>
> Let's just shut up GCC >= 6 in that case and go on with our lives.
>
> See https://gcc.gnu.org/gcc-6/porting_to.html for details.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  compat/nedmalloc/nedmalloc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
> index 677d1b2..3f28c0b 100644
> --- a/compat/nedmalloc/nedmalloc.c
> +++ b/compat/nedmalloc/nedmalloc.c
> @@ -956,6 +956,9 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems, 
> size_t *sizes, void **
>  char *strdup(const char *s1)
>  {
>   char *s2 = 0;
> +#if __GNUC__ >= 6
> +#pragma GCC diagnostic ignored "-Wnonnull-compare"
> +#endif
>   if (s1) {
>   size_t len = strlen(s1) + 1;
>   s2 = malloc(len);

Is it a common convention to place "#pragma GCC diagnostic"
immediately before the statement you want to affect, and have the
same pragma in effect until the end of the compilation unit?

I know this function is at the end and it is not worth doing
push/ignored/pop dance, and I assumed that it is the reason why we
see a single "ignore from here on", which is much simpler, but it is
somewhat distracting.  It made me wonder if it makes it easier to
read and less distracting to have these three lines in front of and
outside the function definition, while thinking that it would have a
documentation value to have it immediately before the statement you
want to affect.  Help me convince myself that this is the best
place.

Thanks.

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

Re: Problem with two copies of same branch diverging

[Junio C Hamano]

Ed Greenberg  writes:

> Hi, Thanks for reading my question.
>
> I have two copies of code checked out at the same branch. Desktop and
> remote server.
>
> I use an IDE that automatically SFTP transfers each save from the
> desktop to the remote server, so I can run my changes on the server
> environment.

You are syncing _ONLY_ the working tree state without syncing Git
state at all, and that is why the server side gets confused.  You
have to stop doing that.

If you do not do any change on the server end, you can simply stop
having a git repository there; just treat its directory as what it
really is: a copy of the working tree, something akin to an
extracted tarball.

If you do change on both, you probably are better off without the
mechanism to copy working tree one-way that you currently have.
Just push or fetch between the two repositories and integrate the
local changes.

Having said all that.

> At the end of the session, I commit the code on my desktop, do a git
> push to the repo.

> When I look at the server, the code there is identical to what's on my
> desktop box and what I just comitted and pushed, but, of course, git
> status thinks it's all modified and wants me to either commit it or
> stash it.  

This is expected as pushing into the remote would not affect what is
checked out, most importantly, the index.  But this ...

> In fact, doing a git log on the server doesn't show my
> latest push.  

... indicates that you are not pushing to update the remote
repository correctly.  Once you get that part working correctly,
after you push at the end of the session, you should be able to do
"git reset" at the other side to tell Git to notice that the updated
working tree files that were transferred behind its back are now in
sync with what is supposed to be checked out.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t5533: make it pass on case-sensitive filesystems

[Junio C Hamano]

Johannes Schindelin  writes:

> The newly-added test case wants to commit a file "c.t" (note the lower
> case) when a previous test case already committed a file "C.t". This
> confuses Git to the point that it thinks "c.t" was not staged when "git
> add c.t" was called.
>
> Simply make the naming of the test commits consistent with the previous
> test cases: use upper-case, and advance in the alphabet.
>
> This came up in local work to rebase the Windows-specific patches to the
> current `next` branch. An identical fix was suggested by John Keeping.
>
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: 
> https://github.com/dscho/git/releases/tag/t5533-case-insensitive-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git t5533-case-insensitive-v1

Thanks.  It may make it easier to see to have a blank line here,
separating them from the diffstat.

>  t/t5533-push-cas.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index 09899af..a2c9e74 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
>   (
>   cd src &&
>   git checkout -b branch master &&
> - test_commit c
> + test_commit F
>   ) &&
>   (
>   cd dst &&

Thanks.

> -- 
> 2.9.0.281.g286a8d9
> 
> base-commit: 9813b109b4ec6630220e5f3d8aff275e23cba59e

A totally unrelated tangent.

This line turns out to be less than useful at least in this
particular case.

The fix is meant for jk/push-force-with-lease-creation topic, but I
had to find it out by the old fashioned way, i.e. running blame for
these lines in 'pu' to find eee98e74f9 is the culprit and then
running "git branch --with eee98e74f9".  The only thing the line
made easier is I _could_ start the blame at the named commit (which
is on 'next') instead of 'pu'.  When I took that "base-commit"
series, I was hoping that it would give us a lot more useful
information.


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

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

[Pranit Bauva]

Hey Junio,

On Thu, Aug 4, 2016 at 10:20 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Hey Junio,
>>
>> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
>>> Pranit Bauva  writes:
>>>
> Also you do not seem to check the error from the function to smudge
> the "result" you are returning from this function.

 Yes I should combine the results from every removal.

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

 The shell code uses rm -f which is silent and it removes only if
 present.
>>>
>>> Isn't that what unlink_or_warn() do?  Call unlink() and happily
>>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
>>> exist in the first place), but otherwise reports an error (imagine:
>>> EPERM).
>>
>> Umm, I am confused. I tried "rm -f" with a non-existing file and it
>> does not show any warning or error.
>
> You are, or you were?  I hope it is the latter, iow, you are no
> longer confused and now understand why unlink_or_warn() was
> suggested.

I meant to use past tense. Did not re-check before sending it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Stefan Beller]

On Thu, Aug 4, 2016 at 12:56 AM, Jeff King  wrote:
> On Thu, Aug 04, 2016 at 12:00:36AM +0200, Michael Haggerty wrote:
>
>> This table shows the number of diff slider groups that were positioned
>> differently than the human-generated values, for various repositories.
>> "default" is the default "git diff" algorithm. "compaction" is Git 2.9.0
>> with the `--compaction-heuristic` option "indent" is an earlier,
>
> s/option/&./
>
>>  static int diff_detect_rename_default;
>> +static int diff_indent_heuristic; /* experimental */
>>  static int diff_compaction_heuristic; /* experimental */
>
> These two flags are mutually exclusive in the xdiff code, so we should
> probably handle that here.
>
> TBH, I do not care that much what:
>
>   [diff]
>   compactionHeuristic = true
>   indentHeuristic = true
>
> does. But right now:
>
>   git config diff.compactionHeuristic true
>   git show --indent-heuristic
>
> still prefers the compaction heuristic, which I think is objectively
> wrong.
>
> So perhaps we need a single variable:
>
>   enum {
> DIFF_HEURISTIC_COMPACTION,
> DIFF_HEURISTIC_INDENT
>   } diff_heuristic;
>
> and set it in last-one-wins fashion (it would be nice if the config and
> command line options were shaped the same way so it's clear to the user
> that they are exclusive, but we may have to keep --compaction-heuristic
> around for compatibility, as an alias for --diff-heuristic=compaction).
>
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> index 642cce1..ee3d812 100755
>> --- a/git-add--interactive.perl
>> +++ b/git-add--interactive.perl
>> @@ -45,6 +45,7 @@ my ($diff_new_color) =
>>  my $normal_color = $repo->get_color("", "reset");
>>
>>  my $diff_algorithm = $repo->config('diff.algorithm');
>> +my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
>>  my $diff_compaction_heuristic = 
>> $repo->config_bool('diff.compactionheuristic');
>
> Nice touch.
>
> Unfortunately the mutual-exclusivity handling will probably bleed over
> to here, too.
>
>> +/*
>> + * If a line is indented more than this, get_indent() just returns this 
>> value.
>> + * This avoids having to do absurd amounts of work for data that are not
>> + * human-readable text, and also ensures that the output of get_indent fits 
>> within
>> + * an int.
>> + */
>> +#define MAX_INDENT 200
>
> Speaking of absurd amounts of work, I was curious if there was a
> noticeable performance penalty for using this heuristic (just because
> it's a lot more complicated than the others). I couldn't detect any
> differences running "git log -p --no-merges -3000" on git.git with no
> heuristic, compaction, and indent. There may be other repositories that
> behave more pathologically (it looks like having 20 blank lines at the
> end of each hunk?), but I'd guess in most cases this will always be
> drowned out in the noise of doing the actual diff.
>
>> +#define START_OF_FILE_BONUS 9
>> +#define END_OF_FILE_BONUS 46
>> +#define TOTAL_BLANK_WEIGHT 4
>> +#define PRE_BLANK_WEIGHT 16
>> +#define RELATIVE_INDENT_BONUS -1
>> +#define RELATIVE_INDENT_HAS_BLANK_BONUS 15
>> +#define RELATIVE_OUTDENT_BONUS -19
>> +#define RELATIVE_OUTDENT_HAS_BLANK_BONUS 2
>> +#define RELATIVE_DEDENT_BONUS -63
>> +#define RELATIVE_DEDENT_HAS_BLANK_BONUS 50
>
> I see there is a comment below here mentioning that these are empirical
> voodoo, but it might be worth one at the top (or just moving these below
> the comment) because the comment looks like it's just associated with
> the function (and these are sufficiently bizarre that anybody reading is
> going to double-take on them).
>
>> +return 10 * score - bonus;
>
> I don't mind this not "10" not being a #define constant, but after
> reading the exchange between you and Stefan, I think it would be nice to
> describe what it is in a comment. The rest of the function is commented
> so nicely that this one left me thinking "huh?" upon seeing the "10".

After a night of sleep I agree with Peffs statement here, it's not about the
#define, it's about the comment. (which the #define would have given in a
short cryptic way in angry capital letters).

I have just reread the scoring function and I think you could pull out the
`score=indent` assignment (it is always assigned except for indent <0)

if (indent == -1)
   score = 0;
else
   score = indent;
... lots of bonus computation below, which in its current implementation
have lots of "score = indent;" lines as well.

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

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

[Junio C Hamano]

Pranit Bauva  writes:

> Hey Junio,
>
> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
>> Pranit Bauva  writes:
>>
 Also you do not seem to check the error from the function to smudge
 the "result" you are returning from this function.
>>>
>>> Yes I should combine the results from every removal.
>>>
 Isn't unlink_or_warn() more correct helper to use here?
>>>
>>> The shell code uses rm -f which is silent and it removes only if
>>> present.
>>
>> Isn't that what unlink_or_warn() do?  Call unlink() and happily
>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
>> exist in the first place), but otherwise reports an error (imagine:
>> EPERM).
>
> Umm, I am confused. I tried "rm -f" with a non-existing file and it
> does not show any warning or error.

You are, or you were?  I hope it is the latter, iow, you are no
longer confused and now understand why unlink_or_warn() was
suggested.

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

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Stefan Beller]

On Thu, Aug 4, 2016 at 8:58 AM, Johannes Schindelin
 wrote:
>
>> If we were to change our workflows drastically, I'd propose to
>> go a way[1] similar to notedb in Gerrit, or git-series,
>
> Gerrit is a huge, non-distributed system. Complex, too. If we change the
> patch submission process, we should make things *easier*, not *harder*. So
> I think Gerrit is pretty much out of the question.

I did not propose to use Gerrit or git-series or git appraise.

However whenever I see these projects talking to each other, the talk focuses on
a "unified review storage format" pretty often, which would make them compatible
with each other. So then I could choose git-series, while you could go with
git appraise (although that is written in go, so maybe too fancy already ;)
Or there could be another new program written in C that follows the "review
format".


>
> Even requiring every contributor to register with GitHub would be too much
> of a limitation, I would wager.
>
> And when I said I have zero interest in tools that use the "latest and
> greatest language", I was hinting at git-series. Rust may be a fine and
> wonderful language. Implementing git-series in Rust, however, immediately
> limited the potential engagement with developers dramatically.
>
> Additionally, I would like to point out that defining a way to store
> reviews in Git is not necessarily improving the way our code contribution
> process works. If you want to record the discussions revolving around the
> code, I think public-inbox already does a pretty good job at that.

Yeah recording is great, but we want to talk about replying and modifying
a series? So if I see a patch flying by on the mailing list, ideally I could
attach a "!fixup, signed off by Stefan" thing to that patch. (I said "thing"
as I do not necessarily mean email here.

>
> I guess I have no really good idea yet, either, how to retain the ease of
> access of sending mails to the list, yet somehow keep a strong tie with
> the original data stored in Git.

Does it have to be email? Transmitting text could be solved differently as well.

With git push/fetch we can interact with a git remote and pertain the state
(commits, ancestor graph) at a full level even including notes that comment
on commits.

git send-email/format-patch recently learned to include a base commit
(xy/format-patch-base), maybe we need a counter part to git send-email
that downloads a series from your mailbox, such that a local branch
can be transmitted to via

"git send-email --base=origin/master --include-notes --name=sb/new-series"

and completely reconstructed (i.e. the commit sha1s even match) including
notes via:

git fetch-email --name=sb/new-series

That way would ensure we have a "simple" way to transmit patches back and forth
and adding potential fixups.


You wrote:
> In short, I agree that our patch submission process is a saber tooth tiger
> that still reflects pre-Git times. While we use Git's tools, the workflow
> really tries to cut out Git as much as possible, in favor of pure mails
> with non-corrupted, non-HTML patches in them, a charmingly anachronistic
> requirement until you try to use state-of-the-art mail clients to send
> them.

And there are two ways out:
* either we teach git how to deal with emails (completely, i.e.
sending+receiving)
* or we change the development model (e.g. no emails any more)

There is no golden third way IMHO.

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

Re: obsolete index in wt_status_print after pre-commit hook runs

[Junio C Hamano]

Andrew Keller  writes:

> In summary, I think I prefer #2 from a usability point of view, however I’m 
> having
> trouble proving that #1 is actually *bad* and should be disallowed.

Yeah, I agree with your argument from the usability and safety point
of view.

> Any thoughts?  Would it be better for the pre-commit hook to be
> officially allowed to edit the index [1], or would it be better
> for the pre-commit hook to explicitly *not* be allowed to edit the
> index [2], or would it be yet even better to simply leave it as it
> is?

It is clear that our stance has been the third one so far.

Another thing I did not see in your analysis is what happens if the
user is doing a partial commit, and how the changes made by
pre-commit hook is propagated back to the main index and the working
tree.

The HEAD may have a file with contents in the "original" state, the
index may have the file with "update 1", and the working tree file
may have it with "update 2".  After the commit is made, the user
will continue working from a state where the HEAD and the index have
"update 1", and the working tree has "update 2".  "git diff file"
output before and after the commit will be identical (i.e. the
difference between "update 1" and "update 2") as expected.

If pre-commit were allowed to munge the index to have the file in
the "update 3" state, the resulting commit would have that version
of the file in its tree.  By definition, "update 1" and "update 3"
are different (that is what it means to allow pre-commit to munge
the index); where should the differences between "update 1" and
"update 3" go?  It is clear that pre-commit thought that the
contents in the "update 1" state is bad and "update 3" state is
better (that is why it made that fix), so after the commit is made,
we would want to have "update 3" in the index.  But what would you
do to the working tree file, which is in "update 2" state?  If you
do not do anything, "git diff" would show the remaining edit the
user had before starting the commit (i.e. difference between "update
1" and "update 2") plus a reversion of the edit pre-commit made
because what the working tree has, "update 2", is based on "update 1"
and has never heard of the change pre-commit did.

But leaving the working tree file as-is is the only safe choice, as
I do not think we want "git commit" to _create_ new conflict in the
working tree by attempting to merge (we _could_, and implementing it
would be a trivial thing to do by calling ll_merge() to three-way
merge "update 2" and "update 3" that are both based on "update 1",
but the result from the end-user's point of view is too _weird_).

So, I tend to think we should not allow pre-commit to munge the
index.  We should be able to detect fairly cheaply if pre-commit
munged the index by remembering the trailing SHA-1 of the index file
given to the pre-commit hook before running it, and reading the
trailing SHA-1 of the index file left after the pre-commit hook and
comparing them.  And we would yell at the user that his pre-commit
munged the index and abort.

Or something like that.



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

Re: [PATCH v4 03/12] pkt-line: add packet_flush_gentle()

[Junio C Hamano]

Jeff King  writes:

>   2. It calls check_pipe(), which will turn EPIPE into death-by-SIGPIPE
>  (in case you had for some reason ignored SIGPIPE).
> ...
>
> Thinking about (2), I'd go so far as to say that the trace actually
> should just be using:
>
>   if (write_in_full(...) < 0)
>   warning("unable to write trace to ...: %s", strerror(errno));
>
> and we should get rid of write_or_whine_pipe entirely.

I like the simplicity the above suggestion gives us.

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

Re: [PATCH v4 01/12] pkt-line: extract set_packet_header()

[Junio C Hamano]

Jeff King  writes:

> The cost of write() may vary on other platforms, but the cost of memcpy
> generally shouldn't. So I'm inclined to say that it is not really worth
> micro-optimizing the interface.
>
> I think the other issue is that format_packet() only lets you send
> string data via "%s", so it cannot be used for arbitrary data that may
> contain NULs. So we do need _some_ other interface to let you send a raw
> data packet, and it's going to look similar to the direct_packet_write()
> thing.

OK.  That is a much better argument than "I already stuff the length
bytes in my buffer" (which will invite "How about stop doing that?")
to justify a new "I have N bytes of data, send it out", whose
signature would look more like write(2) and deserve to be called
packet_write() but unfortunately the name is taken by what should
have called packet_fmt() or something, but that squats on a good
name packet_write().  Sigh.





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

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> > Something like this will make the test more consistent with the rest of
>> > the file:
>> > 
>> > diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
>> > index 5f29664..e5bbbd8 100755
>> > --- a/t/t5533-push-cas.sh
>> > +++ b/t/t5533-push-cas.sh
>> > @@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
>> >(
>> >cd src &&
>> >git checkout -b branch master &&
>> > -  test_commit c
>> > +  test_commit F
>> >) &&
>> >(
>> >cd dst &&
>> 
>> Confirmed. This patch fixes the issue!
>
> Funny. I worked heads-down to have some kind of Continuous Integration to
> run on my laptop, and this breakage came up. I fixed it locally, and only
> then did it occur to me that it might have been fixed already, and then I
> found this mail with a patch identical to mine.
>
> Will send out the patch in a moment.

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

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

[Pranit Bauva]

Hey Junio,

On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>>> Also you do not seem to check the error from the function to smudge
>>> the "result" you are returning from this function.
>>
>> Yes I should combine the results from every removal.
>>
>>> Isn't unlink_or_warn() more correct helper to use here?
>>
>> The shell code uses rm -f which is silent and it removes only if
>> present.
>
> Isn't that what unlink_or_warn() do?  Call unlink() and happily
> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
> exist in the first place), but otherwise reports an error (imagine:
> EPERM).

Umm, I am confused. I tried "rm -f" with a non-existing file and it
does not show any warning or error.

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

[PATCH 2/2] nedmalloc: work around overzealous GCC 6 warning

[Johannes Schindelin]

With GCC 6, the strdup() function is declared with the "nonnull"
attribute, stating that it is not allowed to pass a NULL value as
parameter.

In nedmalloc()'s reimplementation of strdup(), Postel's Law is heeded
and NULL parameters are handled gracefully. GCC 6 complains about that
now because it thinks that NULL cannot be passed to strdup() anyway.

Let's just shut up GCC >= 6 in that case and go on with our lives.

See https://gcc.gnu.org/gcc-6/porting_to.html for details.

Signed-off-by: Johannes Schindelin 
---
 compat/nedmalloc/nedmalloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index 677d1b2..3f28c0b 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -956,6 +956,9 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems, 
size_t *sizes, void **
 char *strdup(const char *s1)
 {
char *s2 = 0;
+#if __GNUC__ >= 6
+#pragma GCC diagnostic ignored "-Wnonnull-compare"
+#endif
if (s1) {
size_t len = strlen(s1) + 1;
s2 = malloc(len);
-- 
2.9.0.281.g286a8d9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Patches to let Git build with GCC 6 and DEVELOPER=SureWhyNot

[Johannes Schindelin]

While working on some local setup to allow my poor little laptop to
build and test the Windows-specific patches of Git for Windows on top of
the upstream branches, my development environment updated to use GCC 6,
and these patches were required.


Johannes Schindelin (2):
  nedmalloc: fix misleading indentation
  nedmalloc: work around overzealous GCC 6 warning

 compat/nedmalloc/nedmalloc.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/gcc-6-v1
Fetch-It-Via: git fetch https://github.com/dscho/git gcc-6-v1
-- 
2.9.0.281.g286a8d9

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

[PATCH 1/2] nedmalloc: fix misleading indentation

[Johannes Schindelin]

Some code in nedmalloc is indented in a funny way that could be
misinterpreted as if a line after a for loop was included in the loop
body, when it is not.

GCC 6 complains about this in DEVELOPER=YepSure mode.

Signed-off-by: Johannes Schindelin 
---
 compat/nedmalloc/nedmalloc.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/compat/nedmalloc/nedmalloc.c b/compat/nedmalloc/nedmalloc.c
index a0a16eb..677d1b2 100644
--- a/compat/nedmalloc/nedmalloc.c
+++ b/compat/nedmalloc/nedmalloc.c
@@ -938,10 +938,10 @@ void **nedpindependent_comalloc(nedpool *p, size_t elems, 
size_t *sizes, void **
void **ret;
threadcache *tc;
int mymspace;
-size_t i, *adjustedsizes=(size_t *) alloca(elems*sizeof(size_t));
-if(!adjustedsizes) return 0;
-for(i=0; i

Re: [RFC/PATCH] rebase--interactive: Add "sign" command

[Junio C Hamano]

Jeff King  writes:

> On Wed, Aug 03, 2016 at 09:08:48AM -0700, Junio C Hamano wrote:
>
>> > However, I could imagine that we actually want this to be more extensible.
>> > After all, all you are doing is to introduce a new rebase -i command that
>> > does nothing else than shelling out to a command.
>> 
>> Yup, I tend to agree.
>> 
>> Adding "sign" feature (i.e. make it pass -S to "commit [--amend]")
>> may be a good thing, but adding "sign" command to do so is not a
>> great design.
>
> I'm not sure what you mean by "feature" here, but it reminded me of
> Michael's proposal to allow options to todo lines:
>
>   http://public-inbox.org/git/530da00e.4090...@alum.mit.edu/
>
> which would allow:
>
>   pick -S 1234abcd
>
> If that's what you meant, I think it is a good idea. :)

Yes, by "feature" I meant "giving the ability to decide if the
resulting commit gets signature", which can and should be orthogonal
to the choice of using editor to reword the message when the commit
is created or "--no-edit" is passed and the original message is used
verbatim.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Johannes Schindelin]

Hi Stefan,

On Wed, 3 Aug 2016, Stefan Beller wrote:

> On Wed, Aug 3, 2016 at 9:07 AM, Johannes Schindelin
>  wrote:
> >
> > On Wed, 3 Aug 2016, Junio C Hamano wrote:
> >
> >> On Wed, Aug 3, 2016 at 4:59 AM, Johannes Schindelin
> >>  wrote:
> >> >
> >> > I disagree, however, with the suggestion to sift through your `pu`
> >> > branch and to somehow replace local branches with the commits found
> >> > there.
> >>
> >> To be more in line with the "e-mailed patch" workflow, I think what I
> >> should do is to send the version I queued with fixups back to the
> >> list as follow-up.  Just like reviewers review, the maintainer
> >> reviews and queues, the original author should be able to work in the
> >> same workflow, i.e. reading and applying an improved version of the
> >> patch from her mailbox.
> >
> > You seem to assume that it isn't cumbersome for people like me to
> > extract patches out of mails and to replace existing commits using
> > those patches.
> >
> > So it probably comes as a huge surprise to you to learn that this *is*
> > cumbersome for me.
> 
> It is also cumbersome for me, because I never had the need to setup a
> proper mail client that has the strength to apply patches. The need was
> not there as I tend to apply only rarely patches by email, so I can go
> the painful way each time.

The reason is clear, too. Mail clients serve humans. That is their
purpose. Humans do not care all that much whether the text was preserved
exactly as the sender wrote it, except rich text (read: HTML), of course.

> > I got too used to the ease of git push, git pull with or without
> > --rebase, and many other Git commands. Having to transmogrify code
> > changes from commits in Git into a completely different universe:
> > plain text patches in my mailbox, and back, losing all kinds of data
> > in the process, is just not at all that easy. And it costs a lot of
> > time.
> >
> > In short: if you start "submitting patches" back to me via mail, it
> > does not help me. It makes things harder for me. In particular when
> > you add your sign-off to every patch and I have to strip it.
> 
> You don't have to strip the sign off, as it shows the flow of the patch,
> e.g.
> 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Junio C Hamano 
> Signed-off-by: Stefan Beller 
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Junio C Hamano 
> 
> may indicate you proposed a patch, Junio picked it up (and fixed a typo
> optionally), I obtained the patch (via mail, via Git?) improved it, you
> improved it further and then Junio took it and merged it upstream.

Recently, I got yelled at because I took one of Junio's patches, made a
couple of changes, and *added* my sign-off.

Before that incident, I agreed with you that it may make for a nice record
of the back-and-forth that eventually resulted in the patch in question.
Now, I am not so sure anymore.

> > If you change your workflow, I would humbly request that you do it in
> > a way that makes things easier on both sides, not harder.
> 
> When attending the Git Merge conference in May, gregkh said roughtly:
> "We deliberately waste developers time, because it is not scarce.
> Maintainers time is scarce however " and it stuck with me. (and I am a
> developer, not a maintainer ;( so at least the kernel community deems it
> ok to waste my time).

Yeah. It was not the only thing I disagreed with in his talk. To be a
little bit blunt (by my standards, not by LKML's standards, that is): the
Linux kernel mailing list is not necessarily anything I would want to use
as a role model.

I agree that maintainers' time is scarce.

I am one.

So of course I agree with that statement. What I disagree with is that it
is okay to *waste* contributors' time. That's just inconsiderate. And I
say that also because I am a contributor *in addition* to being a
maintainer.

As a consequence, I commend Greg for recognizing that the patch submission
process must be light on the maintainer. And I would have commended him
even further if he had realized that proper tooling should waste nothing,
and no one's time.

> While that is true for the kernel community, I guess it is also true for
> the Git community, unless Junio (and the community) want to appoint a
> bunch of maintainer lieutenants, such that they outnumber the number of
> developers, e.g. divided by areas of the code: a refs backend
> maintainer, a submodule maintainer, ...  or rather by area of usage: a
> porcelain UI maintainer, a git-on-server maintainer.

As I mentioned earlier, I do not care much about following LKML's example.

What I see on this here list is that many a potential contributor is
scared away, that we waste precious time (also the maintainer's) pointing
out in what way certain contributions do not follow the guide lines, and
that even 

Re: [PATCH v3] t7063: work around FreeBSD's lazy mtime update feature

[Duy Nguyen]

On Wed, Aug 3, 2016 at 9:07 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> If you mean to tell the user "I won't describe it in detail, if you
>> really want to know,
>> go run blame yourself", spell it out like so. I was hoping that you
>> can summarize
>> in-line there to help the readers here.
>
> Here is a proposed fixup.

Great! Sorry I only have one or two hours these days and could not
propose something else quicker.

>  t/t7063-status-untracked-cache.sh | 16 ++--
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/t/t7063-status-untracked-cache.sh 
> b/t/t7063-status-untracked-cache.sh
> index d31d080..e0a8228 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -4,12 +4,16 @@ test_description='test untracked cache'
>
>  . ./test-lib.sh
>
> -# On some filesystems (e.g. FreeBSD's ext2 and ufs) this and that
> -# happens when we do blah, which forces the untracked cache code to
> -# take the slow path.  A test that wants to make sure the fast path
> -# works correctly should call this helper to make mtime of the
> -# containing directory in sync with the reality after doing blah and
> -# before checking the fast path behaviour
> +# On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
> +# is updated lazily after contents in the directory changes, which
> +# forces the untracked cache code to take the slow path.  A test
> +# that wants to make sure that the fast path works correctly should
> +# call this helper to make mtime of the containing directory in sync
> +# with the reality before checking the fast path behaviour.
> +#
> +# See <20160803174522.5571-1-pclo...@gmail.com> if you want to know
> +# more.
> +
>  sync_mtime () {
> find . -type d -ls >/dev/null
>  }



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

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

[Junio C Hamano]

Pranit Bauva  writes:

>> Also you do not seem to check the error from the function to smudge
>> the "result" you are returning from this function.
>
> Yes I should combine the results from every removal.
>
>> Isn't unlink_or_warn() more correct helper to use here?
>
> The shell code uses rm -f which is silent and it removes only if
> present.

Isn't that what unlink_or_warn() do?  Call unlink() and happily
return if unlink() succeeds or errors with ENOENT (i.e. path didn't
exist in the first place), but otherwise reports an error (imagine:
EPERM).

> So it makes me wonder which would be more appropriate
> unlink_or_warn() or remove_or_warn() or remove_path(). Is
> remove_path() different from its shell equivalent "rm -f"?

Read it again.

>>> + remove_path(git_path_bisect_start());
>>
>> I can see that refs/files-backend.c misuses it already, but
>> remove_path() helper is about removing a path in the working tree,
>> together with any parent directory that becomes empty due to the
>> removal.  You do not expect $GIT_DIR/ to become an empty directory
>> after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even
>> if it becomes empty.  It is a wrong helper function to use here.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  writes:

>>> + res = fprintf(fp, "%s\n%s\n", bad, good);
>>> + res |= fclose(fp);
>>> + return (res < 0) ? -1 : 0;
>>> +}
>>
>> If fprintf(3) were a function that returns 0 on success and negative
>> on error (like fclose(3) is), the pattern to cascade the error
>> return with "res |= another_call()" is appropriate, but the made me
>> hiccup a bit while reading it.  It is not wrong per-se and it would
>> certainly be making it worse if we did something silly like
>>
>> res = fprintf(...) < 0 ? -1 : 0;
>> res |= fclose(fp);
>>
>> so I guess what you have is the most succinct way to do this.
>
> I agree with your point and your suggested code is better!

Puzzled... Read it again, I was not suggesting it---I was saying
"this could be a silly rewrite, which I think is making it worse".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: patch submission process, was Re: [PATCH v6 06/16] merge_recursive: abort properly upon errors

[Johannes Schindelin]

Hi Peff,

On Wed, 3 Aug 2016, Jeff King wrote:

> On Wed, Aug 03, 2016 at 09:53:18AM -0700, Junio C Hamano wrote:
> 
> > > Leaving aside Dscho's questions of whether pulling patches from email is
> > > convenient for most submitters (it certainly is for me, but I recognize
> > > that it is not for many), I would much rather see incremental fixup
> > > patches from you than whole "here's what I queued" responses.
> > 
> > Ah, yes, I misspoke.  It should be either an incremental diff or
> > in-line comment to spell out what got changed as a response to the
> > patch.
> > 
> > I find myself fixing the title the most often, which is part of the
> > "log message" you pointed out that would not convey well with the
> > "incremental diff" approach.
> 
> I mentioned a micro-format elsewhere in my message. And it certainly is
> nice to have something that can be applied in an automatic way.

Indeed. This is what I meant by my (succinct to the point of being
intelligible, admittedly) reword! suggestion.

Let's clarify this idea.

I find myself using fixup! and squash! commits a lot. Actually, let me
pull out the Linux key for that. I use those commits A LOT.

I know, I opposed the introduction of this feature initially (and I think
that my concerns were nicely addressed by Junio's suggestion to guard this
feature behind the --autosquash option). Guess what: I was wrong.

And I am really missing the same functionality for the commit message
munging. These days, I find myself using `git commit --allow-empty
--squash=$COMMIT -c $COMMIT` very often, duplicating the first line,
adding an empty line between them, deleting the "squash! " prefix from the
now-third line, and then editing the commit message as I want to. When it
comes to cleaning up the branch via rebase -ki, I simply jump to the empty
line after the squash! line and delete everything before it.

This is as repetitive, tedious and un-fun to me as having to transmogrify
patches from the nice and cozy Git universe into the not-at-all compatible
universe of mails (I congratulate you personally, Peff, for finding a mail
client that works for you. I am still looking for one that does not suck,
Alpine being the least sucky I settled for).

So my idea was to introduce a new --reword= option to `git commit`
that would commit an empty patch and let the user edit the commit message,
later replacing the original one with the new one. This is not *quite* as
nice as I want it, because it makes the changes unobvious. On the other
hand, I would not find a series of sed commands more obvious, in
particular because that limits you in the ways of sed. And, you know,
regexp. I like them, but I know many people cannot really work with them.

> But in practice, most review comments, for the commit message _or_ the
> text, are given in human-readable terms. And as a human, I read and
> apply them in sequence.

So true. I do the very same.

> That pushes work onto the submitter, but saves work from the reviewers,
> who can quickly say "something like this..." without having to worry
> about making a full change, formatting it as a diff, etc.
> 
> I do think that's the right time-tradeoff to be making, as we have more
> submitters than reviewers.

I agree that it is the right trade-off. TBH I was shocked when I learned
how much effort Junio puts into applying my patches. I do not want that. I
want my branch to reflect pretty precisely (modulo sign-off, of course)
what is going to be integrated into Git's source code.

I'd much prefer to resubmit a cleaned-up version, even if it was just the
commit subjects, and be certain that `pu` and my branch are on the same
page.

Instead, Junio puts in a tremendous amount of work, and it does not help
anybody, because the local branches *still* do not have his fixups, and as
a consequence subsequent iterations of the patch series will have to be
fixed up *again*.

Just compare https://github.com/git/git/compare/1fd7e78...6999bc7 to
https://github.com/dscho/git/compare/f8f7adc...3b4494c (the onelines are
enough to show you just how different things are).

I'd much prefer the contributor (me, in this case) to put in a little more
work, and have things consistent. And avoid unnecessary work on both
sides.

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

Problem with two copies of same branch diverging

[Ed Greenberg]

Hi, Thanks for reading my question.

I have two copies of code checked out at the same branch. Desktop and 
remote server.


I use an IDE that automatically SFTP transfers each save from the 
desktop to the remote server, so I can run my changes on the server 
environment.


At the end of the session, I commit the code on my desktop, do a git 
push to the repo.


When I look at the server, the code there is identical to what's on my 
desktop box and what I just comitted and pushed, but, of course, git 
status thinks it's all modified and wants me to either commit it or 
stash it.  In fact, doing a git log on the server doesn't show my latest 
push.  So I need to pull the changes, but I can't because I have pending 
stuff.


What's a good git workflow for this save-upload-remote test cycle?

Thanks,

--
Ed Greenberg
Glens Falls, NY USA

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

Re: [PATCH] import-tars: support hard links

[Johannes Schindelin]

Hi Junio,

On Wed, 3 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > ---
> > Published-As: 
> > https://github.com/dscho/git/releases/tag/import-tars-hardlink-v1
> 
> A link to a page that lets you download entire source tarball is not
> very useful to most people, except for those who want "this exact
> change on top of some unknown base which may or may not have other
> things they need", which I think is a minority.

True. I added a second line that describes how to fetch it (see the t5533
patch I just sent out).

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

[PATCH] diff-highlight: Add comment for our assumption about --graph output.

[Brian Henderson]

---
 contrib/diff-highlight/diff-highlight | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/diff-highlight/diff-highlight 
b/contrib/diff-highlight/diff-highlight
index ec31356..9364423 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -20,6 +20,9 @@ my @NEW_HIGHLIGHT = (
 my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
+
+# The patch portion of git log -p --graph should only ever have preceding | and
+# not / or \ as merge history only shows up on the commit line.
 my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
 
 my @removed;
-- 
2.9.0

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

[PATCH] t5533: make it pass on case-sensitive filesystems

[Johannes Schindelin]

The newly-added test case wants to commit a file "c.t" (note the lower
case) when a previous test case already committed a file "C.t". This
confuses Git to the point that it thinks "c.t" was not staged when "git
add c.t" was called.

Simply make the naming of the test commits consistent with the previous
test cases: use upper-case, and advance in the alphabet.

This came up in local work to rebase the Windows-specific patches to the
current `next` branch. An identical fix was suggested by John Keeping.

Signed-off-by: Johannes Schindelin 
---
Published-As: 
https://github.com/dscho/git/releases/tag/t5533-case-insensitive-v1
Fetch-It-Via: git fetch https://github.com/dscho/git t5533-case-insensitive-v1
 t/t5533-push-cas.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 09899af..a2c9e74 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
(
cd src &&
git checkout -b branch master &&
-   test_commit c
+   test_commit F
) &&
(
cd dst &&
-- 
2.9.0.281.g286a8d9

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

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

[Johannes Schindelin]

Hi Lars & John,

On Thu, 4 Aug 2016, Lars Schneider wrote:

> > On 04 Aug 2016, at 13:32, John Keeping  wrote:
> > 
> > On Thu, Aug 04, 2016 at 10:03:39AM +0200, Lars Schneider wrote:
> >> 
> >>> * jk/push-force-with-lease-creation (2016-07-26) 3 commits
> >>> - push: allow pushing new branches with --force-with-lease
> >>> - push: add shorthand for --force-with-lease branch creation
> >>> - Documentation/git-push: fix placeholder formatting
> >>> 
> >>> "git push --force-with-lease" already had enough logic to allow
> >>> ensuring that such a push results in creation of a ref (i.e. the
> >>> receiving end did not have another push from sideways that would be
> >>> discarded by our force-pushing), but didn't expose this possibility
> >>> to the users.  It does so now.
> >>> 
> >>> Will merge to 'next'.
> >> 
> >> t5533-push-cas.sh "16 - new branch already exists" seems to be broken 
> >> for OSX on next. Git bisect indicates that "push: add shorthand for 
> >> --force-with-lease branch creation" might be the culprit.
> >> 
> >> https://travis-ci.org/git/git/jobs/149614431
> >> https://api.travis-ci.org/jobs/149614431/log.txt?deansi=true (non-JS)
> > 
> > It seems that the test script has already done "test_commit C", so the
> > newly added "test_commit c" does nothing on a case-insensitive
> > filesystem.
> > 
> > Something like this will make the test more consistent with the rest of
> > the file:
> > 
> > diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> > index 5f29664..e5bbbd8 100755
> > --- a/t/t5533-push-cas.sh
> > +++ b/t/t5533-push-cas.sh
> > @@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
> > (
> > cd src &&
> > git checkout -b branch master &&
> > -   test_commit c
> > +   test_commit F
> > ) &&
> > (
> > cd dst &&
> 
> Confirmed. This patch fixes the issue!

Funny. I worked heads-down to have some kind of Continuous Integration to
run on my laptop, and this breakage came up. I fixed it locally, and only
then did it occur to me that it might have been fixed already, and then I
found this mail with a patch identical to mine.

Will send out the patch in a moment.

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

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

[Johannes Schindelin]

Hi Junio,

On Wed, 3 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > I do not think negative (or non-zero) return is an "abuse" at all.
> > It is misleading in the context of the function whose name has "cmp"
> > in it, but that is not the fault of this function, rather, the
> > breakage is more in the API that calls a function that wants to know
> > only equality a "cmp".  A in-code comment before the function name
> > may be appropriate:
> >
> > /*
> >  * hashmap API calls hashmap_cmp_fn, but it only wants
> >  * "does the key match the entry?" with 0 (matches) and
> >  * non-zero (does not match).
> >  */
> > static int patch_id_match(const struct patch_id *ent,
> >   const struct patch_id *key,
> >   const void *keydata)
> > {
> > ...
> 
> How about this one instead (to be squashed into 4/4)?
> 
> The updated wording directly addresses the puzzlement I initially
> felt "This returns error() which is always negative, so comparing
> (A, B) would say A < B, while comparing (B, A) would say B < A.
> Would it cause a problem in the caller?" while reading the function
> by being explicit that the sign does not matter.

Please squash it in. Kevin is on vacation and I am sure he is fine with
this change.

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

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

[Lars Schneider]

> On 04 Aug 2016, at 13:32, John Keeping  wrote:
> 
> On Thu, Aug 04, 2016 at 10:03:39AM +0200, Lars Schneider wrote:
>> 
>>> 
>>> * jk/push-force-with-lease-creation (2016-07-26) 3 commits
>>> - push: allow pushing new branches with --force-with-lease
>>> - push: add shorthand for --force-with-lease branch creation
>>> - Documentation/git-push: fix placeholder formatting
>>> 
>>> "git push --force-with-lease" already had enough logic to allow
>>> ensuring that such a push results in creation of a ref (i.e. the
>>> receiving end did not have another push from sideways that would be
>>> discarded by our force-pushing), but didn't expose this possibility
>>> to the users.  It does so now.
>>> 
>>> Will merge to 'next'.
>> 
>> t5533-push-cas.sh "16 - new branch already exists" seems to be broken 
>> for OSX on next. Git bisect indicates that "push: add shorthand for 
>> --force-with-lease branch creation" might be the culprit.
>> 
>> https://travis-ci.org/git/git/jobs/149614431
>> https://api.travis-ci.org/jobs/149614431/log.txt?deansi=true (non-JS)
> 
> It seems that the test script has already done "test_commit C", so the
> newly added "test_commit c" does nothing on a case-insensitive
> filesystem.
> 
> Something like this will make the test more consistent with the rest of
> the file:
> 
> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index 5f29664..e5bbbd8 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
>   (
>   cd src &&
>   git checkout -b branch master &&
> - test_commit c
> + test_commit F
>   ) &&
>   (
>   cd dst &&

Confirmed. This patch fixes the issue!

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

Re: [PATCH v4 2/8] status: cleanup API to wt_status_print

[Jeff Hostetler]

On 08/03/2016 05:36 PM, Junio C Hamano wrote:

Jeff Hostetler  writes:


From: Jeff Hostetler 
diff --git a/wt-status.h b/wt-status.h
index 2023a3c..a859a12 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -43,6 +43,15 @@ struct wt_status_change_data {
unsigned new_submodule_commits : 1;
  };

+ enum wt_status_format {
+   STATUS_FORMAT_NONE = 0,
+   STATUS_FORMAT_LONG,
+   STATUS_FORMAT_SHORT,
+   STATUS_FORMAT_PORCELAIN,
+
+   STATUS_FORMAT_UNSPECIFIED
+ };


Is it your editor, or is it your MUA?  This definition is indented
by one SP, which is funny.

Also throughout the series, I saw a handful of blank lines that
should be empty but are not (e.g. three tabs and nothing else on a
new line).  I've fixed them up all but I won't be sending an
interdiff just for them, so please make sure they won't resurface
when/if you reroll.


That's odd.  I'll double check everything and trim
them in case I need to resubmit this.  Sorry.

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

Re: [PATCH v4 1/8] status: rename long-format print routines

[Jeff Hostetler]

On 08/03/2016 05:28 PM, Junio C Hamano wrote:


Signed-off-by: Jeff Hostetler 
Signed-off-by: Jeff Hostetler 


Hmm, are these physically the same people?  If so, which one do you
want to be known as?


Yes, these are both my addresses.  Still struggling a little
with some SMTP issues.  Please use the jeffh...@microsoft.com address.

Thanks.
Jeff


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

[PATCH v4] pager: move pager-specific setup into the build

[Eric Wong]

Allowing PAGER_ENV to be set at build-time allows us to move
pager-specific knowledge out of our build.  This allows us to
set a better default for FreeBSD more(1), which pretends not to
understand ANSI color escapes if the MORE environment variable
is left empty, but accepts the same variables as less(1)

Originally-from:
 https://public-inbox.org/git/xmqq61piw4yf@gitster.dls.corp.google.com/

Helped-by: Junio C Hamano 
Helped-by: Jeff King 
Signed-off-by: Eric Wong 
---
  v4 changes: (diff @ <20160804113410.GA13908@starla>)
  - reworded commit messages and took ownership as suggested
privately by Junio
  - fixed git-sh-setup and add test for untested git_pager

  The following changes since commit f8f7adce9fc50a11a764d57815602dcb818d1816:

Sync with maint (2016-07-28 14:21:18 -0700)

  are available in the git repository at:

git://bogomips.org/git-svn.git pager-env-v4

  for you to fetch changes up to 3b8e70c37e96b4e19475fb6dc480a82b292bd28f:

pager: move pager-specific setup into the build (2016-08-04 11:31:28 +)

  
  Eric Wong (1):
pager: move pager-specific setup into the build

 Makefile | 21 -
 config.mak.uname |  1 +
 git-sh-setup.sh  |  8 +---
 pager.c  | 32 
 t/t7006-pager.sh | 13 +
 5 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 6a13386..fc9b017 100644
--- a/Makefile
+++ b/Makefile
@@ -370,6 +370,14 @@ all::
 # Define HAVE_BSD_SYSCTL if your platform has a BSD-compatible sysctl function.
 #
 # Define HAVE_GETDELIM if your system has the getdelim() function.
+#
+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
+# default environment variables to be passed when a pager is spawned, e.g.
+#
+#PAGER_ENV = LESS=FRX LV=-c
+#
+# to say "export LESS=FRX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1500,6 +1508,10 @@ ifeq ($(PYTHON_PATH),)
 NO_PYTHON = NoThanks
 endif
 
+ifndef PAGER_ENV
+PAGER_ENV = LESS=FRX LV=-c
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
@@ -1629,6 +1641,11 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
+PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
+PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
+BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
@@ -1753,7 +1770,7 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-   $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP)
+   $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -1766,6 +1783,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
 -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \
+-e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
 $@.sh >$@+
 endef
 
@@ -2173,6 +2191,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
+   @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/config.mak.uname b/config.mak.uname
index 17fed2f..b232908 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
HAVE_PATHS_H = YesPlease
GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
HAVE_BSD_SYSCTL = YesPlease
+   PAGER_ENV = LESS=FRX LV=-c MORE=FRX
 endif
 ifeq ($(uname_S),OpenBSD)
NO_STRCASESTR = YesPlease
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 0c34aa6..a8a4576 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -163,9 +163,11 @@ git_pager() {
else
GIT_PAGER=cat
fi
-   : "${LESS=-FRX}"
-   : "${LV=-c}"
-   export LESS LV
+   for vardef in @@PAGER_ENV@@
+   do
+   var=${vardef%%=*}
+   eval ": \"\${$vardef}\" && export $var"
+   done
 
eval "$GIT_PAGER" '"$@"'
 }
diff --git a/pager.c b/pager.c
index 4bc0481..6470b81 100644
--- a/pager.c
+++ b/pager.c
@@ -63,14 +63,38 @@ const char *git_pager(int stdout_is_tty)
   

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

[John Keeping]

On Thu, Aug 04, 2016 at 10:03:39AM +0200, Lars Schneider wrote:
> 
> > 
> > * jk/push-force-with-lease-creation (2016-07-26) 3 commits
> > - push: allow pushing new branches with --force-with-lease
> > - push: add shorthand for --force-with-lease branch creation
> > - Documentation/git-push: fix placeholder formatting
> > 
> > "git push --force-with-lease" already had enough logic to allow
> > ensuring that such a push results in creation of a ref (i.e. the
> > receiving end did not have another push from sideways that would be
> > discarded by our force-pushing), but didn't expose this possibility
> > to the users.  It does so now.
> > 
> > Will merge to 'next'.
> 
> t5533-push-cas.sh "16 - new branch already exists" seems to be broken 
> for OSX on next. Git bisect indicates that "push: add shorthand for 
> --force-with-lease branch creation" might be the culprit.
> 
> https://travis-ci.org/git/git/jobs/149614431
> https://api.travis-ci.org/jobs/149614431/log.txt?deansi=true (non-JS)

It seems that the test script has already done "test_commit C", so the
newly added "test_commit c" does nothing on a case-insensitive
filesystem.

Something like this will make the test more consistent with the rest of
the file:

diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 5f29664..e5bbbd8 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
(
cd src &&
git checkout -b branch master &&
-   test_commit c
+   test_commit F
) &&
(
cd dst &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] pager: move pager-specific setup into the build

[Eric Wong]

Jeff King  wrote:
> On Thu, Aug 04, 2016 at 03:43:01AM +, Eric Wong wrote:
> 
> > +PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
> > +PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
> > +BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
> 
> Here we set up CQ_SQ, but there is no PAGER_ENV_SQ.
> 
> And then...

> > @@ -1766,6 +1782,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> >  -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
> >  -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
> >  -e 's|@@SANE_TEXT_GREP@@|$(SANE_TEXT_GREP)|g' \
> > +-e 's|@@PAGER_ENV@@|$(PAGER_ENV_SQ)|g' \
> >  $@.sh >$@+
> >  endef
> 
> Here we depend on writing PAGER_ENV_SQ, which will be blank (and
> git-sh-setup is broken as a result).

Good catch!  And the reason we didn't notice git-sh-setup is
broken is nobody uses git_pager in-tree from that, anymore.
However, I suspect we'll have to support it indefinitely due to
custom scripts and contrib/examples.

Made the following change for v4:

diff --git a/Makefile b/Makefile
index 0b36b5e..fc9b017 100644
--- a/Makefile
+++ b/Makefile
@@ -1641,6 +1641,7 @@ ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
 endif
 
+PAGER_ENV_SQ = $(subst ','\'',$(PAGER_ENV))
 PAGER_ENV_CQ = "$(subst ",\",$(subst \,\\,$(PAGER_ENV)))"
 PAGER_ENV_CQ_SQ = $(subst ','\'',$(PAGER_ENV_CQ))
 BASIC_CFLAGS += -DPAGER_ENV='$(PAGER_ENV_CQ_SQ)'
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e4fc5c8..c8dc665 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -49,6 +49,19 @@ test_expect_success TTY 'LESS and LV envvars are set for 
pagination' '
grep ^LV= pager-env.out
 '
 
+test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
+   (
+   sane_unset LESS LV &&
+   PAGER="env >pager-env.out; wc" &&
+   export PAGER &&
+   PATH="$(git --exec-path):$PATH" &&
+   export PATH &&
+   test_terminal sh -c ". git-sh-setup && git_pager"
+   ) &&
+   grep ^LESS= pager-env.out &&
+   grep ^LV= pager-env.out
+'
+
 test_expect_success TTY 'some commands do not use a pager' '
rm -f paginated.out &&
test_terminal git rev-list HEAD &&

> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
> > HAVE_PATHS_H = YesPlease
> > GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
> > HAVE_BSD_SYSCTL = YesPlease
> > +   PAGER_ENV = LESS=FRX LV=-c MORE=FRX
> >  endif
> 
> Is it worth setting up PAGER_ENV's default values before including
> config.mak.*, and then using "+=" here? That avoids this line getting
> out of sync with the usual defaults.

Good point, but it makes ordering problematic for folks
who want to override it config.mak or command-line.

We may have to do something like we do for BASIC_CFLAGS and
such, but I'm not sure it's worth the effort when somebody
doesn't wants a different value for one of the flags.

> > +static void setup_pager_env(struct argv_array *env)
> > +{
> 
> I know you said you don't like string parsing in C. Here is a patch (on
> top of yours) that converts the parsing to shell, and generates a
> pre-built array-of-struct (this is similar to the big series I posted
> long ago, but just touching this one spot, not invading the whole
> Makefile). Feel free to ignore it as over-engineered, but I thought I'd
> throw it out there in case it appeals.

Yeah, but I'd rather not introduce more complexity into the
build process, either (unless it's a performance-sensitive part,
which this is not).  Also, while my original 2/2 to make it
configurable at runtime was discarded, I wouldn't rule out
somebody making a compelling case for it and it would be
an easier change from the parse-at-runtime code.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 10/10] convert: add filter..process option

[Jakub Narębski]

[Some of this answer might have been invalidated by v4;
 I might be away from computer for a few days, so I won't be reviewing]

W dniu 03.08.2016 o 15:10, Lars Schneider pisze:
> On 01 Aug 2016, at 00:19, Jakub Narębski  wrote:
>> W dniu 30.07.2016 o 01:38, larsxschnei...@gmail.com pisze:
[...]
 
>> Could this whole "send single file" be put in a separate function?
>> Or is it not worth it?
> 
> This function would have almost the same signature as apply_protocol2_filter
> and therefore I would say it's not worth it since the function is not
> crazy long.
 
All right.  Though I would say that if it makes the function more
readable, then it might be worth it.

[...]
>>> +
>>> +   sigchain_push(SIGPIPE, SIG_IGN);
>>
>> Hmmm... ignoring SIGPIPE was good for one-shot filters.  Is it still
>> O.K. for per-command persistent ones?
> 
> Very good question. You are right... we don't want to ignore any errors
> during the protocol... I will remove it.

I was actually just wondering.

Actually the default behavior if SIGPIPE is not ignored (or if the
SIGPIPE signal is not blocked / masked out) is to *terminate* the
writing program, which we do not want.

The correct solution is to check for error during write, and check
if errno is set to EPIPE.  This means that reader (filter driver
process) has closed pipe, usually due to crash, and we need to handle
that sanely, either restarting or quitting while providing sane
information about error to the user.

Well, we might want to set a signal handler for SIGPIPE, not just
simply ignore it (especially for streaming case; stop streaming
if filter driver crashed); though signal handlers are quite limited
about what might be done in them.  But that's for the future.


Read from closed pipe returns EOF; write to closed pipe results in
SIGPIPE and returns -1 (setting errno to EPIPE).
 
>>
>>> +
>>> +   packet_buf_write(, "%s\n", filter_type);
>>> +   ret &= !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);
>>> +
>>> +   if (ret) {
>>> +   strbuf_reset();
>>> +   packet_buf_write(, "filename=%s\n", path);
>>> +   ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);
>>> +   }
>>
>> Perhaps a better solution would be
>>
>>if (err)
>>  goto fin_error;
>>
>> rather than this.
> 
> OK, I change it to goto error handling style.

Well, at least try it and check if it makes code more readable.
 
>>> +   if (ret) {
>>> +   strbuf_reset();
>>> +   packet_buf_write(, "size=%"PRIuMAX"\n", (uintmax_t)len);
>>> +   ret = !direct_packet_write(process->in, nbuf.buf, nbuf.len, 1);
>>> +   }
>>
>> Or maybe extract writing the header for a file into a separate function?
>> This one gets a bit long...
> 
> Maybe... but I think that would make it harder to understand the protocol. I
> think I would prefer to have all the communication in one function layer.

I don't understand your reasoning here ("make it harder to understand the
protocol").  If you choose good names for function writing header, then
the main function would be the high-level view of protocol, e.g.

   git> 
   git> 
   git> 
   git> 

   git< 
   git< 
   git< 
   git< 
 
[...]
>>> +
>>> +   if (ret) {
>>> +   filter_result = packet_read_line(process->out, NULL);
>>> +   ret = !strcmp(filter_result, "success");
>>> +   }
>>> +
>>> +   sigchain_pop(SIGPIPE);
>>> +
>>> +   if (ret) {
>>> +   strbuf_swap(dst, );
>>> +   } else {
>>> +   if (!filter_result || strcmp(filter_result, "reject")) {
>>> +   // Something went wrong with the protocol filter. Force 
>>> shutdown!

Don't use C++ one-line comments (that's C99-ism).

>>> +   error("external filter '%s' failed", cmd);
>>> +   kill_protocol2_filter(_process_map, entry);
>>> +   }
>>> +   }
>>
>> So if Git gets finish signal "success" from filter, it accepts the output.
>> If Git gets finish signal "reject" from filter, it restarts filter (and
>> reject the output - user can retry the command himself / herself).
>> If Git gets any other finish signal, for example "error" (but this is not
>> standarized), then it rejects the output, keeping the unfiltered result,
>> but keeps filtering.
>>
>> I think it is not described in this detail in the documentation of the
>> new protocol.
> 
> Agreed, will add!

That would be nice.

>>> -   return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
>>> +   if (!ca.drv->clean && ca.drv->process)
>>> +   return apply_protocol2_filter(
>>> +   path, NULL, 0, -1, NULL, ca.drv->process, 
>>> FILTER_CAPABILITIES_CLEAN
>>> +   );
>>> +   else
>>> +   return apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);
>>
>> Could we augment apply_filter() instead, so that the invocation is
>>
>>return apply_filter(path, NULL, 0, -1, NULL, ca.drv, FILTER_CLEAN);
>>
>> Though I am not sure if moving this conditional to 

GIT by github 2.9.2 is listed on Software Informer

[Kasey Bloome]

Good day!

Software.informer.com would like to inform you that your product GIT by github 
2.9.2 has been reviewed by our editors and your program got "100% Clean Award" 
http://git.software.informer.com/.

We would be grateful if you place our award with a link to our review on your 
website. On our part, we can offer featuring your program in our Today's 
Highlight block. This block is shown in the rotator at the top of the main page 
and also on every page of our website in the upper right corner.

We also offer you to take advantage of our free storage by hosting your 
installation package on our servers and listing us as one of the mirror 
downloads for your application. There is a selection of predesigned buttons 
available to fit the look of your website.

Please let me know if you're interested in any of these offers.

We are on the list of the world's 500 most visited websites with over 700,000 
unique visitors every day, so this could get your application some extra 
exposure.

Kind regards,
Kasey Bloome


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

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

[Lars Schneider]

> 
> * jk/push-force-with-lease-creation (2016-07-26) 3 commits
> - push: allow pushing new branches with --force-with-lease
> - push: add shorthand for --force-with-lease branch creation
> - Documentation/git-push: fix placeholder formatting
> 
> "git push --force-with-lease" already had enough logic to allow
> ensuring that such a push results in creation of a ref (i.e. the
> receiving end did not have another push from sideways that would be
> discarded by our force-pushing), but didn't expose this possibility
> to the users.  It does so now.
> 
> Will merge to 'next'.

t5533-push-cas.sh "16 - new branch already exists" seems to be broken 
for OSX on next. Git bisect indicates that "push: add shorthand for 
--force-with-lease branch creation" might be the culprit.

https://travis-ci.org/git/git/jobs/149614431
https://api.travis-ci.org/jobs/149614431/log.txt?deansi=true (non-JS)

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

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Jeff King]

On Thu, Aug 04, 2016 at 12:00:36AM +0200, Michael Haggerty wrote:

> This table shows the number of diff slider groups that were positioned
> differently than the human-generated values, for various repositories.
> "default" is the default "git diff" algorithm. "compaction" is Git 2.9.0
> with the `--compaction-heuristic` option "indent" is an earlier,

s/option/&./

>  static int diff_detect_rename_default;
> +static int diff_indent_heuristic; /* experimental */
>  static int diff_compaction_heuristic; /* experimental */

These two flags are mutually exclusive in the xdiff code, so we should
probably handle that here.

TBH, I do not care that much what:

  [diff]
  compactionHeuristic = true
  indentHeuristic = true

does. But right now:

  git config diff.compactionHeuristic true
  git show --indent-heuristic

still prefers the compaction heuristic, which I think is objectively
wrong.

So perhaps we need a single variable:

  enum {
DIFF_HEURISTIC_COMPACTION,
DIFF_HEURISTIC_INDENT
  } diff_heuristic;

and set it in last-one-wins fashion (it would be nice if the config and
command line options were shaped the same way so it's clear to the user
that they are exclusive, but we may have to keep --compaction-heuristic
around for compatibility, as an alias for --diff-heuristic=compaction).

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 642cce1..ee3d812 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -45,6 +45,7 @@ my ($diff_new_color) =
>  my $normal_color = $repo->get_color("", "reset");
>  
>  my $diff_algorithm = $repo->config('diff.algorithm');
> +my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');
>  my $diff_compaction_heuristic = 
> $repo->config_bool('diff.compactionheuristic');

Nice touch.

Unfortunately the mutual-exclusivity handling will probably bleed over
to here, too.

> +/*
> + * If a line is indented more than this, get_indent() just returns this 
> value.
> + * This avoids having to do absurd amounts of work for data that are not
> + * human-readable text, and also ensures that the output of get_indent fits 
> within
> + * an int.
> + */
> +#define MAX_INDENT 200

Speaking of absurd amounts of work, I was curious if there was a
noticeable performance penalty for using this heuristic (just because
it's a lot more complicated than the others). I couldn't detect any
differences running "git log -p --no-merges -3000" on git.git with no
heuristic, compaction, and indent. There may be other repositories that
behave more pathologically (it looks like having 20 blank lines at the
end of each hunk?), but I'd guess in most cases this will always be
drowned out in the noise of doing the actual diff.

> +#define START_OF_FILE_BONUS 9
> +#define END_OF_FILE_BONUS 46
> +#define TOTAL_BLANK_WEIGHT 4
> +#define PRE_BLANK_WEIGHT 16
> +#define RELATIVE_INDENT_BONUS -1
> +#define RELATIVE_INDENT_HAS_BLANK_BONUS 15
> +#define RELATIVE_OUTDENT_BONUS -19
> +#define RELATIVE_OUTDENT_HAS_BLANK_BONUS 2
> +#define RELATIVE_DEDENT_BONUS -63
> +#define RELATIVE_DEDENT_HAS_BLANK_BONUS 50

I see there is a comment below here mentioning that these are empirical
voodoo, but it might be worth one at the top (or just moving these below
the comment) because the comment looks like it's just associated with
the function (and these are sufficiently bizarre that anybody reading is
going to double-take on them).

> +return 10 * score - bonus;

I don't mind this not "10" not being a #define constant, but after
reading the exchange between you and Stefan, I think it would be nice to
describe what it is in a comment. The rest of the function is commented
so nicely that this one left me thinking "huh?" upon seeing the "10".

The rest looks sane to me, though I am not sure I have absorbed all the
implications. IMHO the most interesting thing is the actual results,
though.

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

Re: [PATCH 0/8] Better heuristics make prettier diffs

[Jeff King]

On Thu, Aug 04, 2016 at 12:00:28AM +0200, Michael Haggerty wrote:

> I've talked about this quite a bit on the list already. The idea is to
> improve ugly diffs like
> 
> @@ -231,6 +231,9 @@ if (!defined $initial_reply_to && $prompting) {
>  }
> 
>  if (!$smtp_server) {
> +   $smtp_server = $repo->config('sendemail.smtpserver');
> +}
> +if (!$smtp_server) {
> foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
> if (-x $_) {
> $smtp_server = $_;

Not that you probably need more random cases of C code, but I happened
to be looking at a diff in git.git today, b333d0d6, which is another
regression for the compaction heuristic. The indent heuristic here gets
it right.

Coincidentally, another example is the final patch in this series.

So I am already happier even without digging further yet. :)

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

Re: [PATCH 3/8] xdl_change_compact(): rename i to end

[Jeff King]

On Thu, Aug 04, 2016 at 12:00:31AM +0200, Michael Haggerty wrote:

> Rename i to end, and alternate between using start and end as the
> indexing variable as appropriate.
> 
> Rename ixref to end_matching_other.
> 
> Add some more comments.

I'd usually complain that there is too much "what" in your commit
message, but in this case, the diff really is hard to read. Having a
summary up front is nice.

There's no "why", but I imagine it is just "I had to do this to even
make sense of this function".

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

Re: [PATCH 5/8] xdl_change_compact(): fix compaction heuristic to adjust io

[Jeff King]

On Thu, Aug 04, 2016 at 12:00:33AM +0200, Michael Haggerty wrote:

> The code branch used for the compaction heuristic incorrectly forgot to
> keep io in sync while the group was shifted. I think that could have
> led to reading past the end of the rchgo array.
> 
> Signed-off-by: Michael Haggerty 
> ---
> I didn't actually try to verify the presence of a bug, because it
> seems like more work than worthwhile. But here is my reasoning:
> 
> If io is not decremented correctly during one iteration of the outer
> `while` loop, then it will loose sync with the `end` counter. In
> particular it will be too large.
> 
> Suppose that the next iterations of the outer `while` loop (i.e.,
> processing the next block of add/delete lines) don't have any sliders.
> Then the `io` counter would be incremented by the number of
> non-changed lines in xdf, which is the same as the number of
> non-changed lines in xdfo that *should have* followed the group that
> experienced the malfunction. But since `io` was too large at the end
> of that iteration, it will be incremented past the end of the
> xdfo->rchg array, and will try to read that memory illegally.

Hmm. In the loop:

  while (rchgo[io])
io++;

that implies that rchgo has a zero-marker that we can rely on hitting.
And it looks like rchgo[io] always ends the loop on a 0. So it seems
like we would just hit that condition again.

That doesn't make it _right_, but I'm not sure I see how it would walk
off the end of the array.  But I'm very sure I don't understand this
code completely, so I may be missing something.

Anyway, I'd suggest putting your cover letter bits into the commit
message. Even though they are all suppositions, they are the kind of
thing that could really help somebody debugging this in 2 years, and are
better than nothing.

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

Re: [PATCH 1/8] xdl_change_compact(): rename some local variables for clarity

[Jeff King]

On Thu, Aug 04, 2016 at 12:00:29AM +0200, Michael Haggerty wrote:

> * ix -> i
> * ixo -> io
> * ixs -> start
> * grpsiz -> groupsize

After your change, I immediately understand three of them. But what is
"io"?

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

Re: [PATCH 2/8] xdl_change_compact(): clarify code

[Jeff King]

On Wed, Aug 03, 2016 at 04:50:46PM -0700, Stefan Beller wrote:

> I was not asking for undoing these, but giving short cryptic answers myself. 
> ;)
> While I agree the variable names are way better than before, the use of while
> instead of for (and then doing another final ++ after the loop) extended some
> one liners to about 5. I am totally fine with that as they are easier
> to read for me as I understand them as Git style, hence easier to read.

One thing I try to do with loops is to use "for" loops only when I truly
want an iteration from point A to point B. If I care about the value of
the iterator _after_ the loop, I prefer a "while" loop.

Not everybody necessarily has the same taste, but I assume Michael does,
since that's what's happening in this hunk:

> -   start = i;
> -   for (i++; rchg[i]; i++);
> -   for (; rchgo[io]; io++);
> +   start = i++;
> +
> +   while (rchg[i])
> +   i++;
> +
> +   while (rchgo[io])
> +  io++;

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