Re: want option to git-rebase

[Jonathan Nieder]

Johannes Sixt wrote:
> Am 19.06.2018 um 03:06 schrieb Jonathan Nieder:
>> Ian Jackson wrote[1]:

>>> git-rebase leaves entries like this in the reflog:
>>>
>>>c15f4d5391 HEAD@{33}: rebase: checkout 
>>> c15f4d5391ff07a718431aca68a73e672fe8870e
>>>
>>> It would be nice if there were an option to control this message.
>>> Particularly, when another tool invokes git-rebase,
[...]
>>  From git(1):
>>
>>   GIT_REFLOG_ACTION
>>  When a ref is updated, reflog entries are created to keep
>>  track of the reason why the ref was updated (which is
>>  typically the name of the high-level command that updated the
>>  ref), in addition to the old and new values of the ref. A
>>  scripted Porcelain command can use set_reflog_action helper
>>  function in git-sh-setup to set its name to this variable when
>>  it is invoked as the top level command by the end user, to be
>>  recorded in the body of the reflog.
>>
>> "git rebase" sets this itself, so it doesn't solve your problem.
>
> If it does so unconditionally, then that is a bug. If a script wants to set
> GIT_REFLOG_ACTION, but finds that it is already set, then it must not change
> the value. set_reflog_action in git-sh-setup does the right thing.
>
> So, if there is another script or application around git-rebase, then it
> should just set GIT_REFLOG_ACTION (if it is not already set) and export the
> environment variable to git-rebase.

Oh, good catch.  "git rebase" already generally does the right thing
when GIT_REFLOG_ACTION is set (by only appending to it and never
replacing it).

Ian, does that work well for you?  If so, any ideas where it should go
in the documentation to be more discoverable for next time?

Footnotes:

- git-rebase--interactive.sh has the following snippet:

case "$orig_reflog_action" in
''|rebase*)
GIT_REFLOG_ACTION="rebase -i ($1)"

  This is a little too aggressive, since it's possible for a
  user-specified reflog action to start with "rebase" and
  contain additional information that shouldn't be removed.

- likewise in git-rebase--preserve-merges.sh.

Thanks,
Jonathan

Re: [PATCH] ewah: delete unused 'rlwit_discharge_empty()'

[Derrick Stolee]

On 6/19/2018 5:51 PM, Ramsay Jones wrote:

From: Junio C Hamano 

Complete the removal of unused 'ewah bitmap' code by removing the now
unused 'rlwit_discharge_empty()' function. Also, the 'ewah_clear()'
function can now be made a file-scope static symbol.

Signed-off-by: Ramsay Jones 
---

Hi Junio,

Can you please add this to the 'ds/ewah-cleanup' branch, before
we forget to do so! ;-)

Thanks!

ATB,
Ramsay Jones



Looks good to me! Thanks!

-Stolee




  ewah/ewah_bitmap.c | 20 
  ewah/ewah_rlw.c|  8 
  ewah/ewok.h|  6 --
  ewah/ewok_rlw.h|  1 -
  4 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index 017c677f9..d59b1afe3 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -276,6 +276,18 @@ void ewah_each_bit(struct ewah_bitmap *self, void 
(*callback)(size_t, void*), vo
}
  }
  
+/**

+ * Clear all the bits in the bitmap. Does not free or resize
+ * memory.
+ */
+static void ewah_clear(struct ewah_bitmap *self)
+{
+   self->buffer_size = 1;
+   self->buffer[0] = 0;
+   self->bit_size = 0;
+   self->rlw = self->buffer;
+}
+
  struct ewah_bitmap *ewah_new(void)
  {
struct ewah_bitmap *self;
@@ -288,14 +300,6 @@ struct ewah_bitmap *ewah_new(void)
return self;
  }
  
-void ewah_clear(struct ewah_bitmap *self)

-{
-   self->buffer_size = 1;
-   self->buffer[0] = 0;
-   self->bit_size = 0;
-   self->rlw = self->buffer;
-}
-
  void ewah_free(struct ewah_bitmap *self)
  {
if (!self)
diff --git a/ewah/ewah_rlw.c b/ewah/ewah_rlw.c
index b9643b7d0..5093d43e2 100644
--- a/ewah/ewah_rlw.c
+++ b/ewah/ewah_rlw.c
@@ -104,11 +104,3 @@ size_t rlwit_discharge(
  
  	return index;

  }
-
-void rlwit_discharge_empty(struct rlw_iterator *it, struct ewah_bitmap *out)
-{
-   while (rlwit_word_size(it) > 0) {
-   ewah_add_empty_words(out, 0, rlwit_word_size(it));
-   rlwit_discard_first_words(it, rlwit_word_size(it));
-   }
-}
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 0c504f28e..84b2a29fa 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -72,12 +72,6 @@ void ewah_pool_free(struct ewah_bitmap *self);
   */
  struct ewah_bitmap *ewah_new(void);
  
-/**

- * Clear all the bits in the bitmap. Does not free or resize
- * memory.
- */
-void ewah_clear(struct ewah_bitmap *self);
-
  /**
   * Free all the memory of the bitmap
   */
diff --git a/ewah/ewok_rlw.h b/ewah/ewok_rlw.h
index bb3c6ff7e..7cdfdd0c0 100644
--- a/ewah/ewok_rlw.h
+++ b/ewah/ewok_rlw.h
@@ -98,7 +98,6 @@ void rlwit_init(struct rlw_iterator *it, struct ewah_bitmap 
*bitmap);
  void rlwit_discard_first_words(struct rlw_iterator *it, size_t x);
  size_t rlwit_discharge(
struct rlw_iterator *it, struct ewah_bitmap *out, size_t max, int 
negate);
-void rlwit_discharge_empty(struct rlw_iterator *it, struct ewah_bitmap *out);
  
  static inline size_t rlwit_word_size(struct rlw_iterator *it)

  {

Re: [PATCH 0/8] ref-in-want

[Jonathan Tan]

> On 06/15, Jonathan Tan wrote:
> > 
> > Supporting patterns would mean that we would possibly be able to
> > eliminate the ls-refs step, thus saving at least a RTT. (Originally I
> > thought that supporting patterns would also allow us to tolerate refs
> > being removed during the fetch process, but I see that this is already
> > handled by the server ignoring "want-ref " wherein  doesn't
> > exist on the server.)
> 
> What's your opinion on this?  Should we keep it how it is in v2 of the
> series where the server ignores refs it doesn't know about or revert to
> what v1 of the series did and have it be a hard error?

I think it should be like in v2 - the server should ignore "want-ref
" lines for refs it doesn't know about. And, after more thought, I
think that the client should die if "fetch " was not
fulfilled, and ignore if a ref in "fetch " was not
fulfilled.

The advantage of doing that is that we make the protocol a bit more
tolerant to adverse conditions (e.g. a rapidly changing repository or an
eventually consistent load-balancing setup), while having little-to-no
effect on regular conditions.

The disadvantage is that there is now one additional place where a
failure can silently occur, but I think that this is a minor
disadvantage. A naive script using "git fetch", in my mind, would assume
that refs/heads/exact exists if "fetch
refs/heads/exact:refs/heads/exact" succeeds, but would not assume that
refs/heads/wildcard-something exists if "fetch
refs/heads/wildcard*:refs/heads/wildcard*" succeeds, which fits in
nicely with the die/ignore behavior I outlined above.

Hello

[Sare Ouedraogo]

--
Am Mr.Sare Ouedraogo.i work in one of the prime bank here in Burkina 
Faso, i want the bank to transfer the money left by our  late customer 
is a foreigner from Korea. can you invest this money  and also help the 
poor'  the amount value at  $13,300,000.00  (Thirteen Million Three 
Hundred Thousand United States American Dollars), left in his account 
still unclaimed. more details will be given to you if you are 
interested.

best regards
Mr. Sare Ouedraogo
--

Re: [PATCH 0/8] ref-in-want

[Brandon Williams]

On 06/15, Jonathan Tan wrote:
> 
> Supporting patterns would mean that we would possibly be able to
> eliminate the ls-refs step, thus saving at least a RTT. (Originally I
> thought that supporting patterns would also allow us to tolerate refs
> being removed during the fetch process, but I see that this is already
> handled by the server ignoring "want-ref " wherein  doesn't
> exist on the server.)

What's your opinion on this?  Should we keep it how it is in v2 of the
series where the server ignores refs it doesn't know about or revert to
what v1 of the series did and have it be a hard error?

I've gone back and forth on what I think we should do so I'd like to
hear at least one more opinion :)

-- 
Brandon Williams

Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

[Brandon Williams]

On 06/19, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > I also think that we should keep this first implementation of
> > ref-in-want simple and *not* include patterns, even if that's what we
> > may want someday down the road.  Adding a new capability in the future
> > for support of such patterns would be relatively simple and easy.
> 
> I am all for many-small-steps over a single-giant-step approach.
> 
> >  The
> > reason why I don't think we should add pattern support just yet is due
> > to a request for "want-ref refs/tags/*" or a like request resulting in a
> > larger than expected packfile every time "fetch --tags" is run.  The
> > issue being that in a fetch request "refs/tags/*" is too broad of a
> > request and could be requesting 100s of tags when all we really wanted
> > was to get the one or two new tags which are present on the remote
> > (because we already have all the other tags present locally).
> 
> I do not quite get this.  Why does it have to result in a large
> packfile?  Doesn't the requester of refs/tags/* still show what it
> has via "have" exchange?

Sorry Jonathan Tan said it much clearer here:
https://public-inbox.org/git/20180615190458.147775-1-jonathanta...@google.com/

-- 
Brandon Williams

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Taylor Blau]

On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote:
> Hi,
>
> Attached is a ``fresh start'' of my series to teach 'git grep --column'.
> Since the last time I sent this, much has changed, notably the semantics
> for deciding which column is the first when given (1) extended
> expressions and (2) --invert.

Hi (again),

I have posted an updated version of this series on my fork available at
[1], in case anyone is interested in the changes before I publish them
here tomorrow.

I am going to let this thread sit overnight to collect any more
feedback.


Thanks,
Taylor

[1]: https://github.com/ttaylorr/git/compare/tb/grep-colno

Re: The state of the object store series

[Stefan Beller]

On Tue, Jun 19, 2018 at 3:37 PM Jonathan Tan  wrote:
>
> > Floating on the mailing list, not cooking yet:
>
> One more is my bitmap one here:

Oh right. Thanks for writing the series!

When writing this I dabbled back and forth whether I only present the series
that are on the critical path to reach the submodule goal or to
include all of them
(see Duys series) and that is how I missed yours.

> https://public-inbox.org/git/cover.1528397984.git.jonathanta...@google.com/
>
> It's not in any branch yet, as far as I can tell, so I've just sent out
> an e-mail letting Junio know [1].

Thanks!

>
> [1] 
> https://public-inbox.org/git/20180619222749.124671-1-jonathanta...@google.com/

Re: The state of the object store series

[Jonathan Tan]

> Floating on the mailing list, not cooking yet:

One more is my bitmap one here:

https://public-inbox.org/git/cover.1528397984.git.jonathanta...@google.com/

It's not in any branch yet, as far as I can tell, so I've just sent out
an e-mail letting Junio know [1].

[1] 
https://public-inbox.org/git/20180619222749.124671-1-jonathanta...@google.com/

Re: Adding nested repository with slash adds files instead of gitlink

[Rafael Ascensão]

On Tue, Jun 19, 2018 at 11:28 AM Heiko Voigt  wrote:
>
> Interesting and nobody complained to the mailinglist?
>

For reference this was sometimes called "Fake Submodules" online.

Re: What's cooking in git.git (Jun 2018, #05; Mon, 18)

[Jonathan Tan]

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

Would it be possible to have my patches that make bitmap_git not global
[1] in this list? Peff seems OK with it. Let me know if you'd like to
see anything else.

The original patch should contain an extra paragraph that I've provided
here [2] in the commit message - let me know if you want a reroll with
that extra paragraph included.

[1] https://public-inbox.org/git/cover.1528397984.git.jonathanta...@google.com/

[2] 
https://public-inbox.org/git/2018065046.d03f8093347dc6c0e9b11...@google.com/

The state of the object store series

[Stefan Beller]

There is an ongoing effort to remove global state currently and switch over
to pass around the relevant data structures; for most of it we end up passing
around 'the_repository' as it contains everything there is.

Merged into master:

fcb6df32546 Merge branch 'sb/oid-object-info'
a2cec42213c Merge branch 'sb/object-store-replace'
3a1ec60c43b Merge branch 'sb/packfiles-in-repository'
cf0b1793ead Merge branch 'sb/object-store'

Currently cooking:

8c69a7d7e80 Merge branch 'sb/object-store-grafts' into pu
42d32d07298 Merge branch 'sb/object-store-alloc' into jch

(both marked for "will merge to next" in the cooking report)

Floating on the mailing list, not cooking yet:

"sb/object-store-lookup"
https://public-inbox.org/git/20180613230522.55335-1-sbel...@google.com/
  This clashes with other series in flight (Stolees get_tree series; see
  https://public-inbox.org/git/709bd61c-70fc-a925-efba-58ab9be26...@gmail.com/
  I'll rebase this series on top of that series once the currently cooking
  series stabilize)

"nd/kill-the_index"
https://public-inbox.org/git/20180616054157.32433-1-pclo...@gmail.com/
  This converts the_index to pass around index pointers instead of
the_repository;
  it fits into the theme, but Duys end goal differs from mine; he is
less submodule focused.

Work that still needs to be done:

"xx/object-store-commit-graph"
  Convert the commit graph to have no global state, but be part of the
  repository struct. I think this can go in parallel to
"sb/object-store-lookup",
  so I'll tackle that next. Thanks Stolee for looking ahead: There is only the
  commit graph itself as well as whether it has been prepared as a global
  variable. So this series will consist of passing around a repository struct
  for all those higher level functions that do not pass around the commit graph
  or parts of it.

"xx/finish-object-stores"
  This requires "xx/object-store-commit-graph" as well as
"sb/object-store-lookup";
  it will convert parse_commit[_gently] to take a repository argument and will
  finish the actual object store part. This might be optional for the goal of
  converting submodules, that I have in mind, but it sure is a nice finishing
  touch.

"xx/convert-revision-walking"
  This series aims to convert get_merge_bases(), in_merge_bases() and all its
  revision walking code to take a repository argument.

"xx/submodule-dont-use-alternates"
  Once "xx/convert-revision-walking" is in, convert the local
find_first_merges(),
  and convert all functions to drop function add_submodule_odb() and instead
  operate on the submodule repository instead of the_repository with the
  submodule objects added as an alternate.

Thanks,
Stefan

[PATCH] ewah: delete unused 'rlwit_discharge_empty()'

[Ramsay Jones]

From: Junio C Hamano 

Complete the removal of unused 'ewah bitmap' code by removing the now
unused 'rlwit_discharge_empty()' function. Also, the 'ewah_clear()'
function can now be made a file-scope static symbol.

Signed-off-by: Ramsay Jones 
---

Hi Junio,

Can you please add this to the 'ds/ewah-cleanup' branch, before
we forget to do so! ;-)

Thanks!

ATB,
Ramsay Jones

 ewah/ewah_bitmap.c | 20 
 ewah/ewah_rlw.c|  8 
 ewah/ewok.h|  6 --
 ewah/ewok_rlw.h|  1 -
 4 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index 017c677f9..d59b1afe3 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -276,6 +276,18 @@ void ewah_each_bit(struct ewah_bitmap *self, void 
(*callback)(size_t, void*), vo
}
 }
 
+/**
+ * Clear all the bits in the bitmap. Does not free or resize
+ * memory.
+ */
+static void ewah_clear(struct ewah_bitmap *self)
+{
+   self->buffer_size = 1;
+   self->buffer[0] = 0;
+   self->bit_size = 0;
+   self->rlw = self->buffer;
+}
+
 struct ewah_bitmap *ewah_new(void)
 {
struct ewah_bitmap *self;
@@ -288,14 +300,6 @@ struct ewah_bitmap *ewah_new(void)
return self;
 }
 
-void ewah_clear(struct ewah_bitmap *self)
-{
-   self->buffer_size = 1;
-   self->buffer[0] = 0;
-   self->bit_size = 0;
-   self->rlw = self->buffer;
-}
-
 void ewah_free(struct ewah_bitmap *self)
 {
if (!self)
diff --git a/ewah/ewah_rlw.c b/ewah/ewah_rlw.c
index b9643b7d0..5093d43e2 100644
--- a/ewah/ewah_rlw.c
+++ b/ewah/ewah_rlw.c
@@ -104,11 +104,3 @@ size_t rlwit_discharge(
 
return index;
 }
-
-void rlwit_discharge_empty(struct rlw_iterator *it, struct ewah_bitmap *out)
-{
-   while (rlwit_word_size(it) > 0) {
-   ewah_add_empty_words(out, 0, rlwit_word_size(it));
-   rlwit_discard_first_words(it, rlwit_word_size(it));
-   }
-}
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 0c504f28e..84b2a29fa 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -72,12 +72,6 @@ void ewah_pool_free(struct ewah_bitmap *self);
  */
 struct ewah_bitmap *ewah_new(void);
 
-/**
- * Clear all the bits in the bitmap. Does not free or resize
- * memory.
- */
-void ewah_clear(struct ewah_bitmap *self);
-
 /**
  * Free all the memory of the bitmap
  */
diff --git a/ewah/ewok_rlw.h b/ewah/ewok_rlw.h
index bb3c6ff7e..7cdfdd0c0 100644
--- a/ewah/ewok_rlw.h
+++ b/ewah/ewok_rlw.h
@@ -98,7 +98,6 @@ void rlwit_init(struct rlw_iterator *it, struct ewah_bitmap 
*bitmap);
 void rlwit_discard_first_words(struct rlw_iterator *it, size_t x);
 size_t rlwit_discharge(
struct rlw_iterator *it, struct ewah_bitmap *out, size_t max, int 
negate);
-void rlwit_discharge_empty(struct rlw_iterator *it, struct ewah_bitmap *out);
 
 static inline size_t rlwit_word_size(struct rlw_iterator *it)
 {
-- 
2.17.0

Re: t5562: gzip -k is not portable

[Rodrigo Campos]

On Tue, Jun 19, 2018 at 07:25:15PM +0200, Torsten Bögershausen wrote:
> Hej Max,
> 
> t5562 fails here under MacOS:
> "gzip -k"  is not portable.

What do you mean with is not portable?

I wrote the patch for gzip[1]. That was in 2013, and is included since version
1.6 IIUC:

$ git tag --contains 0192f02e26ac9fa0a27ed177263ee3ea73d5e95c
v1.6
v1.7
v1.8
v1.9

But that maybe is too new? Or do you mean something else?

[1]: 
http://git.savannah.gnu.org/cgit/gzip.git/commit/?id=0192f02e26ac9fa0a27ed177263ee3ea73d5e95c

Re: t5562: gzip -k is not portable

[Rodrigo Campos]

On Tue, Jun 19, 2018 at 10:40:16PM +0200, Torsten Bögershausen wrote:
> 
> 
> On 06/19/2018 08:22 PM, Eric Sunshine wrote:
> > On Tue, Jun 19, 2018 at 2:06 PM Junio C Hamano  wrote:
> > > Torsten Bögershausen  writes:
> > > > t5562 fails here under MacOS:
> > > > "gzip -k"  is not portable.
> > Very odd. Stock /usr/bin/gzip on my MacOS 10.12.6 _does_ recognize -k,
> > and the test does pass.
> 
> gzip 1.3.12

Oh, this seems to be the issue. As I've said in my prvious email, the first
version to include this was 1.6 IIRC.

The commit adding it is here[1]. But yeah, it was not added 20 years ago, and it
seems there are systems with such old versions of gzip (although can probably
update? Don't know much about mac) :-(


[1]: 
http://git.savannah.gnu.org/cgit/gzip.git/commit/?id=0192f02e26ac9fa0a27ed177263ee3ea73d5e95c

Re: t5562: gzip -k is not portable

[Jeff King]

On Tue, Jun 19, 2018 at 10:40:16PM +0200, Torsten Bögershausen wrote:

> 
> 
> On 06/19/2018 08:22 PM, Eric Sunshine wrote:
> > On Tue, Jun 19, 2018 at 2:06 PM Junio C Hamano  wrote:
> > > Torsten Bögershausen  writes:
> > > > t5562 fails here under MacOS:
> > > > "gzip -k"  is not portable.
> > Very odd. Stock /usr/bin/gzip on my MacOS 10.12.6 _does_ recognize -k,
> > and the test does pass.
> 
> This is the test box running Mac OS X 10.6 speaking.
> The -c seems to need even -f.
> But this doesn't work either:
> 
> gzip 1.3.12
> Copyright (C) 2007 Free Software Foundation, Inc.
> Copyright (C) 1993 Jean-loup Gailly.
> This is free software.  You may redistribute copies of it under the terms of
> the GNU General Public License .
> There is NO WARRANTY, to the extent permitted by law.

Ah, that's it. "-k" came about in gzip v1.6. That's 5 years old, but
obviously some people are still running it.

"-c" goes back quite a while and should be safe, I think.

> expecting success:
>     gzip -f -c fetch_body >fetch_body.gz &&
>     test_copy_bytes 10 fetch_body.gz.trunc &&
>     gzip -f -c push_body >push_body,gz &&
>     test_copy_bytes 10 push_body.gz.trunc
> 
> ./test-lib.sh: line 632: push_body.gz: No such file or directory

Typo in the ">" redirect (comma instead of period)?

-Peff

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Junio C Hamano]

René Scharfe  writes:

> So let's see what your example does:
>
>$ git grep --column --not \( --not -e foo --or --not -e bar \) trace.h
>trace.h:13: *  #define foo(format, ...) bar(format, __VA_ARGS__)
>$ git grep --column --not \( --not -e bar --or --not -e foo \) trace.h
>trace.h:13: *  #define foo(format, ...) bar(format, __VA_ARGS__)
>
> Impressive.  That expression is confusing at first sight, but showing
> the first matching column anyway requires no further explanation.  I
> like it.

OK, obviously many people are a lot more math/logic minded than I
am.  I still think that any code that attempts to show the same
result as '-e bar --and -e foo' from the above is over-engineered,
but as long as it is done corretly and efficiently I won't have any
objection.

Thanks.

Re: t5562: gzip -k is not portable

[Torsten Bögershausen]

On 06/19/2018 08:22 PM, Eric Sunshine wrote:

On Tue, Jun 19, 2018 at 2:06 PM Junio C Hamano  wrote:

Torsten Bögershausen  writes:

t5562 fails here under MacOS:
"gzip -k"  is not portable.

Very odd. Stock /usr/bin/gzip on my MacOS 10.12.6 _does_ recognize -k,
and the test does pass.


This is the test box running Mac OS X 10.6 speaking.
The -c seems to need even -f.
But this doesn't work either:

gzip 1.3.12
Copyright (C) 2007 Free Software Foundation, Inc.
Copyright (C) 1993 Jean-loup Gailly.
This is free software.  You may redistribute copies of it under the terms of
the GNU General Public License .
There is NO WARRANTY, to the extent permitted by law.

Written by Jean-loup Gailly.
prerequisite GZIP ok
expecting success:
    gzip -f -c fetch_body >fetch_body.gz &&
    test_copy_bytes 10 fetch_body.gz.trunc &&
    gzip -f -c push_body >push_body,gz &&
    test_copy_bytes 10 push_body.gz.trunc

./test-lib.sh: line 632: push_body.gz: No such file or directory
not ok 2 - setup, compression related
#
#        gzip -f -c fetch_body >fetch_body.gz &&
#        test_copy_bytes 10 fetch_body.gz.trunc &&
#        gzip -f -c push_body >push_body,gz &&
#        test_copy_bytes 10 push_body.gz.trunc
#



Sigh.  Perhaps -c would help.  Or do BSD implementations also lack -c?

MacOS and BSD do support -c, so this solution would also work (and is
"cleaner" the the other proposal).


diff --git a/t/t5562-http-backend-content-length.sh 
b/t/t5562-http-backend-content-length.sh
@@ -61,9 +61,9 @@ test_expect_success 'setup' '
  test_expect_success GZIP 'setup, compression related' '
-   gzip -k fetch_body &&
+   gzip -c fetch_body >fetch_body.gz &&
 test_copy_bytes 10 fetch_body.gz.trunc &&
-   gzip -k push_body &&
+   gzip -c push_body >push_body.gz &&
 test_copy_bytes 10 push_body.gz.trunc
  '

Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

[Junio C Hamano]

Brandon Williams  writes:

> I also think that we should keep this first implementation of
> ref-in-want simple and *not* include patterns, even if that's what we
> may want someday down the road.  Adding a new capability in the future
> for support of such patterns would be relatively simple and easy.

I am all for many-small-steps over a single-giant-step approach.

>  The
> reason why I don't think we should add pattern support just yet is due
> to a request for "want-ref refs/tags/*" or a like request resulting in a
> larger than expected packfile every time "fetch --tags" is run.  The
> issue being that in a fetch request "refs/tags/*" is too broad of a
> request and could be requesting 100s of tags when all we really wanted
> was to get the one or two new tags which are present on the remote
> (because we already have all the other tags present locally).

I do not quite get this.  Why does it have to result in a large
packfile?  Doesn't the requester of refs/tags/* still show what it
has via "have" exchange?

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[René Scharfe]

Am 19.06.2018 um 21:11 schrieb Jeff King:
> On Tue, Jun 19, 2018 at 08:50:16PM +0200, René Scharfe wrote:
> 
>> Negation causes the whole non-matching line to match, with --column
>> reporting 1 or nothing in such a case, right?  Or I think doing the
>> same when the operator is applied a second time is explainable.

(Not sure where that extra "Or" came from.)

> Yes to your first question.
> 
> Regarding the final sentence, yes, I agree it's explainable. But I
> thought that handling negation like this was one of the main complaints
> of earlier iterations?

That's possible -- I didn't read the full thread, and I didn't argue
for or against any specific behavior in this regard before AFAIR.

So let's see what your example does:

   $ git grep --column --not \( --not -e foo --or --not -e bar \) trace.h
   trace.h:13: *  #define foo(format, ...) bar(format, __VA_ARGS__)
   $ git grep --column --not \( --not -e bar --or --not -e foo \) trace.h
   trace.h:13: *  #define foo(format, ...) bar(format, __VA_ARGS__)

Impressive.  That expression is confusing at first sight, but showing
the first matching column anyway requires no further explanation.  I
like it.

René

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[René Scharfe]

Am 19.06.2018 um 19:44 schrieb Taylor Blau:
> diff --git a/grep.c b/grep.c
> index f3329d82ed..a09935d8c5 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1257,8 +1257,8 @@ static int match_one_pattern(struct grep_pat *p, char 
> *bol, char *eol,
>   return hit;
>   }
> 
> -static int match_expr_eval(struct grep_expr *x, char *bol, char *eol,
> -enum grep_context ctx, ssize_t *col,
> +static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x, char 
> *bol,
> +char *eol, enum grep_context ctx, ssize_t *col,
>  ssize_t *icol, int collect_hits)

Passing opt in is one way.  Piggy-backing on collect_hits and making it
a flags parameter for two bits might be easier.  At least it wouldn't
increase the number of function arguments any further.  Not sure.

Anyway, something like this would be needed as well; or we could
force opt->columnnum to switch opt->extended on:

diff --git a/grep.c b/grep.c
index 8ffa94c791..a724ca3010 100644
--- a/grep.c
+++ b/grep.c
@@ -1325,6 +1321,7 @@ static int match_line(struct grep_opt *opt, char *bol, 
char *eol,
  enum grep_context ctx, int collect_hits)
 {
struct grep_pat *p;
+   int hit = 0;
 
if (opt->extended)
return match_expr(opt, bol, eol, ctx, col, icol,
@@ -1334,11 +1331,14 @@ static int match_line(struct grep_opt *opt, char *bol, 
char *eol,
for (p = opt->pattern_list; p; p = p->next) {
regmatch_t tmp;
if (match_one_pattern(p, bol, eol, ctx, , 0)) {
-   *col = tmp.rm_so;
-   return 1;
+   hit |= 1;
+   if (!opt->columnnum)
+   break;
+   if (*col < 0 || tmp.rm_so < *col)
+   *col = tmp.rm_so;
}
}
-   return 0;
+   return hit;
 }
 
 static int match_next_pattern(struct grep_pat *p, char *bol, char *eol,

Re: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

[Jeff King]

On Tue, Jun 19, 2018 at 07:00:48PM +, Dyer, Edwin wrote:

> Just curious if there was any additional comment on this potential
> OOB? I may have missed it and if so, apologies for the ask.

The fix is in master, and should be part of the upcoming v2.18. See
commit 9d2e330b17 (ewah_read_mmap: bounds-check mmap reads, 2018-06-14).

-Peff

Re: [PATCH 0/8] ref-in-want

[Jonathan Tan]

[snip]

> > in which we have rarely-updated branches that we still want to fetch
> > (e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
> > refs/changes/* branch), having the ref advertisement first means that we
> > can omit them from our "want" or "want-ref" list. But not having them
> > means that we send "want-ref refs/tags/*" to the server, and during
> > negotiation inform the server of our master branch (A), and since the
> > server knows of a common ancestor of all our wants (A, B, C), it will
> > terminate the negotiation and send the objects specific to branches B
> > and C even though it didn't need to.
> > 
> > So maybe we still need to keep the ls-refs step around, and thus, this
> > design of only accepting exact refs is perhaps good enough for now.
> 
> I think that taking a smaller step first it probably better.  This is
> something that we've done in the past with the shallow features and
> later capabilities were added to add different ways to request shallow
> fetches.

I think we're agreeing that the smaller step first is better.

> That being said, if we find that this feature doesn't work as-is and
> needs the extra complexity of patterns from the start then they should
> be added.

I agree (although I would be OK too if we decide to do the small
exact-name step now and then the pattern step later guarded by a
capability, as long as the project understood that multiple support
levels would then exist in the wild).

> But it doesn't seem like there's a concrete reason at the
> moment.

I agree. I thought I had a reason, but not after thinking through the
ideas I explained in [1].

[1] 
https://public-inbox.org/git/20180615190458.147775-1-jonathanta...@google.com/

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Todd Zullinger]

Junio C Hamano wrote:
> Todd Zullinger  writes:
>> Or Junio may just squash this onto js/rebase-i-root-fix.
> 
> Nah, not for a hotfix on the last couple of days before the final.
> We'd need to build on top, not "squash".

Indeed.  I somehow missed that you'd merged and pushed the
changes to master and next when I set this.  I was
mistakenly thinking it was only on the js/rebase-i-root-fix
integration branch.

Thanks,

-- 
Todd

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Jeff King]

On Tue, Jun 19, 2018 at 08:50:16PM +0200, René Scharfe wrote:

> Negation causes the whole non-matching line to match, with --column
> reporting 1 or nothing in such a case, right?  Or I think doing the
> same when the operator is applied a second time is explainable.

Yes to your first question.

Regarding the final sentence, yes, I agree it's explainable. But I
thought that handling negation like this was one of the main complaints
of earlier iterations?

> When ORing multiple expressions I don't pay attention to their order
> as that operator is commutative.  Having results depend on that
> order would at least surprise me.

OK. Let's just disable the short-circuit for --column then (i.e., what
Taylor posted earlier). That's explainable, and I doubt the performance
implications are going to be all that noticeable.

-Peff

RE: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

[Dyer, Edwin]

Greetings, all:

Just curious if there was any additional comment on this potential OOB? I may 
have missed it and if so, apologies for the ask.

Cheers,

Ed


-Original Message-
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf Of 
Luat Nguyen
Sent: Thursday, June 14, 2018 7:00 PM
To: git@vger.kernel.org
Subject: security: potential out-of-bound read at ewah_io.c |ewah_read_mmap|

Hi folks,

Recently, I’ve found a security issue related to out-of-bound read at function 
named `ewah_read_mmap`

Assume that, an attacker can put malicious `./git/index` into a repo by somehow.

Since there is lack of check whether the remaining size of `ptr`is equal to 
`buffer_size` or not.

So the code reads exceed the buffer of `ptr` and reach to higher page. In this 
case, it is `/lib/x86_64-linux-gnu/ld-2.23.so`.

Leads to infoleak. You can find more details and asan crash below.



# xxd .git/index
: 4449 5243  0002   4653 4d4e  DIRCFSMN
0010:  0024  0001 1538 2489 c8fc 3616  ...$.8$...6.
0020:  0014    2000 4141   .. .AA
^ evil size here = 0x2000


* SNIP CODE *

int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len) { … 
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
… 
memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));


[memory map]

0x7f990eca3000 0x7f990eca4000 r--p 1000 0  
/media/sf_Fuzz/vuln_repo/.git/index <— where `ptr` is placed
0x7f990eca4000 0x7f990eca5000 r--p 1000 25000  
/lib/x86_64-linux-gnu/ld-2.23.so <— memcpy will reach here
0x7f990eca5000 0x7f990eca6000 rw-p 1000 26000  
/lib/x86_64-linux-gnu/ld-2.23.so <— and here 


[ ASAN log ]

root@guest:/media/sf_SHARE/vuln_repo# /media/sf_SHARE/git-master-asan/git 
status =
==4324==ERROR: AddressSanitizer: unknown-crash on address 0x7f6f235b at pc 
0x004bba79 bp 0x7ffc75e68850 sp 0x7ffc75e68000 READ of size 65536 at 
0x7f6f235b thread T0
#0 0x4bba78 in __asan_memcpy 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
#1 0x8c910e in ewah_read_mmap 
/media/sf_SHARE/git-master-asan/ewah/ewah_io.c:144:2
#2 0x8e2534 in read_fsmonitor_extension 
/media/sf_SHARE/git-master-asan/fsmonitor.c:46:8
#3 0xa05862 in read_index_extension 
/media/sf_SHARE/git-master-asan/read-cache.c:1615:3
#4 0xa046f3 in do_read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1872:7
#5 0xa03325 in read_index_from 
/media/sf_SHARE/git-master-asan/read-cache.c:1913:8
#6 0xa03231 in read_index 
/media/sf_SHARE/git-master-asan/read-cache.c:1634:9
#7 0x9de5e8 in read_index_preload 
/media/sf_SHARE/git-master-asan/preload-index.c:119:15
#8 0x566cc6 in cmd_status 
/media/sf_SHARE/git-master-asan/builtin/commit.c:1358:2
#9 0x4ede8c in run_builtin /media/sf_SHARE/git-master-asan/git.c:417:11
#10 0x4ea939 in handle_builtin /media/sf_SHARE/git-master-asan/git.c:632:8
#11 0x4ed655 in run_argv /media/sf_SHARE/git-master-asan/git.c:684:4
#12 0x4ea037 in cmd_main /media/sf_SHARE/git-master-asan/git.c:761:19
#13 0x759c8b in main /media/sf_SHARE/git-master-asan/common-main.c:45:9
#14 0x7f6f2243382f in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#15 0x41c268 in _start (/media/sf_SHARE/git-master-asan/git+0x41c268)

Address 0x7f6f235b is a wild pointer.
SUMMARY: AddressSanitizer: unknown-crash 
/tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
 in __asan_memcpy Shadow bytes around the buggy address:
  0x0fee646adfb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adfe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fee646adff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
=>0x0fee646ae000:[fe]fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae010: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae020: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae030: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae040: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
  0x0fee646ae050: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe Shadow byte 
legend (one shadow byte represents 8 application bytes):
  Addressable:   00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:   fa
  Freed heap region:   fd
  Stack left redzone:  f1
  Stack mid redzone:   f2
  Stack right redzone: f3
  Stack after return:  f5
  Stack use after scope:   f8
  Global redzone:  f9
  Global init order:   f6
  Poisoned by user:f7
  Container overflow:  fc
  Array 

Re: [PATCH 00/15] Kill the_index part 1, expose it

[Ben Peart]

On 6/18/2018 2:41 PM, Brandon Williams wrote:

On 06/17, Duy Nguyen wrote:

On Sun, Jun 17, 2018 at 9:02 AM Elijah Newren  wrote:


On Fri, Jun 15, 2018 at 10:41 PM, Nguyễn Thái Ngọc Duy
 wrote:

This is the beginning of the end of the_index. The problem with
the_index is it lets library code anywhere access it freely. This is
not good because from high level you may not realize that the_index is
being used while you don't want to touch index at all, or you want to
use a different index instead.

This is a long series, 86 patches [1], so I'm going to split and
submit it in 15-20 patches at a time. The first two parts are trivial
though and could be safely fast tracked if needed.


You post this small little patch about unpack-trees.c, mentioning you
don't know if it's even correct, and bait me into reviewing it and
then spring on me that it's actually nearly 100 patches that need
review...   Very sneaky.  ;-)


To be fair, it's all Brandon's fault. If he didn't kick the_index out
of dir.c, it would not conflict with one of my out-of-tree patches in
unpack-trees.c, catch my attention and make me go down this rabbit
hole :D


Haha well this is something I've wanted to do for over a year now, glad
you've decided to run with it :)

I guess I've gotten pretty good at getting people to go down rabbit
holes.  First Stefan with the object store refactoring and now you with
the index stuff.  All because I wanted git to be more object oriented
and have less global state ;)



Let me join the chorus of voices excited to see someone finally taking 
the plunge to do this!  I'm very happy about seeing this taken through 
to "the end of the_index."

Re: [PATCH v2 2/8] upload-pack: implement ref-in-want

[Brandon Williams]

On 06/15, Junio C Hamano wrote:
> The story would be different if your request were 
> 
>   git fetch refs/heads/*:refs/remotes/origin/*
> 
> in which case, you are not even saying "I want this and that ref";
> you are saying "all refs in refs/heads/* whoever ends up serving me
> happens to have".  You may initially contact one of my friends and
> learn that there are 'master' and 'bo' branches (and probably
> others), and after conversation end up talking with me who is stale
> and lack 'bo'.  In such a case, I agree that it is not sensible for
> me to fail the request as a whole and instead serve you whatever
> branches I happen to have.  I may lack 'bo' branch due to mirroring
> lag, but I may also have 'extra' branch that others no longer have
> due to mirroring lag of deletion of that branch!
> 
> But then I think your "git fetch refs/heads/*:refs/remotes/origin/*"
> should not fail not just because I do not have 'bo', but you also
> should grab other old branches I have, which you didn't hear about
> when you made the initial contact with my friend in the mirror pool.
> 
> So, given that, would it make sense for 'want-ref ' request to
> name "a particular ref" as the above document says?  I have a
> feeling that it should allow a pattern to be matched at the server
> side (and it is not an error if the pattern did not match anything),
> in addition to asking for a particular ref (in which case, lack of
> that ref should be a hard failure, at least for the mirror that ends
> up serving the packfile and the final "here are the refs your
> request ended up fetching, with their values").

After some more in-office discussion about this I think I should revert
back to making it a hard failure when a client requests a ref which
doesn't exist on the server.  This makes things more consistent with
what happens today if I request a non-existent ref (Although that error
is purely on the client).  Its no worse than we have today and even with
this solution not solving the issues of new/deleted refs (which are rare
operations wrt updates) we still can get the benefit of not failing due
to a ref updating.  This is also very valuable for the servers which
have to to ACL checks on individual ref names.

I also think that we should keep this first implementation of
ref-in-want simple and *not* include patterns, even if that's what we
may want someday down the road.  Adding a new capability in the future
for support of such patterns would be relatively simple and easy.  The
reason why I don't think we should add pattern support just yet is due
to a request for "want-ref refs/tags/*" or a like request resulting in a
larger than expected packfile every time "fetch --tags" is run.  The
issue being that in a fetch request "refs/tags/*" is too broad of a
request and could be requesting 100s of tags when all we really wanted
was to get the one or two new tags which are present on the remote
(because we already have all the other tags present locally).

So I think the best way is to limit these patterns to the ls-refs
request where we can then discover the few tags we're missing and then
request just those tags explicitly, keeping the resulting packfile
smaller.

Thoughts?

-- 
Brandon Williams

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[René Scharfe]

Am 19.06.2018 um 19:48 schrieb Jeff King:
> On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:
> 
>>> The key thing about this iteration is that it doesn't regress
>>> performance, because we always short-circuit where we used to. The other
>>> obvious route is to stop short-circuiting only when "--column" is in
>>> effect, which would have the same property (at the expense of a little
>>> extra code in match_expr_eval()).
>>
>> The performance impact of the exhaustive search for --color scales with
>> the number of shown lines, while it would scale with the total number of
>> lines for --column.  Coloring the results of highly selective patterns
>> is relatively cheap, short-circuiting them still helps significantly.
> 
> I thought that at first, too, but I think we'd still scale with the
> number of shown lines. We're talking about short-circuiting OR, so by
> definition we stop the short-circuit because we matched the first half
> of the OR.
> 
> If you stop short-circuiting AND, then yes, you incur a penalty for
> every line. But I don't think --column would need to do that.

Good point.  So disabling that optimization for --column (or in even
in general) shouldn't be a dramatic loss.

> Although there are interesting cases around inversion. For example:
> 
>git grep --not \( --not -e a --and --not -e b \)
> 
> is equivalent to:
> 
>git grep -e a --or -e b
> 
> Do people care if we actually hunt down the exact column where we
> _didn't_ match "b" in the first case?  The two are equivalent, but I
> have to wonder if somebody writing the first one really cares.

I'd rather not guess the intentions of someone using such convoluted
expressions. ;-)

Negation causes the whole non-matching line to match, with --column
reporting 1 or nothing in such a case, right?  Or I think doing the
same when the operator is applied a second time is explainable.

>> Disabling that optimization for --column wouldn't be a regression since
>> it's a new option..  Picking a random result (based on the order of
>> evaluation) seems sloppy and is probably going to surprise users.
> 
> I don't see it as a random result; short-circuiting logic is well
> understood and we follow the user's ordering.

When ORing multiple expressions I don't pay attention to their order
as that operator is commutative.  Having results depend on that
order would at least surprise me.

> I think the place where it's _most_ ugly is "--column --color", where we
> may color the short-circuited value in the second pass.

The double negative example you gave above has that discrepancy as
well, but I think in that case it just highlights the different
intentions (--color: highlight search terms, --column: show leftmost
match).

>> We could add an optimizer pass to reduce the number of regular
>> expressions in certain cases if that is really too slow.  E.g. this:
> 
> Yes, we actually discussed this kind of transformation. I think it's way
> out of scope for this patch series, though. If we do anything more, I
> think it should be to disable short-circuiting when --column is in use.

Yep.

René

Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C

[Stefan Beller]

On Tue, Jun 19, 2018 at 8:45 AM Alban Gruin  wrote:
>
> This patch series rewrites the reflog operations from shell to C.  This
> is part of the effort to rewrite interactive rebase in C.

This series looks good to me.

Thanks,
Stefan

Re: want option to git-rebase

[Johannes Sixt]

Am 19.06.2018 um 03:06 schrieb Jonathan Nieder:

Ian Jackson wrote[1]:

git-rebase leaves entries like this in the reflog:

   c15f4d5391 HEAD@{33}: rebase: checkout 
c15f4d5391ff07a718431aca68a73e672fe8870e

It would be nice if there were an option to control this message.
Particularly, when another tool invokes git-rebase, the other tool may
specify an interesting --onto, and there is no way to record any
information about that --onto commit.

git-rebase already has a -m option, so I suggest
   --reason=

It doesn't matter much exactly how the provided string is used.
Any of the following would be good IMO:
   
   rebase start: 


 From git(1):

  GIT_REFLOG_ACTION
When a ref is updated, reflog entries are created to keep
track of the reason why the ref was updated (which is
typically the name of the high-level command that updated the
ref), in addition to the old and new values of the ref. A
scripted Porcelain command can use set_reflog_action helper
function in git-sh-setup to set its name to this variable when
it is invoked as the top level command by the end user, to be
recorded in the body of the reflog.

"git rebase" sets this itself, so it doesn't solve your problem.


If it does so unconditionally, then that is a bug. If a script wants to 
set GIT_REFLOG_ACTION, but finds that it is already set, then it must 
not change the value. set_reflog_action in git-sh-setup does the right 
thing.


So, if there is another script or application around git-rebase, then it 
should just set GIT_REFLOG_ACTION (if it is not already set) and export 
the environment variable to git-rebase.


-- Hannes

Re: t5562: gzip -k is not portable

[Eric Sunshine]

On Tue, Jun 19, 2018 at 2:06 PM Junio C Hamano  wrote:
> Torsten Bögershausen  writes:
> > t5562 fails here under MacOS:
> > "gzip -k"  is not portable.

Very odd. Stock /usr/bin/gzip on my MacOS 10.12.6 _does_ recognize -k,
and the test does pass.

> Sigh.  Perhaps -c would help.  Or do BSD implementations also lack -c?

MacOS and BSD do support -c, so this solution would also work (and is
"cleaner" the the other proposal).

> diff --git a/t/t5562-http-backend-content-length.sh 
> b/t/t5562-http-backend-content-length.sh
> @@ -61,9 +61,9 @@ test_expect_success 'setup' '
>  test_expect_success GZIP 'setup, compression related' '
> -   gzip -k fetch_body &&
> +   gzip -c fetch_body >fetch_body.gz &&
> test_copy_bytes 10 fetch_body.gz.trunc &&
> -   gzip -k push_body &&
> +   gzip -c push_body >push_body.gz &&
> test_copy_bytes 10 push_body.gz.trunc
>  '

Re: [PATCH] format-patch: clear UNINTERESTING flag before prepare_bases

[Stefan Beller]

On Mon, Jun 4, 2018 at 8:09 AM Xiaolong Ye  wrote:
>
> When users specify the commit range with 'Z..C' pattern for format-patch, all
> the parents of Z (including Z) would be marked as UNINTERESTING which would
> prevent revision walk in prepare_bases from getting the prerequisite commits,
> thus `git format-patch --base  Z..C` won't be able to 
> generate
> the list of prerequisite patch ids. Clear UNINTERESTING flag with
> clear_object_flags solves this issue.

This makes sense;
Reviewed-by: Stefan Beller 

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Junio C Hamano]

Jeff King  writes:

> Even if it's a double-inversion? The reason we carry both `col` and
> `icol` is that it allows:
>
>   git grep --not --not --not --not -e a
>
> to still say "we found 'a' here". That's a dumb thing to ask for, but it
> is true in the end that we show lines with "a" (and will colorize them
> as such).

Yes.  I think the code is doing too much to cater to a dumb request
;-)

Re: t5562: gzip -k is not portable

[Junio C Hamano]

Torsten Bögershausen  writes:

> Hej Max,
>
> t5562 fails here under MacOS:
> "gzip -k"  is not portable.

Sigh.  Perhaps -c would help.  Or do BSD implementations also lack -c?

 t/t5562-http-backend-content-length.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5562-http-backend-content-length.sh 
b/t/t5562-http-backend-content-length.sh
index 8040d80e04..98b6543827 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -61,9 +61,9 @@ test_expect_success 'setup' '
 '
 
 test_expect_success GZIP 'setup, compression related' '
-   gzip -k fetch_body &&
+   gzip -c fetch_body >fetch_body.gz &&
test_copy_bytes 10 fetch_body.gz.trunc &&
-   gzip -k push_body &&
+   gzip -c push_body >push_body.gz &&
test_copy_bytes 10 push_body.gz.trunc
 '
 

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Jeff King]

On Tue, Jun 19, 2018 at 10:58:30AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Although there are interesting cases around inversion. For example:
> >
> >   git grep --not \( --not -e a --and --not -e b \)
> >
> > is equivalent to:
> >
> >   git grep -e a --or -e b
> >
> > Do people care if we actually hunt down the exact column where we
> > _didn't_ match "b" in the first case?  The two are equivalent, but I
> > have to wonder if somebody writing the first one really cares.
> 
> I may be misunderstanding the question, but I personally would feel
> that "git grep --not " is OK to say "the entire line is at
> fault that it did not satisify the criteria to match ".
> I.e., I'd be happy if --column marked the first column as the
> beginning of the match, or --color painted the entire line in the
> output of the former.

Even if it's a double-inversion? The reason we carry both `col` and
`icol` is that it allows:

  git grep --not --not --not --not -e a

to still say "we found 'a' here". That's a dumb thing to ask for, but it
is true in the end that we show lines with "a" (and will colorize them
as such).

-Peff

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Taylor Blau]

On Tue, Jun 19, 2018 at 10:58:30AM -0700, Junio C Hamano wrote:
> Jeff King  writes:
>
> > Although there are interesting cases around inversion. For example:
> >
> >   git grep --not \( --not -e a --and --not -e b \)
> >
> > is equivalent to:
> >
> >   git grep -e a --or -e b
> >
> > Do people care if we actually hunt down the exact column where we
> > _didn't_ match "b" in the first case?  The two are equivalent, but I
> > have to wonder if somebody writing the first one really cares.
>
> I may be misunderstanding the question, but I personally would feel
> that "git grep --not " is OK to say "the entire line is at
> fault that it did not satisify the criteria to match ".
> I.e., I'd be happy if --column marked the first column as the
> beginning of the match, or --color painted the entire line in the
> output of the former.

I think that Peff is pointing out that a short-circuiting OR will cause
a line like

  b foo a

to print the column for "a", not "b" (when given "-e a --or -e b").

Thanks,
Taylor

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Junio C Hamano]

Jeff King  writes:

> Although there are interesting cases around inversion. For example:
>
>   git grep --not \( --not -e a --and --not -e b \)
>
> is equivalent to:
>
>   git grep -e a --or -e b
>
> Do people care if we actually hunt down the exact column where we
> _didn't_ match "b" in the first case?  The two are equivalent, but I
> have to wonder if somebody writing the first one really cares.

I may be misunderstanding the question, but I personally would feel
that "git grep --not " is OK to say "the entire line is at
fault that it did not satisify the criteria to match ".
I.e., I'd be happy if --column marked the first column as the
beginning of the match, or --color painted the entire line in the
output of the former.

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[René Scharfe]

Am 19.06.2018 um 19:44 schrieb Taylor Blau:
> On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:
>> Am 19.06.2018 um 18:35 schrieb Jeff King:
>>> On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote:
>> We could add an optimizer pass to reduce the number of regular
>> expressions in certain cases if that is really too slow.  E.g. this:
>>
>>  $ git grep -e b -e a
>>
>> ... is equivalent to:
>>
>>  $ git grep -e '\(b\)\|\(a\)'
>>
>> In that example the optimizer should use a single kwset instead of a
>> regex, but you get the idea, namely to leave the short-circuiting to the
>> regex engine or kwset, which probably do a good job of it.
> 
> I think that--while this pushes that decision to the appropriate level
> of indirection--that it is out of scope for this series, and that the
> above patch should do a sufficient job at not surprising users.

Definitely.  I'm not even convinced that performance problem is real --
otherwise someone would have added such an optimization already, right?
:)

René

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Taylor Blau]

On Tue, Jun 19, 2018 at 01:48:47PM -0400, Jeff King wrote:
> On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:
> > Disabling that optimization for --column wouldn't be a regression since
> > it's a new option..  Picking a random result (based on the order of
> > evaluation) seems sloppy and is probably going to surprise users.
>
> I don't see it as a random result; short-circuiting logic is well
> understood and we follow the user's ordering.

Agreed. I would prefer _not_ to apply the patch that I sent to René,
since I think it adds more complexity than its worth (precisely because
the short-circuiting logic is known, though certainly the problem gets
worse as the expression tree grows).

> I think the place where it's _most_ ugly is "--column --color", where we
> may color the short-circuited value in the second pass.

Agreed again, but it's worth noting that --color is the default. To play
devil's advocate, the use-case that I imagine will be most common is
with "git jump," so perhaps that doesn't matter as much.

> > We could add an optimizer pass to reduce the number of regular
> > expressions in certain cases if that is really too slow.  E.g. this:
>
> Yes, we actually discussed this kind of transformation. I think it's way
> out of scope for this patch series, though. If we do anything more, I
> think it should be to disable short-circuiting when --column is in use.

Piggy-backing on what I said to René, agreed again.

> -Peff

Thanks,
Taylor

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Jeff King]

On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:

> > The key thing about this iteration is that it doesn't regress
> > performance, because we always short-circuit where we used to. The other
> > obvious route is to stop short-circuiting only when "--column" is in
> > effect, which would have the same property (at the expense of a little
> > extra code in match_expr_eval()).
> 
> The performance impact of the exhaustive search for --color scales with
> the number of shown lines, while it would scale with the total number of
> lines for --column.  Coloring the results of highly selective patterns
> is relatively cheap, short-circuiting them still helps significantly.

I thought that at first, too, but I think we'd still scale with the
number of shown lines. We're talking about short-circuiting OR, so by
definition we stop the short-circuit because we matched the first half
of the OR.

If you stop short-circuiting AND, then yes, you incur a penalty for
every line. But I don't think --column would need to do that.

Although there are interesting cases around inversion. For example:

  git grep --not \( --not -e a --and --not -e b \)

is equivalent to:

  git grep -e a --or -e b

Do people care if we actually hunt down the exact column where we
_didn't_ match "b" in the first case?  The two are equivalent, but I
have to wonder if somebody writing the first one really cares.

> Disabling that optimization for --column wouldn't be a regression since
> it's a new option..  Picking a random result (based on the order of
> evaluation) seems sloppy and is probably going to surprise users.

I don't see it as a random result; short-circuiting logic is well
understood and we follow the user's ordering.

I think the place where it's _most_ ugly is "--column --color", where we
may color the short-circuited value in the second pass.

> We could add an optimizer pass to reduce the number of regular
> expressions in certain cases if that is really too slow.  E.g. this:

Yes, we actually discussed this kind of transformation. I think it's way
out of scope for this patch series, though. If we do anything more, I
think it should be to disable short-circuiting when --column is in use.

-Peff

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Taylor Blau]

On Tue, Jun 19, 2018 at 07:33:39PM +0200, René Scharfe wrote:
> Am 19.06.2018 um 18:35 schrieb Jeff King:
> > On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote:
> >> The notable case that it does _not_ cover is matching the following
> >> line:
> >>
> >>a ... b
> >>
> >> with the following expression
> >>
> >>git grep --column -e b --or -e a
> >>
> >> This will produce the column for 'b' rather than the column for 'a',
> >> since we short-circuit an --or when the left child finds a match, in
> >> this case 'b'. So, we break the semantics for this case, at the benefit
> >> of not having to do twice the work.
> >>
> >> In the future, I'd like to revisit this, since any performance gains
> >> that we _do_ make in this area are moot when we rescan all lines in
> >> show_line() with --color. A path forward, I imagine, would look like a
> >> list of regmatch_t's, or a set of locations in the expression tree, such
> >> that we could either enumerate the list or walk the tree in order to
> >> colorize the line. But, I think for now that is #leftoverbits.
> >
> > The key thing about this iteration is that it doesn't regress
> > performance, because we always short-circuit where we used to. The other
> > obvious route is to stop short-circuiting only when "--column" is in
> > effect, which would have the same property (at the expense of a little
> > extra code in match_expr_eval()).
>
> The performance impact of the exhaustive search for --color scales with
> the number of shown lines, while it would scale with the total number of
> lines for --column.  Coloring the results of highly selective patterns
> is relatively cheap, short-circuiting them still helps significantly.

Sure. I was pointing out that we are repeating work in cases where it is
unnecessary to do so. It seems natural to me to take one of the two
above approaches, and optimize where we can.

> Disabling that optimization for --column wouldn't be a regression since
> it's a new option..  Picking a random result (based on the order of
> evaluation) seems sloppy and is probably going to surprise users.

That's fair, and something I'm happy to do if others feel OK about it.
Here is a patch to that effect:

diff --git a/grep.c b/grep.c
index f3329d82ed..a09935d8c5 100644
--- a/grep.c
+++ b/grep.c
@@ -1257,8 +1257,8 @@ static int match_one_pattern(struct grep_pat *p, char 
*bol, char *eol,
return hit;
 }

-static int match_expr_eval(struct grep_expr *x, char *bol, char *eol,
-  enum grep_context ctx, ssize_t *col,
+static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x, char 
*bol,
+  char *eol, enum grep_context ctx, ssize_t *col,
   ssize_t *icol, int collect_hits)
 {
int h = 0;
@@ -1282,26 +1282,27 @@ static int match_expr_eval(struct grep_expr *x, char 
*bol, char *eol,
/*
 * Upon visiting a GREP_NODE_NOT, col and icol become swapped.
 */
-   h = !match_expr_eval(x->u.unary, bol, eol, ctx, icol, col, 0);
+   h = !match_expr_eval(opt, x->u.unary, bol, eol, ctx, icol, col, 
0);
break;
case GREP_NODE_AND:
-   if (!match_expr_eval(x->u.binary.left, bol, eol, ctx, col,
+   if (!match_expr_eval(opt, x->u.binary.left, bol, eol, ctx, col,
 icol, 0))
return 0;
-   h = match_expr_eval(x->u.binary.right, bol, eol, ctx, col,
+   h = match_expr_eval(opt, x->u.binary.right, bol, eol, ctx, col,
icol, 0);
break;
case GREP_NODE_OR:
-   if (!collect_hits)
-   return (match_expr_eval(x->u.binary.left, bol, eol, ctx,
+   if (!(collect_hits || opt->columnnum))
+   return (match_expr_eval(opt, x->u.binary.left, bol, 
eol, ctx,
col, icol, 0) ||
-   match_expr_eval(x->u.binary.right, bol, eol,
+   match_expr_eval(opt, x->u.binary.right, bol, 
eol,
ctx, col, icol, 0));
-   h = match_expr_eval(x->u.binary.left, bol, eol, ctx, col,
+   h = match_expr_eval(opt, x->u.binary.left, bol, eol, ctx, col,
icol, 0);
-   x->u.binary.left->hit |= h;
-   h |= match_expr_eval(x->u.binary.right, bol, eol, ctx, col,
-icol, 1);
+   if (collect_hits)
+   x->u.binary.left->hit |= h;
+   h |= match_expr_eval(opt, x->u.binary.right, bol, eol, ctx, col,
+icol, collect_hits);
break;
default:
die("Unexpected node type (internal error) %d", x->node);
@@ -1316,7 +1317,7 @@ 

Re: t5310 broken under Mac OS

[Torsten Bögershausen]

On 06/19/2018 07:35 PM, Eric Sunshine wrote:

On Tue, Jun 19, 2018 at 1:33 PM Torsten Bögershausen  wrote:

expecting success:
  git repack -ad &&
  git rev-list --use-bitmap-index --count --all >expect &&
  bitmap=$(ls .git/objects/pack/*.bitmap) &&
  test_when_finished "rm -f $bitmap" &&
  head -c 512 <$bitmap >$bitmap.tmp &&
  mv $bitmap.tmp $bitmap &&
  git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
  test_cmp expect actual &&
  test_i18ngrep corrupt stderr

override r--r--r--  tb/staff for
.git/objects/pack/pack-8886db3fce4f9657c1a43fee7d3ea4f2a4b5be2d.bitmap?
(y/n [n]) not overwritten
error: 'grep corrupt stderr' didn't find a match in:

This is fixed by [1], isn't it?

[1]: https://public-inbox.org/git/20180616191400.gb8...@sigill.intra.peff.net/

Most probably: yes.
I just ran pu if the day (which is 15th of June)
Thanks for a fast response, sorry for the noise

Re: [PATCH v2 7/8] fetch-pack: put shallow info in output parameter

[Brandon Williams]

On 06/14, Jonathan Tan wrote:
> > @@ -1122,6 +1124,7 @@ static int do_fetch(struct transport *transport,
> > int autotags = (transport->remote->fetch_tags == 1);
> > int retcode = 0;
> > const struct ref *remote_refs;
> > +   struct ref *new_remote_refs = NULL;
> 
> Above, you use the name "updated_remote_refs" - it's probably better to
> standardize on one. I think "updated" is better.

Good catch I'll update the variable name.

> 
> (The transport calling it "fetched_refs" is fine, because that's what
> they are from the perspective of the transport. From the perspective of
> fetch-pack, it is indeed a new or updated set of remote refs.)
> 
> > -   if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) 
> > {
> > +
> > +   if (fetch_refs(transport, ref_map, _remote_refs)) {
> > +   free_refs(ref_map);
> > +   retcode = 1;
> > +   goto cleanup;
> > +   }
> > +   if (new_remote_refs) {
> > +   free_refs(ref_map);
> > +   ref_map = get_ref_map(transport->remote, new_remote_refs, rs,
> > + tags, );
> > +   free_refs(new_remote_refs);
> > +   }
> > +   if (consume_refs(transport, ref_map)) {
> > free_refs(ref_map);
> > retcode = 1;
> > goto cleanup;
> 
> Here, if we got updated remote refs, we need to regenerate ref_map,
> since it is the source of truth.
> 
> Maybe add a comment in the "if (new_remote_refs)" block explaining this
> - something like: Regenerate ref_map using the updated remote refs,
> because the transport would place shallow (and other) information
> there.

That's probably a good idea to give future readers more context into why
this is happening.

> 
> > -   for (i = 0; i < nr_sought; i++)
> > +   for (r = refs; r; r = r->next, i++)
> > if (status[i])
> > -   sought[i]->status = REF_STATUS_REJECT_SHALLOW;
> > +   r->status = REF_STATUS_REJECT_SHALLOW;
> 
> You use i here without initializing it to 0. t5703 also fails with this
> patch - probably related to this, but I didn't check.

Oh yeah that's definitely a bug, thanks for catching that.

> 
> If you initialize i here, I don't think you need to initialize it to 0
> at the top of this function.

-- 
Brandon Williams

Re: t5310 broken under Mac OS

[Eric Sunshine]

On Tue, Jun 19, 2018 at 1:33 PM Torsten Bögershausen  wrote:
> expecting success:
>  git repack -ad &&
>  git rev-list --use-bitmap-index --count --all >expect &&
>  bitmap=$(ls .git/objects/pack/*.bitmap) &&
>  test_when_finished "rm -f $bitmap" &&
>  head -c 512 <$bitmap >$bitmap.tmp &&
>  mv $bitmap.tmp $bitmap &&
>  git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
>  test_cmp expect actual &&
>  test_i18ngrep corrupt stderr
>
> override r--r--r--  tb/staff for
> .git/objects/pack/pack-8886db3fce4f9657c1a43fee7d3ea4f2a4b5be2d.bitmap?
> (y/n [n]) not overwritten
> error: 'grep corrupt stderr' didn't find a match in:

This is fixed by [1], isn't it?

[1]: https://public-inbox.org/git/20180616191400.gb8...@sigill.intra.peff.net/

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[René Scharfe]

Am 19.06.2018 um 18:35 schrieb Jeff King:
> On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote:
>> The notable case that it does _not_ cover is matching the following
>> line:
>>
>>a ... b
>>
>> with the following expression
>>
>>git grep --column -e b --or -e a
>>
>> This will produce the column for 'b' rather than the column for 'a',
>> since we short-circuit an --or when the left child finds a match, in
>> this case 'b'. So, we break the semantics for this case, at the benefit
>> of not having to do twice the work.
>>
>> In the future, I'd like to revisit this, since any performance gains
>> that we _do_ make in this area are moot when we rescan all lines in
>> show_line() with --color. A path forward, I imagine, would look like a
>> list of regmatch_t's, or a set of locations in the expression tree, such
>> that we could either enumerate the list or walk the tree in order to
>> colorize the line. But, I think for now that is #leftoverbits.
> 
> The key thing about this iteration is that it doesn't regress
> performance, because we always short-circuit where we used to. The other
> obvious route is to stop short-circuiting only when "--column" is in
> effect, which would have the same property (at the expense of a little
> extra code in match_expr_eval()).

The performance impact of the exhaustive search for --color scales with
the number of shown lines, while it would scale with the total number of
lines for --column.  Coloring the results of highly selective patterns
is relatively cheap, short-circuiting them still helps significantly.

Disabling that optimization for --column wouldn't be a regression since
it's a new option..  Picking a random result (based on the order of
evaluation) seems sloppy and is probably going to surprise users.

We could add an optimizer pass to reduce the number of regular
expressions in certain cases if that is really too slow.  E.g. this:

$ git grep -e b -e a

... is equivalent to:

$ git grep -e '\(b\)\|\(a\)'

In that example the optimizer should use a single kwset instead of a
regex, but you get the idea, namely to leave the short-circuiting to the
regex engine or kwset, which probably do a good job of it.

René

t5310 broken under Mac OS

[Torsten Bögershausen]

One test case fails here,
but I am to tired to dig further.


ok 42 - pack reuse respects --incremental

expecting success:
    git repack -ad &&
    git rev-list --use-bitmap-index --count --all >expect &&
    bitmap=$(ls .git/objects/pack/*.bitmap) &&
    test_when_finished "rm -f $bitmap" &&
    head -c 512 <$bitmap >$bitmap.tmp &&
    mv $bitmap.tmp $bitmap &&
    git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
    test_cmp expect actual &&
    test_i18ngrep corrupt stderr

override r--r--r--  tb/staff for 
.git/objects/pack/pack-8886db3fce4f9657c1a43fee7d3ea4f2a4b5be2d.bitmap? 
(y/n [n]) not overwritten

error: 'grep corrupt stderr' didn't find a match in:

not ok 43 - truncated bitmap fails gracefully
#
#        git repack -ad &&
#        git rev-list --use-bitmap-index --count --all >expect &&
#        bitmap=$(ls .git/objects/pack/*.bitmap) &&
#        test_when_finished "rm -f $bitmap" &&
#        head -c 512 <$bitmap >$bitmap.tmp &&
#        mv $bitmap.tmp $bitmap &&
#        git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
#        test_cmp expect actual &&
#        test_i18ngrep corrupt stderr
#

# failed 1 among 43 test(s)
1..43

Re: [PATCH 0/8] ref-in-want

[Brandon Williams]

On 06/15, Jonathan Tan wrote:
> (replying to the original since my e-mail is about design)
> 
> > This version of ref-in-want is a bit more restrictive than what Jonathan
> > originally proposed (only full ref names are allowed instead of globs
> > and OIDs), but it is meant to accomplish the same goal (solve the issues
> > of refs changing during negotiation).
> 
> One question remains: are we planning to expand this feature (e.g. to
> support patterns ending in *, or to support any pattern that can appear
> on the LHS of a refspec), and if yes, are we OK with having 2 or more
> versions of the service in the wild, each having different pattern
> support?
> 
> Supporting patterns would mean that we would possibly be able to
> eliminate the ls-refs step, thus saving at least a RTT. (Originally I
> thought that supporting patterns would also allow us to tolerate refs
> being removed during the fetch process, but I see that this is already
> handled by the server ignoring "want-ref " wherein  doesn't
> exist on the server.)
> 
> However, after some in-office discussion, I see that eliminating the
> ls-refs step means that we lose some optimizations that can only be done
> when we see that we already have a sought remote ref. For example, in a
> repo like this:
> 
>  A
>  |
>  O
>  |
>  O B C
>  |/ /
>  O O
>  |/
>  O
> 
> in which we have rarely-updated branches that we still want to fetch
> (e.g. an annotated tag when we fetch refs/tags/* or a Gerrit
> refs/changes/* branch), having the ref advertisement first means that we
> can omit them from our "want" or "want-ref" list. But not having them
> means that we send "want-ref refs/tags/*" to the server, and during
> negotiation inform the server of our master branch (A), and since the
> server knows of a common ancestor of all our wants (A, B, C), it will
> terminate the negotiation and send the objects specific to branches B
> and C even though it didn't need to.
> 
> So maybe we still need to keep the ls-refs step around, and thus, this
> design of only accepting exact refs is perhaps good enough for now.

I think that taking a smaller step first it probably better.  This is
something that we've done in the past with the shallow features and
later capabilities were added to add different ways to request shallow
fetches.

That being said, if we find that this feature doesn't work as-is and
needs the extra complexity of patterns from the start then they should
be added.  But it doesn't seem like there's a concrete reason at the
moment.

-- 
Brandon Williams

t5562: gzip -k is not portable

[Torsten Bögershausen]

Hej Max,

t5562 fails here under MacOS:
"gzip -k"  is not portable.

The following works (there may be better solutions, I didn't dig into 
the test code)


diff --git a/t/t5562-http-backend-content-length.sh 
b/t/t5562-http-backend-content-length.sh

index 8040d80e04..7befe3885c 100755
--- a/t/t5562-http-backend-content-length.sh
+++ b/t/t5562-http-backend-content-length.sh
@@ -61,9 +61,13 @@ test_expect_success 'setup' '
 '

 test_expect_success GZIP 'setup, compression related' '
-   gzip -k fetch_body &&
+   cp fetch_body f &&
+   gzip fetch_body &&
+   mv f fetch_body &&
    test_copy_bytes 10 fetch_body.gz.trunc &&
-   gzip -k push_body &&
+   cp push_body f &&
+   gzip push_body &&
+   mv f push_body &&
    test_copy_bytes 10 push_body.gz.trunc

Re: [PATCH 2/7] grep.c: expose {,inverted} match column in match_line()

[Taylor Blau]

On Tue, Jun 19, 2018 at 09:49:21AM -0700, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > case GREP_NODE_NOT:
> > -   h = !match_expr_eval(x->u.unary, bol, eol, ctx, 0);
> > +   /*
> > +* Upon visiting a GREP_NODE_NOT, imatch and match become
> > +* swapped.
> > +*/
> > +   h = !match_expr_eval(x->u.unary, bol, eol, ctx, icol, col, 0);
>
> A minor nit, but the comment talks about something that are
> different from the variable names; perhaps you called col/icol with
> different names in an earlier incarnation of this patch?

Good catch, thanks. I've amended my local copy.

Thanks,
Taylor

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Taylor Blau]

On Tue, Jun 19, 2018 at 09:46:16AM -0700, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > Attached is a ``fresh start'' of my series to teach 'git grep --column'.
> > Since the last time I sent this, much has changed, notably the semantics
> > for deciding which column is the first when given (1) extended
> > expressions and (2) --invert.
> > ...
> > In the future, I'd like to revisit this, since any performance gains
> > that we _do_ make in this area are moot when we rescan all lines in
> > show_line() with --color.
>
> Sounds like a plan.  From a quick scan, it seems that this is
> sufficiently different from the last round so I won't bother
> rebuilding your "--only" series on top, and instead just drop those
> two series from the older round and queue this as the new and
> improved tb/grep-column topic.

Thank you. I recommend dropping the second series
(tb/grep-only-matching) entirely for now. I will rebase that on top of
this so that you can apply it later in the future with ease.

I don't expect the parts of this series that affect that one to change
much, but I'll hold off on rebasing it in general until this series is
stable, hopefully soon.

> Thanks.

Thanks,
Taylor

Re: [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts

[Junio C Hamano]

Elijah Newren  writes:

> [As an aside, I know there are multiple other outstanding emails for
> me to respond to, unrelated to this patch.  I'll try to get some time
> in the next day or two to respond.  Just responding to this one since
> Junio mentioned picking it up for 2.18.]

Thanks for a reminder.

I thought I said that I'll backburner/ignore the topic and expect
something that can be picked up to be there when I come back after I
cut 2.18 final, but I ended up coming back to read the topic anyway
it seems ;-)

Re: [PATCH 2/7] grep.c: expose {,inverted} match column in match_line()

[Junio C Hamano]

Taylor Blau  writes:

>   case GREP_NODE_NOT:
> - h = !match_expr_eval(x->u.unary, bol, eol, ctx, 0);
> + /*
> +  * Upon visiting a GREP_NODE_NOT, imatch and match become
> +  * swapped.
> +  */
> + h = !match_expr_eval(x->u.unary, bol, eol, ctx, icol, col, 0);

A minor nit, but the comment talks about something that are
different from the variable names; perhaps you called col/icol with
different names in an earlier incarnation of this patch?

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Junio C Hamano]

Taylor Blau  writes:

> Attached is a ``fresh start'' of my series to teach 'git grep --column'.
> Since the last time I sent this, much has changed, notably the semantics
> for deciding which column is the first when given (1) extended
> expressions and (2) --invert.
> ...
> In the future, I'd like to revisit this, since any performance gains
> that we _do_ make in this area are moot when we rescan all lines in
> show_line() with --color.

Sounds like a plan.  From a quick scan, it seems that this is
sufficiently different from the last round so I won't bother
rebuilding your "--only" series on top, and instead just drop those
two series from the older round and queue this as the new and
improved tb/grep-column topic.

Thanks.

Re: OAuth2 support in git?

[Jeff King]

On Tue, Jun 19, 2018 at 02:36:50PM +0200, Christian Halstrick wrote:

> What is not clear to me is how we can make use of the servers initial
> response in order control which credential helper to call and how to
> transport the credentials.

I don't think we'd ever decide _which_ credential helper to call; we
always call all of them, in order, and then quit when we have sufficient
credentials to continue.

But potentially we could feed some extra information to each helper and
let it decide what to do.

> Imagine we try to clone over http. The initial request sent to the
> server may not contain a "Authorization: ..." header and the server
> responds with Unauthorized.  But the server response contains hints
> like a "WWW-Authenticate: Basic realm=..." line or a
> "WWW-Authenticate: Bearer realm=..." line which helps choosing the
> authentication scheme used next. Maybe the server even responds with
> both lines telling I would accept BASIC or BEARER.

So for this example, yeah, I think it might make sense to feed the
credential helper extra context like "authtype=basic" or similar. Most
helpers would ignore it, but smart ones could make a decision based on
it.

And then the response could contain a similar "authtype" key in the
response.

> I can imagine that we want libcurl to deal with that decisions. But
> even then. How do we make sure the our credential helpers can act
> return either user/password or bearer tokens based on the server
> response? If credential helper would have access to the servers
> response (or only relevant parts of it?) it could decide whether to
> feel responsible for that server or not and what data to return.
> 
> And if credential helper could optionally give metadata about the kind
> credential they offer (e.g. "I return user/password" or "I return a
> bearer token") then core code could know where to transport this data.
> E.g. in a "Authorization: Basic ..." or a "Authorization: Bearer ..."
> field.

Yep, I think that all matches my general line of thinking. It would help
if we had some concrete cases. In particular, it's unclear to me if:

  1. A config option to say "treat password as a bearer token" would be
 enough.

  2. We'd need the credential helper to say "I'm giving you a token"
 versus "I'm giving you a password".

  3. We might need _both_ (1) and (2), because some servers would be
 fine with (1) and it lets them Just Work with credential helpers
 that are unaware of bearer tokens in the first place.

I suspect the answer is (3), but I'd probably delay working on (2) until
I saw a situation that really needed it. :)

But I think we're on the same page, so if you're looking into or
developing more concrete cases, those answers should become more clear.

-Peff

Re: [PATCH 0/7] grep.c: teach --column to 'git-grep(1)'

[Jeff King]

On Mon, Jun 18, 2018 at 06:43:01PM -0500, Taylor Blau wrote:

> Attached is a ``fresh start'' of my series to teach 'git grep --column'.
> Since the last time I sent this, much has changed, notably the semantics
> for deciding which column is the first when given (1) extended
> expressions and (2) --invert.
> 
> Both (1) and (2) are described in-depth in patch 2/7, but I am happy to
> answer more questions should they arise here. Peff and I worked on this
> together off-list, and we are both happy with the semantics, and believe
> that it covers most reasonable cases.

So with the exception of some minor type-quibbling in patch 4, this all
looks good to me. Since we discussed this quite a bit off-list, you can
take that review either with a giant grain of salt (reviewing something
I helped to generate in the first place) or a ringing endorsement (I
thought about it a lot more than I would have for a normal review ;) ).

> The notable case that it does _not_ cover is matching the following
> line:
> 
>   a ... b
> 
> with the following expression
> 
>   git grep --column -e b --or -e a
> 
> This will produce the column for 'b' rather than the column for 'a',
> since we short-circuit an --or when the left child finds a match, in
> this case 'b'. So, we break the semantics for this case, at the benefit
> of not having to do twice the work.
> 
> In the future, I'd like to revisit this, since any performance gains
> that we _do_ make in this area are moot when we rescan all lines in
> show_line() with --color. A path forward, I imagine, would look like a
> list of regmatch_t's, or a set of locations in the expression tree, such
> that we could either enumerate the list or walk the tree in order to
> colorize the line. But, I think for now that is #leftoverbits.

The key thing about this iteration is that it doesn't regress
performance, because we always short-circuit where we used to. The other
obvious route is to stop short-circuiting only when "--column" is in
effect, which would have the same property (at the expense of a little
extra code in match_expr_eval()).

-Peff

Re: [PATCH 4/7] grep.c: display column number of first match

[Taylor Blau]

On Tue, Jun 19, 2018 at 12:28:26PM -0400, Jeff King wrote:
> On Mon, Jun 18, 2018 at 06:43:14PM -0500, Taylor Blau wrote:
>
> >  static void show_line(struct grep_opt *opt, char *bol, char *eol,
> > - const char *name, unsigned lno, char sign)
> > + const char *name, unsigned lno, unsigned cno, char sign)
>
> Here "cno" is unsigned. But later...
>
> > +   if (opt->columnnum && cno) {
> > +   char buf[32];
> > +   xsnprintf(buf, sizeof(buf), "%d", cno);
>
> ...we print it with "%d". Should this be "%u"?

Thanks, that's certainly a mistake. I think (per the hunk of this
response below) that it should be "%zu" in the case that we change this
patch to take an ssize_t.

> But ultimately, the column number comes from this code:
>
> > @@ -1785,6 +1796,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
> > grep_source *gs, int colle
> > while (left) {
> > char *eol, ch;
> > int hit;
> > +   ssize_t cno;
> > ssize_t col = -1, icol = -1;
> >
> > /*
> > @@ -1850,7 +1862,15 @@ static int grep_source_1(struct grep_opt *opt, 
> > struct grep_source *gs, int colle
> > show_pre_context(opt, gs, bol, eol, lno);
> > else if (opt->funcname)
> > show_funcname_line(opt, gs, bol, lno);
> > -   show_line(opt, bol, eol, gs->name, lno, ':');
> > +   cno = opt->invert ? icol : col;
> > +   if (cno < 0) {
> > +   /*
> > +* A negative cno means that there was no match.
> > +* Clamp to the beginning of the line.
> > +*/
> > +   cno = 0;
> > +   }
>
> ...which is a ssize_t. Should we just be using ssize_t consistently?
>
> We do at least clamp the negative values here, but on 64-bit systems
> ssize_t is much larger than "unsigned".  I admit that it's probably
> ridiculous for any single line to overflow 32 bits, but it seems like we
> should consistently use size_t/ssize_t for buffer offsets, and then we
> don't have to think about it.

I agree that it's unlikely that a single line will overflow 32 bits, and
certainly at that point we might have other problems to worry about :-).

This was an unsigned in my original patch, and I left it this way in the
revised series for consistency with the other arguments to show_line().
But, I agree with your reasoning and think that this should be an
ssize_t, instead.


Thanks,
Taylor

Re: [GIT PULL] Korean l10n updates for Git 2.18.0

[Junio C Hamano]

Jiang Xin  writes:

> Hi Junio,
>
> The following changes since commit fd8cb379022fc6f5c6d71d12d10c9388b9f5841c:
>
>   l10n: zh_CN: for git v2.18.0 l10n round 1 to 3 (2018-06-18 00:31:45 +0800)
>
> are available in the Git repository at:
>
>   git://github.com/git-l10n/git-po tags/l10n-2.18.0-rnd3.1
>
> for you to fetch changes up to 4898dd2513360bd0cb32ca67ca07c70787c81399:
>
>   l10n: ko.po: Update Korean translation (2018-06-19 02:19:42 +0900)

Thanks.

>
> 
> Merge Korean translation for l10n of Git 2.18.0 round 3
>
> 
> Changwoo Ryu (1):
>   l10n: ko.po: Update Korean translation
>
>  po/TEAMS |4 +-
>  po/ko.po | 6083 
> --
>  2 files changed, 3553 insertions(+), 2534 deletions(-)
>
> --
> Jiang Xin

Re: [PATCH 4/7] grep.c: display column number of first match

[Jeff King]

On Mon, Jun 18, 2018 at 06:43:14PM -0500, Taylor Blau wrote:

>  static void show_line(struct grep_opt *opt, char *bol, char *eol,
> -   const char *name, unsigned lno, char sign)
> +   const char *name, unsigned lno, unsigned cno, char sign)

Here "cno" is unsigned. But later...

> + if (opt->columnnum && cno) {
> + char buf[32];
> + xsnprintf(buf, sizeof(buf), "%d", cno);

...we print it with "%d". Should this be "%u"?

But ultimately, the column number comes from this code:

> @@ -1785,6 +1796,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
> grep_source *gs, int colle
>   while (left) {
>   char *eol, ch;
>   int hit;
> + ssize_t cno;
>   ssize_t col = -1, icol = -1;
>  
>   /*
> @@ -1850,7 +1862,15 @@ static int grep_source_1(struct grep_opt *opt, struct 
> grep_source *gs, int colle
>   show_pre_context(opt, gs, bol, eol, lno);
>   else if (opt->funcname)
>   show_funcname_line(opt, gs, bol, lno);
> - show_line(opt, bol, eol, gs->name, lno, ':');
> + cno = opt->invert ? icol : col;
> + if (cno < 0) {
> + /*
> +  * A negative cno means that there was no match.
> +  * Clamp to the beginning of the line.
> +  */
> + cno = 0;
> + }

...which is a ssize_t. Should we just be using ssize_t consistently?

We do at least clamp the negative values here, but on 64-bit systems
ssize_t is much larger than "unsigned".  I admit that it's probably
ridiculous for any single line to overflow 32 bits, but it seems like we
should consistently use size_t/ssize_t for buffer offsets, and then we
don't have to think about it.

-Peff

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Junio C Hamano]

Todd Zullinger  writes:

> index e500d7c320..352a52e59d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' 
> '
>   set_fake_editor &&
>   FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>   git rebase -i --root &&
> - git show HEAD^ | grep "A changed"
> + git show HEAD^ | grep "A changed" &&
> + test -z "$(git show -s --format=%p HEAD^)"
>  '

The additional test probably will pass when HEAD is a root commit by
failing to refer to HEAD^, resulting an empty output from show.  The
previous step would also give an error and won't emit anything that
would match "A changed", so it probably is OK, though.

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 6:09 PM Duy Nguyen  wrote:
> On Tue, Jun 19, 2018 at 05:16:17PM +0200, Duy Nguyen wrote:
> > No actually, we could do better. Let me see if I can come up with a
> > patch or something...
>
> OK. What we currently do is, when we search for potential untracked
> paths for adding to the index, we unconditionally ignore anything
> inside ".git". For example, if "foo" is a submodule, "git add ." will
> visit "foo/.git" then ignore its content completely.
>
> We could do something very similar: when we visit "foo", if "foo/.git"
> exists, we ignore it as well. In other words, we extend from "ignore
> anything inside a git repository" to "ignore anything inside any other
> git worktree".
>
> The following patch basically does that. If you specify "git add
> foo/bar". It will still visit "foo" first, realize that it's a
> submodule and drop it. At the end, it will not report foo/bar as an
> untracked (i.e. add-able) entry, so you can't add it.

Another note (which I added, then thought otherwise and dropped). I
believe this approach also solves the problem that
die_path_inside_submodule() tries to work around.

When you feed a path inside a submodule, read_directory() code does
not realize it and walk through like it's part of the current worktree
(wrong!). But if read_directory() does the right thing from the
beginning, you don't need this trick. We don't even need this trick if
a submodule is not real on worktree (no ".git" directory there) but
registered in the index as a git link because the d/f check should
catch that and complain loudly anyway when you add a new entry.
-- 
Duy

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 05:16:17PM +0200, Duy Nguyen wrote:
> No actually, we could do better. Let me see if I can come up with a
> patch or something...

OK. What we currently do is, when we search for potential untracked
paths for adding to the index, we unconditionally ignore anything
inside ".git". For example, if "foo" is a submodule, "git add ." will
visit "foo/.git" then ignore its content completely.

We could do something very similar: when we visit "foo", if "foo/.git"
exists, we ignore it as well. In other words, we extend from "ignore
anything inside a git repository" to "ignore anything inside any other
git worktree".

The following patch basically does that. If you specify "git add
foo/bar". It will still visit "foo" first, realize that it's a
submodule and drop it. At the end, it will not report foo/bar as an
untracked (i.e. add-able) entry, so you can't add it.

I didn't test it extensively to see if it breaks anything though. And
I might need to check how it affects untracked cache...

-- 8< --
diff --git a/dir.c b/dir.c
index fe9bf58e4c..8a1a5d8dd5 100644
--- a/dir.c
+++ b/dir.c
@@ -1672,6 +1672,17 @@ static enum path_treatment treat_one_path(struct 
dir_struct *dir,
if (dtype != DT_DIR && has_path_in_index)
return path_none;
 
+   if (dtype == DT_DIR) {
+   int path_len = path->len;
+   int is_submodule;
+
+   strbuf_addstr(path, "/.git");
+   is_submodule = is_directory(path->buf);
+   strbuf_setlen(path, path_len);
+   if (is_submodule)
+   return path_none;
+   }
+
/*
 * When we are looking at a directory P in the working tree,
 * there are three cases:
-- 8< --

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 5:56 PM Junio C Hamano  wrote:
>
> Duy Nguyen  writes:
>
> > On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt  wrote:
> >>
> >> On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
> >> > On 06/18, Duy Nguyen wrote:
> >> > > This sounds like the submodule specific code in pathspec.c, which has
> >> > > been replaced with something else in bw/pathspec-sans-the-index. If
> >> > > you have time, try a version without those changes (e.g. v2.13 or
> >> > > before) to see if it's a possible culprit.
> >> >
> >> > I just tested this with v2.13 and saw the same issue.  I don't actually
> >> > think this ever worked in the way you want it to Heiko.  Maybe git add
> >> > needs to be taught to be more intelligent when trying to add a submodule
> >> > which doesn't exist in the index.
> >>
> >> That was also my guess, since my feeling is that this is a quite rare
> >> use case. Adding submodules alone is not a daily thing, let alone
> >> selecting different changes after 'git submodule add'.
> >>
> >> I also think git could be more intelligent here.
> >
> > Ah.. the "submodule not registered in index" case. I think I remember
> > this (because I remember complaining about it once or two times).
> > Definitely agreed that git-add should do the right thing here.
>
> I am not sure if this even needs to be implemented as "look for the
> submodule in the index".  Even before submodule was added, we knew
> that "git add foo/bar" should reject the request if we find foo is a
> symbolic link, and we should do the same when foo/ is a directory
> that is the top of a working tree under control of another
> repository, no?

Exactly. I started with the intention to do something related to the
index only to slowly realize that it was not the right place. We
traverse directories and stop looking inside a symlink, we can do the
same if we realize it's a submodule.
-- 
Duy

Re: Branch switching with submodules where the submodule replaces a folder aborts unexpectedly

[Sam Kuper]

On 12 Oct 2017 at 11:48 Thomas Braun wrote:
> On 9 Oct 2017 at 23:59, Stefan Beller wrote:
>> On 9 Oct 2017 at 14:29, Thomas Braun wrote:
>>> I'm currently in the progress of pulling some subprojects in a git 
>>> repository of mine into their
>>> own repositories and adding these subprojects back as submodules.
>>>
>>> While doing this I enountered a potential bug as checkout complains on 
>>> branch switching that a
>>> file already exists. [...]
>>>
>>> `error: The following untracked working tree files would be overwritten by 
>>> checkout:`

I am currently attempting the same thing, and experiencing the same
bug, using Git 2.17.1.


>> (And I presume you know about --recurse-submodules as a flag for 
>> git-checkout)

The behaviour seems to be the same regardless of whether I use the
--recurse-submodules flag with git-checkout.


>> This is consistent with our tests, unfortunately. [...]
>>
>>> If I'm misusing git here I'm glad for any advice.
>>
>> You are not.
>
> Glad to know that.

I was also glad to see this reassurance :)

It might have been nice if the reassurance came at an earlier stage,
however: at the CLI, rather than only after searching the mailing
list. A user who does not think to do the latter might well labour
under the misapprehension that they have done something wrong, rather
than encountered a bug. Perhaps, if the bug is not going to be fixed
terribly soon, a sentence or two could be added to the error message
explaining the situation and advising the user of a workaround?

Speaking of which, what is a good workaround in this case?

`git checkout --force `?


>> Apart from this bug report, would you think that such filtering of
>> trees into submodules (and back again) might be an interesting feature
>> of Git or are these cases rare and special?
>
> For me not particularly. In my case it is a one time thing going from an 
> embedded project folder to a submodule.

The option to convert an existing directory into a submodule is
something that I think developers like to have available. It seems
intuitive: "Oh, I see now that what this directory holds is
effectively a separate project. Let me check out a new branch, and
replace the directory with a submodule on that branch. Assuming it
goes well, then I will afterwards merge this new branch into master."

Regardless, the bug has clearly been giving people headaches for
several years (forgive me if you were already aware of these data
points):

- 
https://ryansechrest.com/2014/03/git-error-switching-branch-replacing-directory-submodule/

- http://blog.dcycle.com/blog/105/gitsubmodulizing-and-gitflow/

- https://stackoverflow.com/q/9299063

- https://stackoverflow.com/a/48402543

- https://stackoverflow.com/q/24091246

- https://stackoverflow.com/q/29372450

- https://github.com/supercollider/supercollider/issues/2001

- https://github.com/supercollider/supercollider/issues/2221

Thank you to Thomas for reporting this issue to the mailing list, and
to Stefan for the helpful reply. Thanks as always to the Git
maintainers, and good luck with fixing this bug :)

Re: [PATCH v2 signed off] doc: fix typos in documentation and release notes

[Junio C Hamano]

Xtreak  writes:

> Signed-off-by: Karthikeyan Singaravelan 
> ---
>  Documentation/RelNotes/1.7.11.7.txt | 2 +-
>  Documentation/RelNotes/2.17.0.txt   | 2 +-
>  Documentation/RelNotes/2.18.0.txt   | 2 +-
>  Documentation/diff-options.txt  | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)

Thanks, will apply.

>
> diff --git a/Documentation/RelNotes/1.7.11.7.txt 
> b/Documentation/RelNotes/1.7.11.7.txt
> index e7e79d999bd38..e743a2a8e46eb 100644
> --- a/Documentation/RelNotes/1.7.11.7.txt
> +++ b/Documentation/RelNotes/1.7.11.7.txt
> @@ -25,7 +25,7 @@ Fixes since v1.7.11.6
> references" nor "Reload" did not update what is shown as the
> contents of it, when the user overwrote the tag with "git tag -f".
>  
> - * "git for-each-ref" did not currectly support more than one --sort
> + * "git for-each-ref" did not correctly support more than one --sort
> option.
>  
>   * "git log .." errored out saying it is both rev range and a path
> diff --git a/Documentation/RelNotes/2.17.0.txt 
> b/Documentation/RelNotes/2.17.0.txt
> index d6db0e19cf17b..c2cf891f71adf 100644
> --- a/Documentation/RelNotes/2.17.0.txt
> +++ b/Documentation/RelNotes/2.17.0.txt
> @@ -342,7 +342,7 @@ Fixes since v2.16
> validate the data and connected-ness of objects in the received
> pack; the code to perform this check has been taught about the
> narrow clone's convention that missing objects that are reachable
> -   from objects in a pack that came from a promissor remote is OK.
> +   from objects in a pack that came from a promisor remote is OK.
>  
>   * There was an unused file-scope static variable left in http.c when
> building for versions of libCURL that is older than 7.19.4, which
> diff --git a/Documentation/RelNotes/2.18.0.txt 
> b/Documentation/RelNotes/2.18.0.txt
> index 7c59bd92fbd99..1eb13ece53600 100644
> --- a/Documentation/RelNotes/2.18.0.txt
> +++ b/Documentation/RelNotes/2.18.0.txt
> @@ -324,7 +324,7 @@ Fixes since v2.17
> after giving an error message.
> (merge 3bb0923f06 ps/contains-id-error-message later to maint).
>  
> - * "diff-highlight" filter (in contrib/) learned to undertand "git log
> + * "diff-highlight" filter (in contrib/) learned to understand "git log
> --graph" output better.
> (merge 4551fbba14 jk/diff-highlight-graph-fix later to maint).
>  
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index f466600972f86..bfa3808e49cc0 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -133,7 +133,7 @@ These parameters can also be set individually with 
> `--stat-width=`,
>   as file creations or deletions ("new" or "gone", optionally "+l"
>   if it's a symlink) and mode changes ("+x" or "-x" for adding
>   or removing executable bit respectively) in diffstat. The
> - information is put betwen the filename part and the graph
> + information is put between the filename part and the graph
>   part. Implies `--stat`.
>  
>  --numstat::
>
> --
> https://github.com/git/git/pull/510

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Junio C Hamano]

Todd Zullinger  writes:

> With luck, this will save you a few minutes, assuming the
> commit message is reasonable (or can be improved with help
> from Phillip and others). :)

OK.

> Or Junio may just squash this onto js/rebase-i-root-fix.

Nah, not for a hotfix on the last couple of days before the final.
We'd need to build on top, not "squash".

>
> Thanks.
>
>  t/t3404-rebase-interactive.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index e500d7c320..352a52e59d 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -977,7 +977,8 @@ test_expect_success 'rebase -i --root reword root commit' 
> '
>   set_fake_editor &&
>   FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
>   git rebase -i --root &&
> - git show HEAD^ | grep "A changed"
> + git show HEAD^ | grep "A changed" &&
> + test -z "$(git show -s --format=%p HEAD^)"
>  '
>  
>  test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on 
> non-interactive rebase' '

Re: Adding nested repository with slash adds files instead of gitlink

[Junio C Hamano]

Duy Nguyen  writes:

> On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt  wrote:
>>
>> On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
>> > On 06/18, Duy Nguyen wrote:
>> > > This sounds like the submodule specific code in pathspec.c, which has
>> > > been replaced with something else in bw/pathspec-sans-the-index. If
>> > > you have time, try a version without those changes (e.g. v2.13 or
>> > > before) to see if it's a possible culprit.
>> >
>> > I just tested this with v2.13 and saw the same issue.  I don't actually
>> > think this ever worked in the way you want it to Heiko.  Maybe git add
>> > needs to be taught to be more intelligent when trying to add a submodule
>> > which doesn't exist in the index.
>>
>> That was also my guess, since my feeling is that this is a quite rare
>> use case. Adding submodules alone is not a daily thing, let alone
>> selecting different changes after 'git submodule add'.
>>
>> I also think git could be more intelligent here.
>
> Ah.. the "submodule not registered in index" case. I think I remember
> this (because I remember complaining about it once or two times).
> Definitely agreed that git-add should do the right thing here.

I am not sure if this even needs to be implemented as "look for the
submodule in the index".  Even before submodule was added, we knew
that "git add foo/bar" should reject the request if we find foo is a
symbolic link, and we should do the same when foo/ is a directory
that is the top of a working tree under control of another
repository, no?

Hmm, what happens when we do this?

git init
ln -s /tmp foo
>foo/bar
git add foo/

I think we should say either "let's add foo symlink" or
"foo/. (directory) is beyond symlink" (the latter is preferrable,
but the former is acceptable as long as foo is pointing at a
directory; but foo could be a dangling symlink whose pointee's type
may not be discernable by "git add").

Shouldn't we be reacting pretty much the same when we see this?

git init
git init foo
>foo/bar
git add foo/

That is, either drop '/' and add 'foo' as a submodule, or say
"foo/. (directory) belongs to another repository, cannot add here"
(again, the latter is preferrable for consistency with the symlink
behaviour above).

[GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C.

A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.

Signed-off-by: Alban Gruin 
---
 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 25 -
 sequencer.c| 19 +++
 sequencer.h|  3 +++
 4 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7cd74da2e..19c1dba9a 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
+   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE,
+   CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "checkout-base", ,
N_("checkout the base commit"), CHECKOUT_BASE),
+   OPT_CMDMODE(0, "checkout-onto", ,
+   N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
 
@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!edit_todo_list(flags);
if (command == CHECKOUT_BASE && argc == 2)
return !!checkout_base_commit(, argv[1], verbose);
+   if (command == CHECKOUT_ONTO && argc == 4)
+   return !!checkout_onto(, argv[1], argv[2], argv[3], 
verbose);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index af46cf9c2..7b6142a76 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
 esac
 
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
-   case "$orig_reflog_action" in
-   ''|rebase*)
-   GIT_REFLOG_ACTION="rebase -i ($1)"
-   export GIT_REFLOG_ACTION
-   ;;
-   esac
-}
-
 die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
-   comment_for_reflog start
-   GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-   output git checkout $onto || die_abort "$(gettext "could not detach 
HEAD")"
-   git update-ref ORIG_HEAD $orig_head
-}
-
 get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
 
git rebase--helper --check-todo-list || {
ret=$?
-   checkout_onto
+   git rebase--helper --checkout-onto "$onto_name" "$onto" \
+   "$orig_head" ${verbose:+--verbose}
exit $ret
}
 
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
 
-   checkout_onto
+   git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+   ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
 --continue
diff --git a/sequencer.c b/sequencer.c
index a7a73e3ef..9165bf96c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3161,6 +3161,25 @@ int checkout_base_commit(struct replay_opts *opts, const 
char *commit,
return 0;
 }
 
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+   struct object_id oid;
+   const char *action = reflog_message(opts, "start", "checkout %s", 
onto_name);
+
+   if (get_oid(orig_head, ))
+   return error(_("%s: not a valid OID"), orig_head);
+
+   if (run_git_checkout(opts, onto, verbose, action)) {
+   apply_autostash(opts);
+   sequencer_remove_state(opts);
+   return error(_("could not detach HEAD"));
+   }
+
+   return update_ref(NULL, "ORIG_HEAD", , NULL, 0, 
UPDATE_REFS_MSG_ON_ERR);
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 42c3dda81..eda03ce32 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -102,6 +102,9 @@ void 

[GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

This rewrites setup_reflog_action() from shell to C. The new version is
called checkout_base_commit().

A new command is added to rebase--helper.c, “checkout-base”, as such as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().

The shell version is then stripped in favour of a call to the helper. As
$GIT_REFLOG_ACTION is not longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.

Signed-off-by: Alban Gruin 
---
 builtin/rebase--helper.c   |  9 +++--
 git-rebase--interactive.sh | 16 ++--
 sequencer.c| 28 
 sequencer.h|  3 +++
 4 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..7cd74da2e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
 N_("keep original branch points of cousins")),
+   OPT__VERBOSE(, N_("print subcommands output even if 
they succeed")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive 
rebase"),
EDIT_TODO),
+   OPT_CMDMODE(0, "checkout-base", ,
+   N_("checkout the base commit"), CHECKOUT_BASE),
OPT_END()
};
 
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+   if (command == CHECKOUT_BASE && argc == 2)
+   return !!checkout_base_commit(, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..af46cf9c2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+   comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach 
HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
 }
 
-setup_reflog_action () {
-   comment_for_reflog start
-
-   if test ! -z "$switch_to"
-   then
-   GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-   output git checkout "$switch_to" -- ||
-   die "$(eval_gettext "Could not checkout \$switch_to")"
-
-   comment_for_reflog start
-   fi
-}
-
 init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create 
temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
 
-   setup_reflog_action
+   git rebase--helper --checkout-base "$switch_to" ${verbose:+--verbose}
init_basic_state
 
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 9aa7ddb33..a7a73e3ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts 
*opts,
return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+   int verbose, const char *action)
+{
+   struct 

[GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.

[Alban Gruin]

This adds a new function, run_command_silent_on_success(), to redirect
the stdout and stderr of a command to a strbuf, and then to run that
command. This strbuf is printed only if the command fails. It also takes
a parameter, “verbose”. When true, the command is executed without
redirecting its output. It is functionnally similar to output() from
git-rebase.sh.

run_git_commit() is then refactored to use of
run_command_silent_on_success().

Signed-off-by: Alban Gruin 
---
 sequencer.c | 55 +--
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..9aa7ddb33 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -766,6 +766,29 @@ N_("you have staged changes in your working tree\n"
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
 
+static int run_command_silent_on_success(struct child_process *cmd,
+unsigned verbose)
+{
+   struct strbuf buf = STRBUF_INIT;
+   int rc;
+
+   if (verbose)
+   return run_command(cmd);
+
+   /* hide stderr on success */
+   cmd->stdout_to_stderr = 1;
+   rc = pipe_command(cmd,
+ NULL, 0,
+ /* stdout is already redirected */
+ NULL, 0,
+ , 0);
+
+   if (rc)
+   fputs(buf.buf, stderr);
+   strbuf_release();
+   return rc;
+}
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
 
cmd.git_cmd = 1;
 
-   if (is_rebase_i(opts)) {
-   if (!(flags & EDIT_MSG)) {
-   cmd.stdout_to_stderr = 1;
-   cmd.err = -1;
-   }
+   if (is_rebase_i(opts) && read_env_script(_array)) {
+   const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
-   if (read_env_script(_array)) {
-   const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
-   return error(_(staged_changes_advice),
-gpg_opt, gpg_opt);
-   }
+   return error(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
 
argv_array_push(, "commit");
@@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(, "--allow-empty-message");
 
-   if (cmd.err == -1) {
-   /* hide stderr on success */
-   struct strbuf buf = STRBUF_INIT;
-   int rc = pipe_command(,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- , 0);
-   if (rc)
-   fputs(buf.buf, stderr);
-   strbuf_release();
-   return rc;
-   }
-
-   return run_command();
+   return run_command_silent_on_success(,
+!(is_rebase_i(opts) && !(flags & 
EDIT_MSG)));
 }
 
 static int rest_is_empty(const struct strbuf *sb, int start)
-- 
2.16.4

[GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C

[Alban Gruin]

This patch series rewrites the reflog operations from shell to C.  This
is part of the effort to rewrite interactive rebase in C.

The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.

This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-19).

Changes since v1:

 - Replacing run_command_silent_if_successful() by
   run_command_silent_on_success() in the first commit’s message (thanks
   Christian!)

 - Adding a “verbose” parameter to run_command_silent_on_success()
   (thanks Phillip!)

 - Using OPT__VERBOSE to parse the “--verbose” flag (thanks Stefan!)

 - Fixing some typos and errors in the commit messages

 - Renaming “setup-reflog” to “checkout-base”

 - Renaming checkout_base_commit() to run_git_checkout()

 - Replacing calls to die() by error()

Alban Gruin (3):
  sequencer: add a new function to silence a command, except if it
fails.
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C

 builtin/rebase--helper.c   |  14 ++-
 git-rebase--interactive.sh |  39 +++--
 sequencer.c| 102 +
 sequencer.h|   6 +++
 4 files changed, 99 insertions(+), 62 deletions(-)

-- 
2.16.4

Re: [PATCH 05/23] midx: write header information to lockfile

[Derrick Stolee]

On 6/19/2018 10:59 AM, Duy Nguyen wrote:

On Tue, Jun 19, 2018 at 2:54 PM Derrick Stolee  wrote:

On 6/12/2018 11:00 AM, Duy Nguyen wrote:

On Thu, Jun 7, 2018 at 7:01 PM Derrick Stolee  wrote:

diff --git a/midx.c b/midx.c
index 616af66b13..3e55422a21 100644
--- a/midx.c
+++ b/midx.c
@@ -1,9 +1,62 @@
   #include "git-compat-util.h"
   #include "cache.h"
   #include "dir.h"
+#include "csum-file.h"
+#include "lockfile.h"
   #include "midx.h"

+#define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
+#define MIDX_VERSION 1
+#define MIDX_HASH_VERSION 1 /* SHA-1 */

...

+static size_t write_midx_header(struct hashfile *f,
+   unsigned char num_chunks,
+   uint32_t num_packs)
+{
+   char byte_values[4];
+   hashwrite_be32(f, MIDX_SIGNATURE);
+   byte_values[0] = MIDX_VERSION;
+   byte_values[1] = MIDX_HASH_VERSION;

Quoting from "State of NewHash work, future directions, and discussion" [1]

* If you need to serialize an algorithm identifier into your data
format, use the format_id field of struct git_hash_algo.  It's
designed specifically for that purpose.

[1] 
https://public-inbox.org/git/20180612024252.ga141...@aiede.svl.corp.google.com/T/#m5fdd09dcaf31266c45343fb6c0beaaa3e928bc60

Thanks! I'll also use the_hash_algo->rawsz to infer the length of the
hash function.

BTW, since you're the author of commit-graph.c and may notice it has
the same problem. Don't touch that code. Brian already has some WIP
changes [1]. We just make sure new code does not add extra work for
him. I expect he'll send all those patches out soon.

[1] 
https://github.com/bk2204/git/commit/3f9031e06cfb21534eb7dfff7b54e7598ac1149f


Thanks for the link. It seems he is creating an oid_version() method 
that returns a 1-byte version for the hash version instead of the 4-byte 
signature of the_hash_algo->format_id. I look forward to incorporating 
that into the MIDX format. I'll keep my macros for now, as we work out 
the other details, and while Brain's patch is cooking.


Thanks,
-Stolee

Re: [PATCH 01/15] contrib: add cocci script to replace index compat macros

[Derrick Stolee]

On 6/19/2018 10:51 AM, Duy Nguyen wrote:

On Tue, Jun 19, 2018 at 1:41 PM Derrick Stolee  wrote:

Duy,

Here is the patch that was generated by `make coccicheck`.

Thanks,
-Stolee

-->8--

--- builtin/add.c

Ah right. This is on purpose. I think I mentioned in the commit
message that builtin/ is not touched. Do we run 'make coccicheck'
automatically somewhere? If true, I need to move this script elsewhere
because it's meant to run manually. You run it when you intend to do
more manual fixups afterwards. For builtin/, I think I'll wait until
'struct repository *' conversion is complete then maybe fix them one
by one.


I don't think it is part of the CI runs, but some community members run 
this themselves on 'next' and 'master'.


I'm CC'ing Szeder, as he has contributed a fix to my own Coccinelle 
script [1].


[1] 
https://public-inbox.org/git/20180430093153.13040-1-szeder@gmail.com/
    [PATCH] coccinelle: avoid wrong transformation suggestions from 
commit.cocci

Re: Adding nested repository with slash adds files instead of gitlink

[Duy Nguyen]

On Tue, Jun 19, 2018 at 12:36 PM Heiko Voigt  wrote:
>
> On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
> > On 06/18, Duy Nguyen wrote:
> > > This sounds like the submodule specific code in pathspec.c, which has
> > > been replaced with something else in bw/pathspec-sans-the-index. If
> > > you have time, try a version without those changes (e.g. v2.13 or
> > > before) to see if it's a possible culprit.
> >
> > I just tested this with v2.13 and saw the same issue.  I don't actually
> > think this ever worked in the way you want it to Heiko.  Maybe git add
> > needs to be taught to be more intelligent when trying to add a submodule
> > which doesn't exist in the index.
>
> That was also my guess, since my feeling is that this is a quite rare
> use case. Adding submodules alone is not a daily thing, let alone
> selecting different changes after 'git submodule add'.
>
> I also think git could be more intelligent here.

Ah.. the "submodule not registered in index" case. I think I remember
this (because I remember complaining about it once or two times).
Definitely agreed that git-add should do the right thing here.

Brandon already moved the submodule check outside pathspec code
(wonderful!) so adding more checks based on worktree state should not
be a big work. I think the only concern here is catching submodule
locations so we don't check submodule at the same location multiple
times.

No actually, we could do better. Let me see if I can come up with a
patch or something...
-- 
Duy

Re: [PATCH 05/23] midx: write header information to lockfile

[Duy Nguyen]

On Tue, Jun 19, 2018 at 2:54 PM Derrick Stolee  wrote:
>
> On 6/12/2018 11:00 AM, Duy Nguyen wrote:
> > On Thu, Jun 7, 2018 at 7:01 PM Derrick Stolee  wrote:
> >> diff --git a/midx.c b/midx.c
> >> index 616af66b13..3e55422a21 100644
> >> --- a/midx.c
> >> +++ b/midx.c
> >> @@ -1,9 +1,62 @@
> >>   #include "git-compat-util.h"
> >>   #include "cache.h"
> >>   #include "dir.h"
> >> +#include "csum-file.h"
> >> +#include "lockfile.h"
> >>   #include "midx.h"
> >>
> >> +#define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
> >> +#define MIDX_VERSION 1
> >> +#define MIDX_HASH_VERSION 1 /* SHA-1 */
> > ...
> >> +static size_t write_midx_header(struct hashfile *f,
> >> +   unsigned char num_chunks,
> >> +   uint32_t num_packs)
> >> +{
> >> +   char byte_values[4];
> >> +   hashwrite_be32(f, MIDX_SIGNATURE);
> >> +   byte_values[0] = MIDX_VERSION;
> >> +   byte_values[1] = MIDX_HASH_VERSION;
> > Quoting from "State of NewHash work, future directions, and discussion" [1]
> >
> > * If you need to serialize an algorithm identifier into your data
> >format, use the format_id field of struct git_hash_algo.  It's
> >designed specifically for that purpose.
> >
> > [1] 
> > https://public-inbox.org/git/20180612024252.ga141...@aiede.svl.corp.google.com/T/#m5fdd09dcaf31266c45343fb6c0beaaa3e928bc60
>
> Thanks! I'll also use the_hash_algo->rawsz to infer the length of the
> hash function.

BTW, since you're the author of commit-graph.c and may notice it has
the same problem. Don't touch that code. Brian already has some WIP
changes [1]. We just make sure new code does not add extra work for
him. I expect he'll send all those patches out soon.

[1] 
https://github.com/bk2204/git/commit/3f9031e06cfb21534eb7dfff7b54e7598ac1149f

-- 
Duy

Re: [PATCH 01/15] contrib: add cocci script to replace index compat macros

[Duy Nguyen]

On Tue, Jun 19, 2018 at 1:41 PM Derrick Stolee  wrote:
>
> Duy,
>
> Here is the patch that was generated by `make coccicheck`.
>
> Thanks,
> -Stolee
>
> -->8--
>
> --- builtin/add.c

Ah right. This is on purpose. I think I mentioned in the commit
message that builtin/ is not touched. Do we run 'make coccicheck'
automatically somewhere? If true, I need to move this script elsewhere
because it's meant to run manually. You run it when you intend to do
more manual fixups afterwards. For builtin/, I think I'll wait until
'struct repository *' conversion is complete then maybe fix them one
by one.

> +++ /tmp/cocci-output-206193-4c91ec-add.c
> @@ -38,13 +38,13 @@ static void chmod_pathspec(struct pathsp
>  {
> int i;
>
> -   for (i = 0; i < active_nr; i++) {
> -   struct cache_entry *ce = active_cache[i];
> +   for (i = 0; i < the_index.cache_nr; i++) {
> +   struct cache_entry *ce = the_index.cache[i];
>
> if (pathspec && !ce_path_match(_index, ce, pathspec, 
> NULL))
> continue;
>
> -   if (chmod_cache_entry(ce, flip) < 0)
> +   if (chmod_index_entry(_index, ce, flip) < 0)
> fprintf(stderr, "cannot chmod %cx '%s'\n", flip, 
> ce->name);
> }
>  }
-- 
Duy

Re: [PATCH 00/15] Kill the_index part 1, expose it

[Duy Nguyen]

On Tue, Jun 19, 2018 at 1:48 PM Derrick Stolee  wrote:
> Personally,
> I find it difficult to base a patch off of multiple in-progress branches
> and would rather work off of a "known good" point like the tip of master.

You should always base your patches on 'master' (or even 'maint' if
it's bug fixes that have a chance of entering 'maint'). We all we face
conflicts at some point (mostly when we rebase after something gets
merged to master; or things are merged on 'pu'). But git-rerere should
keep conflict resolution easy (most of the time). I also expect Junio
to kick and scream when patch series conflict badly on 'pu' and he
will decide which one goes first and kick others out temporarily.
-- 
Duy

Re: [PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts

[Elijah Newren]

On Tue, Jun 19, 2018 at 5:46 AM, Phillip Wood  wrote:
> From: Phillip Wood 
>
> Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin",
> 2017-02-09), when a commit marked as 'reword' in an interactive rebase
> has conflicts and fails to apply, when the rebase is resumed that commit
> will be squashed into its parent with its commit message taken.
>
> The issue can be understood better by looking at commit 56dc3ab04b
> ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which
> introduced error_with_patch() for the edit command.  For the edit command,
> it needs to stop the rebase whether or not the patch applies cleanly.  If
> the patch does apply cleanly, then when it resumes it knows it needs to
> amend all changes into the previous commit.  If it does not apply cleanly,
> then the changes should not be amended.  Thus, it passes !res (success of
> applying the 'edit' commit) to error_with_patch() for the to_amend flag.
>
> The problematic line of code actually came from commit 04efc8b57c
> ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02).
> Note that to get to this point in the code:
>   * !!res (i.e. patch application failed)
>   * item->command < TODO_SQUASH
>   * item->command != TODO_EDIT
>   * !is_fixup(item->command) [i.e. not squash or fixup]
> So that means this can only be a failed patch application that is either a
> pick, revert, or reword.  We only need to amend HEAD when rewording the
> root commit or a commit that has been fast-forwarded, for any of the other
> cases we want a new commit, so we should not set the to_amend flag.
>
> Helped-by: Johannes Schindelin 
> Original-patch-by: Elijah Newren 
> Signed-off-by: Phillip Wood 
> ---
>
> I wasn't really sure what to do about the authorship.  This is
> Elijah's patch, with the message tweaked, fixed up with a corrected
> version of Johannes' code and a couple of new tests by my for picking
> and rewording the root commit when it has untracked file confilcts.

Yeah, this one was a little challenging on authorship.  What you've
done here is fine from my angle; thanks for catching the other
important cases I missed.

Junio: As a reminder, this is a bugfix for a regression that appeared
in git 2.13.0; it is not new to the 2.18 cycle.

[As an aside, I know there are multiple other outstanding emails for
me to respond to, unrelated to this patch.  I'll try to get some time
in the next day or two to respond.  Just responding to this one since
Junio mentioned picking it up for 2.18.]

>  sequencer.c   | 23 ++---
>  t/t3404-rebase-interactive.sh | 28 
>  t/t3423-rebase-reword.sh  | 48 +++
>  3 files changed, 96 insertions(+), 3 deletions(-)
>  create mode 100755 t/t3423-rebase-reword.sh
>
> diff --git a/sequencer.c b/sequencer.c
> index 4034c0461b..7bf2b62727 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3214,10 +3214,27 @@ static int pick_commits(struct todo_list *todo_list, 
> struct replay_opts *opts)
> intend_to_amend();
> return error_failed_squash(item->commit, opts,
> item->arg_len, item->arg);
> -   } else if (res && is_rebase_i(opts) && item->commit)
> +   } else if (res && is_rebase_i(opts) && item->commit) {
> +   int to_amend = 0;
> +   struct object_id oid;
> +
> +   /*
> +* If we are rewording and have either
> +* fast-forwarded already, or are about to
> +* create a new root commit, we want to amend,
> +* otherwise we do not.
> +*/
> +   if (item->command == TODO_REWORD &&
> +   !get_oid("HEAD", ) &&
> +   (!oidcmp(>commit->object.oid, ) 
> ||
> +(opts->have_squash_onto &&
> + !oidcmp(>squash_onto, 
> +   to_amend = 1;
> +
> return res | error_with_patch(item->commit,
> -   item->arg, item->arg_len, opts, res,
> -   item->command == TODO_REWORD);
> +   item->arg, item->arg_len, 
> opts,
> +   res, to_amend);
> +   }
> } else if (item->command == TODO_EXEC) {
> char *end_of_arg = (char *)(item->arg + 
> item->arg_len);
> int saved = *end_of_arg;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 

recommendations for practical git config settings for /etc/gitconfig?

[Robert P. J. Day]

  updating some git course material, and i want to add to the config
section at least a small number of example config settings that make
practical sense to add to the system /etc/gitconfig file. that is,
config settings that, even if i don't explain them fully, even novice
git users will appreciate that such settings make sense to apply to
*all* users on a system.

  i imagine that would include any (corporate-mandated) settings
related to networking authentication and protocols,
filesystem-related settings and more, such as:

  * core.protect{HFS,NTFS}
  * whitespace/EOL settings
  * proxy/ssh-related commands
  * http.* settings
  * some sendemail settings

you get the idea. i don't have a ton of experience configuring git in
an enterprise setting, so while there are a zillion config settings
one *could* add to /etc/gitconfig, i'm interested in what experienced
git admins can testify really make sense to set in a corporate
environment. does that make sense?

  open to thoughts ...

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH 05/23] midx: write header information to lockfile

[Derrick Stolee]

On 6/12/2018 11:00 AM, Duy Nguyen wrote:

On Thu, Jun 7, 2018 at 7:01 PM Derrick Stolee  wrote:

diff --git a/midx.c b/midx.c
index 616af66b13..3e55422a21 100644
--- a/midx.c
+++ b/midx.c
@@ -1,9 +1,62 @@
  #include "git-compat-util.h"
  #include "cache.h"
  #include "dir.h"
+#include "csum-file.h"
+#include "lockfile.h"
  #include "midx.h"

+#define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
+#define MIDX_VERSION 1
+#define MIDX_HASH_VERSION 1 /* SHA-1 */

...

+static size_t write_midx_header(struct hashfile *f,
+   unsigned char num_chunks,
+   uint32_t num_packs)
+{
+   char byte_values[4];
+   hashwrite_be32(f, MIDX_SIGNATURE);
+   byte_values[0] = MIDX_VERSION;
+   byte_values[1] = MIDX_HASH_VERSION;

Quoting from "State of NewHash work, future directions, and discussion" [1]

* If you need to serialize an algorithm identifier into your data
   format, use the format_id field of struct git_hash_algo.  It's
   designed specifically for that purpose.

[1] 
https://public-inbox.org/git/20180612024252.ga141...@aiede.svl.corp.google.com/T/#m5fdd09dcaf31266c45343fb6c0beaaa3e928bc60


Thanks! I'll also use the_hash_algo->rawsz to infer the length of the 
hash function.

[PATCH v3] sequencer: do not squash 'reword' commits when we hit conflicts

[Phillip Wood]

From: Phillip Wood 

Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper builtin",
2017-02-09), when a commit marked as 'reword' in an interactive rebase
has conflicts and fails to apply, when the rebase is resumed that commit
will be squashed into its parent with its commit message taken.

The issue can be understood better by looking at commit 56dc3ab04b
("sequencer (rebase -i): implement the 'edit' command", 2017-01-02), which
introduced error_with_patch() for the edit command.  For the edit command,
it needs to stop the rebase whether or not the patch applies cleanly.  If
the patch does apply cleanly, then when it resumes it knows it needs to
amend all changes into the previous commit.  If it does not apply cleanly,
then the changes should not be amended.  Thus, it passes !res (success of
applying the 'edit' commit) to error_with_patch() for the to_amend flag.

The problematic line of code actually came from commit 04efc8b57c
("sequencer (rebase -i): implement the 'reword' command", 2017-01-02).
Note that to get to this point in the code:
  * !!res (i.e. patch application failed)
  * item->command < TODO_SQUASH
  * item->command != TODO_EDIT
  * !is_fixup(item->command) [i.e. not squash or fixup]
So that means this can only be a failed patch application that is either a
pick, revert, or reword.  We only need to amend HEAD when rewording the
root commit or a commit that has been fast-forwarded, for any of the other
cases we want a new commit, so we should not set the to_amend flag.

Helped-by: Johannes Schindelin 
Original-patch-by: Elijah Newren 
Signed-off-by: Phillip Wood 
---

I wasn't really sure what to do about the authorship.  This is
Elijah's patch, with the message tweaked, fixed up with a corrected
version of Johannes' code and a couple of new tests by my for picking
and rewording the root commit when it has untracked file confilcts.

 sequencer.c   | 23 ++---
 t/t3404-rebase-interactive.sh | 28 
 t/t3423-rebase-reword.sh  | 48 +++
 3 files changed, 96 insertions(+), 3 deletions(-)
 create mode 100755 t/t3423-rebase-reword.sh

diff --git a/sequencer.c b/sequencer.c
index 4034c0461b..7bf2b62727 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3214,10 +3214,27 @@ static int pick_commits(struct todo_list *todo_list, 
struct replay_opts *opts)
intend_to_amend();
return error_failed_squash(item->commit, opts,
item->arg_len, item->arg);
-   } else if (res && is_rebase_i(opts) && item->commit)
+   } else if (res && is_rebase_i(opts) && item->commit) {
+   int to_amend = 0;
+   struct object_id oid;
+
+   /*
+* If we are rewording and have either
+* fast-forwarded already, or are about to
+* create a new root commit, we want to amend,
+* otherwise we do not.
+*/
+   if (item->command == TODO_REWORD &&
+   !get_oid("HEAD", ) &&
+   (!oidcmp(>commit->object.oid, ) ||
+(opts->have_squash_onto &&
+ !oidcmp(>squash_onto, 
+   to_amend = 1;
+
return res | error_with_patch(item->commit,
-   item->arg, item->arg_len, opts, res,
-   item->command == TODO_REWORD);
+   item->arg, item->arg_len, opts,
+   res, to_amend);
+   }
} else if (item->command == TODO_EXEC) {
char *end_of_arg = (char *)(item->arg + item->arg_len);
int saved = *end_of_arg;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e500d7c320..3786879e80 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -980,7 +980,35 @@ test_expect_success 'rebase -i --root reword root commit' '
git show HEAD^ | grep "A changed"
 '
 
+test_expect_success 'rebase -i --root when root has untracked file confilct' '
+   test_when_finished "reset_rebase" &&
+   git checkout -b failing-root-pick A &&
+   echo x >file2 &&
+   git rm file1 &&
+   git commit -m "remove file 1 add file 2" &&
+   echo z >file1 &&
+   set_fake_editor &&
+   test_must_fail env FAKE_LINES="1 2" git rebase -i --root &&
+   rm file1 &&
+   git rebase --continue &&
+   test "$(git log -1 --format=%B)" = "remove file 1 add file 2" 

Re: OAuth2 support in git?

[Christian Halstrick]

What is not clear to me is how we can make use of the servers initial
response in
order control which credential helper to call and how to transport the
credentials.

Imagine we try to clone over http. The initial request sent to the server
may not contain a "Authorization: ..." header and the server responds
with Unauthorized.
But the server response contains hints like a "WWW-Authenticate: Basic
realm=..." line
or a "WWW-Authenticate: Bearer realm=..." line which helps choosing the
authentication scheme used next. Maybe the server even responds with both lines
telling I would accept BASIC or BEARER.

I can imagine that we want libcurl to deal with that decisions. But
even then. How
do we make sure the our credential helpers can act return either user/password
or bearer tokens based on the server response? If credential helper
would have access
to the servers response (or only relevant parts of it?) it could
decide whether to
feel responsible for that server or not and what data to return.

And if credential helper could optionally give metadata about the kind
credential they offer
(e.g. "I return user/password" or "I return a bearer token") then core
code could know
where to transport this data. E.g. in a "Authorization: Basic ..." or
a "Authorization: Bearer ..."
field.

Ciao
  Chris

Re: [PATCH 00/15] Kill the_index part 1, expose it

[Derrick Stolee]

On 6/16/2018 1:41 AM, Nguyễn Thái Ngọc Duy wrote:

This is the beginning of the end of the_index. The problem with
the_index is it lets library code anywhere access it freely. This is
not good because from high level you may not realize that the_index is
being used while you don't want to touch index at all, or you want to
use a different index instead.

This is a long series, 86 patches [1], so I'm going to split and
submit it in 15-20 patches at a time. The first two parts are trivial
though and could be safely fast tracked if needed.

This is the first part, which kills the use of index compat macros
outside builtin/ and expose the_index in all library code. Later on we
will ban the_index from one file each time until it's gone for good.

"struct index_state *" will be passed from builtin/ through the call
chain to the function that needs it. In some cases, "struct
repository *" will be passed instead when the whole operation spans
more than just the index.  By the end, the_index becomes part of
"index compat macros" and cannot be used outside builtin/

Part one is mechanical conversion with the help of coccinelle. The
only real patches are the first and the last one.

[1] https://gitlab.com/pclouds/git/commits/really-kill-the-index


This is a good series, and a good goal!

Outside of dropping [PATCH 01/15] until all the changes are applied, 
this patch looks like a good, mechanical change.


There are a lot of cross-cutting changes happening right now, between 
this series of series and Stefan's series of series. Hopefully after 
2.18 is cut, a lot of these can graduate to master quickly. Personally, 
I find it difficult to base a patch off of multiple in-progress branches 
and would rather work off of a "known good" point like the tip of master.


Thanks,
-Stolee

Re: [PATCH 01/15] contrib: add cocci script to replace index compat macros

[Derrick Stolee]

Duy,

Here is the patch that was generated by `make coccicheck`.

Thanks,
-Stolee

-->8--

--- builtin/add.c
+++ /tmp/cocci-output-206193-4c91ec-add.c
@@ -38,13 +38,13 @@ static void chmod_pathspec(struct pathsp
 {
int i;
 
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
+   for (i = 0; i < the_index.cache_nr; i++) {
+   struct cache_entry *ce = the_index.cache[i];
 
if (pathspec && !ce_path_match(_index, ce, pathspec, NULL))
continue;
 
-   if (chmod_cache_entry(ce, flip) < 0)
+   if (chmod_index_entry(_index, ce, flip) < 0)
fprintf(stderr, "cannot chmod %cx '%s'\n", flip, 
ce->name);
}
 }
@@ -129,8 +129,8 @@ static int renormalize_tracked_files(con
 {
int i, retval = 0;
 
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
+   for (i = 0; i < the_index.cache_nr; i++) {
+   struct cache_entry *ce = the_index.cache[i];
 
if (ce_stage(ce))
continue; /* do not touch unmerged paths */
@@ -138,7 +138,8 @@ static int renormalize_tracked_files(con
continue; /* do not touch non blobs */
if (pathspec && !ce_path_match(_index, ce, pathspec, NULL))
continue;
-   retval |= add_file_to_cache(ce->name, flags | HASH_RENORMALIZE);
+   retval |= add_file_to_index(_index, ce->name,
+   flags | HASH_RENORMALIZE);
}
 
return retval;
@@ -230,7 +231,7 @@ static int edit_patch(int argc, const ch
 
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
-   if (read_cache() < 0)
+   if (read_index(_index) < 0)
die(_("Could not read the index"));
 
init_revisions(, the_repository, prefix);
@@ -445,7 +446,7 @@ int cmd_add(int argc, const char **argv,
return 0;
}
 
-   if (read_cache() < 0)
+   if (read_index(_index) < 0)
die(_("index file corrupt"));
 
die_in_unpopulated_submodule(_index, prefix);
--- builtin/am.c
+++ /tmp/cocci-output-206198-3fcbd5-am.c
@@ -1144,7 +1144,7 @@ static void refresh_and_write_cache(void
struct lock_file lock_file = LOCK_INIT;
 
hold_locked_index(_file, LOCK_DIE_ON_ERROR);
-   refresh_cache(REFRESH_QUIET);
+   refresh_index(_index, REFRESH_QUIET, NULL, NULL, NULL);
if (write_locked_index(_index, _file, COMMIT_LOCK))
die(_("unable to write index file"));
 }
@@ -1505,8 +1505,8 @@ static int run_apply(const struct am_sta
 
if (index_file) {
/* Reload index as apply_all_patches() will have modified it. */
-   discard_cache();
-   read_cache_from(index_file);
+   discard_index(_index);
+   read_index_from(_index, index_file, get_git_dir());
}
 
return 0;
@@ -1548,8 +1548,8 @@ static int fall_back_threeway(const stru
if (build_fake_ancestor(state, index_path))
return error("could not build fake ancestor");
 
-   discard_cache();
-   read_cache_from(index_path);
+   discard_index(_index);
+   read_index_from(_index, index_path, get_git_dir());
 
if (write_index_as_tree(_tree, _index, index_path, 0, NULL))
return error(_("Repository lacks necessary blobs to fall back 
on 3-way merge."));
@@ -1581,8 +1581,8 @@ static int fall_back_threeway(const stru
 
say(state, stdout, _("Falling back to patching base and 3-way 
merge..."));
 
-   discard_cache();
-   read_cache();
+   discard_index(_index);
+   read_index(_index);
 
/*
 * This is not so wrong. Depending on which base we picked, orig_tree
@@ -1886,7 +1886,7 @@ static void am_resolve(struct am_state *
die_user_resolve(state);
}
 
-   if (unmerged_cache()) {
+   if (unmerged_index(_index)) {
printf_ln(_("You still have unmerged paths in your index.\n"
"You should 'git add' each file with resolved conflicts 
to mark them as such.\n"
"You might run `git rm` on a file to accept \"deleted 
by them\" for it."));
@@ -1925,7 +1925,7 @@ static int fast_forward_to(struct tree *
 
hold_locked_index(_file, LOCK_DIE_ON_ERROR);
 
-   refresh_cache(REFRESH_QUIET);
+   refresh_index(_index, REFRESH_QUIET, NULL, NULL, NULL);
 
memset(, 0, sizeof(opts));
opts.head_idx = 1;
@@ -2000,7 +2000,7 @@ static int clean_index(const struct obje
if (!remote_tree)
return error(_("Could not parse object '%s'."), 
oid_to_hex(remote));
 
-   read_cache_unmerged();
+   read_index_unmerged(_index);
 
if (fast_forward_to(head_tree, head_tree, 1))

Re: [PATCH 01/15] contrib: add cocci script to replace index compat macros

[Derrick Stolee]

On 6/16/2018 1:41 AM, Nguyễn Thái Ngọc Duy wrote:

Index compat macros are going to be removed to expose the_index and
then reorganized to use the right index instead of simply the_index
because sometimes we want to use a different index.

This cocci script can help with the first step. It can be used later
on on even builtin/ when we start to reorganize code in there, but for
now this is the script that performs all the conversion outside
builtin/


I pulled your code and ran `make coccicheck` and got quite a large patch 
as a result.


Perhaps reorder the commits in your large patch with this one at the 
end? It's helpful to see what your mechanical changes are going to be, 
but let's save it for when `make coccicheck` is stable.




Signed-off-by: Nguyễn Thái Ngọc Duy 
---
  contrib/coccinelle/index-compat.cocci | 184 ++
  1 file changed, 184 insertions(+)
  create mode 100644 contrib/coccinelle/index-compat.cocci

diff --git a/contrib/coccinelle/index-compat.cocci 
b/contrib/coccinelle/index-compat.cocci
new file mode 100644
index 00..ca46711eb6
--- /dev/null
+++ b/contrib/coccinelle/index-compat.cocci
@@ -0,0 +1,184 @@
+@@
+@@
+-active_cache
++the_index.cache
+
+@@
+@@
+-active_nr
++the_index.cache_nr
+
+@@
+@@
+-active_alloc
++the_index.cache_alloc
+
+@@
+@@
+-active_cache_changed
++the_index.cache_changed
+
+@@
+@@
+-active_cache_tree
++the_index.cache_tree
+
+@@
+@@
+- read_cache()
++ read_index(_index)
+
+@@
+expression path;
+@@
+- read_cache_from(path)
++ read_index_from(_index, path, get_git_dir())
+
+@@
+expression pathspec;
+@@
+- read_cache_preload(pathspec)
++ read_index_preload(_index, pathspec)
+
+@@
+@@
+- is_cache_unborn()
++ is_index_unborn(_index)
+
+@@
+@@
+- read_cache_unmerged()
++ read_index_unmerged(_index)
+
+@@
+@@
+- discard_cache()
++ discard_index(_index)
+
+@@
+@@
+- unmerged_cache()
++ unmerged_index(_index)
+
+@@
+expression name;
+expression namelen;
+@@
+- cache_name_pos(name, namelen)
++ index_name_pos(_index, name, namelen)
+
+@@
+expression ce;
+expression option;
+@@
+- add_cache_entry(ce, option)
++ add_index_entry(_index, ce, option)
+
+@@
+expression pos;
+expression new_name;
+@@
+- rename_cache_entry_at(pos, new_name)
++ rename_index_entry_at(_index, pos, new_name)
+
+@@
+expression pos;
+@@
+- remove_cache_entry_at(pos)
++ remove_index_entry_at(_index, pos)
+
+@@
+expression path;
+@@
+- remove_file_from_cache(path)
++ remove_file_from_index(_index, path)
+
+@@
+expression path;
+expression st;
+expression flags;
+@@
+- add_to_cache(path, st, flags)
++ add_to_index(_index, path, st, flags)
+
+@@
+expression path;
+expression flags;
+@@
+- add_file_to_cache(path, flags)
++ add_file_to_index(_index, path, flags)
+
+@@
+expression ce;
+expression flip;
+@@
+-chmod_cache_entry(ce, flip)
++chmod_index_entry(_index, ce, flip)
+
+@@
+expression flags;
+@@
+-refresh_cache(flags)
++refresh_index(_index, flags, NULL, NULL, NULL)
+
+@@
+expression ce;
+expression st;
+expression options;
+@@
+-ce_match_stat(ce, st, options)
++ie_match_stat(_index, ce, st, options)
+
+@@
+expression ce;
+expression st;
+expression options;
+@@
+-ce_modified(ce, st, options)
++ie_modified(_index, ce, st, options)
+
+@@
+expression name;
+expression namelen;
+@@
+-cache_dir_exists(name, namelen)
++index_dir_exists(_index, name, namelen)
+
+@@
+expression name;
+expression namelen;
+expression igncase;
+@@
+-cache_file_exists(name, namelen, igncase)
++index_file_exists(_index, name, namelen, igncase)
+
+@@
+expression name;
+expression namelen;
+@@
+-cache_name_is_other(name, namelen)
++index_name_is_other(_index, name, namelen)
+
+@@
+@@
+-resolve_undo_clear()
++resolve_undo_clear_index(_index)
+
+@@
+expression at;
+@@
+-unmerge_cache_entry_at(at)
++unmerge_index_entry_at(_index, at)
+
+@@
+expression pathspec;
+@@
+-unmerge_cache(pathspec)
++unmerge_index(_index, pathspec)
+
+@@
+expression path;
+expression sz;
+@@
+-read_blob_data_from_cache(path, sz)
++read_blob_data_from_index(_index, path, sz)

Re: want option to git-rebase

[Ian Jackson]

Jonathan Nieder writes ("Re: want  option to git-rebase"):
> Ian Jackson wrote[1]:
> > git-rebase leaves entries like this in the reflog:
> >
> >   c15f4d5391 HEAD@{33}: rebase: checkout 
> > c15f4d5391ff07a718431aca68a73e672fe8870e
...
>  GIT_REFLOG_ACTION
>   When a ref is updated, reflog entries are created to keep
>   track of the reason why the ref was updated (which is
>   typically the name of the high-level command that updated the
>   ref), in addition to the old and new values of the ref. A
>   scripted Porcelain command can use set_reflog_action helper
>   function in git-sh-setup to set its name to this variable when
>   it is invoked as the top level command by the end user, to be
>   recorded in the body of the reflog.
> 
> "git rebase" sets this itself, so it doesn't solve your problem.

Hrm.

> Can you say more about what your tool does?  I'm wondering if it would
> make sense for it to use lower-level commands where GIT_REFLOG_ACTION
> applies, instead of the more user-facing git rebase.

Sure.

http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git-manpage/dgit.git/git-debrebase.1

See the description of git-rebase new-upstream.  It does a lot of
complicated work, synthesising a new pair of commits using plumbing
etc., and then does
  git rebase --onto  

If the user says git rebase --abort, everything should be undone.

Another alternative solution would be to be able to make git reflog
entries without actually updating any ref.

Ian.

-- 
Ian JacksonThese opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

Re: Adding nested repository with slash adds files instead of gitlink

[Heiko Voigt]

On Mon, Jun 18, 2018 at 11:12:15AM -0700, Brandon Williams wrote:
> On 06/18, Duy Nguyen wrote:
> > This sounds like the submodule specific code in pathspec.c, which has
> > been replaced with something else in bw/pathspec-sans-the-index. If
> > you have time, try a version without those changes (e.g. v2.13 or
> > before) to see if it's a possible culprit.
> 
> I just tested this with v2.13 and saw the same issue.  I don't actually
> think this ever worked in the way you want it to Heiko.  Maybe git add
> needs to be taught to be more intelligent when trying to add a submodule
> which doesn't exist in the index.

That was also my guess, since my feeling is that this is a quite rare
use case. Adding submodules alone is not a daily thing, let alone
selecting different changes after 'git submodule add'.

I also think git could be more intelligent here.

Cheers Heiko

Re: Adding nested repository with slash adds files instead of gitlink

[Heiko Voigt]

Hi,

On Mon, Jun 18, 2018 at 05:55:44PM +0200, Kevin Daudt wrote:
> On Mon, Jun 18, 2018 at 01:19:19PM +0200, Heiko Voigt wrote:
> > I just discovered that when you have a slash at the end of a nested
> > repository, the files contained in the repository get added instead of
> > the gitlink.
[...]
> > 
> > I just thought I put this out there. Will have a look if I find the time
> > to cook up a proper testcase and investigate.
> > 
> > Cheers Heiko
> 
> This has been the case as far as I can remember, and is basically lore
> in the #git irc channel).
> 
> This can also be reproduced by just cloning a repo inside another repo
> and running `git add path/`.

Interesting and nobody complained to the mailinglist? IMO, there is no
reason 'git add path' and 'git add path/' should behave differently or
is there?

So it seems it is a very seldom operation (in my daily work it is at
least) and people just accepted it as a quirk of git.

If someone wants to look into changing this feel free, otherwise I will
have a look, but I am not sure when yet.

Cheers Heiko

Re: [PATCH v2] sequencer: do not squash 'reword' commits when wehit conflicts

[Phillip Wood]

Hi Johannes

On 18/06/18 22:42, Johannes Schindelin wrote:
> 
> Hi Phillip,
> 
> On Mon, 18 Jun 2018, Phillip Wood wrote:
> 
>> On 17/06/18 20:28, Johannes Schindelin wrote:
>>>
>>> On Sun, 17 Jun 2018, Phillip Wood wrote:
>>>
 On 17/06/18 06:37, Elijah Newren wrote:
> Ever since commit 18633e1a22 ("rebase -i: use the rebase--helper
> builtin", 2017-02-09), when a commit marked as 'reword' in an
> interactive rebase has conflicts and fails to apply, when the rebase
> is resumed that commit will be squashed into its parent with its
> commit message taken.
>
> The issue can be understood better by looking at commit 56dc3ab04b
> ("sequencer (rebase -i): implement the 'edit' command", 2017-01-02),
> which introduced error_with_patch() for the edit command.  For the
> edit command, it needs to stop the rebase whether or not the patch
> applies cleanly.  If the patch does apply cleanly, then when it
> resumes it knows it needs to amend all changes into the previous
> commit.  If it does not apply cleanly, then the changes should not
> be amended.  Thus, it passes !res (success of applying the 'edit'
> commit) to error_with_patch() for the to_amend flag.
>
> The problematic line of code actually came from commit 04efc8b57c
> ("sequencer (rebase -i): implement the 'reword' command", 2017-01-02).
> Note that to get to this point in the code:
>* !!res (i.e. patch application failed)
>* item->command < TODO_SQUASH
>* item->command != TODO_EDIT
>* !is_fixup(item->command) [i.e. not squash or fixup]
> So that means this can only be a failed patch application that is
> either a pick, revert, or reword.  For any of those cases we want a
> new commit, so we should not set the to_amend flag.

 Unfortunately I'm not sure it's that simple. Looking and do_pick()
 sometimes reword amends HEAD and sometimes it does not. In the
 "normal" case then the commit is picked and committed with '--edit'.
 However when fast-forwarding the code fast forwards to the commit to
 be reworded and then amends it. If the root commit is being reworded
 it takes the same code path. While these cases cannot fail with
 conflicts, it is possible for the user to cancel the commit or for
 them to fail due to collisions with untracked files.

 If I remember correctly the shell version always picks the commit and
 then calls 'git commit --amend' afterwards which is less efficient
 but consistent.

 I'm afraid I don't have a simple solution for fixing this, as
 currently pick_commits() does not know if the commit was called with
 AMEND_MSG, I guess that means adding some kind of flag for do_pick()
 to set.
>>>
>>> Oh, you're right, the fast-forwarding path would pose a problem. I
>>> think there is an easy way to resolve this, though: in the case that
>>> we do want to amend the to-be-reworded commit, we simply have to see
>>> whether HEAD points to the very same commit mentioned in the `reword`
>>> command:
>>
>> That's clever, I think to get it to work for rewording the root commit,
>> it will need to do something like comparing HEAD to squash-onto as well.
> 
>  because squash-onto is a fresh, empty root commit (to be "amended"
> when a non-root commit is to be picked as a new root commit). Good point.
> 
>>> -- snip --
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 2dad7041960..99d33d4e063 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -3691,10 +3691,22 @@ static int pick_commits(struct todo_list
>>> *todo_list, struct replay_opts *opts)
>>> intend_to_amend();
>>> return error_failed_squash(item->commit, 
>>> opts,
>>> item->arg_len, item->arg);
>>> -   } else if (res && is_rebase_i(opts) && item->commit)
>>> +   } else if (res && is_rebase_i(opts) && 
>>> item->commit) {
>>> +   int to_amend = 0;
>>> +
>>> +   if (item->command == TODO_REWORD) {
>>> +   struct object_id head;
>>> +
>>> +   if (!get_oid("HEAD", ) &&
>>> +   !oidcmp(>commit->object.oid,
>>> +   ))
>>> +   to_amend = 1;
> 
> This would now become
> 
>   if (!get_oid("HEAD", ) &&
>   (!oidcmp(>commit->object.oid,
>) ||
>(opts->have_squash_onto &&
> !oidcmp(>squash_onto,
> 
>  

Re: [PATCH 2/6] git-p4: python3: replace dict.has_key(k) with "k in dict"

[Eric Sunshine]

On Tue, Jun 19, 2018 at 4:04 AM Luke Diamand  wrote:
> Python3 does not have the dict.has_key() function, so replace all
> such calls with "k in dict". This will still work with python2.6
> and python2.7.
>
> Converted using 2to3 (plus some hand-editing)
>
> Signed-off-by: Luke Diamand 
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -3141,7 +3141,7 @@ def importP4Labels(self, stream, p4Labels):
> -if change.has_key('change'):
> +if 'change' in change:

Very existential.

All these changes look sensible (as one might expect from automated conversion).

[PATCH 5/6] git-p4: python3: use print() function

[Luke Diamand]

Replace calls to print ... with the function form, print(...), to
allow use with python3 as well as python2.x.

Converted using 2to3 (and some hand-editing).

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

diff --git a/git-p4.py b/git-p4.py
index f127ebce27..714e442d7c 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -892,7 +892,7 @@ def findUpstreamBranchPoint(head = "HEAD"):
 
 def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", 
silent=True):
 if not silent:
-print ("Creating/updating branch(es) in %s based on origin branch(es)"
+print("Creating/updating branch(es) in %s based on origin branch(es)"
% localRefPrefix)
 
 originPrefix = "origin/p4/"
@@ -914,7 +914,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 update = False
 if not gitBranchExists(remoteHead):
 if verbose:
-print "creating %s" % remoteHead
+print("creating %s" % remoteHead)
 update = True
 else:
 settings = 
extractSettingsGitLog(extractLogMessageFromGitCommit(remoteHead))
@@ -923,13 +923,13 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 originP4Change = int(original['change'])
 p4Change = int(settings['change'])
 if originP4Change > p4Change:
-print ("%s (%s) is newer than %s (%s). "
+print("%s (%s) is newer than %s (%s). "
"Updating p4 branch from origin."
% (originHead, originP4Change,
   remoteHead, p4Change))
 update = True
 else:
-print ("Ignoring: %s was imported from %s while "
+print("Ignoring: %s was imported from %s while "
"%s was imported from %s"
% (originHead, ','.join(original['depot-paths']),
   remoteHead, ','.join(settings['depot-paths'])))
@@ -1397,9 +1397,9 @@ def __init__(self):
 def run(self, args):
 j = 0
 for output in p4CmdList(args):
-print 'Element: %d' % j
+print('Element: %d' % j)
 j += 1
-print output
+print(output)
 return True
 
 class P4RollBack(Command):
@@ -1440,14 +1440,14 @@ def run(self, args):
 
 if len(p4Cmd("changes -m 1 "  + ' '.join (['%s...@%s' % (p, 
maxChange)
for p in 
depotPaths]))) == 0:
-print "Branch %s did not exist at change %s, deleting." % 
(ref, maxChange)
+print("Branch %s did not exist at change %s, deleting." % 
(ref, maxChange))
 system("git update-ref -d %s `git rev-parse %s`" % (ref, 
ref))
 continue
 
 while change and int(change) > maxChange:
 changed = True
 if self.verbose:
-print "%s is at %s ; rewinding towards %s" % (ref, 
change, maxChange)
+print("%s is at %s ; rewinding towards %s" % (ref, 
change, maxChange))
 system("git update-ref %s \"%s^\"" % (ref, ref))
 log = extractLogMessageFromGitCommit(ref)
 settings =  extractSettingsGitLog(log)
@@ -1457,7 +1457,7 @@ def run(self, args):
 change = settings['change']
 
 if changed:
-print "%s rewound to %s" % (ref, change)
+print("%s rewound to %s" % (ref, change))
 
 return True
 
@@ -1593,10 +1593,10 @@ def patchRCSKeywords(self, file, pattern):
 except:
 # cleanup our temporary file
 os.unlink(outFileName)
-print "Failed to strip RCS keywords in %s" % file
+print("Failed to strip RCS keywords in %s" % file)
 raise
 
-print "Patched up RCS keywords in %s" % file
+print("Patched up RCS keywords in %s" % file)
 
 def p4UserForCommit(self,id):
 # Return the tuple (perforce user,git email) for a given git commit id
@@ -1616,7 +1616,7 @@ def checkValidP4Users(self,commits):
 if not user:
 msg = "Cannot find p4 user for email %s in commit %s." % 
(email, id)
 if gitConfigBool("git-p4.allowMissingP4Users"):
-print "%s" % msg
+print("%s" % msg)
 else:
 die("Error: %s\nSet git-p4.allowMissingP4Users to true to 
allow this." % msg)
 
@@ -1808,8 +1808,8 @@ def get_diff_description(self, editedFiles, filesToAdd, 
symlinks):
 def 

[PATCH 2/6] git-p4: python3: replace dict.has_key(k) with "k in dict"

[Luke Diamand]

Python3 does not have the dict.has_key() function, so replace all
such calls with "k in dict". This will still work with python2.6
and python2.7.

Converted using 2to3 (plus some hand-editing)

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

diff --git a/git-p4.py b/git-p4.py
index 51e9e64a73..6fcad35104 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -767,7 +767,7 @@ def gitDeleteRef(ref):
 _gitConfig = {}
 
 def gitConfig(key, typeSpecifier=None):
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 cmd = [ "git", "config" ]
 if typeSpecifier:
 cmd += [ typeSpecifier ]
@@ -781,12 +781,12 @@ def gitConfigBool(key):
variable is set to true, and False if set to false or not present
in the config."""
 
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 _gitConfig[key] = gitConfig(key, '--bool') == "true"
 return _gitConfig[key]
 
 def gitConfigInt(key):
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 cmd = [ "git", "config", "--int", key ]
 s = read_pipe(cmd, ignore_error=True)
 v = s.strip()
@@ -797,7 +797,7 @@ def gitConfigInt(key):
 return _gitConfig[key]
 
 def gitConfigList(key):
-if not _gitConfig.has_key(key):
+if key not in _gitConfig:
 s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
 _gitConfig[key] = s.strip().splitlines()
 if _gitConfig[key] == ['']:
@@ -855,7 +855,7 @@ def findUpstreamBranchPoint(head = "HEAD"):
 tip = branches[branch]
 log = extractLogMessageFromGitCommit(tip)
 settings = extractSettingsGitLog(log)
-if settings.has_key("depot-paths"):
+if "depot-paths" in settings:
 paths = ",".join(settings["depot-paths"])
 branchByDepotPath[paths] = "remotes/p4/" + branch
 
@@ -865,9 +865,9 @@ def findUpstreamBranchPoint(head = "HEAD"):
 commit = head + "~%s" % parent
 log = extractLogMessageFromGitCommit(commit)
 settings = extractSettingsGitLog(log)
-if settings.has_key("depot-paths"):
+if "depot-paths" in settings:
 paths = ",".join(settings["depot-paths"])
-if branchByDepotPath.has_key(paths):
+if paths in branchByDepotPath:
 return [branchByDepotPath[paths], settings]
 
 parent = parent + 1
@@ -891,8 +891,8 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 originHead = line
 
 original = 
extractSettingsGitLog(extractLogMessageFromGitCommit(originHead))
-if (not original.has_key('depot-paths')
-or not original.has_key('change')):
+if ('depot-paths' not in original
+or 'change' not in original):
 continue
 
 update = False
@@ -902,7 +902,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = 
"refs/remotes/p4/", silent
 update = True
 else:
 settings = 
extractSettingsGitLog(extractLogMessageFromGitCommit(remoteHead))
-if settings.has_key('change') > 0:
+if 'change' in settings:
 if settings['depot-paths'] == original['depot-paths']:
 originP4Change = int(original['change'])
 p4Change = int(settings['change'])
@@ -1002,7 +1002,7 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 
 # Insert changes in chronological order
 for entry in reversed(result):
-if not entry.has_key('change'):
+if 'change' not in entry:
 continue
 changes.add(int(entry['change']))
 
@@ -1312,7 +1312,7 @@ def p4UserId(self):
 
 results = p4CmdList("user -o")
 for r in results:
-if r.has_key('User'):
+if 'User' in r:
 self.myP4UserId = r['User']
 return r['User']
 die("Could not find your p4 user id")
@@ -1336,7 +1336,7 @@ def getUserMapFromPerforceServer(self):
 self.emails = {}
 
 for output in p4CmdList("users"):
-if not output.has_key("User"):
+if "User" not in output:
 continue
 self.users[output["User"]] = output["FullName"] + " <" + 
output["Email"] + ">"
 self.emails[output["Email"]] = output["User"]
@@ -1588,7 +1588,7 @@ def p4UserForCommit(self,id):
 gitEmail = read_pipe(["git", "log", "--max-count=1",
   "--format=%ae", id])
 gitEmail = gitEmail.strip()
-if not self.emails.has_key(gitEmail):
+if gitEmail not in self.emails:
 return (None,gitEmail)
 else:
 return (self.emails[gitEmail],gitEmail)
@@ -1612,14 +1612,14 @@ def lastP4Changelist(self):
 results = p4CmdList("client -o")# 

[PATCH 4/6] git-p4: python3: basestring workaround

[Luke Diamand]

In Python3, basestring no longer exists, so use this workaround.

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

diff --git a/git-p4.py b/git-p4.py
index 67865d14aa..f127ebce27 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -27,6 +27,22 @@
 import ctypes
 import errno
 
+# support basestring in python3
+try:
+unicode = unicode
+except NameError:
+# 'unicode' is undefined, must be Python 3
+str = str
+unicode = str
+bytes = bytes
+basestring = (str,bytes)
+else:
+# 'unicode' exists, must be Python 2
+str = str
+unicode = unicode
+bytes = str
+basestring = basestring
+
 try:
 from subprocess import CalledProcessError
 except ImportError:
-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 1/6] git-p4: python3: replace <> with !=

[Luke Diamand]

The <> string inequality operator (which doesn't seem to be even
documented) no longer exists in python3. Replace with !=.

This still works with python2.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 0354d4df5c..51e9e64a73 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3590,7 +3590,7 @@ def run(self, args):
 prev_list = prev.split("/")
 cur_list = cur.split("/")
 for i in range(0, min(len(cur_list), 
len(prev_list))):
-if cur_list[i] <> prev_list[i]:
+if cur_list[i] != prev_list[i]:
 i = i - 1
 break
 
-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 0/6] git-p4: small step towards Python3 support

[Luke Diamand]

This patchset is a first small step towards Python3 support for
git-p4.py.

These are all the nice easy changes which can almost be done
automatically using 2to3.

After these changes, it compiles using Python3, but fails to run.
That's because of the bytes vs string change in Python3. Fixing that is
quite a bit harder (but not impossible).

I have some further changes to address this, but they are quite a bit
more invasive, and not actually working yet. It's based very loosely on the
"polystr()" suggestion from Eric on this list.

It still works fine with Python2.7 and Python2.6.

Luke Diamand (6):
  git-p4: python3: replace <> with !=
  git-p4: python3: replace dict.has_key(k) with "k in dict"
  git-p4: python3: remove backticks
  git-p4: python3: basestring workaround
  git-p4: python3: use print() function
  git-p4: python3: fix octal constants

 git-p4.py | 348 --
 1 file changed, 182 insertions(+), 166 deletions(-)

-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 6/6] git-p4: python3: fix octal constants

[Luke Diamand]

See PEP3127. Works fine with python2 as well.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 714e442d7c..b449db1cc9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1841,7 +1841,7 @@ def applyCommit(self, id):
 filesToDelete.remove(path)
 
 dst_mode = int(diff['dst_mode'], 8)
-if dst_mode == 012:
+if dst_mode == 0o12:
 symlinks.add(path)
 
 elif modifier == "D":
-- 
2.18.0.rc1.242.g61856ae69a

[PATCH 3/6] git-p4: python3: remove backticks

[Luke Diamand]

Backticks around a variable are a deprecated alias for repr().
This has been removed in python3, so just use the string
representation instead, which is equivalent.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 6fcad35104..67865d14aa 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -3089,7 +3089,7 @@ def getLabels(self):
 
 l = p4CmdList(["labels"] + ["%s..." % p for p in self.depotPaths])
 if len(l) > 0 and not self.silent:
-print "Finding files belonging to labels in %s" % `self.depotPaths`
+print("Finding files belonging to labels in %s" % self.depotPaths)
 
 for output in l:
 label = output["label"]
-- 
2.18.0.rc1.242.g61856ae69a

Re: [PATCH] t3404: check root commit in 'rebase -i --root reword root commit'

[Johannes Schindelin]

Hi Todd,

On Mon, 18 Jun 2018, Todd Zullinger wrote:

> When testing a reworded root commit, ensure that the squash-onto commit
> which is created and amended is still the root commit.
> 
> Suggested-by: Phillip Wood 
> Helped-by: Johannes Schindelin 
> Signed-off-by: Todd Zullinger 

Trusting that this test passes: ACK!

Ciao,
Dscho

Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

[Johannes Schindelin]

Hi Stefan,

On Mon, 18 Jun 2018, Stefan Beller wrote:

> On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin  wrote:
> >
> > +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> > +   int verbose)
> > +{
> > +   const char *action;
> > +
> > +   if (commit && *commit) {
> 
> While this is defensive programming (checking the pointer before dereferencing
> it, the first condition (commit == NULL) should never be false here,
> as the caller
> checks for argc == 2 ?

But it is not marked as `static` (or is it?). So we should not rely on the
caller to Do The Right Thing.

Ciao,
Dscho