[PATCH] Makefile: put LIBS after LDFLAGS for imap-send

[Steven Penny]

This matches up with the targets git-%, git-http-fetch, git-http-push and
git-remote-testsvn. It must be done this way on Windows else lcrypto cannot find
lgdi32 and lws2_32

Signed-off-by: Steven Penny 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d861bd9..f0f65ea 100644
--- a/Makefile
+++ b/Makefile
@@ -2046,7 +2046,7 @@ git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
 
 git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-   $(LIBS) $(IMAP_SEND_LDFLAGS)
+   $(IMAP_SEND_LDFLAGS) $(LIBS)
 
 git-http-fetch$X: http.o http-walker.o http-fetch.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
-- 
2.8.3

Re: "git fsck" not detecting garbage at the end of blob object files...

[Jeff King]

On Sat, Jan 07, 2017 at 10:47:03PM +0100, Dennis Kaarsemaker wrote:

> On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:
> > I was perusing StackOverflow this morning and ran across this
> > question: 
> > http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
> > 
> > It was a simple question about why "checking objects" was not
> > appearing, but in it was another issue.  The user purposefully
> > corrupted a blob object file to see if `git fsck` would catch it by
> > tacking extra data on at the end.  `git fsck` happily said everything
> > was okay, but when I played with things locally I found out that `git
> > gc` does not like that extra garbage.  I'm not sure what the trade-off
> > needs to be here, but my expectation is that if `git fsck` says
> > everything is okay, then all operations using that object (file)
> > should work too.
> > 
> > Is that unreasonable?  What would be the impact of fixing this issue?
> 
> If you do this with a commit object or tree object, fsck does complain.
> I think it's sensible to do so for blob objects as well.

The existing extra-garbage check is in unpack_sha1_rest(), which is
called as part of read_sha1_file(). And that's what we hit for commits
and trees. However, we check the sha1 of blobs using the streaming
interface (in case they're large). I think you'd want to put a similar
check into read_istream_loose(). But note if you are grepping for it, it
is hidden behind a macro; look for read_method_decl(loose).

I'm actually not sure if this should be downgrade to a warning. It's
true that it's a form of corruption, but it doesn't actually prohibit us
from getting the data we need to complete the operation. Arguably fsck
should be more picky, but it is just relying on the same parse_object()
code path that the rest of git uses.

I doubt anybody cares too much either way, though. It's not like this is
a common thing.

I did notice another interesting case when looking at this. Fsck ends up
in fsck_loose(), which has the sha1 and path of the loose object. It
passes the sha1 to fsck_sha1(), and ignores the path entirely!

So if you have a duplicate copy of the object in a pack, we'd actually
find and check the duplicate. This can happen, e.g., if you had a loose
object and fetched a thin-pack which made a copy of the loose object to
complete the pack).

Probably fsck_loose() should be more picky about making sure we are
reading the data from the loose version we found.

-Peff

Re: [PATCH 0/3] fixing some random blame corner cases

[Junio C Hamano]

Jeff King  writes:

>   [1/3]: blame: fix alignment with --abbrev=40
>   [2/3]: blame: handle --no-abbrev
>   [3/3]: blame: output porcelain "previous" header for each file

Thanks. 1 & 2 obviously look correct.  I'd need to look at 3 when I
am not exhausted, even though I expect it to be also fine from a
cursory read.

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Jeff King]

On Sat, Jan 07, 2017 at 02:03:30PM -0800, Junio C Hamano wrote:

> Is that a longer way to say that the claim "... is designed as a
> book" is false?
> 
> > So I dunno. I really do think "article" is conceptually the most
> > appropriate style, but I agree that there are some book-like things
> > (like appendices).
> 
> ... Yeah, I should have read forward first before starting to insert
> my comments.

To be honest, I'm not sure whether "book" versus "article" was really
considered in the original writing. I think we can call it whichever
produces the output we find most pleasing. I was mostly just pointing at
there are some tradeoffs in the end result in flipping the type.

-Peff

Re: [PATCH] rebase--interactive: count squash commits above 10 correctly

[Junio C Hamano]

Jeff King  writes:

>  git-rebase--interactive.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index b0a6f2b7ba..4734094a3f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -425,7 +425,7 @@ update_squash_messages () {
>   if test -f "$squash_msg"; then
>   mv "$squash_msg" "$squash_msg".bak || exit
>   count=$(($(sed -n \
> - -e "1s/^$comment_char.*\([0-9][0-9]*\).*/\1/p" \
> + -e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \

I would have written it to match ".*[^1-9]\([1-9][0-9]*\)", though.
Even if a foreign language expresses all its words as digits (or has
a digit as the last letter of the last word before the number), a
translation into it must have a non-digit before the number to
disambiguate---but I guess I am being ultra-pedantic, and I think
what you wrote would be sufficient in practice.

As you guys discussed, this code will hopefully disappear in a cycle
or two anyway ;-)  Let's queue this as-is.

Thanks.

>   -e "q" < "$squash_msg".bak)+1))
>   {
>   printf '%s\n' "$comment_char $(eval_ngettext \

Re: [PATCH v5 0/5] road to reentrant real_path

[Junio C Hamano]

Brandon Williams  writes:

> changes in v5:
> * set errno to ELOOP when MAXSYMLINKS is exceded.
> * revert to use MAXSYMLINKS instead of MAXDEPTH.
> * If the OS hasn't defined MAXSYMLINKS, use a fallback value of 32.
>
> Brandon Williams (4):
>   real_path: resolve symlinks by hand
>   real_path: convert real_path_internal to strbuf_realpath
>   real_path: create real_pathdup
>   real_path: have callers use real_pathdup and strbuf_realpath
>
> Johannes Sixt (1):
>   real_path: canonicalize directory separators in root parts
>

How does this relate to the 5-patch real_path: series that has been
on 'next' since last year?

Re: [PATCH] diff: add interhunk context config option

[Junio C Hamano]

Pranit Bauva  writes:

>> diff --git a/diff.c b/diff.c
>> index 84dba60c4..40b4e6afe 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
>>  static int diff_suppress_blank_empty;
>>  static int diff_use_color_default = -1;
>>  static int diff_context_default = 3;
>> +static int diff_interhunk_context_default = 0;

Do not explicitly initialize BSS variables to 0 (or NULL).

>>  static const char *diff_word_regex_cfg;
>>  static const char *external_diff_cmd_cfg;
>>  static const char *diff_order_file_cfg;
>> @@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char 
>> *value, void *cb)
>> return -1;
>> return 0;
>> }
>> +   if (!strcmp(var, "diff.interhunkcontext")) {
>> +   diff_interhunk_context_default = git_config_int(var, value);
>> +   if (diff_interhunk_context_default < 0)
>> +   return -1;
>> +   return 0;
>> +   }
>> if (!strcmp(var, "diff.renames")) {
>> diff_detect_rename_default = git_config_rename(var, value);
>> return 0;
>> @@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
>> options->rename_limit = -1;
>> options->dirstat_permille = diff_dirstat_permille_default;
>> options->context = diff_context_default;
>> +   options->interhunkcontext = diff_interhunk_context_default;

Will this receive -1 if diff.interhunkcontext configuration variable
is misconfigured and the *_default variable receives the error return
value from git_config_int()?


>> options->ws_error_highlight = ws_error_highlight_default;
>> DIFF_OPT_SET(options, RENAME_EMPTY);
>
> On a first look, it seems that we can overwrite the default config
> values by using a different command line argument which is good.
>
> Also, tests are missing. It seems that t/t4032 might be a good place
> to add those tests.
>
> Rest all is quite good! :)
>
> Regards,
> Pranit Bauva

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Johannes Schindelin  writes:

> + if (ssh) {
> + char *split_ssh = xstrdup(ssh);
> + const char **ssh_argv;
> +
> + if (split_cmdline(split_ssh, _argv))
> + ssh_argv0 = xstrdup(ssh_argv[0]);
> + free(split_ssh);
> + free((void *)ssh_argv);
> + } else {
>   /*
>* GIT_SSH is the no-shell version of
>* GIT_SSH_COMMAND (and must remain so for
> @@ -807,8 +813,11 @@ struct child_process *git_connect(int fd[2], const char 
> *url,
>   if (!ssh)
>   ssh = "ssh";
>  
> - ssh_dup = xstrdup(ssh);
> - base = basename(ssh_dup);
> + ssh_argv0 = xstrdup(ssh);
> + }
> +
> + if (ssh_argv0) {
> + const char *base = basename(ssh_argv0);
>  
>   tortoiseplink = !strcasecmp(base, 
> "tortoiseplink") ||
>   !strcasecmp(base, "tortoiseplink.exe");

I suspect that this will break when GIT_SSH_COMMAND, which is meant
to be processed by the shell, hence the user can write anything,
begins with a one-shot environment variable assignment, e.g.

[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink

One possible unintended side effect of this patch is when VAL1 ends
with /plink (or /tortoiseplink) and the command is not either of
these, in which case the "tortoiseplink" and "putty" variables will
tweak the command line for an SSH implementation that does not want
such a tweak to be made.  There may be other unintended fallouts.

Sorry, no concrete suggestion to get this to work comes to my mind
offhand. 

Perhaps give an explicit way to force "tortoiseplink" and "putty"
variables without looking at and guessing from the pathname, so that
the solution does not have to split and peek the command line?


 connect.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/connect.c b/connect.c
index 8cb93b0720..1768122456 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,23 @@ static const char *get_ssh_command(void)
return NULL;
 }
 
+static void get_ssh_variant(int *tortoiseplink, int *putty)
+{
+   const char *variant;
+   if (!git_config_get_string_const("ssh.variant", ))
+   return;
+   if (!strcmp(variant, "tortoiseplink")) {
+   *tortoiseplink = 1;
+   *putty = 1;
+   } else if (!strcmp(variant, "putty")) {
+   *tortoiseplink = 0;
+   *putty = 1;
+   } else {
+   *tortoiseplink = 0;
+   *putty = 0;
+   }
+}
+
 /*
  * This returns a dummy child_process if the transport protocol does not
  * need fork(2), or a struct child_process object if it does.  Once done,
@@ -824,6 +841,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(>args, "-6");
+
+   get_ssh_variant(, );
+
if (tortoiseplink)
argv_array_push(>args, "-batch");
if (port) {

Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
> ...
>>
>> I guess my only defence is that I tried to be a little lazy.
>
> I actually was alluding to going the other way around, spawning
> "diff-tree -p" in the other codepath like this one does.

Pre-emptively, because I expect it will take a while for me to clear
the backlog to get to your reroll already on the mailing list, I do
not mean to say that you must rewrite the other one to spawn to
match this one.  I meant that I wouldn't have minded if the series
were done that way, as this kind of being "little lazy" would reduce
the chance of regression by reducing the number of exchanged parts
and it is not necessarily a bad thing.

Re: [PATCH 0/3] fix ^C killing pager when running alias

[Jacob Keller]

On Sat, Jan 7, 2017 at 3:26 PM, Jacob Keller  wrote:
> On Fri, Jan 6, 2017 at 5:14 PM, Jeff King  wrote:
>> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>>
>>> > In general, I think it is wrong to wait for child processes when a signal
>>> > was received. After all, it is the purpose of a (deadly) signal to have 
>>> > the
>>> > process go away. There may be programs that know it better, like less, but
>>> > git should not attempt to know better in general.
>>> >
>>> > We do apply some special behavior for certain cases like we do for the
>>> > pager. And now the case with aliases is another special situation. The
>>> > parent git process only delegates to the child, and as such it is 
>>> > reasonable
>>> > that it binds its life time to the first child, which executes the 
>>> > expanded
>>> > alias.
>>>
>>> Yeah, I think I agree. That binding is something you want in many cases,
>>> but not necessarily all. The original purpose of clean_on_exit was to
>>> create a binding like that, but of course it can be (and with the
>>> smudge-filter stuff, arguably has been) used for other cases, too.
>>>
>>> I'll work up a patch that makes it a separate option, which should be
>>> pretty easy.
>>
>> Yeah, this did turn out to be really easy. I spent most of the time
>> trying to explain the issue in the commit message in a sane way.
>> Hopefully it didn't end up _too_ long. :)
>>
>> The interesting bit is in the third one. The first is a necessary
>> preparatory step, and the second is a cleanup I noticed in the
>> neighborhood.
>>
>>   [1/3]: execv_dashed_external: use child_process struct
>>   [2/3]: execv_dashed_external: stop exiting with negative code
>>   [3/3]: execv_dashed_external: wait for child on signal death
>>
>>  git.c | 36 +++-
>>  run-command.c | 19 +++
>>  run-command.h |  1 +
>>  3 files changed, 35 insertions(+), 21 deletions(-)
>>
>> -Peff
>
> I don't see the rest of the patches on the list..?
>
> Thanks,
> Jake

They showed up on public inbox so I assume it must be some spam filter
on my end..

Hmm,
Jake

Re: [PATCH 0/3] fix ^C killing pager when running alias

[Jacob Keller]

On Fri, Jan 6, 2017 at 5:14 PM, Jeff King  wrote:
> On Fri, Jan 06, 2017 at 06:20:42PM -0500, Jeff King wrote:
>
>> > In general, I think it is wrong to wait for child processes when a signal
>> > was received. After all, it is the purpose of a (deadly) signal to have the
>> > process go away. There may be programs that know it better, like less, but
>> > git should not attempt to know better in general.
>> >
>> > We do apply some special behavior for certain cases like we do for the
>> > pager. And now the case with aliases is another special situation. The
>> > parent git process only delegates to the child, and as such it is 
>> > reasonable
>> > that it binds its life time to the first child, which executes the expanded
>> > alias.
>>
>> Yeah, I think I agree. That binding is something you want in many cases,
>> but not necessarily all. The original purpose of clean_on_exit was to
>> create a binding like that, but of course it can be (and with the
>> smudge-filter stuff, arguably has been) used for other cases, too.
>>
>> I'll work up a patch that makes it a separate option, which should be
>> pretty easy.
>
> Yeah, this did turn out to be really easy. I spent most of the time
> trying to explain the issue in the commit message in a sane way.
> Hopefully it didn't end up _too_ long. :)
>
> The interesting bit is in the third one. The first is a necessary
> preparatory step, and the second is a cleanup I noticed in the
> neighborhood.
>
>   [1/3]: execv_dashed_external: use child_process struct
>   [2/3]: execv_dashed_external: stop exiting with negative code
>   [3/3]: execv_dashed_external: wait for child on signal death
>
>  git.c | 36 +++-
>  run-command.c | 19 +++
>  run-command.h |  1 +
>  3 files changed, 35 insertions(+), 21 deletions(-)
>
> -Peff

I don't see the rest of the patches on the list..?

Thanks,
Jake

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[brian m. carlson]

On Thu, Jan 05, 2017 at 11:45:57AM -0500, Jeff King wrote:
> On Thu, Jan 05, 2017 at 11:05:29AM +0100, Lars Schneider wrote:
> 
> > > The git-scm.com site uses asciidoctor, too, and I think I have seen some
> > > oddness with the rendering though. So in general I am in favor of making
> > > things work under both asciidoc and asciidoctor.
> > 
> > I am not familiar with both tools but it sounds to me as if "asciidoctor"
> > is kind of the "lowest common denominator". Is this true? If yes, would it
> > make sense to switch TravisCI [1] to asciidocter if this change gets merged?
> 
> I don't think that's quite true.
> 
> The two programs produce different output for certain inputs. We tend to
> see the cases where asciidoc produces the desired output and asciidoctor
> doesn't, because traditionally the documentation was written _for_
> asciidoc. So whenever asciidoctor diverges, it looks like a bug.

This is indeed the case.  Asciidoctor is a bit stricter on some inputs
because it provides significant performance improvements, but it also
has features that AsciiDoc does not.  It also has some bugs that
AsciiDoc does not (again, usually due to performance concerns).

For example, Debian's reproducible builds project would probably like it
if we had better support for Asciidoctor.

> [1] I think we've also traditionally considered asciidoc to be the
> definitive toolchain, and people using asciidoctor are free to
> submit patches to make things work correctly in both places. I'm not
> opposed to changing that attitude, as it seems like asciidoctor is
> faster and more actively maintained these days. But I suspect our
> build chain would need some improvements. Last time I tried building
> with AsciiDoctor it involved a lot manual tweaking of Makefile
> variables. It sounds like Dscho is doing it regularly, though. It
> should probably work out of the box (with something like
> USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.

Yes, that would probably be beneficial.  I'll see if I can come up with
some patches based on Dscho's work.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 2/2] giteveryday: unbreak rendering with AsciiDoctor

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jan 02, 2017 at 05:04:05PM +0100, Johannes Schindelin wrote:
>
>> The "giteveryday" document has a callout list that contains a code
>> block. This is not a problem for AsciiDoc, but AsciiDoctor sadly was
>> explicitly designed *not* to render this correctly [*1*]. The symptom is
>> an unhelpful
>> 
>>  line 322: callout list item index: expected 1 got 12
>>  line 325: no callouts refer to list item 1
>>  line 325: callout list item index: expected 2 got 13
>>  line 327: no callouts refer to list item 2
>> 
>> In Git for Windows, we rely on the speed improvement of AsciiDoctor (on
>> this developer's machine, `make -j15 html` takes roughly 30 seconds with
>> AsciiDoctor, 70 seconds with AsciiDoc), therefore we need a way to
>> render this correctly.
>> 
>> The easiest way out is to simplify the callout list, as suggested by
>> AsciiDoctor's author, even while one may very well disagree with him
>> that a code block hath no place in a callout list.
>
> This looks like a good re-write to avoid the issue while maintaining the
> meaning and flow of the original.

OK.  Ack.

Thanks.

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
>
>> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= 
>> 
>> The `user-manual.txt` is designed as a `book` but the `Makefile` wants
>> to build it as an `article`. This seems to be a problem when building
>> the documentation with `asciidoctor`. Furthermore the parts *Git
>> Glossary* and *Appendix B* had no subsections which is not allowed when
>> building with `asciidoctor`. So lets add a *dummy* section.
>
> The git-scm.com site uses asciidoctor, too, and I think I have seen some
> oddness with the rendering though. So in general I am in favor of making
> things work under both asciidoc and asciidoctor.
>
> I diffed the results of "make user-manual.html" before and after this
> patch. A lot of "h3" chapter titles get bumped to "h2", which is OK. The
> chapter titles now say "Chapter 1. Repositories and Branches" rather
> than just "Repositories and Branches". Likewise, references now say
>
>   Chapter 1, _Repositories and Branches_
>
> rather than:
>
>   the section called "Repositories and Branches".
>
> which is probably OK, though the whole thing is short enough that
> calling the sections chapters feels a bit over-important.

Is that a longer way to say that the claim "... is designed as a
book" is false?

> So I dunno. I really do think "article" is conceptually the most
> appropriate style, but I agree that there are some book-like things
> (like appendices).

... Yeah, I should have read forward first before starting to insert
my comments.

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Junio C Hamano]

Jeff King  writes:

> As far as CI goes, I am not altogether convinced of the usefulness of
> building the documentation. It's very expensive, and the failure mode is
> rarely "whoops, running `make doc` failed". It's almost always that the
> output looks subtly wrong, but that's very hard to check automatically.

I've seen "make doc" break for me when it works for other people,
and if CI can check the formatting for various combinations of
versions of toolchain components, it would have some value.  But
otherwise, I agree that "make doc" in CI would not give us as much
benefit as we spend electricity on it.

If we are to make support for building with AsciiDoctor better, CI
that does "make doc" with both AsciiDoc and AsciiDoctor might be of
help---a "make it buildable with A" patch may accidentally break B
and vice versa.

> [1] I think we've also traditionally considered asciidoc to be the
> definitive toolchain, and people using asciidoctor are free to
> submit patches to make things work correctly in both places. I'm not
> opposed to changing that attitude, as it seems like asciidoctor is
> faster and more actively maintained these days. But I suspect our
> build chain would need some improvements. Last time I tried building
> with AsciiDoctor it involved a lot manual tweaking of Makefile
> variables. It sounds like Dscho is doing it regularly, though. It
> should probably work out of the box (with something like
> USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.

I do not mind getting convinced to declare that we will swap the
positions of these two documentation toolchains in some future date,
and a good first step for it to happen would be an easier out-of-box
experience.

Thanks.

Re: [PATCH v2 05/34] sequencer (rebase -i): learn about the 'verbose' mode

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Tue, 13 Dec 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > @@ -1493,9 +1498,26 @@ static int pick_commits(struct todo_list 
>> > *todo_list, struct replay_opts *opts)
>> >}
>> >  
>> >if (is_rebase_i(opts)) {
>> > +  struct strbuf buf = STRBUF_INIT;
>> > +
>> >/* Stopped in the middle, as planned? */
>> >if (todo_list->current < todo_list->nr)
>> >return 0;
>> > +
>> > +  if (opts->verbose) {
>> > +  const char *argv[] = {
>> > +  "diff-tree", "--stat", NULL, NULL
>> > +  };
>> > +
>> > +  if (!read_oneliner(, rebase_path_orig_head(), 0))
>> > +  return error(_("could not read '%s'"),
>> > +  rebase_path_orig_head());
>> > +  strbuf_addstr(, "..HEAD");
>> > +  argv[2] = buf.buf;
>> > +  run_command_v_opt(argv, RUN_GIT_CMD);
>> > +  strbuf_reset();
>> > +  }
>> > +  strbuf_release();
>> >}
>> 
>> It's a bit curious that the previous step avoided running a separate
>> process and instead did "diff-tree -p" all in C, but this one does not.
>
> I guess my only defence is that I tried to be a little lazy.

I actually was alluding to going the other way around, spawning
"diff-tree -p" in the other codepath like this one does.

Re: "git fsck" not detecting garbage at the end of blob object files...

[Dennis Kaarsemaker]

On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:
> I was perusing StackOverflow this morning and ran across this
> question: 
> http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
> 
> It was a simple question about why "checking objects" was not
> appearing, but in it was another issue.  The user purposefully
> corrupted a blob object file to see if `git fsck` would catch it by
> tacking extra data on at the end.  `git fsck` happily said everything
> was okay, but when I played with things locally I found out that `git
> gc` does not like that extra garbage.  I'm not sure what the trade-off
> needs to be here, but my expectation is that if `git fsck` says
> everything is okay, then all operations using that object (file)
> should work too.
> 
> Is that unreasonable?  What would be the impact of fixing this issue?

If you do this with a commit object or tree object, fsck does complain.
I think it's sensible to do so for blob objects as well.

Editing blob object:

hurricane:/tmp/moo (master)$ hexer 
.git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485 
hurricane:/tmp/moo (master)$ git status
On branch master
nothing to commit, working tree clean
hurricane:/tmp/moo (master)$ git fsck
Checking object directories: 100% (256/256), done.
hurricane:/tmp/moo (master)$ git gc
Counting objects: 3, done.
error: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485'
fatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in 
.git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt
error: failed to run repack

Editing tree object:

hurricane:/tmp/moo (master)$ hexer 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt

Editing commit object:

hurricane:/tmp/moo (master)$ echo test >> 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0 
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
!(128) hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt

D.

Re: [PATCH v3 14/21] read-cache: touch shared index files when used

[Junio C Hamano]

Christian Couder  writes:

> So what should we do if freshen_file() returns 0 which means that the
> freshening failed?

You tell me ;-)  as you are the one who is proposing this feature.

Isn't a failure to freshen it a grave error?  We are letting a
base/shared index file that is known to be in-use go stale and
eventually subject for garbage collection, and the user should be
notified in some way before the actual GC happens that renders the
index file unusable?

What is the failure mode after such a premature GC happens?  What
does the end-user see?  Can you try to (1) split the index (2)
modify bunch of entries (3) remove the base/shared index with /bin/rm
and then see how various Git commands fail?  Do they fail gracefully?

I am trying to gauge the seriousness of ignoring such an error here.

[PATCH] Makefile: POSIX windres

[Steven Penny]

When environment variable POSIXLY_CORRECT is set, the "input -o output" syntax
is not supported.

http://cygwin.com/ml/cygwin/2017-01/msg00036.html

Signed-off-by: Steven Penny 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index d861bd9..a2a1212 100644
--- a/Makefile
+++ b/Makefile
@@ -1816,7 +1816,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
 git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
  $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., 
,$(GIT_VERSION) \
- -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" $< -o $@
+ -DGIT_VERSION="\\\"$(GIT_VERSION)\\\"" -i $< -o $@
 
 # This makes sure we depend on the NO_PERL setting itself.
 $(SCRIPT_PERL_GEN): GIT-BUILD-OPTIONS
-- 
2.8.3

Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var

[Junio C Hamano]

Christian Couder  writes:

> It feels strange that when I do things one way, you suggest another
> way, and the next time in a similar situation when I do things the way
> you suggested previously, then you suggest the way I did it initially
> the first time...

Perhaps because neither is quite satisfactory and I am being forced
to choose between the two unsatifactory designs?  In any case, I
mostly am and was pointing out the issues and not saying the other
one is the most preferred solution in these threads.

I think there should just be one authoritative source of the truth,
and I have always thought it should be the bit set in the index file
when the command line option is used, because that was the way the
feature was introduced first and I am superstitious about end-user
inertia.  And from that point of view, no matter how you make this
new "config" thing interact with it, it would always give a strange
and unsatifactory end-user experience, at least to me.

Perhaps we should declare that config will be the one and only way
in the future and start deprecating the command line option way.
That will remove the need for two to interact with each other.

I dunno.

Re: [PATCH] don't use test_must_fail with grep

[Junio C Hamano]

Pranit Bauva  writes:

> Hey Johannes,
>
> On Sun, Jan 1, 2017 at 8:20 PM, Johannes Sixt  wrote:
>> which makes me wonder: Is the message that we do expect not to occur
>> actually printed on stdout? It sounds much more like an error message, i.e.,
>> text that is printed on stderr. Wouldn't we need this?
>>
>> git p4 commit >actual 2>&1 &&
>> ! grep "git author.*does not match" actual &&
>>
>> -- Hannes
>
> This seems better! Since I am at it, I can remove the traces of pipes
> in an another patch.
>
> Regards,
> Pranit Bauva

I see v3 that has 2>&1 but according to Luke's comment ("the message
comes from cat"), it shouldn't be there?  I am behind clearing the
backlog in my mailbox and I could tweak it out from v3 while
queuing, or I may forget about it after looking at other topics ;-)
in which case you may want to send v4 with the fix?

Re: RFC: --force-with-lease default behaviour

[Junio C Hamano]

"G. Sylvie Davies"  writes:

> I wonder if there's anything one could do to help those who type "git
> fetch" and still want to enjoy "--force-with-lease"...

The entire idea behind "force-with-lease" is that you plan to later
force update the tip of a branch at the remote to replace the commit
that used to be at the tip at some point, that you do not want other
people to have their own work on that branch that will be lost by
your later force-pushing, yet you cannot "lock" a branch at the
remote repository remotely because that goes against the distributed
nature of the development.  Instead of locking others out, forcing
others to wait and sit idle while you complete the material to be
force-pushed (which may never happen), you base your work on one
state of the remote branch, and make sure the remote branch hasn't
advanced in the meantime (or you redo your work)---the cost of the
extra work due to your planned force-pushing is beared by you, not
by others.

There however is no place in Git where you explicitly declare "this
is where I start working on producing a new commit.  That commit
will replace this state and will not fast-forward from it." and
store it locally.  The "--force-with-lease" was designed to take
that information from the command line, expecting that the script
that drives it does something like

#!/bin/sh
LEASE=$(git rev-parse --verify @{u})
# do whatever that requires non-fast-forward push
git commit --amend ...
... maybe more ...
# finally push it out
git push --force-with-lease $LEASE ...

Lazy people decided that as long as they promise to themselves that
they are not going to do anything to cause @{u} to move, they can
use it as a lazy-man's approximate.  Perhaps that was a misguided
attempt to add convenience.

A possible answer to your wordering may be to deprecate the
defaulting to @{u} and always require the expected commit to be
specified explicitly.

Re:

[Information]

Do you need loan? we offer all kinds of loan from minimum amount of $5,000 to 
maximum of $2,000,000 if you are interested contact us via: 
internationalloa...@gmail.comwith the information below:

Full Name:
Country:
Loan Amount:
Loan Duration:
Mobile phone number:
Sex:

Thanks,
Dr Scott.

Re: Git filesystem case-insensitive to case-sensitive system broken

[Torsten Bögershausen]

On Fri, Jan 06, 2017 at 01:56:36PM -0800, Steven Robertson wrote:
> Hello,
> 
> I was doing development on a linux box on AWS, when we found a code
> bug that had me switching to running the code on a Mac instead. We
> discovered that we had accidentally named two files the same when
> looked at case-insensitively, which made git commands afterwards
> display the wrong thing. It looked like this (ignoring some things
> that aren't relevant):
> 
> $ git status
> 
> 
>modified:   tests/test_system/show_19_L.txt
> 
> 
> no changes added to commit (use "git add" and/or "git commit -a")
> 
> $ git checkout tests/test_system/show_19_L.txt
> 
> $ git status
> 
> 
>modified:   tests/test_system/show_19_l.txt
> 
> 
> no changes added to commit (use "git add" and/or "git commit -a")
> 
> $ git checkout tests/test_system/show_19_l.txt
> 
> $ git status
> 
> 
>modified:   tests/test_system/show_19_L.txt
> 
> 
> no changes added to commit (use "git add" and/or "git commit -a")
> 
> $ diff tests/test_system/show_19_L.txt tests/test_system/show_19_l.txt
> 
> $
> 
> 
> Those two files are different in our repo, and as such git thinks that
> we modified one of them when we try and pull it down from github
> again.
> 
> 
> Thanks for looking at this!
> -- Steven

I assume that you are on Mac OS ?
This is what I would have done:

- find the twin of your file:
$  git ls-files | grep -i tests/test_system/show_19_L.txt

- Let's assume it is the little brother:
  "tests/test_system/show_19_l.txt"
$  git mv tests/test_system/show_19_l.txt tests/test_system/show_19_l2.txt

- Check out the original:
$ git checkout tests/test_system/show_19_L.txt

- check:
$ git status

"git fsck" not detecting garbage at the end of blob object files...

[John Szakmeister]

I was perusing StackOverflow this morning and ran across this
question: 
http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/

It was a simple question about why "checking objects" was not
appearing, but in it was another issue.  The user purposefully
corrupted a blob object file to see if `git fsck` would catch it by
tacking extra data on at the end.  `git fsck` happily said everything
was okay, but when I played with things locally I found out that `git
gc` does not like that extra garbage.  I'm not sure what the trade-off
needs to be here, but my expectation is that if `git fsck` says
everything is okay, then all operations using that object (file)
should work too.

Is that unreasonable?  What would be the impact of fixing this issue?

-John

Re: [PATCH] rebase--interactive: count squash commits above 10 correctly

[Jeff King]

On Sat, Jan 07, 2017 at 11:51:23AM +0100, Johannes Schindelin wrote:

> > We can fix this by making the ".*" less greedy. Instead of
> > depending on ".*?" working portably, we can just limit the
> > match to non-digit characters, which accomplishes the same
> > thing.
> 
> Or we could simply require a space before the first digit, which would
> maybe look a little simpler.

Yeah, if we can assume that all translations will always have a space
there. It is almost certainly true for Western languages, but I don't
know about others. It looks like the Korean translation omits a space
_after_ the digits, but there is still one before.

-Peff

Re: [PATCH] rebase--interactive: count squash commits above 10 correctly

[Johannes Schindelin]

Hi Peff,

On Sat, 7 Jan 2017, Jeff King wrote:

> On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:
> 
> We can fix this by making the ".*" less greedy. Instead of
> depending on ".*?" working portably, we can just limit the
> match to non-digit characters, which accomplishes the same
> thing.

Or we could simply require a space before the first digit, which would
maybe look a little simpler.

Ciao,
Dscho

Re: [RFC PATCH 0/5] Localise error headers

[Duy Nguyen]

On Wed, Jan 4, 2017 at 8:25 PM, Duy Nguyen  wrote:
> On Wed, Jan 4, 2017 at 2:45 AM, Stefan Beller  wrote:
>>> In this implementation, the gettext call for the header and the body are 
>>> done
>>> in different places (error function vs. caller) but this call pattern seems 
>>> to
>>> be the easiest variant for the caller, because the message body has to be 
>>> marked
>>> for localisation in any case, and N_() requires more letters than _(), an 
>>> extra
>>> argument to die() etc. even more than the extra "_" in the function name.
>>
>> I see. We have to markup the strings to be translatable such that the .po 
>> files
>> are complete. It would be really handy if there was a way to say "anything 
>> that
>> is fed to this function (die_) needs to be marked for translation.
>>
>> Looking through
>> https://www.gnu.org/software/gettext/manual/html_node/xgettext-Invocation.html
>> such a thing doesn't seem to exist.
>
> I think --keyword is exactly for that purpose: marking more text for
> translations besides standard markers like _() or N_(). Yes we need to
> call gettext() explicitly in die_() later on. We already do that for
> parse-options. We just need to N_() the strings, without actually
> spelling it out.
>
>>
>> So in that case die_(_(...)) seems to be the easiest way forward.
>
> I still prefer changing the die_routine though since die() in many
> cases could be used in both plumbing and porcelain contexts. And we
> have tried to keep plumbing output (and behavior) as stable as
> possible. The approach has some similarity to unpack_trees() which
> shares the same porcelain/plumbing problem.

On the other hand, making die(), not die_(), translatable means the
translators will have to translate them _all_ even if only some will
end up being displayed. That's 2000+ strings according to git-grep.
And some of them, like die("BUG:..") should definitely not be
translated. So +1 to die_(), unless we decide all strings are safe to
translate.
-- 
Duy

Re: [PATCH v5 00/16] pathspec cleanup

[Duy Nguyen]

On Thu, Jan 5, 2017 at 1:03 AM, Brandon Williams  wrote:
> Changes in v5:
> * Move GUARD_PATHSPEC to prevent checking if pathspec is null twice.
> * Mark a string containing 'mnemonic' for translation.

Argh.. I've run out of things to complain about! Ack!
-- 
Duy

Re: [PATCH 3/3] execv_dashed_external: wait for child on signal death

[Duy Nguyen]

On Sat, Jan 7, 2017 at 8:22 AM, Jeff King  wrote:
> When you hit ^C to interrupt a git command going to a pager,
> this usually leaves the pager running. But when a dashed
> external is in use, the pager ends up in a funny state and
> quits (but only after eating one more character from the
> terminal!). This fixes it.
>
> Explaining the reason will require a little background.
>
> ...

I see I have exposed a bug and I'm glad you are around to fix it. Even
reading your explanation is scary enough, let alone finding it.
Thanks!
-- 
Duy

[BUG] push sometimes omits status table when remote dies

[Jeff King]

If I run t5504 under heavy load, it sometimes fails tests 8 and 9, which
make a broken push to the remote. The failure looks like this:

expecting success: 
rm -rf dst &&
git init dst &&
(
cd dst &&
git config transfer.fsckobjects true
) &&
test_must_fail git push --porcelain dst master:refs/heads/test >act 
&&
test_cmp exp act

Initialized empty Git repository in /var/ram/git-stress/root-13/trash 
directory.t5504-fetch-receive-strict/dst/.git/
remote: fatal: object of unexpected type
error: failed to push some refs to 'dst'
--- exp 2017-01-07 08:44:24.465771756 +
+++ act 2017-01-07 08:44:24.493771755 +
@@ -1,2 +0,0 @@
-To dst
-!  refs/heads/master:refs/heads/test   [remote rejected] (unpacker 
error)
not ok 9 - push with transfer.fsckobjects

So it looks like we correctly fail the push, but the expected porcelain
output is missing. I'm _guessing_ what happens is that the local
git-push sees SIGPIPE writing to the remote side, and dies before it
gets a chance to write the message.

I'm not sure if this is something we should be fixing, or if the test is
simply a bit flaky (and we should work around it).

-Peff

[PATCH] rebase--interactive: count squash commits above 10 correctly

[Jeff King]

On Fri, Jan 06, 2017 at 01:04:05AM -0800, Brandon Tolsch wrote:

> git --version: 2.11.0
> 
> When using git rebase -i to squash a series of commits that includes
> more than 10 commits, the generated commit message you are given to
> edit counts the old messages incorrectly.  It will say the total
> number of commits is (actual % 10) (if they were 0-based) and it will
> also count the commits as (actual % 10).

Looks like a regression in v2.10. Here's the fix.

-- >8 --
Subject: rebase--interactive: count squash commits above 10 correctly

We generate the squash commit message incrementally running
a sed script once for each commit. It parses "This is
a combination of  commits" from the first line of the
existing message, adds one to , and uses the result as
the number of our current message.

Since f2d17068fd (i18n: rebase-interactive: mark comments of
squash for translation, 2016-06-17), the first line may be
localized, and sed uses a pretty liberal regex, looking for:

  /^#.*([0-9][0-9]*)/

The "[0-9][0-9]*" tries to match double digits, but it
doesn't quite work.  The first ".*" is greedy, so if you
have:

  This is a combination of 10 commits.

it will eat up "This is a combination of 1", leaving "0" to
match the first "[0-9]" digit, and then skipping the
optional match of "[0-9]*".

As a result, the count resets every 10 commits, and a
15-commit squash would end up as:

  # This is a combination of 5 commits.
  # This is the 1st commit message:
  ...
  # This is the commit message #2:
  ... and so on ..
  # This is the commit message #10:
  ...
  # This is the commit message #1:
  ...
  # This is the commit message #2:
  ... etc, up to 5 ...

We can fix this by making the ".*" less greedy. Instead of
depending on ".*?" working portably, we can just limit the
match to non-digit characters, which accomplishes the same
thing.

Reported-by: Brandon Tolsch 
Signed-off-by: Jeff King 
---

I didn't include a test here, mostly because this bug is so weirdly
specific that it seems unlikely to happen again. Especially in light of
this code going away in favor of the C rebase helper, which Dscho
already confirmed is not buggy (and I did not look at the code, but it
cannot possibly do anything as gross as these repeated sed invocations).

It also is a little tricky to automatically extract the comments (you
have to override GIT_EDITOR, but we also invoke GIT_EDITOR for the insn
sheet).

I was able to replicate and confirm the fix manually with:

  git commit -q --allow-empty -m base
  git commit -q --allow-empty -m squash
  git tag base
  for i in $(seq 15); do
echo $i >$i
git add $i
git commit -qm "squash! squash"
  done
  git rebase --autosquash -i base

I'd also suggest that "the commit message #4" is grammatically
questionable. Probably "This is commit message #4" would be fine.

 git-rebase--interactive.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b0a6f2b7ba..4734094a3f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -425,7 +425,7 @@ update_squash_messages () {
if test -f "$squash_msg"; then
mv "$squash_msg" "$squash_msg".bak || exit
count=$(($(sed -n \
-   -e "1s/^$comment_char.*\([0-9][0-9]*\).*/\1/p" \
+   -e "1s/^$comment_char[^0-9]*\([0-9][0-9]*\).*/\1/p" \
-e "q" < "$squash_msg".bak)+1))
{
printf '%s\n' "$comment_char $(eval_ngettext \
-- 
2.11.0.527.gfef230ca76