[PATCH 7/7] diff-highlight: detect --graph by indent

[Jeff King]

This patch fixes a corner case where diff-highlight may
scramble some diffs when combined with --graph.

Commit 7e4ffb4c17 (diff-highlight: add support for --graph
output, 2016-08-29) taught diff-highlight to skip past the
graph characters at the start of each line with this regex:

  ($COLOR?\|$COLOR?\s+)*

I.e., any series of pipes separated by and followed by
arbitrary whitespace.  We need to match more than just a
single space because the commit in question may be indented
to accommodate other parts of the graph drawing. E.g.:

 * commit 1234abcd
 | ...
 | diff --git ...

has only a single space, but for the last commit before a
fork:

 | | |
 | * | commit 1234abcd
 | |/  ...
 | |   diff --git

the diff lines have more spaces between the pipes and the
start of the diff.

However, when we soak up all of those spaces with the
$GRAPH regex, we may accidentally include the leading space
for a context line. That means we may consider the actual
contents of a context line as part of the diff syntax. In
other words, something like this:

   normal context line
  -old line
  +new line
   -this is a context line with a leading dash

would cause us to see that final context line as a removal
line, and we'd end up showing the hunk in the wrong order:

  normal context line
  -old line
   -this is a context line with a leading dash
  +new line

Instead, let's a be a little more clever about parsing the
graph. We'll look for the actual "*" line that marks the
start of a commit, and record the indentation we see there.
Then we can skip past that indentation when checking whether
the line is a hunk header, removal, addition, etc.

There is one tricky thing: the indentation in bytes may be
different for various lines of the graph due to coloring.
E.g., the "*" on a commit line is generally shown without
color, but on the actual diff lines, it will be replaced
with a colorized "|" character, adding several bytes. We
work around this here by counting "visible" bytes. This is
unfortunately a bit more expensive, making us about twice as
slow to handle --graph output. But since this is meant to be
used interactively anyway, it's tolerably fast (and the
non-graph case is unaffected).

One alternative would be to search for hunk header lines and
use their indentation (since they'd have the same colors as
the diff lines which follow). But that just opens up
different corner cases. If we see:

  | |@@ 1,2 1,3 @@

we cannot know if this is a real diff that has been
indented due to the graph, or if it's a context line that
happens to look like a diff header. We can only be sure of
the indent on the "*" lines, since we know those don't
contain arbitrary data (technically the user could include a
bunch of extra indentation via --format, but that's rare
enough to disregard).

Reported-by: Phillip Wood 
Signed-off-by: Jeff King 
---
 contrib/diff-highlight/DiffHighlight.pm   | 78 +++
 .../diff-highlight/t/t9400-diff-highlight.sh  | 28 +++
 2 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm 
b/contrib/diff-highlight/DiffHighlight.pm
index e07cd5931d..536754583b 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -21,34 +21,82 @@ my $RESET = "\x1b[m";
 my $COLOR = qr/\x1b\[[0-9;]*m/;
 my $BORING = qr/$COLOR|\s/;
 
-# The patch portion of git log -p --graph should only ever have preceding | and
-# not / or \ as merge history only shows up on the commit line.
-my $GRAPH = qr/$COLOR?\|$COLOR?\s+/;
-
 my @removed;
 my @added;
 my $in_hunk;
+my $graph_indent = 0;
 
 our $line_cb = sub { print @_ };
 our $flush_cb = sub { local $| = 1 };
 
-sub handle_line {
+# Count the visible width of a string, excluding any terminal color sequences.
+sub visible_width {
local $_ = shift;
+   my $ret = 0;
+   while (length) {
+   if (s/^$COLOR//) {
+   # skip colors
+   } elsif (s/^.//) {
+   $ret++;
+   }
+   }
+   return $ret;
+}
+
+# Return a substring of $str, omitting $len visible characters from the
+# beginning, where terminal color sequences do not count as visible.
+sub visible_substr {
+   my ($str, $len) = @_;
+   while ($len > 0) {
+   if ($str =~ s/^$COLOR//) {
+   next
+   }
+   $str =~ s/^.//;
+   $len--;
+   }
+   return $str;
+}
+
+sub handle_line {
+   my $orig = shift;
+   local $_ = $orig;
+
+   # match a graph line that begins a commit
+   if (/^(?:$COLOR?\|$COLOR?[ ])* # zero or more leading "|" with space
+$COLOR?\*$COLOR?[ ]   # a "*" with its trailing space
+ (?:$COLOR?\|$COLOR?[ ])* # zero or more trailing "|"
+[ ]*  # trailing whitespace for merges
+   /x) {
+   my $graph_prefix = 

[PATCH 6/7] diff-highlight: use flush() helper consistently

[Jeff King]

The current flush() helper only shows the queued diff but
does not clear the queue. This is conceptually a bug, but it
works because we only call it once at the end of the
program.

Let's teach it to clear the queue, which will let us use it
in more places (one for now, but more in future patches).

Signed-off-by: Jeff King 
---
 contrib/diff-highlight/DiffHighlight.pm | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/contrib/diff-highlight/DiffHighlight.pm 
b/contrib/diff-highlight/DiffHighlight.pm
index 663992e530..e07cd5931d 100644
--- a/contrib/diff-highlight/DiffHighlight.pm
+++ b/contrib/diff-highlight/DiffHighlight.pm
@@ -46,10 +46,7 @@ sub handle_line {
push @added, $_;
}
else {
-   show_hunk(\@removed, \@added);
-   @removed = ();
-   @added = ();
-
+   flush();
$line_cb->($_);
$in_hunk = /^$GRAPH*$COLOR*[\@ ]/;
}
@@ -71,6 +68,8 @@ sub flush {
# Flush any queued hunk (this can happen when there is no trailing
# context in the final diff of the input).
show_hunk(\@removed, \@added);
+   @removed = ();
+   @added = ();
 }
 
 sub highlight_stdin {
-- 
2.17.0.rc0.402.ged0b3fd1ee

[PATCH 5/7] diff-highlight: test graphs with --color

[Jeff King]

Our tests send git's output directly to files or pipes, so
there will never be any color. Let's do at least one --color
test to make sure that we can handle this case (which we
currently can, but will be an easy thing to mess up when we
touch the graph code in a future patch).

We'll just cover the --graph case, since this is much more
complex than the earlier cases (i.e., if it manages to
highlight, then the non-graph case definitely would).

Signed-off-by: Jeff King 
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 33bcdbc90f..bf0c270d60 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -264,6 +264,15 @@ test_expect_success 'diff-highlight works with the --graph 
option' '
test_cmp graph.exp graph.act
 '
 
+# Just reuse the previous graph test, but with --color.  Our trimming
+# doesn't know about color, so just sanity check that something got
+# highlighted.
+test_expect_success 'diff-highlight works with color graph' '
+   git log --branches -p --date-order --graph --color |
+   "$DIFF_HIGHLIGHT" | trim_graph | left_trim >graph &&
+   grep "\[7m" graph
+'
+
 # Most combined diffs won't meet diff-highlight's line-number filter. So we
 # create one here where one side drops a line and the other modifies it. That
 # should result in a diff like:
-- 
2.17.0.rc0.402.ged0b3fd1ee

[PATCH 3/7] diff-highlight: prefer "echo" to "cat" in tests

[Jeff King]

We generate a bunch of one-line files whose contents match
their names, and then generate our commits by cat-ing those
files. Let's just echo the contents directly, which saves
some processes.

Signed-off-by: Jeff King 
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index deab90ed91..3f02d31467 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -68,26 +68,22 @@ test_strip_patch_header () {
 #   D
 #
 dh_test_setup_history () {
-   echo "file1" >file1 &&
-   echo "file2" >file2 &&
-   echo "file3" >file3 &&
-
-   cat file1 >file &&
+   echo file1 >file &&
git add file &&
test_tick &&
git commit -m "D" &&
 
git checkout -b branch &&
-   cat file2 >file &&
+   echo file2 >file &&
test_tick &&
git commit -a -m "E" &&
 
-   cat file3 >file &&
+   echo file3 >file &&
test_tick &&
git commit -a -m "F" &&
 
git checkout master &&
-   cat file2 >file &&
+   echo file2 >file &&
test_tick &&
git commit -a -m "A"
 }
-- 
2.17.0.rc0.402.ged0b3fd1ee

[PATCH 4/7] diff-highlight: test interleaved parallel lines of history

[Jeff King]

The graph test in t9400 covers the case of two simultaneous
branches, but all of the commits during this time are on the
right-hand branch. So we test a graph structure like:

  | |
  | * commit ...
  | |

but we never see the reverse, a commit on the left-hand
branch:

  | |
  * | commit ...
  | |

Since this is an easy thing to get wrong when touching the
graph-matching code, let's cover it by adding one more
commit with its timestamp interleaved with the other branch.

Note that we need to pass --date-order to convince Git to
show it this way (since --topo-order tries to keep lines of
history separate).

Signed-off-by: Jeff King 
---
 .../diff-highlight/t/t9400-diff-highlight.sh  | 22 +--
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 3f02d31467..33bcdbc90f 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -52,15 +52,17 @@ test_strip_patch_header () {
 # dh_test_setup_history generates a contrived graph such that we have at least
 # 1 nesting (E) and 2 nestings (F).
 #
-#A master
+#A---B master
 #   /
 #  D---E---F branch
 #
 #  git log --all --graph
 #  * commit
-#  |A
+#  |B
 #  | * commit
 #  | |F
+#  * | commit
+#  | |A
 #  | * commit
 #  |/
 #  |E
@@ -78,14 +80,20 @@ dh_test_setup_history () {
test_tick &&
git commit -a -m "E" &&
 
+   git checkout master &&
+   echo file2 >file &&
+   test_tick &&
+   git commit -a -m "A" &&
+
+   git checkout branch &&
echo file3 >file &&
test_tick &&
git commit -a -m "F" &&
 
git checkout master &&
-   echo file2 >file &&
+   echo file3 >file &&
test_tick &&
-   git commit -a -m "A"
+   git commit -a -m "B"
 }
 
 left_trim () {
@@ -246,12 +254,12 @@ test_expect_failure 'diff-highlight treats combining code 
points as a unit' '
 test_expect_success 'diff-highlight works with the --graph option' '
dh_test_setup_history &&
 
-   # topo-order so that the order of the commits is the same as with 
--graph
+   # date-order so that the commits are interleaved for both
# trim graph elements so we can do a diff
# trim leading space because our trim_graph is not perfect
-   git log --branches -p --topo-order |
+   git log --branches -p --date-order |
"$DIFF_HIGHLIGHT" | left_trim >graph.exp &&
-   git log --branches -p --graph |
+   git log --branches -p --date-order --graph |
"$DIFF_HIGHLIGHT" | trim_graph | left_trim >graph.act &&
test_cmp graph.exp graph.act
 '
-- 
2.17.0.rc0.402.ged0b3fd1ee

[PATCH 2/7] diff-highlight: use test_tick in graph test

[Jeff King]

The exact ordering output by Git may depend on the commit
timestamps, so let's make sure they're actually
monotonically increasing, and not all the same (or worse,
subject to how long the test script takes to run).

Let's use test_tick to make sure this is stable. Note that
we actually have to rearrange the order of the branches to
match the expected graph structure (which means that
previously we might racily have been testing a slightly
different output, though the test is written in such a way
that we'd still pass).

Signed-off-by: Jeff King 
---
 .../diff-highlight/t/t9400-diff-highlight.sh   | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index bf62f036c7..deab90ed91 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -52,9 +52,9 @@ test_strip_patch_header () {
 # dh_test_setup_history generates a contrived graph such that we have at least
 # 1 nesting (E) and 2 nestings (F).
 #
-#A branch
+#A master
 #   /
-#  D---E---F master
+#  D---E---F branch
 #
 #  git log --all --graph
 #  * commit
@@ -74,18 +74,22 @@ dh_test_setup_history () {
 
cat file1 >file &&
git add file &&
+   test_tick &&
git commit -m "D" &&
 
git checkout -b branch &&
cat file2 >file &&
-   git commit -a -m "A" &&
-
-   git checkout master &&
-   cat file2 >file &&
+   test_tick &&
git commit -a -m "E" &&
 
cat file3 >file &&
-   git commit -a -m "F"
+   test_tick &&
+   git commit -a -m "F" &&
+
+   git checkout master &&
+   cat file2 >file &&
+   test_tick &&
+   git commit -a -m "A"
 }
 
 left_trim () {
-- 
2.17.0.rc0.402.ged0b3fd1ee

[PATCH 1/7] diff-highlight: correct test graph diagram

[Jeff King]

We actually branch "A" off of "D". The sample "--graph"
output is right, but the left-to-right diagram is
misleading. Let's fix it.

Signed-off-by: Jeff King 
---
 contrib/diff-highlight/t/t9400-diff-highlight.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/diff-highlight/t/t9400-diff-highlight.sh 
b/contrib/diff-highlight/t/t9400-diff-highlight.sh
index 3b43dbed74..bf62f036c7 100755
--- a/contrib/diff-highlight/t/t9400-diff-highlight.sh
+++ b/contrib/diff-highlight/t/t9400-diff-highlight.sh
@@ -52,8 +52,8 @@ test_strip_patch_header () {
 # dh_test_setup_history generates a contrived graph such that we have at least
 # 1 nesting (E) and 2 nestings (F).
 #
-#A branch
-#   /
+#A branch
+#   /
 #  D---E---F master
 #
 #  git log --all --graph
-- 
2.17.0.rc0.402.ged0b3fd1ee

Re: [BUG] log --graph corrupts patch

[Jeff King]

On Tue, Mar 20, 2018 at 11:58:14AM -0400, Jeff King wrote:

> The issue bisects to 7e4ffb4c17 (diff-highlight: add support for --graph
> output, 2016-08-29). I think the problem is the "\s+" at the end of the
> $GRAPH regex, which soaks up the space for the context, and accidentally
> treats the "-" line as a preimage removal.
> 
> But just switching that to "\s" doesn't quite work. We may have an
> arbitrary number of spaces between the graph ascii-art and the diff.
> E.g., if you have a commit at the base of a branch (the test in
> contrib/diff-highlight shows this case).
> 
> So I think you'd have to record the indent of the previous hunk header,
> and then make sure that the indent matched that. But even there, I think
> we're subject to false positives if a commit message contains a hunk
> header (it's indented an extra 4 characters, but we'd accidentally soak
> that up thinking it was graph indentation).
> 
> To make it bullet-proof, I think we'd have to actually parse the graph
> structure, finding a "*" line and then accepting only an indent that
> matched it.

Wow. Nerd snipe successful. This turned out to be quite tricky, but also
kind of interesting.

Here's a series which fixes it. The meaty bits are in the final commit;
the rest is just preparatory cleanup, and adding some tests (all are
cases which I managed to break while fixing this).

  [1/7]: diff-highlight: correct test graph diagram
  [2/7]: diff-highlight: use test_tick in graph test
  [3/7]: diff-highlight: prefer "echo" to "cat" in tests
  [4/7]: diff-highlight: test interleaved parallel lines of history
  [5/7]: diff-highlight: test graphs with --color
  [6/7]: diff-highlight: factor out flush_hunk() helper
  [7/7]: diff-highlight: detect --graph by indent

 contrib/diff-highlight/DiffHighlight.pm   | 89 +++
 .../diff-highlight/t/t9400-diff-highlight.sh  | 81 +
 2 files changed, 133 insertions(+), 37 deletions(-)

-Peff

Great News!!!

[Amnesty International]

We have a great about your E-mail address!!! 

You Won  $950,500.00 USD on Amnesty International 
UK online E-mail Promotion. For more details about 
your prize claims, file with the following;

Names: 
Country: 
Tel:

Regards,
Mr. David Ford

Re: [RFC PATCH 0/3] rebase-interactive

[Wink Saville]

On Tue, Mar 20, 2018 at 2:47 PM, Eric Sunshine  wrote:
> On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville  wrote:
>> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine  
>> wrote:
>>> A problem with this approach is that it loses "blame" information. A
>>> git-blame of git-rebase--interactive--lib.sh shows all code in that
>>> file as having arisen spontaneously from thin air; it is unable to
>>> trace its real history. It would be much better to actually _move_
>>> code to the new file (and update callers if necessary), which would
>>> preserve provenance.
>>>
>>> Ideally, patches which move code around should do so verbatim (or at
>>> least as close to verbatim as possible) to ease review burden.
>>> Sometimes changes to code are needed to make it relocatable before
>>> movement, in which case those changes should be made as separate
>>> preparatory patches, again to ease review.
>>>
>>> As it is, without detailed spelunking, it is not immediately clear to
>>> a reviewer which functions in git-rebase--interactive--lib.sh are
>>> newly written, and which are merely moved (or moved and edited) from
>>> git-rebase--interactive.sh. This shortcoming suggests that the patch
>>> series could be re-worked to do the refactoring in a more piecemeal
>>> fashion which more clearly holds the hands of those trying to
>>> understand the changes. (For instance, one or more preparatory patches
>>> may be needed to make the code relocatable, followed by verbatim code
>>> relocation, possibly iterating these steps if some changes depend upon
>>> earlier changes, etc.)
>>
>> Must all intermediate commits continue build and pass tests?
>
> Yes, not just because it is good hygiene, but also because it
> preserves git-bisect'ability.

That's what I figured.

Anyway, I've played around and my current thoughts are to not create
any new files and
keep git_rebase__interactive and the new
git_rebase__interactive__preserve_merges
functions in git-rebase--interactive.sh.

Doing that will preserve the blame for the existing functions. But if
I do indentation
reformating as I extract functions that will be shared between
git_rebase__interactive
and git_rebase__interactive__preserve_merges then we still lose the blame
information unless the "-w" parameter is passed to blame. I could choose to
not do the indentation, but that doesn't seem like a good idea.

An alternative is that we don't accept the refactoring. What I'd
probably do is use
the refactored code to figure out a fix for the bug and then back port
the fix to the
existing code.

My opinion is that to not accept "improved" code because we lose blame
information
is not a good trade off. Of course what I might think is "improved"
others may rightfully
say the refactoring is gratuitous. If that is the case than not doing
the refactoring is the
right solution. I don't see a right or wong here, just a typical
engineering trade off.

Thoughts or other ideas?

Re: .gitattributes override behavior (possible bug, or documentation bug)

[Dakota Hawkins]

Thinking about this a little more, I'm now attracted to the idea that
its .gitignore that's weird.

As I understand it, .gitignore stops recursion when there's a
directory match (`somedir/`) but also explicitly allows nested
.gitnore file _as well as_ exclusion (`!*.txt`).

So, in the following (contrived) example, the user doesn't get what they want:

repo/
|- .git/
|- .gitignore   # /ignore-most/
|- ignore-most/
|  |- .gitignore# !*.txt
|  |- please_ignore.png
|  |- dont_ignore_me.txt

`repo/ignore-most/dont_ignore_me.txt` is still ignored, despite what
seems like the obvious intention of the user.

Maybe a unified "best-practices" would first-and-foremost recommend
against matching directories at all (makes sense, git doesn't manage
directories). In the above example, changing `/ignore-most/` to
`/ignore-most/*` has the "desired" effect.

What do you think?

On Tue, Mar 20, 2018 at 12:28 PM, Duy Nguyen  wrote:
> On Tue, Mar 20, 2018 at 5:40 AM, Jeff King  wrote:
>> On Tue, Mar 20, 2018 at 12:25:27AM -0400, Dakota Hawkins wrote:
>>
>>> > Right. The technical reason is mostly "that is not how it was designed,
>>> > and it would possibly break some corner cases if we switched it now".
>>>
>>> I'm just spitballing here, but do you guys think there's any subset of
>>> the combined .gitignore and .gitattributes matching functionality that
>>> could at least serve as a good "best-practices, going forward"
>>> (because of consistency) for both? I will say every time I do this for
>>> a new repo and have to do something even slightly complicated or
>>> different from what I've done before with .gitattributes/.gitignore
>>> that it takes me a long-ish time to figure it out. It's like I'm
>>> vaguely aware of pitfalls I've encountered in the past in certain
>>> areas but don't remember exactly what they are, so I consult the docs,
>>> which are (in sum) confusing and lead to more time spent
>>> trying/failing/trying/works/fails-later/etc.
>>>
>>> One "this subset of rules will work for both this way" would be
>
> You know, you (Dakota) could implement the new "exclude" attribute in
> .gitattributes and ignore .gitignore files completely. That makes it
> works "for both" ;-) The effort is probably not small though.
>
>>> awesome even if the matching capabilities are technically divergent,
>>> but on the other hand that might paint both into a corner in terms of
>>> functionality.
>>
>> As far as I know, they should be the same with the exception of this
>> recursion, and the negative-pattern thing. But I'm cc-ing Duy, who is
>> the resident expert on ignore and attributes matching (whether he wants
>> to be or not ;) ).
>
> Ha ha ha.
>
>> I wouldn't be surprised if there's something I don't know about.
>
> The only thing from the top of my head is what made me fail to unify
> the implementation of the two. It's basically different order of
> evaluation [1] when your patterns are spread out in multiple files. I
> think it makes gitattr and gitignore behavior different too (but I
> didn't try to verify).
>
> Apart from that, the two should behave the same way besides the
> exceptions you pointed out.
>
> [1] 
> https://public-inbox.org/git/%3CCACsJy8B8kYU7bkD8SiK354z4u=sy3hhbe4jvwnt_1pxod1c...@mail.gmail.com%3E/
>
>> So I think the "recommended subset" is basically "everything but these
>> few constructs". We just need to document them. ;)
>>
>> I probably should cc'd Duy on the documentation patch, too:
>>
>>   https://public-inbox.org/git/20180320041454.ga15...@sigill.intra.peff.net/
>>
>> -Peff
> --
> Duy

Re: [PATCH 0/2] -Wuninitialized

[Ramsay Jones]

On 20/03/18 14:46, Johannes Schindelin wrote:
> Hi Ramsay,
> 
> On Mon, 19 Mar 2018, Ramsay Jones wrote:
> 
>> This series removes all 'self-initialised' variables (ie.  var =
>> var;).  This construct has been used to silence gcc
>> '-W[maybe-]uninitialized' warnings in the past [1]. Unfortunately, this
>> construct causes warnings to be issued by MSVC [2], along with clang
>> static analysis complaining about an 'Assigned value is garbage or
>> undefined'. The number of these constructs has dropped over the years
>> (eg. see [3] and [4]), so there are currently only 6 remaining in the
>> current codebase. As demonstrated below, 5 of these no longer cause gcc
>> to issue warnings.
> 
> Thank you so much for working on this!

These patches are based on a very old branch (that goes back
at least as far as 2010, see [1]). (I have too many in my repo,
so it will be good to remove this one)!

> In Git for Windows, to work around the MSVC issues you mention, I have
> this ugly work-around (essentially, it has a FAKE_INIT() macro that either
> performs that GCC workaround or initializes the variable to NULL/0):
> 
>   https://github.com/git-for-windows/git/commit/474155f32a

Oh, wow! (Hmm, actually that doesn't look too bad :-D )

> FWIW I just tested your patches with Visual Studio 2017 and there are no
> C4700 warnings (the equivalent of GCC's "may be uninitialized" warning)
> [*1*].
> 
> You can find the patches (your patches rebased onto Git for Windows'
> master, plus a patch adding the project files for Visual Studio) here:
> 
> https://github.com/git-for-windows/git/compare/master...dscho:msvc-uninitialized-warning-test

Thanks for testing the patches.

> So here is my ACK, in case you want it ;-)

Thanks!

ATB,
Ramsay Jones

[1] https://public-inbox.org/git/4cfa8d4d.2020...@ramsay1.demon.co.uk/

Re: [PATCH 2/2] read-cache: fix an -Wmaybe-uninitialized warning

[Ramsay Jones]

On 20/03/18 04:36, Jeff King wrote:
> On Mon, Mar 19, 2018 at 05:56:11PM +, Ramsay Jones wrote:
> 
[snip]
>> diff --git a/read-cache.c b/read-cache.c
>> index 2eb81a66b..49607ddcd 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -2104,13 +2104,15 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, 
>> struct cache_entry *ce,
>>struct strbuf *previous_name, struct 
>> ondisk_cache_entry *ondisk)
>>  {
>>  int size;
>> -int saved_namelen = saved_namelen; /* compiler workaround */
>>  int result;
>> +unsigned int saved_namelen;
>> +int stripped_name = 0;
> 
> Maybe too clever, but I think you could just do:
> 
>   unsigned int saved_namelen = 0;
>   ...
>   saved_namelen = ce_namelen(ce);
>   ...
>   if (saved_namelen)
>   ce->ce_namelen = saved_namelen;
>   ce->ce_flags &= ~CE_STRIP_NAME;
> 
> the zero-length name case (if that's even legal) would work out the
> same.

Yeah, that was one option that I looked at. The first option
was to initialise saved_namelen to -1 (it was still an int) then
the test became if (saved_namelen >= 0). However, that started
me thinking about the zero-length case - should I assert if
((ce->ce_flags & CE_STRIP_NAME) && (ce_namelen(ce) == 0))? etc.

In the end, I decided that I wanted it to be 'drop dead' obvious
what was going on! Hopefully, the result was just that. :-D

ATB,
Ramsay Jones

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Eric Sunshine]

On Tue, Mar 20, 2018 at 6:30 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> int ret = 0;
>> void *buf = get_obj(oid, obj, , );
>> if (!buf)
>> ret = strbuf_error(_("missing object %s for %s"),
>> oid_to_hex(oid), ref->refname);
>> else if (!*obj)
>> ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
>> oid_to_hex(oid), ref->refname);
>> else
>> grab_values(ref->value, deref, *obj, buf, size);
>>if (!eaten)
>> free(buf);
>> return ret;
>
> I have no idea what strbuf_error() that does not take any strbuf is
> doing,...

strbuf_error() was a possibility proposed in [1], and it does take a
strbuf. Failure to pass in a strbuf here is just a typo.

> ... but I think you can initialize ret to -1 (i.e. assume the
> worst at the beginning), and then make the "ok, we didn't get any
> errors" case do
>
> else {
> grab_values(...);
> ret = 0;
> }

Yes, that also works.

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

(Relief Coordinator, United Nations)

[UNCU]

United Nations Compensation Unit, Emergency Relief Coordinator, United Nations.




Congratulations Beneficiary,




We have been having a meeting for quit sometime now and we just came to a 
logical conclusion 72 hours ago in affiliation with the World Bank president. 
Your email was listed among those that are yet to receive their compensation 
payment. The United Nations in Affiliation with World Bank have agreed to 
compensate them with the sum of USD1,500, 000.00 (One Million Five Hundred 
Thousand United States Dollars) only.


For this reason, you are to receive your payment through a certified ATM MASTER 
CARD. Note, with this Master Card you can withdraw money from any part of the 
World without being disturbed or delay and please for no reason should you 
disclose your account information as your account information is not and can 
never be needed before you receive your card payment. All that is required of 
your now is to contact our 100% trust officials by the Name of Mrs. Sarah 
Nonye. Below is her contact information:


Name: Mrs. Sarah Nonye
Email: sarah_no...@hotmail.com


Please ensure that you follow the directives and instructions of Mrs. Sarah 
Nonye so that within 72 hours you would have received your card payment and 
your secret pin code issued directly to you for security reasons. We apologize 
on behalf of the United Nation Organization for any delay you might have 
encountered in receiving your fund in the past. Congratulations, and I look 
forward to hear from you as soon as you confirm your payment making the world a 
better place.


Yours Faithfully,


Dave Skellorn
Emergency Relief Coordinator, United Nations.

Re: [PATCH 0/2] -Wuninitialized

[Ramsay Jones]

On 20/03/18 04:32, Jeff King wrote:
> On Mon, Mar 19, 2018 at 05:53:09PM +, Ramsay Jones wrote:
 
>> If we now add a patch to remove all self-initialization, which would be the
>> first patch plus the obvious change to 'saved_namelen' in read-cache.c, then
>> note the warnings issued by various compilers at various optimization levels
>> on several different platforms [5]:
>>
>> O0  O1  O2  O3  Os   Og
>>  1) gcc 4.8.3   |   -  1,20 11,18-19  1-4,21-23  1,5-17
>>  2) gcc 4.8.4   |   -  1,20 1   1 1-4,21-23  
>> 1,5-8,10-13,15-16 
>>  3) clang 3.4   |   -   -   -   --   n/a 
>>  4) gcc 5.4.0   |   -   1   1   1 1,3-4,21   
>> 1,5-8,10-13,16-16
>>  5) clang 3.8.0 |   -   -   -   --   n/a 
>>  6) gcc 5.4.0   |   -   1   1   1   1-4 1,5-17 
>>  7) clang 3.8.0 |   -   -   -   --   n/a 
>>  8) gcc 6.4.0   |   -   1   11,18-191,4 1,5-17
>>  9) clang 5.0.1 |   -   -   -   ---
>> 10) gcc 7.2.1   |   -   1   1   1   1,4 1,5-17
> 
> So I guess this could create headaches for people using DEVELOPER=1 on
> as ancient a compiler as 4.8.4, but most other people should be OK. I
> think I can live with that as a cutoff, and the Travis builds should
> work there.

Yeah, I have an even older laptop (Windows 95 era), but I'm not
sure if it will even boot these days. (It does have gcc on it
IIRC, but who knows which version). ;-)

> (And if we do the detect-compiler stuff from the other nearby thread,
> somebody who cares can even loosen the warnings for those old gcc
> versions).
> 
> I'm neglecting anybody doing -O3 or -Os here, but IMHO those are
> sufficiently rare that the builder can tweak their own settings.

I have occasionally used -O3, never used -Os (except for this
exercise) and have been meaning to try -Og (in anger) for a while. ;-)

> I wonder if people use -Og, though? I don't (I usually do -O0 for my
> edit-compile-debug cycle).

In the gcc documentation, the -Og description says:

  "... It should be the optimization level of choice for the standard
   edit-compile-debug cycle, ..."

Like you, I use -O0 (very old habits). As I said above, I have been
meaning to try -Og, but, well round tuits ... ;-)

[BTW, gcc also supports -Ofast, but I don't think we want to go
there ...]

ATB,
Ramsay Jones

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Junio C Hamano]

Eric Sunshine  writes:

> Overall, with the need for resource cleanup, this function becomes
> unusually noisy after this change. It could be tamed by doing
> something like this:
>
> int ret = 0;
> void *buf = get_obj(oid, obj, , );
> if (!buf)
> ret = strbuf_error(_("missing object %s for %s"),
> oid_to_hex(oid), ref->refname);
> else if (!*obj)
> ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
> oid_to_hex(oid), ref->refname);
> else
> grab_values(ref->value, deref, *obj, buf, size);
>if (!eaten)
> free(buf);
> return ret;

I have no idea what strbuf_error() that does not take any strbuf is
doing, but I think you can initialize ret to -1 (i.e. assume the
worst at the beginning), and then make the "ok, we didn't get any
errors" case do

else {
grab_values(...);
ret = 0;
}

Re: [PATCH] sha1_name: use bsearch_hash() for abbreviations

[Jonathan Tan]

On Tue, 20 Mar 2018 16:03:25 -0400
Derrick Stolee  wrote:

> This patch updates the abbreviation code to use bsearch_hash() as defined
> in [1]. It gets a nice speedup since the old implementation did not use
> the fanout table at all.

You can refer to the patch as:

  b4e00f7306a1 ("packfile: refactor hash search with fanout table",
  2018-02-15)

Also, might be worth noting that this patch builds on
jt/binsearch-with-fanout.

> One caveat about the patch: there is a place where I cast a sha1 hash
> into a struct object_id pointer. This is because the abbreviation code
> still uses 'const unsigned char *' instead of structs. I wanted to avoid
> a hashcpy() in these calls, but perhaps that is not too heavy a cost.

I recall a discussion that there were alignment issues with doing this,
but I might have be remembering wrongly - in my limited knowledge of C
alignment, both "unsigned char *" and "struct object_id *" have the same
constraints, but I'm not sure.

> + const unsigned char *index_fanout = p->index_data;
[snip]
> + return bsearch_hash(oid->hash, (const uint32_t*)index_fanout,
> + index_lookup, index_lookup_width, result);

This cast to "const uint32_t *" is safe, because p->index_data points to
a mmap-ed region (which has very good alignment, as far as I know). I
wonder if we should document alignment guarantees on p->index_data, and
if yes, what guarantees to declare.

What's cooking in git.git (Mar 2018, #04; Tue, 20)

[Junio C Hamano]

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

Hopefully 2.17-rc1 can be tagged tomorrow, with a handful of topics
marked for 'master' in this issue of "What's cooking" report.

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

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

--
[Graduated to "master"]

* ab/man-sec-list (2018-03-08) 1 commit
  (merged to 'next' on 2018-03-09 at 9626b691e2)
 + git manpage: note git-secur...@googlegroups.com

 Doc update.


* ab/perl-fixes (2018-03-05) 13 commits
  (merged to 'next' on 2018-03-09 at 262d84c1ba)
 + perl Git::LoadCPAN: emit better errors under NO_PERL_CPAN_FALLBACKS
 + Makefile: add NO_PERL_CPAN_FALLBACKS knob
 + perl: move the perl/Git/FromCPAN tree to perl/FromCPAN
 + perl: generalize the Git::LoadCPAN facility
 + perl: move CPAN loader wrappers to another namespace
 + perl: update our copy of Mail::Address
 + perl: update our ancient copy of Error.pm
 + git-send-email: unconditionally use Net::{SMTP,Domain}
 + Git.pm: hard-depend on the File::{Temp,Spec} modules
 + gitweb: hard-depend on the Digest::MD5 5.8 module
 + Git.pm: add the "use warnings" pragma
 + Git.pm: remove redundant "use strict" from sub-package
 + perl: *.pm files should not have the executable bit

 Clean-up to various pieces of Perl code we have.


* cl/send-email-reply-to (2018-03-06) 2 commits
  (merged to 'next' on 2018-03-09 at 3d3c3ab441)
 + send-email: support separate Reply-To address
 + send-email: rename variable for clarity
 (this branch uses np/send-email-header-parsing.)

 "git send-email" learned "--reply-to=" option.


* np/send-email-header-parsing (2017-12-15) 1 commit
  (merged to 'next' on 2018-03-09 at 91ef7216f7)
 + send-email: extract email-parsing code into a subroutine
 (this branch is used by cl/send-email-reply-to.)

 Code refactoring.


* sg/cvs-tests-with-x (2018-03-08) 2 commits
  (merged to 'next' on 2018-03-09 at 6ec749c7b7)
 + t9402-git-cvsserver-refs: don't check the stderr of a subshell
 + t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'

 Allow running a couple of tests with "sh -x".


* tl/userdiff-csharp-async (2018-03-08) 1 commit
  (merged to 'next' on 2018-03-09 at 6dcf76e118)
 + userdiff.c: add C# async keyword in diff pattern

 Update funcname pattern used for C# to recognize "async" keyword.

--
[New Topics]

* bp/refresh-cache-ent-rehash-fix (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at bac8745f08)
 + Fix bugs preventing adding updated cache entries to the name hash

 The codepath to replace an existing entry in the index had a bug in
 updating the name hash structure, which has been fixed.

 Will merge to 'master'.


* jt/transfer-fsck-with-promissor (2018-03-15) 2 commits
  (merged to 'next' on 2018-03-15 at 6d1ccc965b)
 + fetch-pack: do not check links for partial fetch
 + index-pack: support checking objects but not links

 The transfer.fsckobjects configuration tells "git fetch" to
 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.

 Will merge to 'master'.


* ml/filter-branch-no-op-error (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at ba8ac48dec)
 + filter-branch: return 2 when nothing to rewrite

 "git filter-branch" learned to use a different exit code to allow
 the callers to tell the case where there was no new commits to
 rewrite from other error cases.

 Will cook in 'next'.


* ab/install-symlinks (2018-03-15) 3 commits
  (merged to 'next' on 2018-03-15 at 99d6bd6cb3)
 + Makefile: optionally symlink libexec/git-core binaries to bin/git
 + Makefile: add a gitexecdir_relative variable
 + Makefile: fix broken bindir_relative variable

 The build procedure learned to optionally use symbolic links
 (instead of hardlinks and copies) to install "git-foo" for built-in
 commands, whose binaries are all identical.

 Will cook in 'next'.


* ks/t3200-typofix (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at 8b8d397787)
 + t/t3200: fix a typo in a test description

 Test typofix.

 Will merge to 'master'.


* rj/http-code-cleanup (2018-03-15) 1 commit
  (merged to 'next' on 2018-03-15 at 0dfd462ff8)
 + http: fix an unused variable warning for 'curl_no_proxy'

 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
 has been fixed.

 Will merge to 'master'.
 This will become unnecessary, when we follow-through the
 jk/drop-ancient-curl 

Re: [GSoC] Convert “git stash” to builtin proposal

[Christian Couder]

Hi,

On Tue, Mar 20, 2018 at 9:09 PM, Paul-Sebastian Ungureanu
 wrote:
>
> * Convert function: this step is basically makes up the goal of this
> project.

Could you explain a bit more how you would convert a function? Or
could you explain for example how you would convert "git stash list"
below?

> * Ensure that no regression occurred: considering that there are plenty
> of tests and that I have a good understanding of the function, this
> should be a trivial task.
>
> * Finally write more tests to ensure best code coverage.

Maybe, as Dscho suggested to another potential GSoC student, it would
be better to write more tests before converting the function.

> # Useful resources
> There has been a lot of progress made in this direction already and I
> believe it will serve of great help. However, from my understanding it
> is not yet mergeable and it requires changes.
>
> https://public-inbox.org/git/20170608005535.13080-1-j...@teichroeb.net/
> T/#m8849c7ce0ad8516cc206dd6910b79591bf9b3acd

Maybe you should Cc the author of this patch series.

Also please notice that the patch series started with adding some tests.

> I am expecting to submit a patch for every command that is converted.
> This way, I have well set milestones and results will be seen as soon
> as possible. Moreover, seeing my contributions getting merged will be a
> huge confidence boost to myself.

> Each “convert” phase of the project below is not only about converting
> from Shell to C, but also ensuring that everything is documented, there
> are enough tests and there are no regressions.
>
> 14th May - 20th May - Convert "git stash list"

For example could you explain a bit more which functions/commands you
would create in which file and how you would call them to convert `git
stash list`

Re: [PATCH v2 2/2] git-svn: allow empty email-address in authors-prog and authors-file

[Eric Wong]

Andreas Heiduk  wrote:
> Am 19.03.2018 um 00:04 schrieb Eric Wong:
> > Andreas Heiduk  wrote:
> >> The email address in --authors-file and --authors-prog can be empty but
> >> git-svn translated it into a syntethic email address in the form
> >> $USERNAME@$REPO_UUID. Now git-svn behaves like git-commit: If the email
> >> is explicitly set to the empty string, the commit does not contain
> >> an email address.
> > 
> > What is missing is WHY "<>" is preferable to "<$USERNAME@$REPO_UUID>".
> >
> > $USERNAME is good anyways since projects/organizations tie their
> > SVN usernames to email usernames via LDAP, making it easy to
> > infer their email address from $USERNAME.  The latter can also
> > be used to disambiguate authors if they happen to have the same
> > real name.
> 
> That's still available and it's even still the default.

OK.

> But: If the user of git-svn takes the burden of writing an authors
> script or maintaining an authors file then he should have full control
> over the result as long as git can handle the output reasonably.
> Currently that's the case for git but not for git-svn.

Fair enough.

> jondoe <>
> 
> just means: "There is intentionally no email address." For an
> internal, ephemeral repository that can be OK. It has the advantage,
> that no automatic system (Jira, Jenkins, ...) will try to send emails to 
> 
> jondoe 

OK, that's a good reason to allow "<>" and should be in the
commit message.

> Further steps: Eric Sunshine mentioned [1] that you might have concerns about
> the change of behavior per se. For me the patch is not so much a new feature 
> but
> a bugfix bringing git-svn in sync with git itself. Adding an option parameter 
> to enable the new behavior seems strange to me. But there might be other ways
> to achieve the same effect:

New options are not desirable, either, as they increase
testing/maintenance overhead.  So I'm inclined to take your
patch with only an updated commit message...

No rush, though; will wait another bit for others to comment and
I expect to be preoccupied this week with other projects and
weather problems on the forecast :<

Re: [RFC PATCH 0/3] rebase-interactive

[Eric Sunshine]

On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville  wrote:
> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine  
> wrote:
>> A problem with this approach is that it loses "blame" information. A
>> git-blame of git-rebase--interactive--lib.sh shows all code in that
>> file as having arisen spontaneously from thin air; it is unable to
>> trace its real history. It would be much better to actually _move_
>> code to the new file (and update callers if necessary), which would
>> preserve provenance.
>>
>> Ideally, patches which move code around should do so verbatim (or at
>> least as close to verbatim as possible) to ease review burden.
>> Sometimes changes to code are needed to make it relocatable before
>> movement, in which case those changes should be made as separate
>> preparatory patches, again to ease review.
>>
>> As it is, without detailed spelunking, it is not immediately clear to
>> a reviewer which functions in git-rebase--interactive--lib.sh are
>> newly written, and which are merely moved (or moved and edited) from
>> git-rebase--interactive.sh. This shortcoming suggests that the patch
>> series could be re-worked to do the refactoring in a more piecemeal
>> fashion which more clearly holds the hands of those trying to
>> understand the changes. (For instance, one or more preparatory patches
>> may be needed to make the code relocatable, followed by verbatim code
>> relocation, possibly iterating these steps if some changes depend upon
>> earlier changes, etc.)
>
> Must all intermediate commits continue build and pass tests?

Yes, not just because it is good hygiene, but also because it
preserves git-bisect'ability.

Re: [RFC PATCH 0/3] rebase-interactive

[Wink Saville]

On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine  wrote:
> On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville  wrote:
>> Patch 0001 creates a library of functions which can be
>> used by git-rebase--interactive and
>> git-rebase--interactive--preserve-merges. The functions are
>> those that exist in git-rebase--interactive.sh plus new
>> functions created from the body of git_rebase_interactive
>> that will be used git_rebase_interactive in the third patch
>> and git_rebase_interactive_preserve_merges in the second
>> patch. None of the functions are invoked so there is no
>> logic changes and the system builds and passes all tests
>> on travis-ci.org.
>>
>> Patch 0002 creates git-rebase--interactive--preserve-merges.sh
>> with the function git_rebase_interactive_preserve_merges. The contents
>> of the function are refactored from git_rebase_interactive and
>> uses existing and new functions in the library. A small modification
>> of git-rebase is also done to invoke the new function when the -p
>> switch is used with git-rebase. When this is applied on top of
>> 0001 the system builds and passes all tests on travis-ci.org.
>>
>> The final patch, 0003, removes all unused code from
>> git_rebase_interactive and uses the functions from the library
>> where appropriate. And, of course, when applied on top of 0002
>> the system builds and passes all tests on travis-ci.org.
>
> A problem with this approach is that it loses "blame" information. A
> git-blame of git-rebase--interactive--lib.sh shows all code in that
> file as having arisen spontaneously from thin air; it is unable to
> trace its real history. It would be much better to actually _move_
> code to the new file (and update callers if necessary), which would
> preserve provenance.
>
> Ideally, patches which move code around should do so verbatim (or at
> least as close to verbatim as possible) to ease review burden.
> Sometimes changes to code are needed to make it relocatable before
> movement, in which case those changes should be made as separate
> preparatory patches, again to ease review.
>
> As it is, without detailed spelunking, it is not immediately clear to
> a reviewer which functions in git-rebase--interactive--lib.sh are
> newly written, and which are merely moved (or moved and edited) from
> git-rebase--interactive.sh. This shortcoming suggests that the patch
> series could be re-worked to do the refactoring in a more piecemeal
> fashion which more clearly holds the hands of those trying to
> understand the changes. (For instance, one or more preparatory patches
> may be needed to make the code relocatable, followed by verbatim code
> relocation, possibly iterating these steps if some changes depend upon
> earlier changes, etc.)
>
> Thanks.

Must all intermediate commits continue build and pass tests?

Re: [RFC PATCH 0/3] rebase-interactive

[Eric Sunshine]

On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville  wrote:
> Patch 0001 creates a library of functions which can be
> used by git-rebase--interactive and
> git-rebase--interactive--preserve-merges. The functions are
> those that exist in git-rebase--interactive.sh plus new
> functions created from the body of git_rebase_interactive
> that will be used git_rebase_interactive in the third patch
> and git_rebase_interactive_preserve_merges in the second
> patch. None of the functions are invoked so there is no
> logic changes and the system builds and passes all tests
> on travis-ci.org.
>
> Patch 0002 creates git-rebase--interactive--preserve-merges.sh
> with the function git_rebase_interactive_preserve_merges. The contents
> of the function are refactored from git_rebase_interactive and
> uses existing and new functions in the library. A small modification
> of git-rebase is also done to invoke the new function when the -p
> switch is used with git-rebase. When this is applied on top of
> 0001 the system builds and passes all tests on travis-ci.org.
>
> The final patch, 0003, removes all unused code from
> git_rebase_interactive and uses the functions from the library
> where appropriate. And, of course, when applied on top of 0002
> the system builds and passes all tests on travis-ci.org.

A problem with this approach is that it loses "blame" information. A
git-blame of git-rebase--interactive--lib.sh shows all code in that
file as having arisen spontaneously from thin air; it is unable to
trace its real history. It would be much better to actually _move_
code to the new file (and update callers if necessary), which would
preserve provenance.

Ideally, patches which move code around should do so verbatim (or at
least as close to verbatim as possible) to ease review burden.
Sometimes changes to code are needed to make it relocatable before
movement, in which case those changes should be made as separate
preparatory patches, again to ease review.

As it is, without detailed spelunking, it is not immediately clear to
a reviewer which functions in git-rebase--interactive--lib.sh are
newly written, and which are merely moved (or moved and edited) from
git-rebase--interactive.sh. This shortcoming suggests that the patch
series could be re-worked to do the refactoring in a more piecemeal
fashion which more clearly holds the hands of those trying to
understand the changes. (For instance, one or more preparatory patches
may be needed to make the code relocatable, followed by verbatim code
relocation, possibly iterating these steps if some changes depend upon
earlier changes, etc.)

Thanks.

[RFC PATCH 3/3] rebase-interactive: refactor git-rebase--interactive to use library

[Wink Saville]

Refactor git-rebase--interactive to use git-rebase--interactive--lib
this simplifies and reduces the size of the code by about 1000 LoC.

Signed-off-by: Wink Saville 
---
 git-rebase--interactive.sh | 1019 +---
 1 file changed, 19 insertions(+), 1000 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfea..3d2b89e1c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1,3 +1,4 @@
+#!/bin/sh
 # This shell script fragment is sourced by git-rebase to implement
 # its interactive mode.  "git rebase --interactive" makes it easy
 # to fix up commits in the middle of a series and rearrange commits.
@@ -6,742 +7,8 @@
 #
 # The original idea comes from Eric W. Biederman, in
 # https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
-#
-# The file containing rebase commands, comments, and empty lines.
-# This file is created by "git rebase -i" then edited by the user.  As
-# the lines are processed, they are removed from the front of this
-# file and written to the tail of $done.
-todo="$state_dir"/git-rebase-todo
-
-# The rebase command lines that have already been processed.  A line
-# is moved here when it is first handled, before any associated user
-# actions.
-done="$state_dir"/done
-
-# The commit message that is planned to be used for any changes that
-# need to be committed following a user interaction.
-msg="$state_dir"/message
-
-# The file into which is accumulated the suggested commit message for
-# squash/fixup commands.  When the first of a series of squash/fixups
-# is seen, the file is created and the commit message from the
-# previous commit and from the first squash/fixup commit are written
-# to it.  The commit message for each subsequent squash/fixup commit
-# is appended to the file as it is processed.
-#
-# The first line of the file is of the form
-# # This is a combination of $count commits.
-# where $count is the number of commits whose messages have been
-# written to the file so far (including the initial "pick" commit).
-# Each time that a commit message is processed, this line is read and
-# updated.  It is deleted just before the combined commit is made.
-squash_msg="$state_dir"/message-squash
-
-# If the current series of squash/fixups has not yet included a squash
-# command, then this file exists and holds the commit message of the
-# original "pick" commit.  (If the series ends without a "squash"
-# command, then this can be used as the commit message of the combined
-# commit without opening the editor.)
-fixup_msg="$state_dir"/message-fixup
-
-# $rewritten is the name of a directory containing files for each
-# commit that is reachable by at least one merge base of $head and
-# $upstream. They are not necessarily rewritten, but their children
-# might be.  This ensures that commits on merged, but otherwise
-# unrelated side branches are left alone. (Think "X" in the man page's
-# example.)
-rewritten="$state_dir"/rewritten
-
-dropped="$state_dir"/dropped
-
-end="$state_dir"/end
-msgnum="$state_dir"/msgnum
-
-# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
-# GIT_AUTHOR_DATE that will be used for the commit that is currently
-# being rebased.
-author_script="$state_dir"/author-script
-
-# When an "edit" rebase command is being processed, the SHA1 of the
-# commit to be edited is recorded in this file.  When "git rebase
-# --continue" is executed, if there are any staged changes then they
-# will be amended to the HEAD commit, but only provided the HEAD
-# commit is still the commit to be edited.  When any other rebase
-# command is processed, this file is deleted.
-amend="$state_dir"/amend
-
-# For the post-rewrite hook, we make a list of rewritten commits and
-# their new sha1s.  The rewritten-pending list keeps the sha1s of
-# commits that have been processed, but not committed yet,
-# e.g. because they are waiting for a 'squash' command.
-rewritten_list="$state_dir"/rewritten-list
-rewritten_pending="$state_dir"/rewritten-pending
-
-# Work around Git for Windows' Bash whose "read" does not strip CRLF
-# and leaves CR at the end instead.
-cr=$(printf "\015")
-
-strategy_args=${strategy:+--strategy=$strategy}
-test -n "$strategy_opts" &&
-eval '
-   for strategy_opt in '"$strategy_opts"'
-   do
-   strategy_args="$strategy_args -X$(git rev-parse --sq-quote 
"${strategy_opt#--}")"
-   done
-'
-
-GIT_CHERRY_PICK_HELP="$resolvemsg"
-export GIT_CHERRY_PICK_HELP
-
-comment_char=$(git config --get core.commentchar 2>/dev/null)
-case "$comment_char" in
-'' | auto)
-   comment_char="#"
-   ;;
-?)
-   ;;
-*)
-   comment_char=$(echo "$comment_char" | cut -c1)
-   ;;
-esac
-
-warn () {
-   printf '%s\n' "$*" >&2
-}
-
-# Output the commit message for the specified commit.
-commit_message () {
-   git cat-file commit "$1" | sed "1,/^$/d"
-}
-
-orig_reflog_action="$GIT_REFLOG_ACTION"

[RFC PATCH 1/3] rebase-interactive: create git-rebase--interactive--lib.sh

[Wink Saville]

Create a library that can be used for interactive rebasing by
separate scripts. In particular, the current git-rebase--interactive.sh
will be reduced to a single function which uses the library and a new
file, git-rebase--interactive--preserve-merges.sh will also be a single
function that uses the library.

Signed-off-by: Wink Saville 
---
 .gitignore  |   1 +
 Makefile|   1 +
 git-rebase--interactive--lib.sh | 944 
 3 files changed, 946 insertions(+)
 create mode 100644 git-rebase--interactive--lib.sh

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..4ea246780 100644
--- a/.gitignore
+++ b/.gitignore
@@ -116,6 +116,7 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
+/git-rebase--interactive--lib
 /git-rebase--merge
 /git-receive-pack
 /git-reflog
diff --git a/Makefile b/Makefile
index de4b8f0c0..f13540da6 100644
--- a/Makefile
+++ b/Makefile
@@ -567,6 +567,7 @@ SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--interactive
+SCRIPT_LIB += git-rebase--interactive--lib
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
diff --git a/git-rebase--interactive--lib.sh b/git-rebase--interactive--lib.sh
new file mode 100644
index 0..0cb902f98
--- /dev/null
+++ b/git-rebase--interactive--lib.sh
@@ -0,0 +1,944 @@
+#!/bin/sh
+# This shell script fragment is sourced by git-rebase--interactive
+# and git-rebase--interactive--preserve-merges in suppor of the
+# interactive mode.  "git rebase --interactive" makes it easy
+# to fix up commits in the middle of a series and rearrange commits.
+#
+# Copyright (c) 2006 Johannes E. Schindelin
+#
+# The original idea comes from Eric W. Biederman, in
+# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
+#
+
+# The file containing rebase commands, comments, and empty lines.
+# This file is created by "git rebase -i" then edited by the user.  As
+# the lines are processed, they are removed from the front of this
+# file and written to the tail of $done.
+todo="$state_dir"/git-rebase-todo
+
+# The rebase command lines that have already been processed.  A line
+# is moved here when it is first handled, before any associated user
+# actions.
+done="$state_dir"/done
+
+# The commit message that is planned to be used for any changes that
+# need to be committed following a user interaction.
+msg="$state_dir"/message
+
+# The file into which is accumulated the suggested commit message for
+# squash/fixup commands.  When the first of a series of squash/fixups
+# is seen, the file is created and the commit message from the
+# previous commit and from the first squash/fixup commit are written
+# to it.  The commit message for each subsequent squash/fixup commit
+# is appended to the file as it is processed.
+#
+# The first line of the file is of the form
+# # This is a combination of $count commits.
+# where $count is the number of commits whose messages have been
+# written to the file so far (including the initial "pick" commit).
+# Each time that a commit message is processed, this line is read and
+# updated.  It is deleted just before the combined commit is made.
+squash_msg="$state_dir"/message-squash
+
+# If the current series of squash/fixups has not yet included a squash
+# command, then this file exists and holds the commit message of the
+# original "pick" commit.  (If the series ends without a "squash"
+# command, then this can be used as the commit message of the combined
+# commit without opening the editor.)
+fixup_msg="$state_dir"/message-fixup
+
+# $rewritten is the name of a directory containing files for each
+# commit that is reachable by at least one merge base of $head and
+# $upstream. They are not necessarily rewritten, but their children
+# might be.  This ensures that commits on merged, but otherwise
+# unrelated side branches are left alone. (Think "X" in the man page's
+# example.)
+rewritten="$state_dir"/rewritten
+
+dropped="$state_dir"/dropped
+
+end="$state_dir"/end
+msgnum="$state_dir"/msgnum
+
+# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+# GIT_AUTHOR_DATE that will be used for the commit that is currently
+# being rebased.
+author_script="$state_dir"/author-script
+
+# When an "edit" rebase command is being processed, the SHA1 of the
+# commit to be edited is recorded in this file.  When "git rebase
+# --continue" is executed, if there are any staged changes then they
+# will be amended to the HEAD commit, but only provided the HEAD
+# commit is still the commit to be edited.  When any other rebase
+# command is processed, this file is deleted.
+amend="$state_dir"/amend
+
+# For the post-rewrite hook, we make a list of rewritten commits and
+# their new sha1s.  The rewritten-pending list keeps the sha1s of
+# commits that have been processed, but not committed yet,
+# e.g. 

[RFC PATCH 2/3] rebase-interactive: create git-rebase--interactive--preserve-merges

[Wink Saville]

Extract the code that is executed when $preserve_merges is t from
git-rebase--interactive to git-rebase--interactive--preserve-merges.sh.
The extracted code uses functions from git-rebase--interactve--lib.

Signed-off-by: Wink Saville 
---
 .gitignore  |   1 +
 Makefile|   1 +
 git-rebase--interactive--preserve-merges.sh | 134 
 git-rebase.sh   |   7 +-
 4 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 git-rebase--interactive--preserve-merges.sh

diff --git a/.gitignore b/.gitignore
index 4ea246780..c57a6b563 100644
--- a/.gitignore
+++ b/.gitignore
@@ -116,6 +116,7 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
+/git-rebase--interactive--preserve-merges
 /git-rebase--interactive--lib
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index f13540da6..543e0a659 100644
--- a/Makefile
+++ b/Makefile
@@ -567,6 +567,7 @@ SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--interactive
+SCRIPT_LIB += git-rebase--interactive--preserve-merges
 SCRIPT_LIB += git-rebase--interactive--lib
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
diff --git a/git-rebase--interactive--preserve-merges.sh 
b/git-rebase--interactive--preserve-merges.sh
new file mode 100644
index 0..e00b5c990
--- /dev/null
+++ b/git-rebase--interactive--preserve-merges.sh
@@ -0,0 +1,134 @@
+#!/bin/sh
+# This shell script fragment is sourced by git-rebase to implement
+# its interactive mode with --preserve-merges flag.
+# "git rebase --interactive" makes it easy to fix up commits in the
+# middle of a series and rearrange commits and adding --preserve-merges
+# requests it to preserve merges while rebase.
+#
+# Copyright (c) 2006 Johannes E. Schindelin
+#
+# The original idea comes from Eric W. Biederman, in
+# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
+
+. git-rebase--interactive--lib
+
+# The whole contents of this file is run by dot-sourcing it from
+# inside a shell function.  It used to be that "return"s we see
+# below were not inside any function, and expected to return
+# to the function that dot-sourced us.
+#
+# However, older (9.x) versions of FreeBSD /bin/sh misbehave on such a
+# construct and continue to run the statements that follow such a "return".
+# As a work-around, we introduce an extra layer of a function
+# here, and immediately call it after defining it.
+git_rebase__interactive__preserve_merges () {
+   initiate_action "$action"
+   ret=$?
+   if test $ret == 0; then
+   return 0
+   fi
+
+   setup_reflog_action
+   init_basic_state
+
+   if test -z "$rebase_root"
+   then
+   mkdir "$rewritten" &&
+   for c in $(git merge-base --all $orig_head $upstream)
+   do
+   echo $onto > "$rewritten"/$c ||
+   die "$(gettext "Could not init rewritten commits")"
+   done
+   else
+   mkdir "$rewritten" &&
+   echo $onto > "$rewritten"/root ||
+   die "$(gettext "Could not init rewritten commits")"
+   fi
+
+   # No cherry-pick because our first pass is to determine
+   # parents to rewrite and skipping dropped commits would
+   # prematurely end our probe
+   merges_option=
+
+   shorthead=$(git rev-parse --short $orig_head)
+   shortonto=$(git rev-parse --short $onto)
+   if test -z "$rebase_root"
+   # this is now equivalent to ! -z "$upstream"
+   then
+   shortupstream=$(git rev-parse --short $upstream)
+   revisions=$upstream...$orig_head
+   shortrevisions=$shortupstream..$shorthead
+   else
+   revisions=$onto...$orig_head
+   shortrevisions=$shorthead
+   fi
+
+   # The 'rev-list .. | sed'
+   #   requires %m to parse; where as the the instruction
+   #   requires %H to parse
+   format=$(git config --get rebase.instructionFormat)
+   git rev-list $merges_option --format="%m%H ${format:-%s}" \
+   --reverse --left-right --topo-order \
+   $revisions ${restrict_revision+^$restrict_revision} | \
+   sed -n "s/^>//p" |
+   while read -r sha1 rest
+   do
+   if test -z "$keep_empty" \
+   && is_empty_commit $sha1 \
+   && ! is_merge_commit $sha1
+   then
+   comment_out="$comment_char "
+   else
+   comment_out=
+   fi
+
+   if test -z "$rebase_root"
+   then
+   preserve=t
+   for p in $(git rev-list --parents -1 $sha1 | \
+   cut -d' 

[RFC PATCH 0/3] rebase-interactive

[Wink Saville]

I've not worked on the git sources before and while looking into
fixing test_expect_failure 'exchange two commits with -p' in
t3404-rebase-interactive.sh, I found it difficult to understand
the git testing infracture and git-rebase--interactive.sh.

So as part of learning my way around I thought I'd refactor
git-rebase--interactive to make it easier for me to understand.
At this point I do have some understanding and will be working
on fixing the bug. In the mean time I'm requesting comments on
this refactoring patch sequence.

Patch 0001 creates a library of functions which can be
used by git-rebase--interactive and
git-rebase--interactive--preserve-merges. The functions are
those that exist in git-rebase--interactive.sh plus new
functions created from the body of git_rebase_interactive
that will be used git_rebase_interactive in the third patch
and git_rebase_interactive_preserve_merges in the second
patch. None of the functions are invoked so there is no
logic changes and the system builds and passes all tests
on travis-ci.org.

Patch 0002 creates git-rebase--interactive--preserve-merges.sh
with the function git_rebase_interactive_preserve_merges. The contents
of the function are refactored from git_rebase_interactive and
uses existing and new functions in the library. A small modification
of git-rebase is also done to invoke the new function when the -p
switch is used with git-rebase. When this is applied on top of
0001 the system builds and passes all tests on travis-ci.org.

The final patch, 0003, removes all unused code from
git_rebase_interactive and uses the functions from the library
where appropriate. And, of course, when applied on top of 0002
the system builds and passes all tests on travis-ci.org.

Wink Saville (3):
  rebase-interactive: create git-rebase--interactive--lib.sh
  rebase-interactive: create git-rebase--interactive--preserve-merges
  rebase-interactive: refactor git-rebase--interactive to use library

 .gitignore  |2 +
 Makefile|2 +
 git-rebase--interactive--lib.sh |  944 +
 git-rebase--interactive--preserve-merges.sh |  134 
 git-rebase--interactive.sh  | 1019 +--
 git-rebase.sh   |7 +-
 6 files changed, 1107 insertions(+), 1001 deletions(-)
 create mode 100644 git-rebase--interactive--lib.sh
 create mode 100644 git-rebase--interactive--preserve-merges.sh

-- 
2.16.2

[GSoC] Convert “git stash” to builtin proposal

[Paul-Sebastian Ungureanu]

Hello,

I have completed the first draft of my proposal for the Google Summer
of Code, which can be found at [1] or below for those who prefer the
text version.

Any feedback is greatly appreciated. Thanks!

[1]
https://docs.google.com/document/d/1TtdD7zgUsEOszHG5HX1at4zHMDsPmSBYWqy
dOPTTpV8/edit?usp=sharing

Best regards,
Paul-Sebastian Ungureanu

# Convert “git stash” to builtin

# Name and Contact Information
Hello! My name is Paul-Sebastian Ungureanu. I am currently a first year
Computer Science & Engineering student at Politehnica University of
Bucharest, Romania.

My email address is ungureanupaulsebast...@gmail.com and my phone
number is [CENSORED]. You can also find me on #git IRC channel as
ungps.

FLOSS manual recommends students to include in their proposal their
postal address and mention a relative as a emergency contact. My
permanent address is [CENSORED]. In case of an emergency, you may
contact my brother, [CENSORED] by email at [CENSORED] or by phone at
[CENSORED].

# Synopsis
Currently, many components of Git are still in the form of Shell or
Perl scripts. This choice makes sense especially when considering how
faster new features can be implemented in Shell and Perl scripts,
rather than writing them in C. However, in production, where Git runs
daily on millions of computers with distinct configurations (i.e.
different operating systems) some problems appear (i.e. POSIX-to-
Windows path conversion issues).

The idea of this project is to take “git-stash.sh” and reimplement it
in C. Apart from fixing compatibility issues and increasing the
performance by going one step closer to native code, I believe this is
an excellent excuse to fix long-standing bugs or implement new minor
features.

# Benefits to community
The essential benefit brought by rewriting Git commands is the
increased compatibility with a large number computers with distinct
configuration. I believe that this project can have a positive impact
on a large mass of developers around the world. By rewriting the code
behind some popular commands and making them “built-in”, developers
will have a better overall experience when using Git and get to focus
on what really matters to them.

As a side effect, there will be a number of other improvements:
increased performance, ability to rethink the design of some code that
suffered from patching along the time, have the chance to create new
features and fix old bugs.

In theory, switching from Bash or Shell scripts, Git will be one step
closer to native code which should have a positive impact on the
performance. Being able to start from a clean slate is a great
opportunity to rethink old designs that may have been patched a lot
during their lifetime. This way, with the help of my mentors, I can
think of new ways to try and remove some limitations of the current
system (if there are any).

Moreover, I believe that the community will benefit greatly from new
features and bug fixes that I could help with. Even though this is not
really one of the main goals of this project, I believe that it would
be easier to fix bugs or implement new features while rewriting the
code. However, I will have to discuss with my mentors and carefully
review issues as I would not want to divagate from the purpose of the
project.

As a last point, I believe it is good to have a more homogenous
codebase, where the majority of the code would be written in C. This
could increase the number of contributions to the project as there are
maybe more programmers who are familiar with C, and not so much with
Perl or shell scripting.

# Deliverables
Deliverable of this project is “git stash” completely rewritten in
portable C code. Along with the new code, there will be some additions
and changes brought to tests to cover any new behaviour.

# Related work
Looking over the list of the other proposed projects, I believe that
“Convert interactive rebase to C” and “git bisect improvements” are the
most alike with this project and may be stretch goal of this project.

Moreover, there is a chance that other scripts could benefit from this
project if it were to be taken as an example for future conversions.

# Biographical information
I am a freshman at Politehnica University of Bucharest, which is
considered to be one of the most prestigious universities in the
Eastern Europe. I consider myself an ambitious software engineer that
enjoys competition. This has been proven by my participation to
programming competitions and extracurricular projects. As much as I
like competitions, I also love working with and meeting people that
share my interests for programming and technology.

Even though, in the last two years I found myself to be more interested
in Android software development rather than competitive programming, I
still take part in most of the competitions, such as contests organized
by Google HashCode, Google KickStart and ACM.

I have a good grasp of programming languages such as C and C++ and I
consider both of them 

[PATCH] sha1_name: use bsearch_hash() for abbreviations

[Derrick Stolee]

This patch updates the abbreviation code to use bsearch_hash() as defined
in [1]. It gets a nice speedup since the old implementation did not use
the fanout table at all.

One caveat about the patch: there is a place where I cast a sha1 hash
into a struct object_id pointer. This is because the abbreviation code
still uses 'const unsigned char *' instead of structs. I wanted to avoid
a hashcpy() in these calls, but perhaps that is not too heavy a cost.

I look forward to feedback on this.

Thanks,
-Stolee

[1] 
https://public-inbox.org/git/007f3a4c84cb1c07255404ad1ea9f797129c5cf0.1517609773.git.jonathanta...@google.com/
[PATCH 2/2] packfile: refactor hash search with fanout table

-- >8 --

When computing abbreviation lengths for an object ID against a single
packfile, the method find_abbrev_len_for_pack() currently implements
binary search. This is one of several implementations. One issue with
this implementation is that it ignores the fanout table in the pack-
index.

Translate this binary search to use the existing bsearch_hash() method
that correctly uses a fanout table. To keep the details about pack-
index version 1 or 2 out of sha1_name.c, create a bsearch_pack() method
in packfile.c.

Due to the use of the fanout table, the abbreviation computation is
slightly faster than before. For a fully-repacked copy of the Linux
repo, the following 'git log' commands improved:

* git log --oneline --parents --raw
  Before: 66.83s
  After:  64.85s
  Rel %:  -2.9%

* git log --oneline --parents
  Before: 7.85s
  After:  7.29s
  Rel %: -7.1%

Signed-off-by: Derrick Stolee 
---
 packfile.c  | 23 +++
 packfile.h  |  8 
 sha1_name.c | 24 
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/packfile.c b/packfile.c
index 7c1a2519fc..ea0388f6dd 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1654,6 +1654,29 @@ void *unpack_entry(struct packed_git *p, off_t 
obj_offset,
return data;
 }
 
+int bsearch_pack(const struct object_id *oid, const struct packed_git *p, 
uint32_t *result)
+{
+   const unsigned char *index_fanout = p->index_data;
+   const unsigned char *index_lookup;
+   int index_lookup_width;
+
+   if (!index_fanout)
+   BUG("bsearch_pack called without a valid pack-index");
+
+   index_lookup = index_fanout + 4 * 256;
+   if (p->index_version == 1) {
+   index_lookup_width = 24;
+   index_lookup += 4;
+   } else {
+   index_lookup_width = 20;
+   index_fanout += 8;
+   index_lookup += 8;
+   }
+
+   return bsearch_hash(oid->hash, (const uint32_t*)index_fanout,
+   index_lookup, index_lookup_width, result);
+}
+
 const unsigned char *nth_packed_object_sha1(struct packed_git *p,
uint32_t n)
 {
diff --git a/packfile.h b/packfile.h
index a7fca598d6..ec08cb2bb0 100644
--- a/packfile.h
+++ b/packfile.h
@@ -78,6 +78,14 @@ extern struct packed_git *add_packed_git(const char *path, 
size_t path_len, int
  */
 extern void check_pack_index_ptr(const struct packed_git *p, const void *ptr);
 
+/*
+ * Perform binary search on a pack-index for a given oid. Packfile is expected 
to
+ * have a valid pack-index.
+ *
+ * See 'bsearch_hash' for more information.
+ */
+int bsearch_pack(const struct object_id *oid, const struct packed_git *p, 
uint32_t *result);
+
 /*
  * Return the SHA-1 of the nth object within the specified packfile.
  * Open the index if it is not already open.  The return value points
diff --git a/sha1_name.c b/sha1_name.c
index 735c1c0b8e..be3627b915 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -512,32 +512,16 @@ static void find_abbrev_len_for_pack(struct packed_git *p,
 struct min_abbrev_data *mad)
 {
int match = 0;
-   uint32_t num, last, first = 0;
+   uint32_t num, first = 0;
struct object_id oid;
+   const struct object_id *mad_oid;
 
if (open_pack_index(p) || !p->num_objects)
return;
 
num = p->num_objects;
-   last = num;
-   while (first < last) {
-   uint32_t mid = first + (last - first) / 2;
-   const unsigned char *current;
-   int cmp;
-
-   current = nth_packed_object_sha1(p, mid);
-   cmp = hashcmp(mad->hash, current);
-   if (!cmp) {
-   match = 1;
-   first = mid;
-   break;
-   }
-   if (cmp > 0) {
-   first = mid + 1;
-   continue;
-   }
-   last = mid;
-   }
+   mad_oid = (const struct object_id *)mad->hash;
+   match = bsearch_pack(mad_oid, p, );
 
/*
 * first is now the position in the packfile where we would insert
-- 
2.17.0.rc0

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Junio C Hamano]

Phillip Wood  writes:

> On 20/03/18 15:42, Johannes Schindelin wrote:
> ...
>> As indicated in another reply, I'd rather rebase the --recreate-merges
>> patches on top of your --keep-empty patch series. This obviously means
>> that I would fold essentially all of your 2/2 changes into my
>> "rebase-helper --make-script: introduce a flag to recreate merges"
>> 
>> The 1/2 (with s/failure/success/g) would then be added to the
>> --recreate-merges patch series at the end.
>> 
>> Would that be okay with you?
>
> Yes, that's fine, it would give a clearer history

With or without the above plan, what we saw from you were a bit
messy to queue.  The --keep-empty fix series is based on 'maint',
while the --signoff series depends on changes that happened to
sequencer between 'maint' and 'master', but yet depends on the
former.

In what I'll be pushing out at the end of today's integration run,
I'll have two topics organized this way:

 - pw/rebase-keep-empty-fixes: built by applying the three
   '--keep-empty' patches on top of 'maint'.

 - pw/rebase-signoff: built by first merging the above to 0f57f731
   ("Merge branch 'pw/sequencer-in-process-commit'", 2018-02-13) and
   then applying "rebase --signoff" series.

Also, I'll revert merge of Dscho's recreate-merges topic to 'next';
doing so would probably have to invalidate a few evil merges I've
been carrying to resolve conflicts between it and bc/object-id
topic, so today's integration cycle may take a bit longer than
usual.

Re: Understanding Binary Deltas within Packfile

[Duy Nguyen]

On Tue, Mar 20, 2018 at 7:34 PM, Luke Robison  wrote:
> On 3/20/2018 11:03 AM, Duy Nguyen wrote:
>>
>> On Tue, Mar 20, 2018 at 4:43 PM, Luke Robison 
>> wrote:
>>>
>>> Is there any documentation of the contents of the binary delta datain a
>>> packfile, and how to interpret them?  I found
>>>
>>> https://github.com/git/git/blob/master/Documentation/technical/pack-format.txt
>>> documenting the packfile itself, but the "compressed delta data" seems
>>> largely undocumented.  The source code of
>>> https://github.com/git/git/blob/master/diff-delta.c is pretty dense.
>>
>> The output is consumed by patch_delta()  in patch-delta.c if I'm not
>> mistaken. This function is less than 100 lines, probably much easier
>> to see the delta format.
>
> Thank you, that was much easier to read, and I've got my prototype working
> now.  I also found this site to be quite helpful:
> http://stefan.saasen.me/articles/git-clone-in-haskell-from-the-bottom-up/#delta_encoding

By the way, I forgot to add, if you want to improve pack-format.txt
(since you study it anyway), patches are always welcome.
-- 
Duy

Re: [PATCH v2 3/3] rebase --keep-empty: always use interactive rebase

[Phillip Wood]

On 20/03/18 16:08, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 20 Mar 2018, Phillip Wood wrote:
> 
>>  git-rebase--am.sh | 79 
>> ---
> 
> Those are a lot of changes, but pretty much all of it is a change in
> indentation.

Yes it's a big diff for what is really just deleting a few lines.

> All three patches look good to me.

That's good, thanks for looking at these and the other ones

Best Wishes

Phillip

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Phillip Wood]

On 20/03/18 15:42, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 20 Mar 2018, Phillip Wood wrote:
> 
>> From: Phillip Wood 
>>
>> These patches apply on top of js/rebase-recreate-merge. They extend
>> the --keep-empty fix from maint [1] to work with --recreate-merges.
> 
> As indicated in another reply, I'd rather rebase the --recreate-merges
> patches on top of your --keep-empty patch series. This obviously means
> that I would fold essentially all of your 2/2 changes into my
> "rebase-helper --make-script: introduce a flag to recreate merges"
> 
> The 1/2 (with s/failure/success/g) would then be added to the
> --recreate-merges patch series at the end.
> 
> Would that be okay with you?

Yes, that's fine, it would give a clearer history

Best Wishes

Phillip

Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits

[Phillip Wood]

On 20/03/18 17:34, Junio C Hamano wrote:
> Johannes Schindelin  writes:
> 
>>> +   if (!keep_empty && is_empty)
>>> strbuf_addf(, "%c ", comment_line_char);
> 
> We are not trying to preserve an empty one, and have found an empty
> one, so we comment it out, and then...
> 
>>> +   if (is_empty || !(commit->object.flags & PATCHSAME)) {
>>
>> May I suggest inverting the logic here, to make the code more obvious and
>> also to avoid indenting the block even further?
>>
>>  if (!is_empty && (commit->object.flags & PATCHSAME))
>>  continue;
> 
> ... if a non-empty one that already appears in the upstream, we do
> not do anything to it.  There is no room for keep-empty or lack of
> it to affect what happens to these commits.
> 
> Otherwise the insn is emitted for the commit.
> 
>>> +   strbuf_addf(, "%s %s ", insn,
>>> +   oid_to_hex(>object.oid));
>>> +   pretty_print_commit(, commit, );
>>> +   strbuf_addch(, '\n');
>>> +   fputs(buf.buf, out);
>>> +   }
> 
> I tend to agree that the suggested structure is easier to follow
> than Phillip's version.
> 
> But I wonder if this is even easier to follow.  It makes it even
> more clear that patchsame commits that are not empty are discarded
> unconditionally.
> 
>   while ((commit = get_revision())) {
>   int is_empty  = is_original_commit_empty(commit);
>   if (!is_empty && (commit->object.flags & PATCHSAME))
>   continue;
>   strbuf_reset();
>   if (!keep_empty && is_empty)
>   strbuf_addf(, "%c ", comment_line_char);
>   strbuf_addf(, "%s %s ", insn,
>   oid_to_hex(>object.oid));
>   pretty_print_commit(, commit, );
>   strbuf_addch(, '\n');
>   fputs(buf.buf, out);
>   }
> 
> Or did I screw up the rewrite?
> 
I've not tested it but I think it's OK, I agree that it is clearer than
my version

Best Wishes

Phillip

Re: [GSoC][PATCH v6] parse-options: do not show usage upon invalid option value

[Eric Sunshine]

On Tue, Mar 20, 2018 at 1:50 PM, Paul-Sebastian Ungureanu
 wrote:
> Usually, the usage should be shown only if the user does not know what
> options are available. If the user specifies an invalid value, the user
> is already aware of the available options. In this case, there is no
> point in displaying the usage anymore.
>
> This patch applies to "git tag --contains", "git branch --contains",
> "git branch --points-at", "git for-each-ref --contains" and many more.
>
> Signed-off-by: Paul-Sebastian Ungureanu 
> ---

As this is a very active mailing list, with reviewers poring over many
(sometimes dozens of) patches each day, it is easy for a reviewer to
forget exact details of each individual submission and review comment.
As a patch contributor, you can help ease reviewers' burden by adding
commentary to your submission here, below the "---" line following
your sign-off. In particular, reviewers find it very helpful when you:

* describe in some detail what changed since the previous version of
the patch, showing that you understood and took into consideration
each of the review comments from the previous iteration

* a link, like this[1], pointing at the previous round (and perhaps
further back) so reviewers can refresh their memories about issues
raised previously; this also helps people wanting to review this round
who were not involved in earlier rounds

[1]: 
https://public-inbox.org/git/20180319185436.14309-1-ungureanupaulsebast...@gmail.com/

Assalamu`Alaikum.

[Mohammad Ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($14.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Re: Understanding Binary Deltas within Packfile

[Luke Robison]

On 3/20/2018 11:03 AM, Duy Nguyen wrote:

On Tue, Mar 20, 2018 at 4:43 PM, Luke Robison  wrote:

Is there any documentation of the contents of the binary delta datain a
packfile, and how to interpret them?  I found
https://github.com/git/git/blob/master/Documentation/technical/pack-format.txt
documenting the packfile itself, but the "compressed delta data" seems
largely undocumented.  The source code of
https://github.com/git/git/blob/master/diff-delta.c is pretty dense.

The output is consumed by patch_delta()  in patch-delta.c if I'm not
mistaken. This function is less than 100 lines, probably much easier
to see the delta format.
Thank you, that was much easier to read, and I've got my prototype 
working now.  I also found this site to be quite helpful:

http://stefan.saasen.me/articles/git-clone-in-haskell-from-the-bottom-up/#delta_encoding





smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v6 09/11] pack-objects: shrink size field in struct object_entry

[Junio C Hamano]

Duy Nguyen  writes:

> This "size" field contains the delta size if the in-pack object is a
> delta. So blindly falling back to object_sha1_info() which returns the
> canonical object size is definitely wrong.

Yup.  Also we need to be careful when going back to the packfile to
read the size in question.  A different packfile that has the same
object may have delta that was constructed differently and of wrong
size.

> Please eject the series
> from 'pu' until I fix this. The bug won't likely affect anyone (since
> they must have 4GB+ objects to trigger it) but better safe than sorry.

> BTW can you apply this patch? This broken && chain made me think the
> problem was in the next test. It would have saved me lots of time if I
> saw this "BUG" line coming from the previous test.

Thanks, will do.

>
> -- 8< --
> Subject: [PATCH] t9300: fix broken && chain
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  t/t9300-fast-import.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index e4d06accc4..e2a0ae4075 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -348,7 +348,7 @@ test_expect_success 'B: accept branch name "TEMP_TAG"' '
>   INPUT_END
>  
>   test_when_finished "rm -f .git/TEMP_TAG
> - git gc
> + git gc &&
>   git prune" &&
>   git fast-importtest -f .git/TEMP_TAG &&
> @@ -365,7 +365,7 @@ test_expect_success 'B: accept empty committer' '
>   INPUT_END
>  
>   test_when_finished "git update-ref -d refs/heads/empty-committer-1
> - git gc
> + git gc &&
>   git prune" &&
>   git fast-importout=$(git fsck) &&

Re: [PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Eric Sunshine]

On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya
 wrote:
> ref-filter: get_ref_atom_value() error handling

This doesn't tell us much about what this patch is doing. Perhaps a
better subject would be:

ref-filter: libify get_ref_atom_value()

> Finish removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
>
> Change the signature of get_ref_atom_value() and underlying functions
> by adding return value and strbuf parameter for error message.
> Return value equals 0 upon success and -1 upon failure (there
> could be more error codes further if necessary).
> Upon failure, error message is appended to the strbuf.
>
> Signed-off-by: Olga Telezhnaia 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom 
> *atom, struct ref_array_item *re
> +static int get_object(struct ref_array_item *ref, const struct object_id 
> *oid,
> +  int deref, struct object **obj, struct strbuf *err)
>  {
> void *buf = get_obj(oid, obj, , );
> -   if (!buf)
> -   die(_("missing object %s for %s"),
> -   oid_to_hex(oid), ref->refname);
> -   if (!*obj)
> -   die(_("parse_object_buffer failed on %s for %s"),
> -   oid_to_hex(oid), ref->refname);
> -
> +   if (!buf) {
> +   strbuf_addf(err, _("missing object %s for %s"), 
> oid_to_hex(oid),
> +   ref->refname);
> +   if (!eaten)
> +   free(buf);

Since we are inside the 'if (!buf)' conditional, we _know_ that 'buf'
is NULL, therefore this free(buff) is unnecessary.

> +   return -1;
> +   }
> +   if (!*obj) {
> +   strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
> +   oid_to_hex(oid), ref->refname);
> +   if (!eaten)
> +   free(buf);
> +   return -1;
> +   }
> grab_values(ref->value, deref, *obj, buf, size);
> if (!eaten)
> free(buf);
> +   return 0;
>  }

Overall, with the need for resource cleanup, this function becomes
unusually noisy after this change. It could be tamed by doing
something like this:

int ret = 0;
void *buf = get_obj(oid, obj, , );
if (!buf)
ret = strbuf_error(_("missing object %s for %s"),
oid_to_hex(oid), ref->refname);
else if (!*obj)
ret = strbuf_error(_("parse_object_buffer failed on %s for %s"),
oid_to_hex(oid), ref->refname);
else
grab_values(ref->value, deref, *obj, buf, size);
   if (!eaten)
free(buf);
return ret;

Re: [PATCH v4 2/5] ref-filter: add return value && strbuf to handlers

[Eric Sunshine]

On Tue, Mar 20, 2018 at 12:05 PM, Olga Telezhnaya
 wrote:
> Continue removing die() calls from ref-filter formatting logic,
> so that it could be used by other commands.
> [...]
> Signed-off-by: Olga Telezhnaia 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -596,19 +603,24 @@ static int is_empty(const char *s)
> +static int then_atom_handler(struct atom_value *atomv, struct 
> ref_formatting_state *state,
> +struct strbuf *err)
>  {
> struct ref_formatting_stack *cur = state->stack;
> struct if_then_else *if_then_else = NULL;
>
> if (cur->at_end == if_then_else_handler)
> if_then_else = (struct if_then_else *)cur->at_end_data;
> -   if (!if_then_else)
> -   die(_("format: %%(then) atom used without an %%(if) atom"));
> -   if (if_then_else->then_atom_seen)
> -   die(_("format: %%(then) atom used more than once"));
> -   if (if_then_else->else_atom_seen)
> -   die(_("format: %%(then) atom used after %%(else)"));
> +   if (!if_then_else) {
> +   strbuf_addstr(err, _("format: %(then) atom used without an 
> %(if) atom"));
> +   return -1;
> +   } else if (if_then_else->then_atom_seen) {
> +   strbuf_addstr(err, _("format: %(then) atom used more than 
> once"));
> +   return -1;
> +   } else if (if_then_else->else_atom_seen) {
> +   strbuf_addstr(err, _("format: %(then) atom used after 
> %(else)"));
> +   return -1;
> +   }

This pattern of transforming:

if (cond)
die("...");

to:

if (cond) {
strbuf_add*(...);
return -1;
}

is repeated many, many times throughout this patch series and makes
for quite noisy diffs. Such repetition and noise suggests that this
patch series (and other similar ones) could benefit from a convenience
function similar to the existing error() function. For instance:

int strbuf_error(struct strbuf *buf, const char *fmt, ...);

which appends the formatted message to 'buf' and unconditionally
returns -1. Thus, the above transformation simplifies to:

if (cond)
return strbuf_error(, "...", ...);

which makes for a much less noisy diff, thus is easier to review.

A new patch introducing such a function at the head of this patch
series might be welcome.

Re: [PATCH v6 09/11] pack-objects: shrink size field in struct object_entry

[Duy Nguyen]

On Mon, Mar 19, 2018 at 5:43 PM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> +static inline void oe_set_size(struct object_entry *e,
>> +unsigned long size)
>> +{
>> + e->size_ = size;
>> + e->size_valid = e->size_ == size;
>
> A quite similar comment as my earlier one applies here.  I wonder if
> this is easier to read?
>
> e->size_valid = fits_in_32bits(size);
> if (e->size_valid)
> e->size_ = size;

I wonder if wrapping this "==" with something like this would help readability?

#define truncated(a,b) (a) != (b)

Then we could write

e->size_valid = !truncated(e->size_, size);
-- 
Duy

Re: [PATCH v6 09/11] pack-objects: shrink size field in struct object_entry

[Duy Nguyen]

On Mon, Mar 19, 2018 at 01:10:49PM -0700, Junio C Hamano wrote:
> > ... I was trying to exercise this
> > code the other day by reducing size_ field down to 4 bits, and a
> > couple tests broke but I still don't understand how.
> 
> Off by one?  Two or more copies of the same objects available whose
> oe_size() are different?
> 

No. I did indeed not understand pack-objects enough :)

This "size" field contains the delta size if the in-pack object is a
delta. So blindly falling back to object_sha1_info() which returns the
canonical object size is definitely wrong. Please eject the series
from 'pu' until I fix this. The bug won't likely affect anyone (since
they must have 4GB+ objects to trigger it) but better safe than sorry.

BTW can you apply this patch? This broken && chain made me think the
problem was in the next test. It would have saved me lots of time if I
saw this "BUG" line coming from the previous test.

-- 8< --
Subject: [PATCH] t9300: fix broken && chain

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t9300-fast-import.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index e4d06accc4..e2a0ae4075 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -348,7 +348,7 @@ test_expect_success 'B: accept branch name "TEMP_TAG"' '
INPUT_END
 
test_when_finished "rm -f .git/TEMP_TAG
-   git gc
+   git gc &&
git prune" &&
git fast-import 

[GSoC][PATCH v6] parse-options: do not show usage upon invalid option value

[Paul-Sebastian Ungureanu]

Usually, the usage should be shown only if the user does not know what
options are available. If the user specifies an invalid value, the user
is already aware of the available options. In this case, there is no
point in displaying the usage anymore.

This patch applies to "git tag --contains", "git branch --contains",
"git branch --points-at", "git for-each-ref --contains" and many more.

Signed-off-by: Paul-Sebastian Ungureanu 
---
 builtin/blame.c   |   1 +
 builtin/shortlog.c|   1 +
 builtin/update-index.c|   1 +
 parse-options.c   |  20 ---
 parse-options.h   |   1 +
 t/t0040-parse-options.sh  |   2 +-
 t/t0041-usage.sh  | 107 ++
 t/t3404-rebase-interactive.sh |   6 +-
 8 files changed, 125 insertions(+), 14 deletions(-)
 create mode 100755 t/t0041-usage.sh

diff --git a/builtin/blame.c b/builtin/blame.c
index 9dcb367b9..e8c6a4d6a 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -729,6 +729,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
for (;;) {
switch (parse_options_step(, options, blame_opt_usage)) {
case PARSE_OPT_HELP:
+   case PARSE_OPT_ERROR:
exit(129);
case PARSE_OPT_DONE:
if (ctx.argv[0])
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index e29875b84..be4df6a03 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -283,6 +283,7 @@ int cmd_shortlog(int argc, const char **argv, const char 
*prefix)
for (;;) {
switch (parse_options_step(, options, shortlog_usage)) {
case PARSE_OPT_HELP:
+   case PARSE_OPT_ERROR:
exit(129);
case PARSE_OPT_DONE:
goto parse_done;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 58d1c2d28..34adf55a7 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1059,6 +1059,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
break;
switch (parseopt_state) {
case PARSE_OPT_HELP:
+   case PARSE_OPT_ERROR:
exit(129);
case PARSE_OPT_NON_OPTION:
case PARSE_OPT_DONE:
diff --git a/parse-options.c b/parse-options.c
index d02eb8b01..47c09a82b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -317,14 +317,16 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, 
const char *arg,
return get_value(p, options, all_opts, flags ^ opt_flags);
}
 
-   if (ambiguous_option)
-   return error("Ambiguous option: %s "
+   if (ambiguous_option) {
+   error("Ambiguous option: %s "
"(could be --%s%s or --%s%s)",
arg,
(ambiguous_flags & OPT_UNSET) ?  "no-" : "",
ambiguous_option->long_name,
(abbrev_flags & OPT_UNSET) ?  "no-" : "",
abbrev_option->long_name);
+   return -3;
+   }
if (abbrev_option)
return get_value(p, abbrev_option, all_opts, abbrev_flags);
return -2;
@@ -434,7 +436,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
   const char * const usagestr[])
 {
int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
-   int err = 0;
 
/* we must reset ->opt, unknown short option leave it dangling */
ctx->opt = NULL;
@@ -459,7 +460,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
ctx->opt = arg + 1;
switch (parse_short_opt(ctx, options)) {
case -1:
-   goto show_usage_error;
+   return PARSE_OPT_ERROR;
case -2:
if (ctx->opt)
check_typos(arg + 1, options);
@@ -472,7 +473,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
while (ctx->opt) {
switch (parse_short_opt(ctx, options)) {
case -1:
-   goto show_usage_error;
+   return PARSE_OPT_ERROR;
case -2:
if (internal_help && *ctx->opt == 'h')
goto show_usage;
@@ -504,9 +505,11 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
goto show_usage;
switch (parse_long_opt(ctx, arg + 2, options)) {
case -1:
-   goto show_usage_error;
+   return 

Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits

[Junio C Hamano]

Johannes Schindelin  writes:

>> +if (!keep_empty && is_empty)
>>  strbuf_addf(, "%c ", comment_line_char);

We are not trying to preserve an empty one, and have found an empty
one, so we comment it out, and then...

>> +if (is_empty || !(commit->object.flags & PATCHSAME)) {
>
> May I suggest inverting the logic here, to make the code more obvious and
> also to avoid indenting the block even further?
>
>   if (!is_empty && (commit->object.flags & PATCHSAME))
>   continue;

... if a non-empty one that already appears in the upstream, we do
not do anything to it.  There is no room for keep-empty or lack of
it to affect what happens to these commits.

Otherwise the insn is emitted for the commit.

>> +strbuf_addf(, "%s %s ", insn,
>> +oid_to_hex(>object.oid));
>> +pretty_print_commit(, commit, );
>> +strbuf_addch(, '\n');
>> +fputs(buf.buf, out);
>> +}

I tend to agree that the suggested structure is easier to follow
than Phillip's version.

But I wonder if this is even easier to follow.  It makes it even
more clear that patchsame commits that are not empty are discarded
unconditionally.

while ((commit = get_revision())) {
int is_empty  = is_original_commit_empty(commit);
if (!is_empty && (commit->object.flags & PATCHSAME))
continue;
strbuf_reset();
if (!keep_empty && is_empty)
strbuf_addf(, "%c ", comment_line_char);
strbuf_addf(, "%s %s ", insn,
oid_to_hex(>object.oid));
pretty_print_commit(, commit, );
strbuf_addch(, '\n');
fputs(buf.buf, out);
}

Or did I screw up the rewrite?

We have deposited the check of your fund (2.500,000,00USD) through Money Gram

[John Fleming]

We have deposited the check of your fund (2.500,000,00USD) through Money Gram 
after our finally meeting regarding your funds. All you will do is to contact 
Money Gram director Mr. Christ Orji via E-mail(moneygramfi...@outlook.com) He 
will give you more direction on how you will be receiving the funds daily.

Remember to send him your Full information to avoid wrong transfer such as:

Receiver's Name___
Address:

Country: 
Phone
Number: _
I.D
Card:_

Though, I, John Fleming have sent $5000 in your name today So contact the 
Director Chrit Orji  as soon as you receive this email or call him +229-
65583120 and tell him to give you the
reference number to pick the $5000 .
Please let us know as soon as you receive your first payment so that we can 
send another payment today or tomorrow morning.


Best regards,
John Fleming

Re: PATCH 1/2] -Wuninitialized: remove some 'init-self' workarounds

[Junio C Hamano]

Ramsay Jones  writes:

> You may not have noticed that I messed up the Subject line of
> this patch - I managed to drop the '[' character in the patch
> prefix string. :(

I did notice the lack of '[' while reading, and then totally forgot
by the time I started queuing various topics.

Thanks for reminding.

> So, the commit 7bc195d877 on the 'pu' branch is titled:
>
>   "PATCH 1/2] -Wininitialized: remove some 'init-self' workarounds"
>
> Ahem! Sorry about that.
>
> ATB,
> Ramsay Jones

Re: [PATCH v5 2/3] stash push: avoid printing errors

[Junio C Hamano]

Thomas Gummerer  writes:

> ...
> Fix this by avoiding the 'git clean' if a pathspec is given, and use the
> pipeline that's used for pathspec mode to get rid of the untracked files
> as well.

That made me wonder if we can get rid of 'git clean' altogether by
pretending that we saw a pathspec '.' that match everything when no
pathspec is given---that way we only have to worry about a single
codepath.

But I guess doing it this way can minimize potential damage.  Those
who do not use pathspec when running "git stash" won't be affected
even if this change had some bugs ;-)

> diff --git a/git-stash.sh b/git-stash.sh
> index 4c92ec931f..5e06f96da5 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -308,14 +308,16 @@ push_stash () {
>   if test -z "$patch_mode"
>   then
>   test "$untracked" = "all" && CLEAN_X_OPTION=-x || 
> CLEAN_X_OPTION=
> - if test -n "$untracked"
> + if test -n "$untracked" && test $# = 0
>   then
>   git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
>   fi
>  
>   if test $# != 0
>   then
> - git add -u -- "$@"
> + test -z "$untracked" && UPDATE_OPTION="-u" || 
> UPDATE_OPTION=
> + test "$untracked" = "all" && FORCE_OPTION="--force" || 
> FORCE_OPTION=
> + git add $UPDATE_OPTION $FORCE_OPTION -- "$@"
>   git diff-index -p --cached --binary HEAD -- "$@" |
>   git apply --index -R
>   else

Thanks, I'll take the change as-is.

I however wonder if we restructure the code to

if test $# = 0
then
# no pathspec
if test -n "$untracked"
then
git clean --force --quiet -d $CLEAN_OPTION -- "$@"
fi
git reset --hard -q
else
test -z "$untracked" && UPDATE=-u || UPDATE=
test "$untracked" = all && FORCE=--force || FORCE=
git add $UPDATE $FORCE-- "$@"
git diff-index -p --cached --binary HEAD -- "$@" |
git apply --index -R
fi

it becomes easier to understand what is going on.

That way, once we have a plumbing command to help the else clause of
the above, i.e. "git checkout --index  -- "
[*1*], then we can lose the if/then/else and rewrite the whole "we
have created stash, so it's time to get rid of local modifications
to the paths that match the pathspec" code to:

if test "$untracked"
then
git clean --force --quiet -d $CLEAN_OPTION -- "$@"
fi
git checkout --index HEAD -- "$@"

[Footnote]
cf. https://public-inbox.org/git/xmqq4loqplou@gitster.mtv.corp.google.com/

What we want in the case in the code in question when there is
pathspec is "match the index entries and the working tree files to
those that appear in a given tree for paths that match the given
pathspec".  This is close to "git checkout  -- "
but not quite.  Current "git checkout  -- " is
an overlay operation in that paths that match  but do not
exist in  are *NOT* removed from the working tree.  We
obviously cannot change the behaviour of the command.

But we can add an option to ask for the new behaviour.  In general,
for an operation that affects the index and the working tree, we can
have "--cached" mode that affects only the index without touching
the working tree, and "--index" mode that affects both.

"git reset  -- ", which is a UI mistake, is
better spelled "git checkout --cached  -- ".  We
take paths that match  from  and stuff into the
index, and remove entries from the index for paths that do not exist
in .  And if we extend that to "--index" mode, that is
exactly what we want to happen.

Great News!!

[Amnesty International]

We have a great about your E-mail address!!! 

You Won  $950,500.00 USD on Amnesty International 
UK online E-mail Promotion. For more details about 
your prize claims, file with the following;

Names: 
Country: 
Tel:

Regards,
Mr. David Ford

Re: [PATCH 0/2] routines to generate JSON data

[Jeff Hostetler]

On 3/20/2018 1:42 AM, Jeff King wrote:

On Mon, Mar 19, 2018 at 06:19:26AM -0400, Jeff Hostetler wrote:


To make the above work, I think you'd have to store a little more state.
E.g., the "array_append" functions check "out->len" to see if they need
to add a separating comma. That wouldn't work if we might be part of a
nested array. So I think you'd need a context struct like:

struct json_writer {
  int first_item;
  struct strbuf out;
};
#define JSON_WRITER_INIT { 1, STRBUF_INIT }

to store the state and the output. As a bonus, you could also use it to
store some other sanity checks (e.g., keep a "depth" counter and BUG()
when somebody tries to access the finished strbuf with a hanging-open
object or array).


Yeah, I thought about that, but I think it gets more complex than that.
I'd need a stack of "first_item" values.  Or maybe the _begin() needs to
increment a depth and set first_item and the _end() needs to always
unset first_item.  I'll look at this gain.


I think you may be able to get by with just unsetting first_item for any
"end". Because as you "pop" to whatever data structure is holding
whatever has ended, you know it's no longer the first item (whatever
just ended was there before it).

I admit I haven't thought too hard on it, though, so maybe I'm missing
something.


I'll take a look.  Thanks.

 

The thing I liked about the bottom-up construction is that it is easier
to collect multiple sets in parallel and combine them during the final
roll-up.  With the in-line nesting, you're tempted to try to construct
the resulting JSON in a single series and that may not fit what the code
is trying to do.  For example, if I wanted to collect an array of error
messages as they are generated and an array of argv arrays and any alias
expansions, then put together a final JSON string containing them and
the final exit code, I'd need to build it in parts.  I can build these
parts in pieces of JSON and combine them at the end -- or build up other
similar data structures (string arrays, lists, or whatever) and then
have a JSON conversion step.  But we can make it work both ways, I just
wanted to keep it simpler.


Yeah, I agree that kind of bottom-up construction would be nice for some
cases. I'm mostly worried about inefficiency copying the strings over
and over as we build up the final output.  Maybe that's premature
worrying, though.

If the first_item thing isn't too painful, then it might be nice to have
both approaches available.


True.

 

In general I'd really prefer to keep the shell script as the driver for
the tests, and have t/helper programs just be conduits. E.g., something
like:

cat >expect <<-\EOF &&
{"key": "value", "foo": 42}
EOF
test-json-writer >actual \
  object_begin \
  object_append_string key value \
  object_append_int foo 42 \
  object_end &&
test_cmp expect actual

It's a bit tedious (though fairly mechanical) to expose the API in this
way, but it makes it much easier to debug, modify, or add tests later
on (for example, I had to modify the C program to find out that my
append example above wouldn't work).


Yeah, I wasn't sure if such a simple api required exposing all that
machinery to the shell or not.  And the api is fairly self-contained
and not depending on a lot of disk/repo setup or anything, so my tests
would be essentially static WRT everything else.

With my t0019 script you should have been able use -x -v to see what
was failing.


I was able to run the test-helper directly. The tricky thing is that I
had to write new C code to test my theory about how the API worked.
Admittedly that's not something most people would do regularly, but I
often seem to end up doing that kind of probing and debugging. Many
times I've found the more generic t/helper programs useful.

I also wonder if various parts of the system embrace JSON, if we'd want
to have a tool for generating it as part of other tests (e.g., to create
"expect" files).


Ok, let me see what I can come up with.

Thanks
Jeff

Re: [PATCH] doc/gitattributes: mention non-recursive behavior

[Duy Nguyen]

On Tue, Mar 20, 2018 at 5:14 AM, Jeff King  wrote:
> On Tue, Mar 20, 2018 at 12:04:11AM -0400, Jeff King wrote:
>
>> > I guess my takeaway is that it would be _good_ if the gitattributes
>> > documentation contained the caveat about not matching directories
>> > recursively, but _great_ if gitattributes and gitignore (and whatever
>> > else there is) were consistent.
>>
>> I agree it would be nice if they were consistent (and pathspecs, too).
>> But unfortunately at this point there's a maze of backwards
>> compatibility to deal with.
>
> So let's not forget to do the easy half there. Here's a patch.
>
> -- >8 --
> Subject: [PATCH] doc/gitattributes: mention non-recursive behavior
>
> The gitattributes documentation claims that the pattern
> rules are largely the same as for gitignore. However, the
> rules for recursion are different.
>
> In an ideal world, we would make them the same (if for
> nothing else than consistency and simplicity), but that
> would create backwards compatibility issues. For some
> discussion, see this thread:
>
>   https://public-inbox.org/git/slrnkldd3g.1l4@majutsushi.net/
>
> But let's at least document the differences instead of
> actively misleading the user by claiming that they're the
> same.
>
> Signed-off-by: Jeff King 
> ---
>  Documentation/gitattributes.txt | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index d52b254a22..1094fe2b5b 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -56,9 +56,16 @@ Unspecified::
>
>  When more than one pattern matches the path, a later line
>  overrides an earlier line.  This overriding is done per
> -attribute.  The rules how the pattern matches paths are the
> -same as in `.gitignore` files; see linkgit:gitignore[5].
> -Unlike `.gitignore`, negative patterns are forbidden.
> +attribute.
> +
> +The rules by which the pattern matches paths are the same as in
> +`.gitignore` files (see linkgit:gitignore[5]), with a few exceptions:
> +
> +  - negative patterns are forbidden

After 8b1bd02415 (Make !pattern in .gitattributes non-fatal -
2013-03-01) maybe we could use the verb "ignored" too instead of
"forbidden"

> +
> +  - patterns that match a directory do not recursively match paths
> +inside that directory (so using the trailing-slash `path/` syntax is

Technically gitignore does not match paths inside either. It simply
ignores the whole dir and not traverse in (which is more of an
optimization than anything). That is coincidentally perceived as
recursively ignoring. Anyway yes it's good to spell out the
differences here for gitattributes.

> +pointless in an attributes file; use `path/**` instead)

We probably could do this internally too (converting "path/" to
"path/**") but we need to deal with corner cases (e.g. "path" without
the trailing slash, but is a directory). So yes, suggesting the user
to do it instead may be easier.

>
>  When deciding what attributes are assigned to a path, Git
>  consults `$GIT_DIR/info/attributes` file (which has the highest
> --
> 2.17.0.rc0.402.ged0b3fd1ee
>



-- 
Duy

Re: [RFC][GSoC] Project proposal: convert interactive rebase to C

[Johannes Schindelin]

Hi Alban,

thank you for your proposal!

I will only comment on the parts that I feel could use improvement, the
rest is fine by me.

On Sat, 17 Mar 2018, Alban Gruin wrote:

> APPROXIMATIVE TIMELINE
> 
> Community bonding — April 23, 2018 – May 14, 2018
> During the community bonding, I would like to dive into git’s codebase,
> and to understand what git-rebase--interactive does under the hood. At
> the same time, I’d communicate with the community and my mentor, seeking
> for clarifications, and asking questions about how things should or
> should not be done.
> 
> Weeks 1 & 2 — May 14, 2018 – May 28, 2018
> First, I would refactor --preserve-merges in its own shell script, as
> described in Dscho’s email.

Could you go into detail a bit here? Like, describe what parts of the
git-rebase--interactive.sh script would need to be duplicated, which ones
would be truly moved, etc

> Weeks 3 & 4 — May 18, 2018 – June 11, 2018
> Then, I would start to rewrite git-rebase--interactive, and get rid of git-
> rebase--helper.

I think this is a bit premature, as the rebase--helper would not only
serve the --interactive part, but in later conversions also --am and
--merge, and only in the very end, when git-rebase.sh gets converted,
would we be able to simply rename the rebase--helper to rebase.

Also, I have a hunch that there is actually almost nothing left to rewrite
after my sequencer improvements that made it into Git v2.13.0, together
with the upcoming changes (which are on top of the --recreate-merges patch
series, hence I did not send them to the mailing list yet) in
https://github.com/dscho/git/commit/c261f17a4a3e

So I would like to see more details here... ;-)

> Weeks 5 to 9 — June 11, 2018 – July 15, 2018
> During this period, I would continue to rewrite git-rebase--interactive.

It would be good if the proposal said what parts of the conversion are
tricky, to merit spending a month on them.

> Weeks 10 & 11 — July 16, 2018 – July 29, 2018
> In the second half of July, I would look for bugs in the new code, test it, 
> and improve its coverage.

As I mentioned in a related mail, the test suite coverage would be most
sensibly extended *before* starting to rewrite code in C, as it helps
catching bugs early and avoids having to merge buggy code that needs to be
fixed immediately.

> Weeks 12 — July 30, 2018 – August 5, 2018
> In the last week, I would polish the code where needed, in order to
> improve for performance or to make the code more readable.

Thank you for sharing this draft with us!
Johannes

Re: .gitattributes override behavior (possible bug, or documentation bug)

[Duy Nguyen]

On Tue, Mar 20, 2018 at 5:40 AM, Jeff King  wrote:
> On Tue, Mar 20, 2018 at 12:25:27AM -0400, Dakota Hawkins wrote:
>
>> > Right. The technical reason is mostly "that is not how it was designed,
>> > and it would possibly break some corner cases if we switched it now".
>>
>> I'm just spitballing here, but do you guys think there's any subset of
>> the combined .gitignore and .gitattributes matching functionality that
>> could at least serve as a good "best-practices, going forward"
>> (because of consistency) for both? I will say every time I do this for
>> a new repo and have to do something even slightly complicated or
>> different from what I've done before with .gitattributes/.gitignore
>> that it takes me a long-ish time to figure it out. It's like I'm
>> vaguely aware of pitfalls I've encountered in the past in certain
>> areas but don't remember exactly what they are, so I consult the docs,
>> which are (in sum) confusing and lead to more time spent
>> trying/failing/trying/works/fails-later/etc.
>>
>> One "this subset of rules will work for both this way" would be

You know, you (Dakota) could implement the new "exclude" attribute in
.gitattributes and ignore .gitignore files completely. That makes it
works "for both" ;-) The effort is probably not small though.

>> awesome even if the matching capabilities are technically divergent,
>> but on the other hand that might paint both into a corner in terms of
>> functionality.
>
> As far as I know, they should be the same with the exception of this
> recursion, and the negative-pattern thing. But I'm cc-ing Duy, who is
> the resident expert on ignore and attributes matching (whether he wants
> to be or not ;) ).

Ha ha ha.

> I wouldn't be surprised if there's something I don't know about.

The only thing from the top of my head is what made me fail to unify
the implementation of the two. It's basically different order of
evaluation [1] when your patterns are spread out in multiple files. I
think it makes gitattr and gitignore behavior different too (but I
didn't try to verify).

Apart from that, the two should behave the same way besides the
exceptions you pointed out.

[1] 
https://public-inbox.org/git/%3CCACsJy8B8kYU7bkD8SiK354z4u=sy3hhbe4jvwnt_1pxod1c...@mail.gmail.com%3E/

> So I think the "recommended subset" is basically "everything but these
> few constructs". We just need to document them. ;)
>
> I probably should cc'd Duy on the documentation patch, too:
>
>   https://public-inbox.org/git/20180320041454.ga15...@sigill.intra.peff.net/
>
> -Peff
-- 
Duy

CAN I TRUST YOU??

[Sgt. Britta Lopez]

Good Day,
How are u doing today ? Apologies! I am a military woman ,seeking
your kind assistance to move the sum of ($7M USD) to you, as far
as i can be assured that my money will be safe in your care until
i complete my service here in Afghanistan and come over next
month.
This is legitimate, and there is no danger involved.If
interested, reply immediately for detailed information.
 
Reply to this email sgt.brittalo...@gmail.com
Regards ,
Sgt. Britta Lopez

Re: [PATCH v2 3/3] rebase --keep-empty: always use interactive rebase

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

>  git-rebase--am.sh | 79 
> ---

Those are a lot of changes, but pretty much all of it is a change in
indentation.

All three patches look good to me.

Thanks,
Dscho

[PATCH v4 0/5] ref-filter: remove die() calls from formatting logic

[Оля Тележная]

Only commit messages were updated since last review (there was no
comments about the code).

Thank you!
Olga

[PATCH v4 4/5] ref-filter: add return value to parsers

[Olga Telezhnaya]

Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of parsers by adding return value and
strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure (there
could be more error codes further if necessary).
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 177 +++
 1 file changed, 118 insertions(+), 59 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2313a33f0baa4..b90cec1056954 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,22 +101,28 @@ static struct used_atom {
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 
-static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom 
*atom,
+const char *color_value, struct strbuf *err)
 {
-   if (!color_value)
-   die(_("expected format: %%(color:)"));
-   if (color_parse(color_value, atom->u.color) < 0)
-   die(_("unrecognized color: %%(color:%s)"), color_value);
+   if (!color_value) {
+   strbuf_addstr(err, _("expected format: %(color:)"));
+   return -1;
+   }
+   if (color_parse(color_value, atom->u.color) < 0) {
+   strbuf_addf(err, _("unrecognized color: %%(color:%s)"), 
color_value);
+   return -1;
+   }
/*
 * We check this after we've parsed the color, which lets us complain
 * about syntactically bogus color names even if they won't be used.
 */
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+   return 0;
 }
 
-static void refname_atom_parser_internal(struct refname_atom *atom,
-const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char 
*arg,
+const char *name, struct strbuf *err)
 {
if (!arg)
atom->option = R_NORMAL;
@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct 
refname_atom *atom,
else if (skip_prefix(arg, "lstrip=", ) ||
 skip_prefix(arg, "strip=", )) {
atom->option = R_LSTRIP;
-   if (strtol_i(arg, 10, >lstrip))
-   die(_("Integer value expected refname:lstrip=%s"), arg);
+   if (strtol_i(arg, 10, >lstrip)) {
+   strbuf_addf(err, _("Integer value expected 
refname:lstrip=%s"), arg);
+   return -1;
+   }
} else if (skip_prefix(arg, "rstrip=", )) {
atom->option = R_RSTRIP;
-   if (strtol_i(arg, 10, >rstrip))
-   die(_("Integer value expected refname:rstrip=%s"), arg);
-   } else
-   die(_("unrecognized %%(%s) argument: %s"), name, arg);
+   if (strtol_i(arg, 10, >rstrip)) {
+   strbuf_addf(err, _("Integer value expected 
refname:rstrip=%s"), arg);
+   return -1;
+   }
+   } else {
+   strbuf_addf(err, _("unrecognized %%(%s) argument: %s"), name, 
arg);
+   return -1;
+   }
+   return 0;
 }
 
-static void remote_ref_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct 
used_atom *atom,
+ const char *arg, struct strbuf *err)
 {
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format 
*format, struct used_
 
if (!arg) {
atom->u.remote_ref.option = RR_REF;
-   refname_atom_parser_internal(>u.remote_ref.refname,
-arg, atom->name);
-   return;
+   return refname_atom_parser_internal(>u.remote_ref.refname,
+   arg, atom->name, err);
}
 
atom->u.remote_ref.nobracket = 0;
@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct 
ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
-   
refname_atom_parser_internal(>u.remote_ref.refname,
-arg, atom->name);
+   if 
(refname_atom_parser_internal(>u.remote_ref.refname,
+arg, atom->name, err))
+   return -1;
 

[PATCH v4 2/5] ref-filter: add return value && strbuf to handlers

[Olga Telezhnaya]

Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of handlers by adding return value
and strbuf parameter for errors.
Return value equals 0 upon success and -1 upon failure (there could be
more error codes further if necessary). Upon failure, error message is
appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 71 
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 54fae00bdd410..d120360104806 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -387,7 +387,8 @@ struct ref_formatting_state {
 
 struct atom_value {
const char *s;
-   void (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state);
+   int (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state,
+  struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
 };
@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char 
*str, int quote_style)
}
 }
 
-static void append_atom(struct atom_value *v, struct ref_formatting_state 
*state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state 
*state,
+  struct strbuf *unused_err)
 {
/*
 * Quote formatting is only done when the stack has a single
@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct 
ref_formatting_state *state
quote_formatting(>stack->output, v->s, 
state->quote_style);
else
strbuf_addstr(>stack->output, v->s);
+   return 0;
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack 
**stack)
strbuf_release();
 }
 
-static void align_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+ struct strbuf *unused_err)
 {
struct ref_formatting_stack *new_stack;
 
@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, 
struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = >atom->u.align;
+   return 0;
 }
 
 static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -572,7 +577,8 @@ static void if_then_else_handler(struct 
ref_formatting_stack **stack)
free(if_then_else);
 }
 
-static void if_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+  struct strbuf *unused_err)
 {
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct 
if_then_else), 1);
@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, 
struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+   return 0;
 }
 
 static int is_empty(const char *s)
@@ -596,19 +603,24 @@ static int is_empty(const char *s)
return 1;
 }
 
-static void then_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+struct strbuf *err)
 {
struct ref_formatting_stack *cur = state->stack;
struct if_then_else *if_then_else = NULL;
 
if (cur->at_end == if_then_else_handler)
if_then_else = (struct if_then_else *)cur->at_end_data;
-   if (!if_then_else)
-   die(_("format: %%(then) atom used without an %%(if) atom"));
-   if (if_then_else->then_atom_seen)
-   die(_("format: %%(then) atom used more than once"));
-   if (if_then_else->else_atom_seen)
-   die(_("format: %%(then) atom used after %%(else)"));
+   if (!if_then_else) {
+   strbuf_addstr(err, _("format: %(then) atom used without an 
%(if) atom"));
+   return -1;
+   } else if (if_then_else->then_atom_seen) {
+   strbuf_addstr(err, _("format: %(then) atom used more than 
once"));
+   return -1;
+   } else if (if_then_else->else_atom_seen) {
+   strbuf_addstr(err, _("format: %(then) atom used after 
%(else)"));
+   return -1;
+   }
if_then_else->then_atom_seen = 1;
/*
 * If the 'equals' or 'notequals' attribute is used then
@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, 
struct ref_formatting_st
} else if (cur->output.len 

[PATCH v4 5/5] ref-filter: get_ref_atom_value() error handling

[Olga Telezhnaya]

Finish removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of get_ref_atom_value() and underlying functions
by adding return value and strbuf parameter for error message.
Return value equals 0 upon success and -1 upon failure (there
could be more error codes further if necessary).
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 55 ---
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index b90cec1056954..85f971663a4aa 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1445,28 +1445,36 @@ static const char *get_refname(struct used_atom *atom, 
struct ref_array_item *re
return show_ref(>u.refname, ref->refname);
 }
 
-static void get_object(struct ref_array_item *ref, const struct object_id *oid,
-  int deref, struct object **obj)
+static int get_object(struct ref_array_item *ref, const struct object_id *oid,
+  int deref, struct object **obj, struct strbuf *err)
 {
int eaten;
unsigned long size;
void *buf = get_obj(oid, obj, , );
-   if (!buf)
-   die(_("missing object %s for %s"),
-   oid_to_hex(oid), ref->refname);
-   if (!*obj)
-   die(_("parse_object_buffer failed on %s for %s"),
-   oid_to_hex(oid), ref->refname);
-
+   if (!buf) {
+   strbuf_addf(err, _("missing object %s for %s"), oid_to_hex(oid),
+   ref->refname);
+   if (!eaten)
+   free(buf);
+   return -1;
+   }
+   if (!*obj) {
+   strbuf_addf(err, _("parse_object_buffer failed on %s for %s"),
+   oid_to_hex(oid), ref->refname);
+   if (!eaten)
+   free(buf);
+   return -1;
+   }
grab_values(ref->value, deref, *obj, buf, size);
if (!eaten)
free(buf);
+   return 0;
 }
 
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 {
struct object *obj;
int i;
@@ -1588,16 +1596,17 @@ static void populate_value(struct ref_array_item *ref)
break;
}
if (used_atom_cnt <= i)
-   return;
+   return 0;
 
-   get_object(ref, >objectname, 0, );
+   if (get_object(ref, >objectname, 0, , err))
+   return -1;
 
/*
 * If there is no atom that wants to know about tagged
 * object, we are done.
 */
if (!need_tagged || (obj->type != OBJ_TAG))
-   return;
+   return 0;
 
/*
 * If it is a tag object, see if we use a value that derefs
@@ -1611,20 +1620,23 @@ static void populate_value(struct ref_array_item *ref)
 * is not consistent with what deref_tag() does
 * which peels the onion to the core.
 */
-   get_object(ref, tagged, 1, );
+   return get_object(ref, tagged, 1, , err);
 }
 
 /*
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct 
atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom,
+ struct atom_value **v, struct strbuf *err)
 {
if (!ref->value) {
-   populate_value(ref);
+   if (populate_value(ref, err))
+   return -1;
fill_missing_values(ref->value);
}
*v = >value[atom];
+   return 0;
 }
 
 /*
@@ -2148,9 +2160,13 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct 
ref_array_item *a, stru
int cmp;
cmp_type cmp_type = used_atom[s->atom].type;
int (*cmp_fn)(const char *, const char *);
+   struct strbuf err = STRBUF_INIT;
 
-   get_ref_atom_value(a, s->atom, );
-   get_ref_atom_value(b, s->atom, );
+   if (get_ref_atom_value(a, s->atom, , ))
+   die("%s", err.buf);
+   if (get_ref_atom_value(b, s->atom, , ))
+   die("%s", err.buf);
+   strbuf_release();
cmp_fn = s->ignore_case ? strcasecmp : strcmp;
if (s->version)
cmp = versioncmp(va->s, vb->s);
@@ -2230,7 +2246,8 @@ int format_ref_array_item(struct ref_array_item *info,
pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
if (pos < 0)
return -1;
-   get_ref_atom_value(info, pos, );
+   if (get_ref_atom_value(info, pos, , error_buf))
+   return -1;

[PATCH v4 1/5] ref-filter: start adding strbufs with errors

[Olga Telezhnaya]

This is a first step in removing die() calls from ref-filter
formatting logic, so that it could be used by other commands
that do not want to die during formatting process.
die() calls related to bugs in code will not be touched in this patch.

Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.

This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and adjusts
its callers. While at it, reduce the scope of the out-variable.

Signed-off-by: Olga Telezhnaia 
---
 builtin/branch.c |  7 +--
 ref-filter.c | 17 -
 ref-filter.h |  7 ---
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6d0cea9d4bcc4..c21e5a04a0177 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
-   struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
 
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
ref_array_sort(sorting, );
 
for (i = 0; i < array.nr; i++) {
-   format_ref_array_item(array.items[i], format, );
+   struct strbuf out = STRBUF_INIT;
+   struct strbuf err = STRBUF_INIT;
+   if (format_ref_array_item(array.items[i], format, , ))
+   die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are 
incompatible");
 /* format to a string_list to let print_columns() do 
its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+   strbuf_release();
strbuf_release();
}
 
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..54fae00bdd410 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char 
*ep, struct ref_formatting
}
 }
 
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
   const struct ref_format *format,
-  struct strbuf *final_buf)
+  struct strbuf *final_buf,
+  struct strbuf *error_buf)
 {
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,
resetv.s = GIT_COLOR_RESET;
append_atom(, );
}
-   if (state.stack->prev)
-   die(_("format: %%(end) atom missing"));
+   if (state.stack->prev) {
+   strbuf_addstr(error_buf, _("format: %(end) atom missing"));
+   return -1;
+   }
strbuf_addbuf(final_buf, >output);
pop_stack_element();
+   return 0;
 }
 
 void show_ref_array_item(struct ref_array_item *info,
 const struct ref_format *format)
 {
struct strbuf final_buf = STRBUF_INIT;
+   struct strbuf error_buf = STRBUF_INIT;
 
-   format_ref_array_item(info, format, _buf);
+   if (format_ref_array_item(info, format, _buf, _buf))
+   die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+   strbuf_release(_buf);
strbuf_release(_buf);
putchar('\n');
 }
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
-  const struct ref_format *format,
-  struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const struct ref_format 
*format);
 /*  Parse a single sort specifier and add it to the list */

--

[PATCH v4 3/5] ref-filter: change parsing function error handling

[Olga Telezhnaya]

Continue removing die() calls from ref-filter formatting logic,
so that it could be used by other commands.

Change the signature of parse_ref_filter_atom() by adding
strbuf parameter for error message.
The function returns the position in the used_atom[] array
(as before) for the given atom, or -1 to signal an error (there
could be more negative error codes further if necessary).
Upon failure, error message is appended to the strbuf.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 36 ++--
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index d120360104806..2313a33f0baa4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
-const char *atom, const char *ep)
+const char *atom, const char *ep,
+struct strbuf *err)
 {
const char *sp;
const char *arg;
@@ -406,8 +407,10 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
sp = atom;
if (*sp == '*' && sp < ep)
sp++; /* deref */
-   if (ep <= sp)
-   die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+   if (ep <= sp) {
+   strbuf_addf(err, _("malformed field name: %.*s"), 
(int)(ep-atom), atom);
+   return -1;
+   }
 
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
@@ -432,8 +435,10 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
break;
}
 
-   if (ARRAY_SIZE(valid_atom) <= i)
-   die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+   if (ARRAY_SIZE(valid_atom) <= i) {
+   strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), 
atom);
+   return -1;
+   }
 
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -725,17 +730,21 @@ int verify_ref_format(struct ref_format *format)
 
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+   struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
 
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
-   at = parse_ref_filter_atom(format, sp + 2, ep);
+   at = parse_ref_filter_atom(format, sp + 2, ep, );
+   if (at < 0)
+   die("%s", err.buf);
cp = ep + 1;
 
if (skip_prefix(used_atom[at].name, "color:", ))
format->need_color_reset_at_eol = !!strcmp(color, 
"reset");
+   strbuf_release();
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2154,13 +2163,15 @@ int format_ref_array_item(struct ref_array_item *info,
 
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+   int pos;
 
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, );
-   get_ref_atom_value(info,
-  parse_ref_filter_atom(format, sp + 2, ep),
-  );
+   pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
+   if (pos < 0)
+   return -1;
+   get_ref_atom_value(info, pos, );
if (atomv->handler(atomv, , error_buf))
return -1;
}
@@ -2215,7 +2226,12 @@ static int parse_sorting_atom(const char *atom)
 */
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
-   return parse_ref_filter_atom(, atom, end);
+   struct strbuf err = STRBUF_INIT;
+   int res = parse_ref_filter_atom(, atom, end, );
+   if (res < 0)
+   die("%s", err.buf);
+   strbuf_release();
+   return res;
 }
 
 /*  If no sorting option is given, use refname to sort as default */

--
https://github.com/git/git/pull/466

Re: Understanding Binary Deltas within Packfile

[Duy Nguyen]

On Tue, Mar 20, 2018 at 4:43 PM, Luke Robison  wrote:
> Is there any documentation of the contents of the binary delta datain a
> packfile, and how to interpret them?  I found
> https://github.com/git/git/blob/master/Documentation/technical/pack-format.txt
> documenting the packfile itself, but the "compressed delta data" seems
> largely undocumented.  The source code of
> https://github.com/git/git/blob/master/diff-delta.c is pretty dense.

The output is consumed by patch_delta()  in patch-delta.c if I'm not
mistaken. This function is less than 100 lines, probably much easier
to see the delta format.

>  This
> is what I've got so for:
>
> +-+
> | Varint src_size
> | Varint trg_size
> | 1-byte inscnt
> | N-bytes up to 16 bytes of trg_buf
> |
> | A series of Operations
> |
> +-
>
>
> Any details on how to interpret the opcodes would be greatly appreciated.
>
> Thanks,
> Luke
>



-- 
Duy

Re: [BUG] log --graph corrupts patch

[Jeff King]

On Tue, Mar 20, 2018 at 09:58:14AM +, Phillip Wood wrote:

> > Are you using any exotic filters for your pager? If you use "git
> > --no-pager" does the problem persist?
> 
> Hi Peff, thanks for taking the time to check this, I had forgotten about
> the pager. I'm using diff-highlight and it seems that is causing the
> problems.

Heh. Lucky guess. Unsurprisingly, I use diff-highlight, too. But I did
not see it because I never bothered to upgrade my personal copy of the
script, which has been working for me for ages, to the one in contrib/.

But indeed, I can easily reproduce the problem with that version of the
script. Here's a pretty minimal reproduction:

-- >8 --

cat >bad <<\EOF
* commit whatever
| other stuff irrelevant
|
| diff --git a/foo b/foo
| --- a/foo
| --- b/foo
| @@ -100,6 +100,9
|  some
|  context
|  lines
| +some
| +new
| +lines
|  -context line with a leading minus
|  and other
|  context
EOF

contrib/diff-highlight/diff-highlight 

RE: Bug With git rebase -p

[Joseph Strauss]

Perfect. Thank you.

-Original Message-
From: Johannes Schindelin [mailto:johannes.schinde...@gmx.de] 
Sent: Tuesday, March 20, 2018 10:53 AM
To: Junio C Hamano 
Cc: Joseph Strauss ; git@vger.kernel.org
Subject: Re: Bug With git rebase -p

Hi,

On Mon, 19 Mar 2018, Junio C Hamano wrote:

> Joseph Strauss  writes:
> 
> > I found the following erroneous behavior with "git rebase -p".
> >
> > My current version is git version 2.16.2.windows.1
> >
> > I made an example at GitHub, https://github.com/jkstrauss/git-bug/
> >
> > There seem to be two problems when rebasing merge commits with git rebase 
> > -p :
> >   1. All lines of merge commits' messages get collapse into a single line.
> >   2. When an asterisk is present in the middle of the line it gets replaced 
> > with the file names of the current directory.
> 
> I suspect that this has already been independently discovered
> (twice) and corrected with
> 
> https://public-inbox.org/git/20180208204241.19324-1-gregory.herrero@or
> acle.com/
> 
> and is included in v2.17-rc0 (and later ;-).

As it is included in v2.17.0-rc0, Joseph, could you verify that the version at

https://github.com/git-for-windows/git/releases/tag/v2.17.0-rc0.windows.1

fixes this issue for you?

Thanks,
Johannes

Understanding Binary Deltas within Packfile

[Luke Robison]

Is there any documentation of the contents of the binary delta datain a 
packfile, and how to interpret them?  I found 
https://github.com/git/git/blob/master/Documentation/technical/pack-format.txt 
documenting the packfile itself, but the "compressed delta data" seems 
largely undocumented.  The source code of 
https://github.com/git/git/blob/master/diff-delta.c is pretty dense.  
This is what I've got so for:


+-+
| Varint src_size
| Varint trg_size
| 1-byte inscnt
| N-bytes up to 16 bytes of trg_buf
|
| A series of Operations
|
+-


Any details on how to interpret the opcodes would be greatly appreciated.

Thanks,
Luke

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

> From: Phillip Wood 
> 
> These patches apply on top of js/rebase-recreate-merge. They extend
> the --keep-empty fix from maint [1] to work with --recreate-merges.

As indicated in another reply, I'd rather rebase the --recreate-merges
patches on top of your --keep-empty patch series. This obviously means
that I would fold essentially all of your 2/2 changes into my
"rebase-helper --make-script: introduce a flag to recreate merges"

The 1/2 (with s/failure/success/g) would then be added to the
--recreate-merges patch series at the end.

Would that be okay with you?

Ciao,
Dscho

Re: [PATCH 2/2] rebase --recreate-merges --keep-empty: don't prune empty commits

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

> From: Phillip Wood 
> 
> If there are empty commits on the left hand side of $upstream...HEAD
> then the empty commits on the right hand side that we want to keep are
> pruned because they are marked as cherry-picks. Fix this by keeping
> the commits that are empty or are not marked as cherry-picks.

Thank you!

> @@ -3172,12 +3173,9 @@ int sequencer_make_script(FILE *out, int argc, const 
> char **argv,
>  
>   init_revisions(, NULL);
>   revs.verbose_header = 1;
> - if (recreate_merges)
> - revs.cherry_mark = 1;
> - else {
> + if (!recreate_merges)
>   revs.max_parents = 1;
> - revs.cherry_pick = 1;
> - }
> + revs.cherry_mark = 1;
>   revs.limited = 1;
>   revs.reverse = 1;
>   revs.right_only = 1;

Yeah, this looks ugly. I'd rather have your patch series applied first and
then rebase my --recreate-merges patch series on top.

Ciao,
Dscho

Re: [PATCH 0/3] rebase --keep-empty/--root fixes

[Johannes Schindelin]

... and now also Cc:ing the list and Junio...


On Tue, 20 Mar 2018, Johannes Schindelin wrote:

> Hi Phillip,
> 
> On Tue, 20 Mar 2018, Phillip Wood wrote:
> 
> > Phillip Wood (3):
> >   rebase --root: stop assuming squash_onto is unset
> >   rebase -i --keep-empty: don't prune empty commits
> >   rebase: respect --no-keep-empty
> 
> Those patches look good. I offered a suggestion to 2/3 to avoid indenting
> a code block, but I would also be okay to leave it as-is.
> 
> Thanks,
> Dscho
> 

Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 4d3f60594c..96ff812c8d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2470,7 +2470,7 @@ int sequencer_make_script(FILE *out, int argc, const 
> char **argv,
>   init_revisions(, NULL);
>   revs.verbose_header = 1;
>   revs.max_parents = 1;
> - revs.cherry_pick = 1;
> + revs.cherry_mark = 1;

This will conflict with my --recreate-merges patch series, but in a good
way.

> @@ -2495,14 +2495,18 @@ int sequencer_make_script(FILE *out, int argc, const 
> char **argv,
>   return error(_("make_script: error preparing revisions"));
>  
>   while ((commit = get_revision())) {
> + int is_empty  = is_original_commit_empty(commit);
> +
>   strbuf_reset();
> - if (!keep_empty && is_original_commit_empty(commit))
> + if (!keep_empty && is_empty)
>   strbuf_addf(, "%c ", comment_line_char);
> - strbuf_addf(, "%s %s ", insn,
> - oid_to_hex(>object.oid));
> - pretty_print_commit(, commit, );
> - strbuf_addch(, '\n');
> - fputs(buf.buf, out);
> + if (is_empty || !(commit->object.flags & PATCHSAME)) {

May I suggest inverting the logic here, to make the code more obvious and
also to avoid indenting the block even further?

if (!is_empty && (commit->object.flags & PATCHSAME))
continue;

> + strbuf_addf(, "%s %s ", insn,
> + oid_to_hex(>object.oid));
> + pretty_print_commit(, commit, );
> + strbuf_addch(, '\n');
> + fputs(buf.buf, out);
> + }
>   }
>   strbuf_release();
>   return 0;

Thanks,
Dscho

Re: [RFC] [GSoC] Project proposal: convert scripts to builtins

[Johannes Schindelin]

Hi Pratik,

thank you so much for posting this inline, to make it easier to review.

I will quote only on specific parts below; Please just assume that I like
the other parts and have nothing to add.

On Tue, 20 Mar 2018, Pratik Karki wrote:

> 
> Timeline and Development Cycle
> --
> 
> -   Apr 23: Accepted student proposals announced.
> -   Apr 23 onwards: Researching of all the test suites. Discussion of
> possible test improvements in for `git-stash` and `git-rebase`.
> -   May 1: Rewriting skeleton begins.

I would have liked more detail here. Like, maybe even a rudimentary
initial version identifying, say, a part of `git stash` and/or `git
rebase` that could be put into a builtin (stash--helper and
rebase--helper, respectively).

It is my experience from several GSoCs working on this huge overarching
project to convert the scripts (which are good prototypes, but lack in
stringency in addition to performance) to C that even the individual
scripts are too much to stem for a single GSoC.

> -   May 13: Making `builtin/stash.c` ready for review process. (This
> goes on for some time.)

There have been two past efforts to turn stash into a builtin:

https://github.com/git-for-windows/git/pull/508

and

https://public-inbox.org/git/20171110231314.30711-1-j...@teichroeb.net/

It would be good to read up on those and incorporate the learnings into
the proposal.

> -   May 26: Making `builtin/rebase.c` ready for review process. (This
> goes on for some time.)

The `git-rebase.sh` script is itself not terribly interesting, as it hands
off to `git-rebase--am.sh`, `git-rebase--interactive.sh` and
`git-rebase--merge.sh`, respectively.

Converting `git-rebase` into a builtin without first converting all of
those scripts would make little sense.

It would probably be better to choose one of those latter scripts and move
their functionality into a builtin, in an incremental fashion.

By doing it incrementally, you can also avoid...

> -   June 10: Make second versions with more improvements and more
> batteries ready for next review cycle.

... leaving two weeks between checkpoints. Also, doing it incrementally
lets you avoid sitting on your hands while waiting for the first patches
to be reviewed.

> -   June 20: Writing new tests and using more code-coverage tools to
> squash bugs present.

Typically it helps a lot to have those tests *during* the conversion.
That's how I found most of the bugs when converting difftool, for example.

> -   June 25 - Jul 20: Start optimizing `builtin/stash.c` and
> `builtin/rebase.c`. Benchmarking and profiling is done. They are
> exclusively compared to their original shell scripts, to check
> whether they are more performant or not and the results are
> published in the mailing list for further discussion.

Could you add details how you would perform benchmarking and profiling?

> -   Jul 20 - Aug 5: More optimizing and polishing of `builtin/stash.c`
> and `builtin/rebase.c` and running of new tests series written and
> send them for code review.
> -   Aug 14: Submit final patches.
> -   Aug 22: Results announced.
> -   Apr 24 - Aug 14: Documentation is written. "What I'm working on" is
> written and posted in my blog regarding GSoC with Git.

The timeline is a bit ambitious. I would like to caution you that these
are all big tasks, and maybe you want to cut down on the deliverables, and
add more detail what exactly you want to deliver (such as: what part of
stash/rebase do you find under-tested in our test suite and would
therefore want to augment, what parts of stash/rebase do you think you
would handle first, and how?).

Ciao,
Johannes

Re: Bug With git rebase -p

[Johannes Schindelin]

Hi,

On Mon, 19 Mar 2018, Junio C Hamano wrote:

> Joseph Strauss  writes:
> 
> > I found the following erroneous behavior with "git rebase -p".
> >
> > My current version is git version 2.16.2.windows.1
> >
> > I made an example at GitHub, https://github.com/jkstrauss/git-bug/
> >
> > There seem to be two problems when rebasing merge commits with git rebase 
> > -p :
> >   1. All lines of merge commits' messages get collapse into a single line.
> >   2. When an asterisk is present in the middle of the line it gets replaced 
> > with the file names of the current directory.
> 
> I suspect that this has already been independently discovered
> (twice) and corrected with
> 
> https://public-inbox.org/git/20180208204241.19324-1-gregory.herr...@oracle.com/
> 
> and is included in v2.17-rc0 (and later ;-).

As it is included in v2.17.0-rc0, Joseph, could you verify that the
version at

https://github.com/git-for-windows/git/releases/tag/v2.17.0-rc0.windows.1

fixes this issue for you?

Thanks,
Johannes

Re: [PATCH 0/2] -Wuninitialized

[Johannes Schindelin]

Hi Ramsay,

On Mon, 19 Mar 2018, Ramsay Jones wrote:

> This series removes all 'self-initialised' variables (ie.  var =
> var;).  This construct has been used to silence gcc
> '-W[maybe-]uninitialized' warnings in the past [1]. Unfortunately, this
> construct causes warnings to be issued by MSVC [2], along with clang
> static analysis complaining about an 'Assigned value is garbage or
> undefined'. The number of these constructs has dropped over the years
> (eg. see [3] and [4]), so there are currently only 6 remaining in the
> current codebase. As demonstrated below, 5 of these no longer cause gcc
> to issue warnings.

Thank you so much for working on this!

In Git for Windows, to work around the MSVC issues you mention, I have
this ugly work-around (essentially, it has a FAKE_INIT() macro that either
performs that GCC workaround or initializes the variable to NULL/0):

https://github.com/git-for-windows/git/commit/474155f32a

FWIW I just tested your patches with Visual Studio 2017 and there are no
C4700 warnings (the equivalent of GCC's "may be uninitialized" warning)
[*1*].

You can find the patches (your patches rebased onto Git for Windows'
master, plus a patch adding the project files for Visual Studio) here:

https://github.com/git-for-windows/git/compare/master...dscho:msvc-uninitialized-warning-test

So here is my ACK, in case you want it ;-)

Ciao,
Dscho

Footnote *1*: there actually was one, but in a Windows-only patch. That's
what that last (fixup!) patch on my branch is all about.

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Igor Djordjevic  writes:

> Hi Sergey,
>
> On 19/03/2018 06:44, Sergey Organov wrote:
>> 
>> > > > > > Second side note: if we can fast-forward, currently we prefer
>> > > > > > that, and I think we should keep that behavior with -R, too.
>> > > > >
>> > > > > I agree.
>> > > >
>> > > > I'm admittedly somewhat lost in the discussion, but are you
>> > > > talking fast-forward on _rebasing_ existing merge? Where would it
>> > > > go in any of the suggested algorithms of rebasing and why?
>> > > >
>> > > > I readily see how it can break merges. E.g., any "git merge
>> > > > --ff-only --no-ff" merge will magically disappear. So, even if
>> > > > somehow supported, fast-forward should not be performed by default
>> > > > during _rebasing_ of a merge.
>> > >
>> > > Hmm, now that you brought this up, I can only agree, of course.
>> > >
>> > > What I had in my mind was more similar to "no-rebase-cousins", like 
>> > > if we can get away without actually rebasing the merge but still 
>> > > using the original one, do it. But I guess that`s not what Johannes 
>> > > originally asked about.
>> > >
>> > > This is another definitive difference between rebasing (`pick`?) and 
>> > > recreating (`merge`) a merge commit - in the case where we`re rebasing, 
>> > > of course it doesn`t make sense to drop commit this time (due to 
>> > > fast-forward). This does make sense in recreating the merge (only).
>> >
>> > Eh, I might take this back. I think my original interpretation (and 
>> > agreement) to fast-forwarding is correct.
>> >
>> > But the confusion here comes from `--no-ff` as used for merging, as 
>> > opposed to `--no-ff` as used for rebasing. I _think_ Johannes meant 
>> > the latter one.
>> >
>> > In rebasing, `--no-ff` means that even if a commit inside todo list 
>> > isn`t to be changed, do not reuse it but create a new one. Here`s 
>> > excerpt from the docs[1]:
>> >
>> >   --no-ff
>> > With --interactive, cherry-pick all rebased commits instead of 
>> > fast-forwarding over the unchanged ones. This ensures that the 
>> > entire history of the rebased branch is composed of new commits.
>> >
>> > Without --interactive, this is a synonym for --force-rebase.
>> >
>> >
>> > So fast-forwarding in case of rebasing (merge commits as well) is 
>> > something you would want by default, as it wouldn`t drop/lose 
>> > anything, but merely reuse existing commit (if unchanged), instead of 
>> > cherry-picking (rebasing) it into a new (merge) commit anyway.
>> 
>> This sounds like breakage. E.g., it seems to be breaking every "-x ours"
>> merge out there.
>
> Either you are not understanding how rebase fast-forward works, or 
> I`m missing what you are pointing to... Mind explaining how can 
> something that`s left unchanged suddenly become a breakage?

It was misunderstanding on my side indeed, sorry.

>
>> Fast-forwarding existing merge, one way or another, still seems to be
>> wrong idea to me, as merge commit is not only about content change, but
>> also about joint point at particular place in the DAG.
>
> Not sure what this has to do with rebase fast-forwarding, either - 
> nothing changes for fast-forwarded (merge or non-merge) commit in 
> question, both content, joint point and everything else stays exactly 
> the same. If anything changed, then it can`t/won`t be fast-forwarded, 
> being unchanged is a prerequisite.
>
> Let me elaborate a bit. Here`s a starting diagram:

[... detailed explanation skipped for brevity ...]

> Does this settle your concerns, or I`m missing something?

Yes, it does, thank you! Leaving as many leading commits as possible
unchanged during rebase is what fast-forward mean in this case then, and
it's pretty OK with me.

>> As for fast-forwarding re-merge, explicitly requested, I'm not sure. On
>> one hand, it's inline with the default "git merge" behavior, on the
>> other hand, it still feels wrong, somehow.
>
> Regarding fast-forwarding in context of merging, in case where we are 
> recreating merges (not rebasing them), following existing `git merge` 
> logic might make sense, where I would expect rebasing todo list `merge` 
> command to pick-up tricks from `git merge` as needed, like learning 
> to accept `--no-ff` option, for example, thus not fast-forwarding 
> merges (on request) even when possible.
>
> Though, I do agree that in case you want to recreate an existing merge 
> (instead of just rebasing it), `merge` command fast-forwarding might 
> probably not be what you want for the most of the time, but I`m afraid 
> having rebase todo list `merge` command default behavior different than 
> `git merge` default one (in regards to fast-forwarding) would be 
> confusing... or not?
>
> From what I could grasp so far, usually Git commands` default 
> behavior is (explained to be) chosen per "most common use case", so 
> might be non fast-forwarding would be fine as default for rebase todo 
> list `merge` command, even though different than 

Re: [PATCH] mingw: abort on invalid strftime formats

[Johannes Schindelin]

Hi Junio,

On Mon, 19 Mar 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Windows, strftime() does not silently ignore invalid formats, but
> > warns about them and then returns 0 and sets errno to EINVAL.
> >
> > Unfortunately, Git does not expect such a behavior, as it disagrees
> > with strftime()'s semantics on Linux. As a consequence, Git
> > misinterprets the return value 0 as "I need more space" and grows the
> > buffer. As the larger buffer does not fix the format, the buffer grows
> > and grows and grows until we are out of memory and abort.
> 
> Yuck, it is bad that the callers assume 0 is always "need more space",
> as "If a conversion specification does not correspond to any of the
> above, the behavior is undefined." is what we get from POSIX.

Yeah, well, our own rules state that we are sometimes stricter than POSIX
when it is pragmatic. This is one of those cases, methinks.

> > So let's just bite the bullet and override strftime() for MINGW and
> > abort on an invalid format string. While this does not provide the
> > best user experience, it is the best we can do.
> 
> As long as we allow arbitrary end-user input passed to strftime(), this
> is probably a step in the right direction.  
> 
> As to the future direction, I however wonder if we can return an error
> from here and make the caller cooperate a bit more.  Of course,
> implementation of strftime() that silently ignore invalid formats would
> not be able to return correct errors like an updated mingw_strftime()
> that does not die but communicates error to its caller would, though.

And I would not know what the caller should do in that case, quite
honestly... Would strftime() somehow tell us *which* placeholder it took
offense with? And would we "quote" that?

> My "git grep" is hitting only one caller of strftime(), which is
> strbuf_addftime(), which already does some magic to the format
> string, so such a future enhancement may not be _so_ bad.

Right, except that I do not think I could get the exact error condition
necessary to know *how* to munge the format further, not unless I invest a
serious amount of work in it ;-)

> Will apply, thanks.  I do not think there is no reason to cook this
> in 'next', and would assume this can instead go directly to 'master'
> to be part of v2.17-rc1 and onward, right?

Thanks. Given that this patch has been in Git for Windows for quite a
while without problems, I think it is safe to get it directly into
`master`. FWIW this late in the -rc phase, I would be very reluctant to
send anything I deem unworthy of `master` anyway.

Ciao,
Dscho

[PATCH v2 3/3] rebase --keep-empty: always use interactive rebase

[Phillip Wood]

From: Phillip Wood 

rebase --merge accepts --keep-empty but just ignores it, by using an
implicit interactive rebase the user still gets the rename detection
of a merge based rebase but with with --keep-empty support.

If rebase --keep-empty without --interactive or --merge stops for the
user to resolve merge conflicts then 'git rebase --continue' will
fail. This is because it uses a different code path that does not
create $git_dir/rebase-apply. As rebase --keep-empty was implemented
using cherry-pick it has never supported the am options and now that
interactive rebases support --signoff there is no loss of
functionality by using an implicit interactive rebase.

Signed-off-by: Phillip Wood 
---
 git-rebase--am.sh | 79 ---
 git-rebase.sh |  5 +++
 t/t3421-rebase-topology-linear.sh |  4 +-
 3 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index be3f068922..ae596d3731 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -41,60 +41,47 @@ else
 fi
 
 ret=0
-if test -n "$keep_empty"
-then
-   # we have to do this the hard way.  git format-patch completely squashes
-   # empty commits and even if it didn't the format doesn't really lend
-   # itself well to recording empty patches.  fortunately, cherry-pick
-   # makes this easy
-   git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
-   $allow_rerere_autoupdate --right-only "$revisions" \
-   $allow_empty_message \
-   ${restrict_revision+^$restrict_revision}
-   ret=$?
-else
-   rm -f "$GIT_DIR/rebased-patches"
+rm -f "$GIT_DIR/rebased-patches"
 
-   git format-patch -k --stdout --full-index --cherry-pick --right-only \
-   --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
-   --pretty=mboxrd \
-   $git_format_patch_opt \
-   "$revisions" ${restrict_revision+^$restrict_revision} \
-   >"$GIT_DIR/rebased-patches"
-   ret=$?
+git format-patch -k --stdout --full-index --cherry-pick --right-only \
+   --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
+   --pretty=mboxrd \
+   $git_format_patch_opt \
+   "$revisions" ${restrict_revision+^$restrict_revision} \
+   >"$GIT_DIR/rebased-patches"
+ret=$?
 
-   if test 0 != $ret
-   then
-   rm -f "$GIT_DIR/rebased-patches"
-   case "$head_name" in
-   refs/heads/*)
-   git checkout -q "$head_name"
-   ;;
-   *)
-   git checkout -q "$orig_head"
-   ;;
-   esac
+if test 0 != $ret
+then
+   rm -f "$GIT_DIR/rebased-patches"
+   case "$head_name" in
+   refs/heads/*)
+   git checkout -q "$head_name"
+   ;;
+   *)
+   git checkout -q "$orig_head"
+   ;;
+   esac
 
-   cat >&2 <<-EOF
+   cat >&2 <<-EOF
 
-   git encountered an error while preparing the patches to replay
-   these revisions:
+   git encountered an error while preparing the patches to replay
+   these revisions:
 
-   $revisions
+   $revisions
 
-   As a result, git cannot rebase them.
-   EOF
-   return $ret
-   fi
+   As a result, git cannot rebase them.
+   EOF
+   return $ret
+fi
 
-   git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" \
-   --patch-format=mboxrd \
-   $allow_rerere_autoupdate \
-   ${gpg_sign_opt:+"$gpg_sign_opt"} <"$GIT_DIR/rebased-patches"
-   ret=$?
+git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" \
+   --patch-format=mboxrd \
+   $allow_rerere_autoupdate \
+   ${gpg_sign_opt:+"$gpg_sign_opt"} <"$GIT_DIR/rebased-patches"
+ret=$?
 
-   rm -f "$GIT_DIR/rebased-patches"
-fi
+rm -f "$GIT_DIR/rebased-patches"
 
 if test 0 != $ret
 then
diff --git a/git-rebase.sh b/git-rebase.sh
index 7d3bb9d443..ad1a3bd3ef 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -459,6 +459,11 @@ then
test -z "$interactive_rebase" && interactive_rebase=implied
 fi
 
+if test -n "$keep_empty"
+then
+   test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
 if test -n "$interactive_rebase"
 then
type=interactive
diff --git a/t/t3421-rebase-topology-linear.sh 
b/t/t3421-rebase-topology-linear.sh
index 68fe2003ef..13e5c1a2f1 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -199,7 +199,7 @@ test_run_rebase () {
"
 }
 test_run_rebase success ''
-test_run_rebase failure -m
+test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase failure -p
 
@@ -214,7 +214,7 @@ test_run_rebase () {
"
 }

[PATCH v2 1/3] rebase: extend --signoff support

[Phillip Wood]

From: Phillip Wood 

Allow --signoff to be used with --interactive and --merge. In
interactive mode only commits marked to be picked, edited or reworded
will be signed off.

The main motivation for this patch was to allow one to run 'git rebase
--exec "make check" --signoff' which is useful when preparing a patch
series for publication and is more convenient than doing the signoff
with another --exec command.

This change also allows --root without --onto to work with --signoff
as well (--root with --onto was already supported).

Signed-off-by: Phillip Wood 
---

Notes:
changes since v1:
 - added support for --signoff with --interactive and --merge
 - split out the --preserve-merges test into the next commit
 - updated the documentation for --signoff
 - reworded commit message to reflect above changes

 Documentation/git-rebase.txt |  7 ---
 git-rebase--interactive.sh   |  6 +++---
 git-rebase--merge.sh |  2 +-
 git-rebase.sh| 20 +++-
 sequencer.c  |  8 +++-
 t/t3428-rebase-signoff.sh| 38 ++
 6 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3277ca1432..dd852068b1 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -364,9 +364,10 @@ default is `--no-fork-point`, otherwise the default is 
`--fork-point`.
Incompatible with the --interactive option.
 
 --signoff::
-   This flag is passed to 'git am' to sign off all the rebased
-   commits (see linkgit:git-am[1]). Incompatible with the
-   --interactive option.
+   Add a Signed-off-by: trailer to all the rebased commits. Note
+   that if `--interactive` is given then only commits marked to be
+   picked, edited or reworded will have the trailer added. Incompatible
+   with the `--preserve-merges` option.
 
 -i::
 --interactive::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 331c8dfeac..104de328ee 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -285,7 +285,7 @@ pick_one () {
pick_one_preserving_merges "$@" && return
output eval git cherry-pick $allow_rerere_autoupdate 
$allow_empty_message \
${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")} \
-   "$strategy_args" $empty_args $ff "$@"
+   $signoff "$strategy_args" $empty_args $ff "$@"
 
# If cherry-pick dies it leaves the to-be-picked commit unrecorded. 
Reschedule
# previous task so this commit is not lost.
@@ -527,10 +527,10 @@ do_pick () {
# resolve before manually running git commit --amend then git
# rebase --continue.
git commit --allow-empty --allow-empty-message --amend \
-  --no-post-rewrite -n -q -C $sha1 &&
+  --no-post-rewrite -n -q -C $sha1 $signoff &&
pick_one -n $sha1 &&
git commit --allow-empty --allow-empty-message \
-  --amend --no-post-rewrite -n -q -C $sha1 \
+  --amend --no-post-rewrite -n -q -C $sha1 
$signoff \
   ${gpg_sign_opt:+"$gpg_sign_opt"} ||
   die_with_patch $sha1 "$(eval_gettext "Could 
not apply \$sha1... \$rest")"
else
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index ceb715453c..368b63671d 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -28,7 +28,7 @@ continue_merge () {
if ! git diff-index --quiet --ignore-submodules HEAD --
then
if ! git commit ${gpg_sign_opt:+"$gpg_sign_opt"} 
$allow_empty_message \
-   --no-verify -C "$cmt"
+   $signoff --no-verify -C "$cmt"
then
echo "Commit failed, please do not call \"git commit\""
echo "directly, but instead do one of the following: "
diff --git a/git-rebase.sh b/git-rebase.sh
index a1f6e5de6a..5d854657a7 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -92,6 +92,7 @@ preserve_merges=
 autosquash=
 keep_empty=
 allow_empty_message=
+signoff=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 case "$(git config --bool commit.gpgsign)" in
 true)  gpg_sign_opt=-S ;;
@@ -121,6 +122,10 @@ read_basic_state () {
allow_rerere_autoupdate="$(cat 
"$state_dir"/allow_rerere_autoupdate)"
test -f "$state_dir"/gpg_sign_opt &&
gpg_sign_opt="$(cat "$state_dir"/gpg_sign_opt)"
+   test -f "$state_dir"/signoff && {
+   signoff="$(cat "$state_dir"/signoff)"
+   force_rebase=t
+   }
 }
 
 write_basic_state () {
@@ -135,6 +140,7 @@ 

[PATCH v2 2/3] rebase -p: error out if --signoff is given

[Phillip Wood]

From: Phillip Wood 

rebase --preserve-merges does not support --signoff so error out
rather than just silently ignoring it so that the user knows the
commits will not be signed off.

Signed-off-by: Phillip Wood 
---
 git-rebase.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index 5d854657a7..7d3bb9d443 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -479,6 +479,8 @@ fi
 
 if test -n "$signoff"
 then
+   test -n "$preserve_merges" &&
+   die "$(gettext "error: cannot combine '--signoff' with 
'--preserve-merges'")"
git_am_opt="$git_am_opt $signoff"
force_rebase=t
 fi
-- 
2.16.2

[PATCH v2 0/3] rebase: extend --signoff support

[Phillip Wood]

From: Phillip Wood 

Thanks for the feedback on v1, based on that I've added support for
--interacive --signoff and --merge --signoff. I've split this series
so the fixes from patch two in the last verion are now in a separate
series [1]. The tests in this version depends on those fixes. The
third patch is new.

[1] 
https://public-inbox.org/git/20180320100315.15261-1-phillip.w...@talktalk.net/T/#t

Phillip Wood (3):
  rebase: extend --signoff support
  rebase -p: error out if --signoff is given
  rebase --keep-empty: always use interactive rebase

 Documentation/git-rebase.txt  |  7 ++--
 git-rebase--am.sh | 79 ---
 git-rebase--interactive.sh|  6 +--
 git-rebase--merge.sh  |  2 +-
 git-rebase.sh | 27 -
 sequencer.c   |  8 +++-
 t/t3421-rebase-topology-linear.sh |  4 +-
 t/t3428-rebase-signoff.sh | 38 +++
 8 files changed, 114 insertions(+), 57 deletions(-)

-- 
2.16.2

Re: [PATCH] filter-branch: use printf instead of echo -e

[Michele Locati]

Il 20/03/2018 10:53, Ian Campbell ha scritto:

On Tue, 2018-03-20 at 00:22 -0400, Jeff King wrote:

Author cc'd in case there's something more interesting going on.


That code was written years ago, if I had a good reason at the time
I've forgotten what it was and I can't think of a fresh one now.
Switching to printf seems like a reasonable thing to do.

Perhaps switch the remaining `/bin/echo` (there are two without `-e`)
uses to just `echo` for consistency with the rest of the file?

Ian.



Thanks for reply, Ian.
Charles already suggested to replace /bin/echo with echo, and I already 
updated the patch accordingly: see

https://marc.info/?l=git=152147479517707=2
Junio already queued that PATCH-v2.

--
Michele

Re: [PATCH v5 0/3] stash push -u -- fixes

[Marc Strapetz]

On 20.03.2018 00:21, Thomas Gummerer wrote:

Thanks again Marc for all the testing and Junio for fixing up my brown
paper bag copy-pasto.

This iteration addresses the breakage Marc noticed with the latest
version of the patches, adds some more tests, and moves all the new
tests to t3905 instead of t3903, which I just noticed exists and is a
much better match for the new tests.

Patch 1 and 3 are the same as in the previous round, Patch 2 is mostly
rewritten.  Instead of trying to avoid part of the pipeline we're
using to get rid of changes, we now are getting rid of the 'git clean'
call in the pathspec case, and use the existing pipeline to get rid of
changes in untracked files as well.  I'm not adding an interdiff,
because Patch 2 is mostly rewritten and the other two are unchanged,
so it is probably easiest to just review patch 2.


Thanks, Thomas. All of my manual tests have been working fine now.

-Marc

[PATCH 2/2] rebase --recreate-merges --keep-empty: don't prune empty commits

[Phillip Wood]

From: Phillip Wood 

If there are empty commits on the left hand side of $upstream...HEAD
then the empty commits on the right hand side that we want to keep are
pruned because they are marked as cherry-picks. Fix this by keeping
the commits that are empty or are not marked as cherry-picks.

Signed-off-by: Phillip Wood 
---
 sequencer.c   | 30 --
 t/t3421-rebase-topology-linear.sh |  4 ++--
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d8cc63dbe4..da87c390ed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2975,14 +2975,15 @@ static int make_script_with_merges(struct 
pretty_print_context *pp,
 */
while ((commit = get_revision(revs))) {
struct commit_list *to_merge;
-   int is_octopus;
+   int is_octopus, is_empty;
const char *p1, *p2;
struct object_id *oid;
 
tail = _list_insert(commit, tail)->next;
oidset_insert(, >object.oid);
 
-   if ((commit->object.flags & PATCHSAME))
+   is_empty = is_original_commit_empty(commit);
+   if (!is_empty && (commit->object.flags & PATCHSAME))
continue;
 
strbuf_reset();
@@ -2992,7 +2993,7 @@ static int make_script_with_merges(struct 
pretty_print_context *pp,
if (!to_merge) {
/* non-merge commit: easy case */
strbuf_reset();
-   if (!keep_empty && is_original_commit_empty(commit))
+   if (!keep_empty && is_empty)
strbuf_addf(, "%c ", comment_line_char);
strbuf_addf(, "%s %s %s", cmd_pick,
oid_to_hex(>object.oid),
@@ -3172,12 +3173,9 @@ int sequencer_make_script(FILE *out, int argc, const 
char **argv,
 
init_revisions(, NULL);
revs.verbose_header = 1;
-   if (recreate_merges)
-   revs.cherry_mark = 1;
-   else {
+   if (!recreate_merges)
revs.max_parents = 1;
-   revs.cherry_pick = 1;
-   }
+   revs.cherry_mark = 1;
revs.limited = 1;
revs.reverse = 1;
revs.right_only = 1;
@@ -3205,14 +3203,18 @@ int sequencer_make_script(FILE *out, int argc, const 
char **argv,
return make_script_with_merges(, , out, flags);
 
while ((commit = get_revision())) {
+   int is_empty  = is_original_commit_empty(commit);
+
strbuf_reset();
-   if (!keep_empty && is_original_commit_empty(commit))
+   if (!keep_empty && is_empty)
strbuf_addf(, "%c ", comment_line_char);
-   strbuf_addf(, "%s %s ", insn,
-   oid_to_hex(>object.oid));
-   pretty_print_commit(, commit, );
-   strbuf_addch(, '\n');
-   fputs(buf.buf, out);
+   if (is_empty || !(commit->object.flags & PATCHSAME)) {
+   strbuf_addf(, "%s %s ", insn,
+   oid_to_hex(>object.oid));
+   pretty_print_commit(, commit, );
+   strbuf_addch(, '\n');
+   fputs(buf.buf, out);
+   }
}
strbuf_release();
return 0;
diff --git a/t/t3421-rebase-topology-linear.sh 
b/t/t3421-rebase-topology-linear.sh
index cb7f176f1d..7384010075 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -215,9 +215,9 @@ test_run_rebase () {
 }
 test_run_rebase success ''
 test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success -i
 test_run_rebase failure -p
-test_run_rebase failure --recreate-merges
+test_run_rebase success --recreate-merges
 
 #   m
 #  /
-- 
2.16.2

[PATCH 1/2] add failing test for rebase --recreate-merges --keep-empty

[Phillip Wood]

From: Phillip Wood 

If there are empty commits on the left hand side of $upstream...HEAD
then the empty commits on the right hand side that we want to keep are
being pruned. This will be fixed in the next commit.

Signed-off-by: Phillip Wood 
---
 t/t3421-rebase-topology-linear.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3421-rebase-topology-linear.sh 
b/t/t3421-rebase-topology-linear.sh
index 68fe2003ef..cb7f176f1d 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -217,6 +217,7 @@ test_run_rebase success ''
 test_run_rebase failure -m
 test_run_rebase failure -i
 test_run_rebase failure -p
+test_run_rebase failure --recreate-merges
 
 #   m
 #  /
-- 
2.16.2

[PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Phillip Wood]

From: Phillip Wood 

These patches apply on top of js/rebase-recreate-merge. They extend
the --keep-empty fix from maint [1] to work with --recreate-merges.

[1] 
https://public-inbox.org/git/20180320100315.15261-3-phillip.w...@talktalk.net/T/#u

Phillip Wood (2):
  add failing test for rebase --recreate-merges --keep-empty
  rebase --recreate-merges --keep-empty: don't prune empty commits

 sequencer.c   | 30 --
 t/t3421-rebase-topology-linear.sh |  3 ++-
 2 files changed, 18 insertions(+), 15 deletions(-)

-- 
2.16.2

[PATCH 0/3] rebase --keep-empty/--root fixes

[Phillip Wood]

From: Phillip Wood 

These patches are based on top of maint. The first two are a reworking
of "[PATCH 2/2] rebase --root -k: fix when root commit is empty" [1]

[1] 
https://public-inbox.org/git/2018031427.14217-2-phillip.w...@talktalk.net/

Phillip Wood (3):
  rebase --root: stop assuming squash_onto is unset
  rebase -i --keep-empty: don't prune empty commits
  rebase: respect --no-keep-empty

 git-rebase.sh |  4 
 sequencer.c   | 18 +++---
 t/t3421-rebase-topology-linear.sh |  2 +-
 3 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.16.2

[PATCH 3/3] rebase: respect --no-keep-empty

[Phillip Wood]

From: Phillip Wood 

$OPT_SPEC has always allowed --no-keep-empty so lets start handling
it.

Signed-off-by: Phillip Wood 
---
 git-rebase.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index 8b1b892d72..37b8f13971 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -263,6 +263,9 @@ do
--keep-empty)
keep_empty=yes
;;
+   --no-keep-empty)
+   keep_empty=
+   ;;
--preserve-merges)
preserve_merges=t
test -z "$interactive_rebase" && interactive_rebase=implied
-- 
2.16.2

[PATCH 2/3] rebase -i --keep-empty: don't prune empty commits

[Phillip Wood]

From: Phillip Wood 

If there are empty commits on the left hand side of $upstream...HEAD
then the empty commits on the right hand side that we want to keep are
pruned by --cherry-pick. Fix this by using --cherry-mark instead of
--cherry-pick and keeping the commits that are empty or are not marked
as cherry-picks.

Signed-off-by: Phillip Wood 
---
 sequencer.c   | 18 +++---
 t/t3421-rebase-topology-linear.sh |  2 +-
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4d3f60594c..96ff812c8d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2470,7 +2470,7 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
init_revisions(, NULL);
revs.verbose_header = 1;
revs.max_parents = 1;
-   revs.cherry_pick = 1;
+   revs.cherry_mark = 1;
revs.limited = 1;
revs.reverse = 1;
revs.right_only = 1;
@@ -2495,14 +2495,18 @@ int sequencer_make_script(FILE *out, int argc, const 
char **argv,
return error(_("make_script: error preparing revisions"));
 
while ((commit = get_revision())) {
+   int is_empty  = is_original_commit_empty(commit);
+
strbuf_reset();
-   if (!keep_empty && is_original_commit_empty(commit))
+   if (!keep_empty && is_empty)
strbuf_addf(, "%c ", comment_line_char);
-   strbuf_addf(, "%s %s ", insn,
-   oid_to_hex(>object.oid));
-   pretty_print_commit(, commit, );
-   strbuf_addch(, '\n');
-   fputs(buf.buf, out);
+   if (is_empty || !(commit->object.flags & PATCHSAME)) {
+   strbuf_addf(, "%s %s ", insn,
+   oid_to_hex(>object.oid));
+   pretty_print_commit(, commit, );
+   strbuf_addch(, '\n');
+   fputs(buf.buf, out);
+   }
}
strbuf_release();
return 0;
diff --git a/t/t3421-rebase-topology-linear.sh 
b/t/t3421-rebase-topology-linear.sh
index 68fe2003ef..52fc6885e5 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -215,7 +215,7 @@ test_run_rebase () {
 }
 test_run_rebase success ''
 test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success -i
 test_run_rebase failure -p
 
 #   m
-- 
2.16.2

[PATCH 1/3] rebase --root: stop assuming squash_onto is unset

[Phillip Wood]

From: Phillip Wood 

If the user set the environment variable 'squash_onto', the 'rebase'
command would erroneously assume that the user passed the option
'--root'.

Signed-off-by: Phillip Wood 
---
 git-rebase.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index fd72a35c65..8b1b892d72 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -60,6 +60,7 @@ $(gettext 'Resolve all conflicts manually, mark them as 
resolved with
 You can instead skip this commit: run "git rebase --skip".
 To abort and get back to the state before "git rebase", run "git rebase 
--abort".')
 "
+squash_onto=
 unset onto
 unset restrict_revision
 cmd=
-- 
2.16.2

Re: [BUG] log --graph corrupts patch

[Phillip Wood]

On 20/03/18 06:09, Jeff King wrote:
> On Mon, Mar 19, 2018 at 10:21:56AM +, Phillip Wood wrote:
> 
>> I've just been reviewing some patches with 'git log --graph --patch' and
>> came across what looked like a bug:
>>
>> | @@ -272,6 +272,9 @@ do
>> |   --keep-empty)
>> |   keep_empty=yes
>> |   ;;
>> |   --allow-empty-message)
>> | + --no-keep-empty)
>> | + keep_empty=
>> | + ;;
>> |   allow_empty_message=--allow-empty-message
>> |   ;;
>>
>> However when I looked at the file it was fine, "--allow-empty-message)"
>> was actually below the insertions. 'git log --patch' gives the correct
>> patch:
>> [...]
>> git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch
> 
> That's really strange. I can't seem to replicate it here, though; it
> looks correct with our without --graph. Knowing how the graph code is
> implemented, it seems like an unlikely bug for us to output lines out of
> order (but of course anything's possible).
> 
> Are you using any exotic filters for your pager? If you use "git
> --no-pager" does the problem persist?

Hi Peff, thanks for taking the time to check this, I had forgotten about
the pager. I'm using diff-highlight and it seems that is causing the
problems.

Thanks again

Phillip

> -Peff
> 

Re: [PATCH] filter-branch: use printf instead of echo -e

[Ian Campbell]

On Tue, 2018-03-20 at 00:22 -0400, Jeff King wrote:
> Author cc'd in case there's something more interesting going on.

That code was written years ago, if I had a good reason at the time
I've forgotten what it was and I can't think of a fresh one now.
Switching to printf seems like a reasonable thing to do.

Perhaps switch the remaining `/bin/echo` (there are two without `-e`)
uses to just `echo` for consistency with the rest of the file?

Ian.

Re: [RFC] [GSoC] Project proposal: convert scripts to builtins

[Pratik Karki]

Hi,
This is my draft for my proposal on "Convert Scripts to builtins" for GSoC.
Please review and provide feedback.


Cheers,
Pratik Karki


Convert Scripts to builtins
===

Abstract


Many components of Git are still in the form of shell and Perl scripts.
This has certain advantages of being extensible but causes problems in
production code on multiple platforms like Windows.
I propose to rewrite a couple of shell and perl scripts into portable
and performant C code, making them built-ins. The major advantage of
doing this is improvement in efficiency and performance.

Much more scripts like `git-am` , `git-pull`, `git-branch` have already
been rewritten in C. Much more scripts like `git-rebase`, `git-stash`,
`git-add --interactive` are still present in shell and perl scripts.
I propose to work in `git-rebase` and `git-stash`.

Shell Scripts:
--

Although shell scripts are more faster can be extensible in
functionality and can be more easier to write, they introduce certain
disadvantages.

1.  Dependencies:
 The scripting languages and shell scripts make more productive code
but there is an overhead of dependencies. The shell scripts are
lighter and simpler and call other executables to perform
non-trivial tasks. Taking `git-stash` shell script for example.
`sed`, `rm`, `echo`, `test` are constantly present in `git-stash`.
These look common to POSIX platforms but for non-POSIX platforms
there needs some extra work for porting these commands. For example,
in Git for Windows, the workaround for these commands in non-POSIX
platform adds some extra utilities and adds MSYS2 shell commands and
needs to pack language runtime for Perl. This increases the
installation size and requires more disk space. Again, adding more
batteries again needs implementation in all of the dependency
libraries and executables.

2.  Inefficiency:
 Git has internal caches for configuration values, the repository
index and repository objects. The porcelain commands do not have
access to git's internal API and so they spawn git processes to
perform git operations. For every git invocation, git would re-read
the user's configuration files, repository index, repopulate the
filesystem cache, etc. This leads to overhead and unnecessary I/O.
Windows is known to have worse I/O performance compared to Linux.
There is also slower I/O performance of HDD compared to SSD. This
unnecessary I/O operations causes runtime overhead and becomes
slower in poor I/O performance setups. Now, writing the porcelain
into C built-ins leverages the git API and there is no need of
spawning separate git processes, caching can be used to reduce
unnecessary I/O processes.

3.  Spawing processes is less performant:
 Shell scripts usually spawn a lot of processes. Shell scripts are
very lighter and hence have limited functionalites. For
`git-stash.sh` to work it needs to perform lots of git operations
like `git rev-parse` `git config` and thus spawns git executable
processes for performing these operations. Again for invoking
`git config` and providing configuration values, it spawn new
processes to handle that. Spawning is implemented by `fork()` and
`exec()` by shells. Now, on systems that do not support
copy-on-write semantics for `fork()`, there is duplication of the
memory of the parent process for every `fork()` call which turns out
to be an expensive process. Now, in Windows, Git uses MSYS2
exclusively to emulate `fork()` but since, Windows doesnot support
forking semantics natively, the workaround provided by MSYS2
emulates `fork()` without [copy-on-write
semantics](https://www.cygwin.com/faq.html#faq.api.fork). Doing this
creates another layer over Windows processes and thus slows git.

Rewriting C built-ins
-

These above mentioned problems need to be fixed. The only fix for these
problems would be to write built-ins in C for all these shell scripts
leveraging the git API. Writing in built-in reduces the dependency
required by shell scripts. Since, Git is native executable in Windows,
doing this can make MSYS2 POSIX emulation obsolete. Then, using git's
internal API and C data types, built-in `git_config_get_value()` can be
used to get configuration value rather than spawning another git-config
process. This removes the necessary to re-read git configuration cache
everytime and reduces I/O. Furthermore, git-stash and git-rebase will be
more faster and show consistent behaviour as instead of spawing another
process and parsing command-line arguments manually, they can be
hardcoded to be built-in and leverage all the required git's internal
API's like `parse-options`.

To implement git-stash and git-rebase in C, I propose to avoid spawning
lots of external git processes and reduce redundant I/O by taking
advantage of the internal 

Re: [RFC] [GSoC] Project proposal: convert scripts to builtins

[Christian Couder]

Hi,

On Tue, Mar 20, 2018 at 10:00 AM, Pratik Karki  wrote:
> Hi,
> This is my draft for my proposal on "Convert Scripts to builtin" for GSoC.
> Please review and provide feedbacks.
>
> https://gist.github.com/prertik/daaa73a39d3ce30811d9a208043dc235

It would be easier for us to comment if the markdown was sent inline.

Thanks,
Christian.

FROM Mr USMAN ABU

[usman abu]

FROM Mr USMAN  ABU
BILLS AND EXCHANGE MANAGER,
In BANK OF AFRICA (B.O.A)
Ouagadougou Burkina Faso, IN WEST AFRCA


Please i want you to read this later very carefully and please do not
joke with this message. I am Mr USMAN  AU, the bill and exchange
manager in the boa bank of Africa Ouagadougou Burkina Faso.

In my department, I found the deposited fund amounted ($18.5 million)
one of my bank clients who died Along with his entire family in air
crash, On July 31 2000 please go through the
website.http://news.bbc.co.uk/2/hi/europe/859479.stm

Can you be able and capable to assist me provide your bank account
where this $18.5 million will transfer into? Because I don’t want the
money to go into our bank treasury account as an unclaimed fund, so
this is the reason why I contacted you so that our bank will release
this money to you as the nest of kin to the deceased customer. Please
I would like you to keep this proposal as top secret.

I shall give you 40% of the total fund as soon as this $18.5 million
transferred into your bank account and I shall visit you in your
country for the sharing of the funds.
>From banking experience it will take up to ( 7 ) working days to
conclude this transfer. I want you to also know that this transaction
will involve a little expense which will be shared among both of us.

Your Urgent response is needed immediately to enable me to send to you
application later which you have to fill and apply to the bank for the
transfer of the $18.5 million USA dollars. Before I send to you the
application later, you have to Fill this form bellow and resend it
back to me immediately for more trust.
Your Name
Your home addresses 
Your country 
Your city 
Your home telephone 
Your private telephone 
Your age 
Your occupation 
Your religion…
Your international passport or ID card 

Thanks and my regards to your family.
Mr USMAN  ABU

[RFC] [GSoC] Project proposal: convert scripts to builtins

[Pratik Karki]

Hi,
This is my draft for my proposal on "Convert Scripts to builtin" for GSoC.
Please review and provide feedbacks.

https://gist.github.com/prertik/daaa73a39d3ce30811d9a208043dc235

Cheers,
Pratik Karki

[no subject]

[Plurality Diversity]

my name is Mrs. Alice Walton, I have a mission for you  which I intend using 
for CHARITY email me: ( i...@alicewonders.us ) for more details

Re: [PATCH v4 4/4] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  wrote:
> [...]
> However we can do a little better than that, and check the branch out if
> it is not checked out anywhere else.  This will help users who just want
> to check an existing branch out into a new worktree, and save a few
> keystrokes.
> [...]
> We will still 'die()' if the branch is checked out in another worktree,
> unless the --force flag is passed.
>
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -61,8 +61,13 @@ $ git worktree add --track -b   
> /
>  If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
> -then, as a convenience, a new branch based at HEAD is created automatically,
> -as if `-b $(basename )` was specified.
> +then, as a convenience, a worktree with a branch named after
> +`$(basename )` (call it ``) is created.  If ``
> +doesn't exist, a new branch based on HEAD is automatically created as
> +if `-b ` was given.  If `` exists in the repository,
> +it will be checked out in the new worktree, if it's not checked out
> +anywhere else, otherwise the command will refuse to create the
> +worktree.

Should this mention --force?

... refuse to create the worktree (unless --force is used).

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -29,6 +29,7 @@ struct add_opts {
> int keep_locked;
> const char *new_branch;
> int force_new_branch;
> +   int checkout_existing_branch;

As 'force_new_branch' and 'checkout_existing_branch' are mutually
exclusive, I wonder if they should be combined into an enum to make it
easier to reason about.

> @@ -318,8 +319,11 @@ static int add_worktree(const char *path, const char 
> *refname,
> -   if (opts->new_branch)
> -   fprintf(stderr, _("creating new branch '%s'"), 
> opts->new_branch);
> +   if (opts->checkout_existing_branch)
> +   fprintf(stderr, _("checking out branch '%s'"),
> +   refname);

As with "creating branch" in 2/4, "checking out branch..." here is
missing a newline.

> +   else if (opts->new_branch)
> +   fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
>
> fprintf(stderr, _("worktree HEAD is now at %s"),
> find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -198,13 +198,22 @@ test_expect_success '"add" with  omitted' '
> -test_expect_success '"add" auto-vivify does not clobber existing branch' '
> +test_expect_success '"add" auto-vivify checks out existing branch' '
> test_commit c1 &&
> test_commit c2 &&
> git branch precious HEAD~1 &&
> -   test_must_fail git worktree add precious &&
> +   git worktree add precious &&
> test_cmp_rev HEAD~1 precious &&
> -   test_path_is_missing precious
> +   (
> +   cd precious &&
> +   test_cmp_rev precious HEAD
> +   )
> +'

This test is no longer checking auto-vivification ("bringing a new
branch to life automatically"), but rather branch name inference, so
the title is now misleading. Perhaps retitle it to '"add" checks out
existing branch of dwim'd name' or something.

(The name "precious" is also now misleading since it's no longer
checking that a precious branch does not get clobbered, however,
changing the name would make the diff unnecessarily noisy, so it's
probably okay as is.)

> +test_expect_success '"add" auto-vivify fails with checked out branch' '
> +   git checkout -b test-branch &&
> +   test_must_fail git worktree add test-branch &&
> +   test_path_is_missing test-branch
>  '

Should we also have a corresponding test that this "fail" can be
overridden by --force? (I realize that --force is tested elsewhere,
but only with an explicitly-provided branch name, whereas this dwims
the branch name.)

Re: [PATCH v4 2/4] worktree: be clearer when "add" dwim-ery kicks in

[Eric Sunshine]

On Tue, Mar 20, 2018 at 3:26 AM, Eric Sunshine  wrote:
> On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  wrote:
>> Currently there is no indication in the "git worktree add" output that
>> a new branch was created.  This would be especially useful information
>> in the case where the dwim of "git worktree add " kicks in, as the
>> user didn't explicitly ask for a new branch, but we create one from
>> them.
>>
>> Print some additional output showing that a branch was created and the
>> branch name to help the user.
>> [...]
>> Signed-off-by: Thomas Gummerer 
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char 
>> *refname,
>> +   if (opts->new_branch)
>> +   fprintf(stderr, _("creating new branch '%s'"), 
>> opts->new_branch);
>> +
>> fprintf(stderr, _("worktree HEAD is now at %s"),
>> find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
>
> The "creating" message is missing a newline, which results in rather
> ugly output:
>
> creating new branch 'foo'worktree HEAD is now at ...

Also, I believe that the agreement[1] was that this message should say
merely "creating branch", not "creating _new_ branch". And, indeed,
patch 4/4 stealthily drops "new" from the message, but it really ought
to be introduced with correct text in this patch, not fixed by 4/4.

[1]: https://public-inbox.org/git/xmqqh8qv9ojb@gitster-ct.c.googlers.com/

Re: [PATCH v4 2/4] worktree: be clearer when "add" dwim-ery kicks in

[Eric Sunshine]

On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  wrote:
> Currently there is no indication in the "git worktree add" output that
> a new branch was created.  This would be especially useful information
> in the case where the dwim of "git worktree add " kicks in, as the
> user didn't explicitly ask for a new branch, but we create one from
> them.
>
> Print some additional output showing that a branch was created and the
> branch name to help the user.
> [...]
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char 
> *refname,
> +   if (opts->new_branch)
> +   fprintf(stderr, _("creating new branch '%s'"), 
> opts->new_branch);
> +
> fprintf(stderr, _("worktree HEAD is now at %s"),
> find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

The "creating" message is missing a newline, which results in rather
ugly output:

creating new branch 'foo'worktree HEAD is now at ...

Re: [PATCH v4 2/4] worktree: be clearer when "add" dwim-ery kicks in

[Eric Sunshine]

On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  wrote:
> Currently there is no indication in the "git worktree add" output that
> a new branch was created.  This would be especially useful information
> in the case where the dwim of "git worktree add " kicks in, as the
> user didn't explicitly ask for a new branch, but we create one from
> them.
>
> Print some additional output showing that a branch was created and the
> branch name to help the user.

Good idea.

> This will also be useful in the next commit, which introduces a new kind
> of dwim-ery of checking out the branch if it exists instead of refusing
> to create a new worktree in that case, and where it's nice to tell the
> user which kind of dwim-ery kicked in.
>
> Signed-off-by: Thomas Gummerer 

Re: [PATCH v4 1/4] worktree: improve message when creating a new worktree

[Eric Sunshine]

On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer  wrote:
> [...]
> Fix these inconsistencies, and no longer show the identifier by making
> the 'git reset --hard' call quiet, and printing the message directly
> from the builtin command instead.
>
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -303,8 +303,6 @@ static int add_worktree(const char *path, const char 
> *refname,
> strbuf_addf(, "%s/commondir", sb_repo.buf);
> write_file(sb.buf, "../..");
>
> -   fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);

A minor regression with this change is that error messages from
git-update-ref or git-symbolic-ref -- which could be emitted after
this point but before the new "worktree HEAD is now at..." message --
are now somewhat orphaned. I'm not sure that it matters, though.

> argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, 
> sb_git.buf);
> argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, 
> path);
> cp.git_cmd = 1;
> @@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char 
> *refname,
> +   fprintf(stderr, _("worktree HEAD is now at %s"),
> +   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

I wonder if this should say "new worktree HEAD is now at..." to
clearly indicate that it is talking about HEAD in the _new_ worktree,
not HEAD in the current worktree.

> +   strbuf_reset();
> +   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> +   if (sb.len > 0)
> +   fprintf(stderr, " %s", sb.buf);
> +   fputc('\n', stderr);

Re: [BUG] log --graph corrupts patch

[Jeff King]

On Mon, Mar 19, 2018 at 10:21:56AM +, Phillip Wood wrote:

> I've just been reviewing some patches with 'git log --graph --patch' and
> came across what looked like a bug:
> 
> | @@ -272,6 +272,9 @@ do
> |   --keep-empty)
> |   keep_empty=yes
> |   ;;
> |   --allow-empty-message)
> | + --no-keep-empty)
> | + keep_empty=
> | + ;;
> |   allow_empty_message=--allow-empty-message
> |   ;;
> 
> However when I looked at the file it was fine, "--allow-empty-message)"
> was actually below the insertions. 'git log --patch' gives the correct
> patch:
> [...]
> git fetch https://github.com/phillipwood/git.git log-graph-breaks-patch

That's really strange. I can't seem to replicate it here, though; it
looks correct with our without --graph. Knowing how the graph code is
implemented, it seems like an unlikely bug for us to output lines out of
order (but of course anything's possible).

Are you using any exotic filters for your pager? If you use "git
--no-pager" does the problem persist?

-Peff