Re: [PATCH v2 00/21]

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Changes since v1:
>
>  - fopen_or_warn() and warn_on_fopen_errors() are introduced. The
>latter's name is probably not great...
>  - A new patch (first one) to convert a bunch to using xfopen()
>  - Fix test failure on Windows, found by Johannes Sixt
>  - Fix the memory leak in log.c, found by Jeff
>
> There are still a couple of fopen() remained, but I'm getting slow
> again so let's get these in first and worry about the rest when
> somebody has time.
>
> Nguyễn Thái Ngọc Duy (21):
>   Use xfopen() in more places
>   clone: use xfopen() instead of fopen()
>   config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
>   wrapper.c: add warn_on_fopen_errors()
>   wrapper.c: add fopen_or_warn()
>   attr.c: use fopen_or_warn()
>   ident.c: use fopen_or_warn()
>   bisect: report on fopen() error
>   blame: report error on open if graft_file is a directory
>   log: report errno on failure to fopen() a file
>   log: fix memory leak in open_next_file()
>   commit.c: report error on failure to fopen() the graft file
>   remote.c: report error on failure to fopen()
>   rerere.c: report error on failure to fopen()
>   rerere.c: report correct errno
>   sequencer.c: report error on failure to fopen()
>   server-info: report error on failure to fopen()
>   wt-status.c: report error on failure to fopen()
>   xdiff-interface.c: report errno on failure to stat() or fopen()
>   config.c: handle error on failing to fopen()
>   t1308: add a test case on open a config directory

Thanks.  If the number of parts affected by this series were
smaller, it may have made the review easier to have the introduction
of a helper and its use in a single larger patch, but there are
spread across many, some with files that are touched by different
in-flight topics, and these "collection of smaller patches" makes it
easier to manage both while reviewing and also merging.

All looked good, even though I do share the doubt on the name
"warn-on-fopen-errors"; when something applies equally to fopen(3)
and underlying open(2), I would tend to call that with open not
fopen myself.  But that is a minor point.

Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  dir.c |  3 +--
>  git-compat-util.h |  2 ++
>  wrapper.c | 10 ++
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char 
> *base, int baselen,
>  
>   fd = open(fname, O_RDONLY);
>   if (fd < 0 || fstat(fd, ) < 0) {
> - if (errno != ENOENT)
> - warn_on_inaccessible(fname);
> + warn_on_fopen_errors(fname);

At least in the original, a reader can guess that, because errno
cannot be NOENT if open(2) succeeded and fstat(2) failed, we call
warn_on_inaccessible() only when we fail to open.

This change makes it harder to see why this is OK when the failure
is not in open(2) but in fstat(2).

fd = open(fname, O_RDONLY);
if (fd < 0 || fstat(fd, ) < 0) {
-   if (errno != ENOENT);
-   warn_on_inaccessible(fname);
-   if (0 <= fd)
+   if (fd < 0)
+   warn_on_fopen_errors(fname);
+   else
close(fd);
... and the remainder of the original ...

or something like that, perhaps?

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -418,6 +418,16 @@ FILE *fopen_for_writing(const char *path)
>   return ret;
>  }
>  
> +int warn_on_fopen_errors(const char *path)
> +{

Does this need to be returning "int", or should it be "void",
because it always warns when there is an issue, the caller does not
get to choose to give its own warning?

> + if (errno != ENOENT && errno != ENOTDIR) {
> + warn_on_inaccessible(path);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
>  int xmkstemp(char *template)
>  {
>   int fd;

Re: Bug report: Git Stash; spelling mistake of stash followed by a yes prints character 'y' infinite times.

[Junio C Hamano]

Jeff King  writes:

>> 4. Now git says git: 'stahs' is not a git command. See 'git --help'.
>> 
>> Did you mean this?
>> 
>> stash
>
> After this step git exits, and you're back at your shell prompt...

Perhaps the suggestion message should be rephrased not to sound like
it is asking confirmation?

Re: [PATCH 0/5] Abide by our own rules regarding line endings

[Junio C Hamano]

Johannes Schindelin  writes:

> For starters, those files include shell scripts: the most prevalent
> shell interpreter in use (and certainly used in Git for Windows) is
> Bash, and Bash does not handle CR/LF line endings gracefully.

Good to know.  I am not sure if it is OK for shell scripts not to
honor the platform convention, though.

Stated from the opposite angle, I would not be surprised if your
shell scripts do not work on Linux if you set core.autocrlf to true.
Git may honor it, but shells on Linux (or BSD for that matter) do
not pay attention to core.autocrlf and they are within their rights
to complain on an extra CR at the end of the line.  IOW, I would
doubt that it should be our goal to set core.autocrlf on a platform
whose native line endings is LF and make the tests to pass.

> Related to shell scripts: when generating common-cmds.h, we use tools
> that generally operate on the assumption that input and output
> deliminate their lines using LF-only line endings. Consequently, they
> would happily copy the CR byte verbatim into the strings in
> common-cmds.h, which in turn makes the C preprocessor barf (that
> interprets them as MacOS-style line endings).

This indeed is a problem.  "add\r" is not a name of a common
command, obviously, regardless of how the text file that lists the
names of the commands is encoded.  I am undecided if it is a problem
in the source text (i.e. command-list.txt is not a platform neutral
"text" but has to be encoded with LF endings) or the bug in the
tools used in the generate-cmdlist.sh script, though.  Shouldn't the
tools be aware of the platform convention of what text files are and
how their eol looks like?

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Junio C Hamano]

Johannes Schindelin  writes:

>> If 'git-rebase--interactive.sh' is bound to be replaced, I could
>> just shrink this to the Documentation cleanup (patches 4 and 5)
>> and rework the rest on top of your new implementation.
>
> I kind of hoped that Junio would chime in with his verdict. That would be
> the ultimate deciding factor, I think.

What I can predict is that within two or three release cycles
(unless you completely lose interest) the todo-list generation will
be all in C and that I anticipate that the C version may even be
capable of generating different kind of todo command (e.g. to
support tools like your Garden Shears more natively), so the
mid-term direction definitely is that any enhancement would in the
end needs to happen on top of or in coordination with the C rewrite
we've been discussing recently.

I didn't know what the comfort levels of Liam working with scripted
vs C code, and the "vertict" depends on that, I would think.  We may
want to discuss the enhancement in the original scripted form Liam
did with new tests while the C rewrite is still cooking, and either
Liam, you or somebody else can make it work with your C rewrite when
both are ready.  If Liam feels comfortable working with you and the
code after the C rewrite that is still in-flight, it is also fine if
the next iteration from Liam were on top of your series.

Re: Proposal for missing blob support in Git repos

[Junio C Hamano]

Jonathan Tan  writes:

> I see the semantics as "don't write what you already have", where
> "have" means what you have in local storage, but if you extend "have"
> to what upstream has, then yes, you're right that this changes
> (ignoring shallow clones).
>
> This does remove a resistance that we have against hash collision (in
> that normally we would have the correct object for a given hash and
> can resist other servers trying to introduce a wrong object, but now
> that is no longer the case), but I think it's better than consulting
> the hook whenever you want to write anything (which is also a change
> in semantics in that you're consulting an external source whenever
> you're writing an object, besides the performance implications).

As long as the above pros-and-cons analysis is understood and we are
striking a balance between performance and strictness with such an
understanding of the implications, I am perfectly fine with the
proposal.  That is why my comment has never been "I think that is
wrong" but consistently was "I wonder if that is a good thing."

Thanks.

Re: [PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Mon, 1 May 2017, Junio C Hamano wrote:
>
>> Junio C Hamano  writes:
>> 
>> > Johannes Schindelin  writes:
>> >
>> >> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> >> index 92a5d8a5d26..a4ce73fb1e9 100644
>> >> --- a/builtin/name-rev.c
>> >> +++ b/builtin/name-rev.c
>> >> @@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
>> >>   struct rev_name *name = (struct rev_name *)commit->util;
>> >>   struct commit_list *parents;
>> >>   int parent_number = 1;
>> >> + char *p = NULL;
>> >>  
>> >>   parse_commit(commit);
>> >>  
>> >> @@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
>> >>   return;
>> >>  
>> >>   if (deref) {
>> >> - tip_name = xstrfmt("%s^0", tip_name);
>> >> + tip_name = p = xstrfmt("%s^0", tip_name);
>> 
>> I'll rename 'p' to 'to_free' while queuing, though.  Without a
>> descriptive name, it was confusing to view while resolving conflicts
>> with another in-flight topic.
>
> Good point. I also used `p` in builtin/mktree.c and setup.c. While you
> seem to have renamed it in builtin/mktree.c, I think setup.c also needs
> the same fixup.

Yes, no question about it. I just left it as-is as I saw it is
likely to be rerolled anyway due to other issues ;-)

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Brandon Williams]

On 05/03, Samuel Lijin wrote:
> On Wed, May 3, 2017 at 12:14 PM, Stefan Beller  wrote:
> > On Wed, May 3, 2017 at 4:31 AM, Samuel Lijin  wrote:
> >>
> >> Just to throw out an example, I'm relatively new to the codebase (I've
> >> been lurking on the mailing list for a few months now) and for a
> >> recent project (I'm an undergrad wrapping up my senior year, and one
> >> of my classes' final projects was to do something that involved
> >> concurrency) I took a shot at parallelizing the estimate_similarity()
> >> calls in diffcore_rename(). The only way I was able to get it to work
> >> was by dropping global mutexes in one or two files (the code for those
> >> mutexes still makes me cringe), because of concurrent writes to global
> >> data structures.
> >
> > That sounds like a challenge. As we have many globals, we need to be
> > very careful about threading.
> >
> > Also an interesting discussion about threading:
> > https://public-inbox.org/git/9e4733910708111412t48c1beaahfbaa2c68a02f6...@mail.gmail.com/
> 
> Thanks.
> 
> > Are the patches available for discussion?
> 
> I was planning on revisiting the patch series before sending it out -
> the changes in attr.c and sha1_file.c are not pretty (and I'm pretty
> sure one of them is non-portable) - but it is published at
> https://github.com/sxlijin/git/commits/parallelize.thread-pool.1 (it's
> based off v2.12.2).

I spent a good chunk of time reworking the attribute code a few months
back in order to make it thread-safe so you may want to rebase on top of
a newer version and see if your changes get a little less messy (if so
then I did my job :).

-- 
Brandon Williams

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Brandon Williams]

On 05/03, Junio C Hamano wrote:
> Duy Nguyen  writes:
> 
> >>> In the long run we may want to drop the macros guarded by
> >>> NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
> >>
> >> Why?
> >
> > Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
> 
> That was to mark the code that implements the underlying machinery
> that the code in a particular file has already been vetted and
> converted to be capable of working with an arbitrary index_state
> instance, not just the_index instance.
> 
> Your mentioning "at least outside builtin/" shows that you have a
> good understanding of the issue; they are infrastructure code and
> many of them may become more useful if they are enhanced to take an
> index_state instance instead of always working with the_index.  With
> an infrastructure code that insists on working on the_index, an
> application that has something valuable in the_index already needs
> to somehow save it elsewhere before calling the function and use the
> result from the_index before restoring its version.
> 
> I think unpack-trees.c used to assume that it is OK to use the_index,
> but after it started taking src/dst_index, it gained NO_THE_INDEX_COMP
> thing.  That's the kind of change in this area we would want to see.
> 
> > I attempted the same thing once or twice in the past, had the same
> > impression and dropped it. But I think it's good to get rid of cache_*
> > macros, at least outside builtin/ directory.
> 
> One thing that needs to be kept in mind is that a mechanical
> replacement of active_cache[] to the_index.cache[] in the
> infrastructure code does not get us closer to such an improvement.
> In fact, such a patch only has negative value (keep reading until
> the end of the paragraph that begins with "One thing is certain" to
> understand why).
> 
> The entry point to a codechain for an important infrastructure code
> that can currently only work with the_index needs to be identified.
> There may be many.  They need to be made to take an index_state
> instance and to pass it through the callchain.  That kind of change
> would be an improvement, and will get us closer to mark the
> resulting code with NO_THE_INDEX_COMP thing.
> 
> I think somebody else said this, but I agree that in the longer
> term, we would want to have a "repository" abstraction for in-core
> data structure.
> 
> Imagine that a program wants to read into an index_state the tip
> commit of a branch whose name is the same as the current branch in
> one submodule and do something with the objects, while keeping
> the_index for the top-level superproject.  Thanks to Michael's and
> your recent work, we are almost there to have "ref-store" that
> represents the set of refs the submodule in question has that a
> "repository" object that represents the submodule can point at.  We
> can learn the object name at the tip of a specific ref using that
> infrastructure.  Thanks to an ancient change under discussion, we
> can already read the contents of the index file into an instance of
> an index_state that is different from the_index.  But after doing
> these operations, we'd need to actually get to the contents of the
> objects the index_state holds.  How should that work in the
> "repository object represents an in-core repository" world?
> 
> What is missing is a way to represent the object store, so that the
> "repository" object can point at an instance of it (which in turn
> may point at a set of its alternates) [*1*].  With that, perhaps the
> most general kind of the infrastructure API that works with an
> index_state instance may additionally take a repository object, or
> perhaps an index_state may have a pointer to the repository where
> the objects listed in it must be taken from (in which case the
> function may take only an index_state).  In the end the enhanced
> function has to allow us to read from the object store of the
> submodule, not from the superproject's.
> 
> Depending on how the design of the repository object goes, the
> interface to these functions may have to change.
> 
> But one thing is certain. Many users of the API, just like a lot of
> builtin/ code works only with the_index today, will not have to work
> with a non-default repository or a non-default index_state.  Only
> the more advanced and complex "multi-repo" Porcelains would have to
> care.  Keeping active_cache[], active_nr, etc. abstraction would
> isolate the simpler callers from the inevitable code churn we would
> need while getting the "repository" abstraction right.  This is why
> I see that you understood the issues well when you said "builtiln/".
> 
> If we want to have the "repository" abstraction, the "object store"
> is the next thing we should be working on, and such a work can take
> inspiration from how the old  active_alloc, active_cache_changed and *active_cache_tree> tuple was
> turned into an "index" object while allowing 

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Junio C Hamano]

Duy Nguyen  writes:

>>> In the long run we may want to drop the macros guarded by
>>> NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
>>
>> Why?
>
> Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k

That was to mark the code that implements the underlying machinery
that the code in a particular file has already been vetted and
converted to be capable of working with an arbitrary index_state
instance, not just the_index instance.

Your mentioning "at least outside builtin/" shows that you have a
good understanding of the issue; they are infrastructure code and
many of them may become more useful if they are enhanced to take an
index_state instance instead of always working with the_index.  With
an infrastructure code that insists on working on the_index, an
application that has something valuable in the_index already needs
to somehow save it elsewhere before calling the function and use the
result from the_index before restoring its version.

I think unpack-trees.c used to assume that it is OK to use the_index,
but after it started taking src/dst_index, it gained NO_THE_INDEX_COMP
thing.  That's the kind of change in this area we would want to see.

> I attempted the same thing once or twice in the past, had the same
> impression and dropped it. But I think it's good to get rid of cache_*
> macros, at least outside builtin/ directory.

One thing that needs to be kept in mind is that a mechanical
replacement of active_cache[] to the_index.cache[] in the
infrastructure code does not get us closer to such an improvement.
In fact, such a patch only has negative value (keep reading until
the end of the paragraph that begins with "One thing is certain" to
understand why).

The entry point to a codechain for an important infrastructure code
that can currently only work with the_index needs to be identified.
There may be many.  They need to be made to take an index_state
instance and to pass it through the callchain.  That kind of change
would be an improvement, and will get us closer to mark the
resulting code with NO_THE_INDEX_COMP thing.

I think somebody else said this, but I agree that in the longer
term, we would want to have a "repository" abstraction for in-core
data structure.

Imagine that a program wants to read into an index_state the tip
commit of a branch whose name is the same as the current branch in
one submodule and do something with the objects, while keeping
the_index for the top-level superproject.  Thanks to Michael's and
your recent work, we are almost there to have "ref-store" that
represents the set of refs the submodule in question has that a
"repository" object that represents the submodule can point at.  We
can learn the object name at the tip of a specific ref using that
infrastructure.  Thanks to an ancient change under discussion, we
can already read the contents of the index file into an instance of
an index_state that is different from the_index.  But after doing
these operations, we'd need to actually get to the contents of the
objects the index_state holds.  How should that work in the
"repository object represents an in-core repository" world?

What is missing is a way to represent the object store, so that the
"repository" object can point at an instance of it (which in turn
may point at a set of its alternates) [*1*].  With that, perhaps the
most general kind of the infrastructure API that works with an
index_state instance may additionally take a repository object, or
perhaps an index_state may have a pointer to the repository where
the objects listed in it must be taken from (in which case the
function may take only an index_state).  In the end the enhanced
function has to allow us to read from the object store of the
submodule, not from the superproject's.

Depending on how the design of the repository object goes, the
interface to these functions may have to change.

But one thing is certain. Many users of the API, just like a lot of
builtin/ code works only with the_index today, will not have to work
with a non-default repository or a non-default index_state.  Only
the more advanced and complex "multi-repo" Porcelains would have to
care.  Keeping active_cache[], active_nr, etc. abstraction would
isolate the simpler callers from the inevitable code churn we would
need while getting the "repository" abstraction right.  This is why
I see that you understood the issues well when you said "builtiln/".

If we want to have the "repository" abstraction, the "object store"
is the next thing we should be working on, and such a work can take
inspiration from how the old  tuple was
turned into an "index" object while allowing the majority of code
that want to deal with the singleton instance keep using the old
names via CPP macros.

Also, dropping compatibility macros at the end of the series is
unfriendly to fellow developers with WIPs 

Re: [PATCH] send-email: new option to walkaround email server limits

[Ramkumar Ramachandra]

For the list, in plain text:

IIUC, they use the date received to sort. I think this might stem from
a historical cruft: emails sometimes took non-trivial amounts of time
to transit, back in the old days. MUAs (especially web-based ones)
probably did not want to violate user expectation by placing a new
email under several already-read emails. I would argue that this was
reasonable behavior. Further, since email is near-instantaneous today,
it really makes no difference which way the MUA sorts. Except for git
send-email.

It might be acceptable to put in a "practical hack" to help out MUAs
in the context of "near instant forwarding". I would argue against it
on grounds of it being an ugly hack, for very little benefit. The
patch that began this thread is also ugly, but has the important
consequence of enabling some people to use git send-email at all.

p.s- We should probably allow emails from mobile devices on the list.
It usually contains a HTML subpart.

Re: [PATCH] i18n: remove i18n from tag reflog message

[Junio C Hamano]

Jean-Noël AVILA  writes:

> Le dimanche 30 avril 2017, 18:45:05 CEST Junio C Hamano a écrit :
>> Jean-Noel Avila  writes:
>> > The building of the reflog message is using strbuf, which is not
>> > friendly with internationalization frameworks. No other reflog
>> > messages are translated right now and switching all the messages to
>> > i18n would require a major rework of the way the messages are built.
>> 
>> Thanks for spotting.  Before we declare that reflog messages are for
>> i18n, we'd need to either drop (or redesign the implementation of)
>> the "checkout -" feature, which relies on the exact phrasing of how
>> reflog entries from "git checkout" looks like.
>> 
>> Will queue and merge down to 'master' quickly.
>> 
>
> I didn't know this "side effect". Maybe adding a test against it would be 
> requiered. Unfortunately, I don't know enough of it.

Sorry, but I am not sure what "side effect" you are referring to.  I
am saying that in the current codebase not translating tag reflog
message (i.e. your fix) is the right thing to do.

builtin/checkout.c does

strbuf_addf(, "checkout: moving from %s to %s",
old_desc ? old_desc : "(invalid)", new->name);

when "git checkout" is used to switch from old to new branch.  The
above phrasing is used by sha1_name.c::grab_nth_branch_switch() to
know what branch you were on, and not having _() around the message
is the right thing to do, at least in the current codebase.

If somebody wants to i18n the reflog message, that needs to be done
at the reading/dispaying end, I would think.  Instead of being purely
textual records, a new reflog format would have to be a machine
parsable message-id with parameters, and the code that wants to
understand the reflog record like grab_nth_branch_switch(), as
opposed to show the record to the humans, would just use the machine
parsable raw data, while "git reflog" and friends that format the
reflog record would give the message-id to gettext and feed the
parameters recorded using the result of gettext() as the format
string to fprintf(), or something like that.

That would solve the issue in multi-user repository Ævar raised,
too.  You'd see Chinese reflog entry not because the person who did
the operation that created the reflog entry were writing Chinese,
but because your locale is set to show Chinese when you tried to
read the reflog.  German users would see the reflog entry that
records what the Chinese developer did in German if we follow that
route.

Re: [PATCH] i18n: remove i18n from tag reflog message

[Junio C Hamano]

Jean-Noël AVILA  writes:

> Le dimanche 30 avril 2017, 18:45:05 CEST Junio C Hamano a écrit :
>> Jean-Noel Avila  writes:
>> > The building of the reflog message is using strbuf, which is not
>> > friendly with internationalization frameworks. No other reflog
>> > messages are translated right now and switching all the messages to
>> > i18n would require a major rework of the way the messages are built.
>> 
>> Thanks for spotting.  Before we declare that reflog messages are for
>> i18n, we'd need to either drop (or redesign the implementation of)
>> the "checkout -" feature, which relies on the exact phrasing of how
>> reflog entries from "git checkout" looks like.
>> 
>> Will queue and merge down to 'master' quickly.
>> 
>
> I didn't know this "side effect". Maybe adding a test against it would be 
> requiered. Unfortunately, I don't know enough of it.

Sorry, but I am not sure what "side effect" you are referring to.  I
am saying that in the current codebase not translating tag reflog
message (i.e. your fix) is the right thing to do.

builtin/checkout.c does

strbuf_addf(, "checkout: moving from %s to %s",
old_desc ? old_desc : "(invalid)", new->name);

when "git checkout" is used to switch from old to new branch.  The
above phrasing is used by sha1_name.c::grab_nth_branch_switch() to
know what branch you were on, and not having _() around the message
is the right thing to do, at least in the current codebase.

If somebody wants to i18n the reflog message, that needs to be done
at the reading/dispaying end, I would think.  Instead of being purely
textual records, a new reflog format would have to be a machine
parsable message-id with parameters, and the code that wants to
understand the reflog record like grab_nth_branch_switch(), as
opposed to show the record to the humans, would just use the machine
parsable raw data, while "git reflog" and friends that format the
reflog record would give the message-id to gettext and feed the
parameters recorded using the result of gettext() as the format
string to fprintf(), or something like that.

Re: [PATCH] send-email: new option to walkaround email server limits

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> It makes sense to have a configurable delay for git-send-email
> unrelated to this option, I'd use a facility like that.
>
> A lot of mail clients just sort based on date/msgid or whatever not
> date/subject, so the rapid-fire output of send-email often arrives out
> of order because it's all sent at the same second. I'd use some option
> where I could send a series as "all" and have it sleep(N) in between
> sending mails.

Hmph.  When sending many messages, send-email first grabs the
current time, counts backwards N seconds for N message series,
and uses that timestamp that is N seconds in the past for the first
message, incrementing the timestamp by 1 second per each subsequent
ones.

I found that this trick is sufficient to cause receiving MUAs sort
messages based on date, as the Date: field will have the timestamps
that increases by 1 second in between messages in a batch.  

There might be MUAs that do not use the value of the Date: field
when told to sort by date (perhaps they use the timestamp of the
message file they received at the final hop to them?), but it is
hopeless to help such MUAs unless the mail path guarantees the order
at the originator, which is not how "store and forward" e-mails
work.

So...

Re: [PATCH v2 00/53] object_id part 8

[brian m. carlson]

On Mon, May 01, 2017 at 02:28:53AM +, brian m. carlson wrote:
> This is the eighth series of patches to convert unsigned char [20] to
> struct object_id.  This series converts lookup_commit, lookup_blob,
> lookup_tree, lookup_tag, and finally parse_object to struct object_id.

I've made the changes suggested by reviewers and will be sending out a
reroll soon (probably tomorrow, since running the testsuite for 53
patches is slow).
-- 
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 v2 02/53] Clean up outstanding object_id transforms.

[brian m. carlson]

On Tue, May 02, 2017 at 11:05:14AM -0700, Brandon Williams wrote:
> On 05/01, brian m. carlson wrote:
> > -   if (!logobj && commit_reflog->recno >= 0 && 
> > is_null_sha1(reflog->ooid.hash)) {
> > +   if (!logobj && commit_reflog->recno >= 0 && is_null_oid(>ooid)) 
> > {
> 
> Not relevant to this series but I was confused for a second seeing
> 'ooid' as I have no clue what that means :)

These were originally nsha1 and osha1, now noid and ooid.  They are,
respectively, the new and old object IDs.  We've used those in various
places around the codebase.
-- 
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

[PATCH] archive-tar: fix a sparse 'constant too large' warning

[Ramsay Jones]

Commit dddbad728c ("timestamp_t: a new data type for timestamps",
26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an
unsigned long, which was used at the time to represent timestamps in
git. A later commit 28f4aee3fb ("use uintmax_t for timestamps",
26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp
representation type.

When building on a 32-bit Linux system, sparse complains that a constant
(USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too
large; 'warning: constant 0777UL is so big it is unsigned long
long' on lines 335 and 338 of archive-tar.c. Note that both gcc and
clang only issue a warning if this constant is used in a context that
requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX
is no longer equal to 0x, even on a 32-bit system, the macro
USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as
an 'unsigned long' constant).

In order to suppress the warning, change the definition of the macro
constant to use an 'ULL' type suffix.

Signed-off-by: Ramsay Jones 
---

Hi Junio,

I try to build git on 32-bit Linux about once a week (but it can
sometimes be less often than that!). During tonight's build sparse
chirped at me while building the 'next' branch.

This patch was built on top of next.

ATB,
Ramsay Jones

 archive-tar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/archive-tar.c b/archive-tar.c
index 319a5b1c7..6dddc0cff 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -33,7 +33,7 @@ static int write_tar_filter_archive(const struct archiver *ar,
 #if TIME_MAX == 0x
 #define USTAR_MAX_MTIME TIME_MAX
 #else
-#define USTAR_MAX_MTIME 0777UL
+#define USTAR_MAX_MTIME 0777ULL
 #endif
 
 /* writes out the whole block, but only if it is full */
-- 
2.12.0

Re: [PATCH v2 03/53] Convert struct cache_tree to use struct object_id

[brian m. carlson]

On Tue, May 02, 2017 at 11:13:08AM -0700, Brandon Williams wrote:
> Should this 20 be converted to GIT_{SHA1,MAX}_RAWSZ?  I know this is a
> generated patch so maybe this is addressed, or will be addressed later?

Since this is a purely mechanical patch, I plan on addressing it later.
I'll probably do a search for 20s and 40s in the codebase as a future
series I work on.
-- 
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 v2 11/53] fast-import: convert to struct object_id

[brian m. carlson]

On Mon, May 01, 2017 at 06:27:58PM -0400, Jeff King wrote:
> If you have 40 hex digits, then you have 20 hex pairs. But delimiting
> them all takes only 19 slashes, since they only go in between pairs[1].
> 
> So the fully expanded formula is:
> 
>   GIT_MAX_HEXSZ +   (1) actual hex bytes
>   (GIT_MAX_HEXSZ / 2) - 1 + (2) internal slashes between pairs
>   1 (3) NUL terminator
> 
> which simplifies to 3/2 GIT_MAX_HEXSZ. It may be better to write it out
> (the compiler can simplify) or explain that in the comment, though. It
> took me a minute to figure out that it was correct, too.

I did simplify it like that, but I can improve the comment and the
computation, sure.  Clearly my comment wasn't good enough to avoid
confusion, so I appreciate the feedback.  I probably didn't think about
because I've touched the notes code recently, where the use of "19" made
it more obvious.
-- 
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 v2 39/53] refs/files-backend: convert many internals to struct object_id

[brian m. carlson]

On Mon, May 01, 2017 at 04:24:23PM -0700, Jonathan Tan wrote:
> On 04/30/2017 07:29 PM, brian m. carlson wrote:
> > -   if (line->len <= 42)
> > +   if (!line->len)
> > return NULL;
> 
> I would omit this check - parse_oid_hex already exits early if the first
> character is NUL. (The existing code makes a bit more sense, in that it
> avoids checking the first few characters if we already know a bit more about
> the string.)

Okay, I can do that.

> Looks fine, up to here.
> 
> (Also, you requested extra-careful review for this patch, but this patch
> seems mostly mechanical to me.)

Because the rebase ended up moving around a lot of code in a way that I
felt was risky.
-- 
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

Incorrect handling of interactive rebases with root flag

[Frank Paulo Filho]

If I run an interactive rebase on a branch without the --root flag, git
does not modify (change the hash, etc.) any initial commits that are
being "pick"ed in the same order. However, if I use --root, every single
commit is modified. 

Here's an example:

* Old (immediately after running "git rebase -i --root"):
pick d89 root
pick 3e7 bar
pick 57e baz

* New (after editing the "git-rebase-todo" file):
pick d89 root
pick 57e baz
pick 3e7 bar

After this, AFAIK, only baz and bar's hashes should change. However, the
root commit's hash (d89) also changes.

A simpler way to reproduce this is to just run `git rebase -i  --root`
and rebase without changing anything. Every commit will be modified.

Is this supposed to happen?

[PATCH v2] travis-ci: retry if Git for Windows CI returns HTTP error 502 or 503

[Lars Schneider]

The Git for Windows CI web app sometimes returns HTTP errors of
"502 bad gateway" or "503 service unavailable" [1]. We also need to
check the HTTP content because the GfW web app seems to pass through
(error) results from other Azure calls with HTTP code 200.
Wait a little and retry the request if this happens.

[1] 
https://docs.microsoft.com/en-in/azure/app-service-web/app-service-web-troubleshoot-http-502-http-503

Signed-off-by: Lars Schneider 
---

Hi Junio,

I can't really test this as my TravisCI account does not have the
extended timeout and I am unable to reproduce the error.

It would be great if we could test this is a little bit in pu.

Thanks,
Lars

Notes:
Base Ref: next
Web-Diff: https://github.com/larsxschneider/git/commit/af0f0f0eb8
Checkout: git fetch https://github.com/larsxschneider/git 
travisci/win-retry-v2 && git checkout af0f0f0eb8

Interdiff (v1..v2):

diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
index 7a9aa9c6a7..3e5a0abee0 100755
--- a/ci/run-windows-build.sh
+++ b/ci/run-windows-build.sh
@@ -14,26 +14,33 @@ COMMIT=$2

 gfwci () {
local CURL_ERROR_CODE HTTP_CODE
-   exec 3>&1
+   CONTENT_FILE=$(mktemp -t "git-windows-ci-XX")
while test -z $HTTP_CODE
do
HTTP_CODE=$(curl \
-H "Authentication: Bearer $GFW_CI_TOKEN" \
--silent --retry 5 --write-out '%{HTTP_CODE}' \
-   --output >(sed "$(printf '1s/^\xef\xbb\xbf//')" >cat >&3) \
+   --output >(sed "$(printf '1s/^\xef\xbb\xbf//')" >$CONTENT_FILE) 
\
"https://git-for-windows-ci.azurewebsites.net/api/TestNow?$1; \
)
CURL_ERROR_CODE=$?
# The GfW CI web app sometimes returns HTTP errors of
# "502 bad gateway" or "503 service unavailable".
-   # Wait a little and retry if it happens. More info:
+   # We also need to check the HTTP content because the GfW web
+   # app seems to pass through (error) results from other Azure
+   # calls with HTTP code 200.
+   # Wait a little and retry if we detect this error. More info:
# 
https://docs.microsoft.com/en-in/azure/app-service-web/app-service-web-troubleshoot-http-502-http-503
-   if test $HTTP_CODE -eq 502 || test $HTTP_CODE -eq 503
+   if test $HTTP_CODE -eq 502 ||
+  test $HTTP_CODE -eq 503 ||
+  grep "502 - Web server received an invalid response" 
$CONTENT_FILE >/dev/null
then
sleep 10
HTTP_CODE=
fi
done
+   cat $CONTENT_FILE
+   rm $CONTENT_FILE
if test $CURL_ERROR_CODE -ne 0
then
return $CURL_ERROR_CODE

\0

 ci/run-windows-build.sh | 23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/ci/run-windows-build.sh b/ci/run-windows-build.sh
index e043440799..3e5a0abee0 100755
--- a/ci/run-windows-build.sh
+++ b/ci/run-windows-build.sh
@@ -14,14 +14,33 @@ COMMIT=$2

 gfwci () {
local CURL_ERROR_CODE HTTP_CODE
-   exec 3>&1
+   CONTENT_FILE=$(mktemp -t "git-windows-ci-XX")
+   while test -z $HTTP_CODE
+   do
HTTP_CODE=$(curl \
-H "Authentication: Bearer $GFW_CI_TOKEN" \
--silent --retry 5 --write-out '%{HTTP_CODE}' \
-   --output >(sed "$(printf '1s/^\xef\xbb\xbf//')" >cat >&3) \
+   --output >(sed "$(printf '1s/^\xef\xbb\xbf//')" >$CONTENT_FILE) 
\
"https://git-for-windows-ci.azurewebsites.net/api/TestNow?$1; \
)
CURL_ERROR_CODE=$?
+   # The GfW CI web app sometimes returns HTTP errors of
+   # "502 bad gateway" or "503 service unavailable".
+   # We also need to check the HTTP content because the GfW web
+   # app seems to pass through (error) results from other Azure
+   # calls with HTTP code 200.
+   # Wait a little and retry if we detect this error. More info:
+   # 
https://docs.microsoft.com/en-in/azure/app-service-web/app-service-web-troubleshoot-http-502-http-503
+   if test $HTTP_CODE -eq 502 ||
+  test $HTTP_CODE -eq 503 ||
+  grep "502 - Web server received an invalid response" 
$CONTENT_FILE >/dev/null
+   then
+   sleep 10
+   HTTP_CODE=
+   fi
+   done
+   cat $CONTENT_FILE
+   rm $CONTENT_FILE
if test $CURL_ERROR_CODE -ne 0
then
return $CURL_ERROR_CODE

base-commit: 1ea7e62026c5dde4d8be80b2544696fc6aa70121
--
2.12.2

Re: [PATCH 00/10] RFC Partial Clone and Fetch

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> The binary search to lookup a packfile offset from a .idx file
> (which involves disk reads) would take longer for all lookups (not
> just lookups for missing blobs) - I think I prefer keeping the lists
> separate, to avoid pessimizing the (likely) usual case where the
> relevant blobs are all already in local repo storage.

Another relevant operation is looking up objects by offset or
index_nr.  The current implementation involves building an in-memory
reverse index on demand by reading the idx file and sorting it by
offset --- see pack-revindex.c::create_pack_revindex.  This takes O(n
log n) time where n is the size of the idx file.

That said, it could be avoided by storing an on-disk reverse index
with the pack.  That's something we've been wanting to do anyway.

Thanks,
Jonathan

[PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck

[Jean-Noel Avila]

"git read-tree -m" requires a tree argument to name the tree to be
merged in.  Git uses a cutesy error message to say so and why:

$ git read-tree -m
warning: read-tree: emptying the index with no arguments is
deprecated; use --empty
fatal: just how do you expect me to merge 0 trees?
$ git read-tree -m --empty
fatal: just how do you expect me to merge 0 trees?

When lucky, that could produce an ah-hah moment for the user, but it's
more likely to irritate and distract them.

Instead, tell the user plainly that the tree argument is
required. Also document this requirement in the git-read-tree(1)
manpage where there is room to explain it in a more straightforward way.

Signed-off-by: Jean-Noel Avila 
Signed-off-by: Jonathan Nieder 
---
 Documentation/git-read-tree.txt | 8 
 builtin/read-tree.c | 7 ---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index ed9d63ef4..7cd9c6306 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -135,10 +135,10 @@ OPTIONS
 
 Merging
 ---
-If `-m` is specified, 'git read-tree' can perform 3 kinds of
-merge, a single tree merge if only 1 tree is given, a
-fast-forward merge with 2 trees, or a 3-way merge if 3 trees are
-provided.
+If `-m` is specified, at least one tree must be given on the command
+line. 'git read-tree' can perform 3 kinds of merge, a single tree
+merge if only 1 tree is given, a fast-forward merge with 2 trees, or a
+3-way merge if 3 trees are provided.
 
 
 Single Tree Merge
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 23e212ee8..68c5b0ca4 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -132,7 +132,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
OPT_BOOL(0, "empty", _empty,
N_("only empty the index")),
OPT__VERBOSE(_update, N_("be verbose")),
-   OPT_GROUP(N_("Merging")),
+   OPT_GROUP(N_("Merging (needs at least one tree-ish")),
OPT_BOOL('m', NULL, ,
 N_("perform a merge in addition to a read")),
OPT_BOOL(0, "trivial", _merges_only,
@@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
setup_work_tree();
 
if (opts.merge) {
-   if (stage < 2)
-   die("just how do you expect me to merge %d trees?", 
stage-1);
switch (stage - 1) {
+   case 0:
+   die("you must specify at least one tree to merge");
+   break;
case 1:
opts.fn = opts.prefix ? bind_merge : oneway_merge;
break;
-- 
2.12.0

[PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open

[Jean-Noel Avila]

git-filter-branch requires the specification of a branch by one way or
another. If no branch appears to have been specified, we know the user
got the usage wrong but we don't know what they were trying to do ---
e.g. maybe they specified the ref to rewrite but in the wrong place.

The safest solution is to just print the usage in this case.

Signed-off-by: Jean-Noel Avila 
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 2b8cdba15..bda2bae23 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \
 sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
 
 test -s "$tempdir"/heads ||
-   die "Which ref do you want to rewrite?"
+   usage
 
 GIT_INDEX_FILE="$(pwd)/../index"
 export GIT_INDEX_FILE
-- 
2.12.0

[PATCH v2 1/3] usability: don't ask questions if no reply is required

[Jean-Noel Avila]

There has been a bug report by a corporate user that stated that
"spelling mistake of stash followed by a yes prints character 'y'
infinite times."

This analysis was false. When the spelling of a command contains
errors, the git program tries to help the user by providing candidates
which are close to the unexisting command. E.g Git prints the
following:

git: 'stahs' is not a git command. See 'git --help'.
Did you mean this?

stash

and then exits.

The problem with this hint is that it is not formally indicated as an
hint and the user is in fact encouraged to reply to the question,
whereas the Git command is already finished.

The user was unlucky enough that it was the command he was looking
for, and replied "yes" on the command line, effectively launching the
`yes` program.

The initial error is that the Git programs, when launched in
command-line mode (without interaction) must not ask questions,
because these questions would normally require a user input as a reply
while they won't handle indeed. That's a source of confusion on UX
level.

To improve the general usability of the Git suite, the following rule
was applied:

if the sentence
 * appears in a non-interactive session
 * is printed last before exit
 * is a question addressing the user ("you")

the sentence is turned into affirmative and proposes the option.

The basic rewording of the question sentences has been extended to
other spots found in the source.

Requested at https://github.com/git/git-scm.com/issues/999 by rpai1

Signed-off-by: Jean-Noel Avila 
---
 builtin/am.c   | 4 ++--
 builtin/checkout.c | 2 +-
 help.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e..f5afa438d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1312,7 +1312,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
}
 
if (is_empty_file(am_path(state, "patch"))) {
-   printf_ln(_("Patch is empty. Was it split wrong?"));
+   printf_ln(_("Patch is empty. It may have been split wrong."));
die_user_resolve(state);
}
 
@@ -1940,7 +1940,7 @@ static void am_resolve(struct am_state *state)
 
if (unmerged_cache()) {
printf_ln(_("You still have unmerged paths in your index.\n"
-   "Did you forget to use 'git add'?"));
+   "You might want to use 'git add' on them."));
die_user_resolve(state);
}
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f3..05037b9b6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1287,7 +1287,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
 */
if (opts.new_branch && argc == 1)
die(_("Cannot update paths and switch to branch '%s' at 
the same time.\n"
- "Did you intend to checkout '%s' which can not be 
resolved as commit?"),
+ "'%s' can not be resolved as commit, but it 
should."),
opts.new_branch, argv[0]);
 
if (opts.force_detach)
diff --git a/help.c b/help.c
index bc6cd19cf..4658a55c6 100644
--- a/help.c
+++ b/help.c
@@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
 
if (SIMILAR_ENOUGH(best_similarity)) {
fprintf_ln(stderr,
-  Q_("\nDid you mean this?",
- "\nDid you mean one of these?",
+  Q_("\nThe most approaching command is",
+ "\nThe most approaching commands are",
   n));
 
for (i = 0; i < n; i++)
-- 
2.12.0

Re: [PATCH v3 12/25] split_commit_in_progress(): fix memory leak

[René Scharfe]

Am 02.05.2017 um 18:02 schrieb Johannes Schindelin:

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
  wt-status.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 0a6e16dbe0f..1f3f6bcb980 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status *s)
char *rebase_orig_head = 
read_line_from_git_path("rebase-merge/orig-head");
  
  	if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||

-   !s->branch || strcmp(s->branch, "HEAD"))
+   !s->branch || strcmp(s->branch, "HEAD")) {
+   free(head);
+   free(orig_head);
+   free(rebase_amend);
+   free(rebase_orig_head);
return split_in_progress;
+   }
  
  	if (!strcmp(rebase_amend, rebase_orig_head)) {

if (strcmp(head, rebase_amend))



The return line could be replaced by "; else" to achieve the same
result, without duplicating the free calls.

René

Re: [PATCH 00/10] RFC Partial Clone and Fetch

[Jonathan Tan]

On 05/03/2017 09:38 AM, Jeff Hostetler wrote:



On 3/8/2017 1:50 PM, g...@jeffhostetler.com wrote:

From: Jeff Hostetler 


[RFC] Partial Clone and Fetch
=
[...]
E. Unresolved Thoughts
==

*TODO* The server should optionally return (in a side-band?) a list
of the blobs that it omitted from the packfile (and possibly the sizes
or sha1_object_info() data for them) during the fetch-pack/upload-pack
operation.  This would allow the client to distinguish from invalid
SHAs and missing ones.  Size information would allow the client to
maybe choose between various servers.


Since I first posted this, Jonathan Tan has started a related
discussion on missing blob support.
https://public-inbox.org/git/cagf8dgk05+f4ux-8+imfvqd0n2jp6yxj18ag8udaeh6qc6s...@mail.gmail.com/T/


I want to respond to both of these threads here.
-


Thanks for your input. I see that you have explained both "storing 
'positive' information about missing blobs" and "what to store with 
those positive information"; I'll just comment on the former for now.



Missing-Blob Support


Let me offer up an alternative idea for representing
missing blobs.  This is differs from both of our previous
proposals.  (I don't have any code for this new proposal,
I just want to think out loud a bit and see if this is a
direction worth pursuing -- or a complete non-starter.)

Both proposals talk about detecting and adapting to a missing
blob and ways to recover -- when we fail to find a blob.
Comments on the thread asked about:
() being able to detect missing blobs vs corrupt repos
() being unable to detect duplicate blobs
() expense of blob search.

Suppose we store "positive" information about missing blobs?
This would let us know that a blob is intentionally missing
and possibly some meta-data about it.


I thought about this (see "Some alternative designs" in [1]), listing 
some similar benefits, but concluded that "it is difficult to scale to 
large repos".


Firstly, to be clear, by large repos I meant (and mean) the svn-style 
"monorepos" that Jonathan Nieder mentions as use case "A" [2].


My concern is that such lists (whether in separate file(s) or in .idx 
files) would be too unwieldy to manipulate. Even if we design things to 
avoid modifying such lists (for example, by adding a new list whenever 
we fetch instead of trying to modify an existing one), we would at least 
need to sort their contents (for example, when generating an .idx in the 
first place). For a repo with 10M-100M blobs [3], this might be doable 
on today's computers, but I would be concerned if a repo would exceed 
such numbers.


[1] <20170426221346.25337-1-jonathanta...@google.com>
[2] <20170503182725.gc28...@aiede.svl.corp.google.com>
[3] In Microsoft's announcement of Git Virtual File System [4], they 
mentioned "over 3.5 million files" in the Windows codebase. I'm not sure 
if this refers to files in a snapshot (that is, working copy) or all 
historical versions.
[4] 
https://blogs.msdn.microsoft.com/visualstudioalm/2017/02/03/announcing-gvfs-git-virtual-file-system/



1. Suppose we update the .pack file format slightly.
   () We use the 5 value in "enum object_type" to mean a
  "missing-blob".
   () We update git-pack-object as I did in my RFC, but have it
  create type 5 entries for the blobs that are omitted,
  rather than nothing.
   () Hopefully, the same logic that currently keeps pack-object
  from sending unnecessary blobs on subsequent fetches can
  also be used to keep it from sending unnecessary missing-blob
  entries.
   () The type 5 missing-blob entry would contain the SHA-1 of the
  blob and some meta-data to be explained later.


My original idea was to have sorted list(s) of hashes in separate 
file(s) much like the currently existing shallow file; it would have the 
semantics of "a hash here might be present or absent; if it is absent, 
use the hook". (Initially I thought that one list would be sufficient, 
but after reading your idea and considering it some more, multiple lists 
might be better.)


Your idea of storing them in an .idx (and possibly corresponding .pack 
file) is similar, I think. Although mine is probably simpler - at least, 
we wouldn't need a new object_type.


As described above, I don't think this list-of-hashes idea will work 
(because of the large numbers of blobs involved), but I'll compare it to 
yours anyway just in case we end up being convinced that this general 
idea works.



2. Make a similar change in the .idx format and git-index-pack
   to include them there.  Then blob lookup operations could
   definitively determine that a blob exists and is just not
   present locally.

3. With this, packfile-based blob-lookup operations can get a
   "missing-blob" result.
   () It should be possible to short-cut searching in other
  packfiles (because we don't have 

Re: git-clone --config order & fetching extra refs during initial clone

[Jeff King]

On Wed, May 03, 2017 at 04:42:58PM +0200, SZEDER Gábor wrote:

> write_refspec_config() nicely encapsulates writing the proper fetch
> refspec configuration according to the given command line options.  Of
> course these options are already known right at the start, so solely
> in this regard we could call this function earlier.  However, in some
> cases, e.g. '--single-branch', the refspec to be written to the config
> depends not only on the given options but on the refs in the remote
> repository, too, so it can only be written after we got the remote's
> refs.

Good point. We can't really consider clone to be a blind "init + config
+ fetch + checkout" because those middle two steps sometimes overlap
each other.  It really does need to be its own beast.

> The root issue is that 'git clone' calls directly into the fetch
> machinery instead of running 'git fetch' (either via run_command() or
> cmd_fetch()), and some of these "higher-level" config variables are
> only handled in 'builtin/fetch.c' but not in 'git clone'.  By
> "handle" I mean "parse and act accordingly": as it happens, these
> config values are parsed alright when 'git clone' calls remote_get(),
> but it never looks at the relevant fields in the resulting 'struct
> remote'.
> 
> Luckily, many "lower-level" config variables are working properly even
> during 'git clone', because they are handled in the transport layer,
> e.g. 'git clone -c url.https://github.com/.insteadof=gh: gh:git/git'
> does the right thing.

Yeah, and I think that insteadOf is working as intended there (clone
sets it early enough that all of the rest of the code sees it). And the
bug is just that there's special handling in builtin/fetch.c that clone
needs to replicate.

The right solution there is probably pushing that logic down into the
transport layer. Or at the very least abstracting it into a function so
that both clone and fetch can call it without replicating the logic.

> My patch deals with 'remote..refspec', i.e. 'remote->fetch'.
> Apparently some extra care is necessary for 'remote..tagOpt' and
> 'remote->fetch_tags', too.  Perhaps there are more, I haven't checked
> again, and maybe we'll add similar config variables in the future.  So
> I don't think that dealing with such config variables one by one in
> 'git clone', too, is the right long-term solution...  but perhaps it's
> sufficient for the time being?

I think your patch is a strict improvement and we don't need to hold up
waiting for a perfect fix (and because of the --single-branch thing you
mentioned, this may be the best we can do anyway).

> Running a fully-fledged 'git fetch' seems to be simpler and safer
> conceptually, as it would naturally handle all fetch-related config
> variables, present and future.  However, it's not without drawbacks:
> 'git clone' must set the proper config before running 'git fetch' (or
> at least set equivalent cmdline options), which in some cases requires
> the refs in the remote repository, making an additional "list remote
> refs" step necessary (i.e. both 'clone' and 'fetch' would have to
> retrieve the refs from the remote, resulting in more network I/O).

I don't think we ever want to request two ref advertisements; they're
too expensive. If clone needs to do work between the advertisement and
the actual fetch (and it sounds like it does), then it should be using
the transport layer directly. Which is what it's already doing.

-Peff

Re: [PATCH] config.mak.uname: set NO_REGEX=NeedsStartEnd on AIX

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Wed, May 3, 2017 at 12:47 PM, Jonathan Nieder  wrote:

>> Is there e.g. a build farm where we can check for this kind of thing
>> more systematically on supported platforms?
>
> There is the OpenSuse build farm that provides builds for all kinds of
> linux distributions, though we'd rather be looking for *all*
> operating systems, not just various flavors of linux.
>
> After some research, I found
> https://gcc.gnu.org/wiki/CompileFarm
> https://launchpad.net/builders
> https://buildd.debian.org/
>
> The gcc build farm would include AIX, maybe we could talk to
> them to have more CI support on more platforms?

Thanks for the pointers.

> Also you're a DD, maybe we could hook up git testing on debian
> to test for different hardware platforms?

https://buildd.debian.org/status/package.php?p=git=experimental
shows test suite results for Debian's various platforms running "next".

Hope that helps,
Jonathan

Re: Cloning a custom refspec does not work as expected

[Jeff King]

On Wed, May 03, 2017 at 02:16:47PM +0200, Sebastian Schuberth wrote:

> I'd assume that this would work:
> 
> $ git clone -c 
> remote.origin.fetch=+refs/changes/*:refs/remotes/origin/changes/*
> https://gerrit.googlesource.com/git-repo
> 
> In fact, this *does* add the custom refs correct to .git/config, but
> they are not fetched during the clone. I can manually fix that by
> doing
> 
> $ cd git-repo
> $ git fetch
> 
> afterwards, but the whole point is to avoid exactly that separate step.
> 
> Is this a code bug or documentation bug?

I think it's a code bug. More discussion in:

  
http://public-inbox.org/git/robbat2-20170225t185056-4482727...@orbis-terrarum.net/

-Peff

Re: [PATCH] config.mak.uname: set NO_REGEX=NeedsStartEnd on AIX

[Stefan Beller]

On Wed, May 3, 2017 at 12:47 PM, Jonathan Nieder  wrote:
>
> Is there e.g. a build farm where we can check for this kind of thing
> more systematically on supported platforms?

There is the OpenSuse build farm that provides builds for all kinds of
linux distributions, though we'd rather be looking for *all*
operating systems, not just various flavors of linux.

After some research, I found
https://gcc.gnu.org/wiki/CompileFarm
https://launchpad.net/builders
https://buildd.debian.org/

The gcc build farm would include AIX, maybe we could talk to
them to have more CI support on more platforms?

Also you're a DD, maybe we could hook up git testing on debian
to test for different hardware platforms?

Thanks,
Stefan

Re: [PATCH v3 01/25] mingw: avoid memory leak when splitting PATH

[René Scharfe]

Am 02.05.2017 um 18:00 schrieb Johannes Schindelin:

In the (admittedly, concocted) case that PATH consists only of colons, we
would leak the duplicated string.


Nit: It's about semicolons, right?  At least that's what get_path_split
is searching for.  Or is there some kind of translation going on?

René

Re: [PATCH v3 02/25] winansi: avoid use of uninitialized value

[René Scharfe]

Am 02.05.2017 um 18:01 schrieb Johannes Schindelin:

When stdout is not connected to a Win32 console, we incorrectly used an
uninitialized value for the "plain" character attributes.

Detected by Coverity.

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

diff --git a/compat/winansi.c b/compat/winansi.c
index 793420f9d0d..fd6910746c8 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,6 +105,8 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, ))
return 0;
+   sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
+   FOREGROUND_RED;
} else if (!GetConsoleScreenBufferInfo(hcon, ))
return 0;
  


So is_console is called with fd being 1 (stdout), 2 (stderr) and 0
(stdin), in that order.  If the first two calls abort early for some
reason we may end up here.  The added code is for white text on black
background.  An alias for that combination, FOREGROUND_ALL, is defined
a few lines down; for a minimal fix it's not worth moving it up.  attr
and plain_attr are both initialized to sbi.wAttributes.

That as a bit more complicated than it looked initially.  The order of
calls is important, "stdout" in the commit message includes stderr as
well and it doesn't just affect plain_attr.

Would a value of 0 (black text on black background) suffice if only
stdin is connected to a console?  Colors don't matter if there is
nothing to see, right?

Anyway, the change makes sense as is, but it took me a while to get it.

René

Re: [PATCH] config.mak.uname: set NO_REGEX=NeedsStartEnd on AIX

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Set the NO_REGEX=NeedsStartEnd Makefile flag by default on AIX.
>
> Since commit 2f8952250a ("regex: add regexec_buf() that can work on a
> non NUL-terminated string", 2016-09-21) git has errored out at
> compile-time if the regular expression library doesn't support
> REG_STARTEND.
>
> While looking through Google search results for the use of NO_REGEX I
> found a Chef recipe that set this on AIX[1], looking through the
> documentation for the latest version of AIX (7.2, released October
> 2015) shows that its regexec() doesn't have REG_STARTEND.
>
> 1. 
> https://github.com/chef/omnibus-software/commit/e247e36761#diff-3df898345d670979b74acc0bf71d8c47
> 2. 
> https://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.basetrf2/regexec.htm
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  config.mak.uname | 1 +
>  1 file changed, 1 insertion(+)

Thanks.

Reviewed-by: Jonathan Nieder 

Is there e.g. a build farm where we can check for this kind of thing
more systematically on supported platforms?

Re: [PATCH v3] l10n: de.po: update German translation

[Christian Brabandt]

On Mi, 03 Mai 2017, Ralf Thielow wrote:

> Ref or Reference is translated as "Referenz" while
> Rev or Revision is translated as "Commit" so I think
> the translation is correct.

Oh right. I also noticed that sometimes complete sentences were used and 
sometimes not. It would be nice to have a consistent style there too.

Thanks for updating. 

Best,
Christian
-- 
Hallo Briefmarkensammler!

Re: [PATCH 3/4] read-tree.c: rework UI when merging no trees

[Jean-Noël AVILA]

Le mercredi 3 mai 2017, 10:04:01 CEST Jonathan Nieder a écrit :
> Hi,
> 
> Jean-Noel Avila wrote:
> > Subject: read-tree.c: rework UI when merging no trees
> 
> nit: this is about user-facing behavior, not an implementation detail,
> so the part before the colon can be the command that changed
> (read-tree:).
> 
> nit: the word "rework" is dangerous in a commit message in the same
> way as the word "fix" --- it stands for "make better", in a vague way
> that leaves the reader guessing about how.  Usually a more specific
> description can work better.
> 

In fact, this patch is two fold:

 * reword the question in the die() call. I realize now that when passed to 
die(), the string is prepended with "fatal:". That's an hint that the question 
does not require a reply, but  ruling out any doubt would be better.
 * rework the local logic which was inherited from history. This is 
functionally equivalent to the previous version, just cleaner.

> > The initial test was inherited from a previous commit, but it is no
> > longer needed, given the following switch case. Moreover, the question
> > sentence ending the program has been replace by an assertative one.
> > 
> > Signed-off-by: Jean-Noel Avila 
> 
> This can have a simpler, short-and-sweet motivation:
> 
>   read-tree -m: make error message for merging 0 trees less smart-alecky
> 
>   "git read-tree -m" requires a tree argument to name the tree to be
>   merged in.  Git uses a cutesy error message to say so and why:
> 
>   $ git read-tree -m
>   warning: read-tree: emptying the index with no arguments is 
> deprecated;
> use --empty fatal: just how do you expect me to merge 0 trees?
>   $ git read-tree -m --empty
>   fatal: just how do you expect me to merge 0 trees?
> 
>   When lucky, that could produce an ah-hah moment for the user, but it's
>   more likely to irritate and distract them.
> 
>   Instead, tell the user plainly that the tree argument is required. Also
>   document this requirement in the git-read-tree(1) manpage where there is
>   room to explain it in a more straightforward way.
> 

Thank you very much for this message! May I s-o-b you?

As hinted, I'll add the documentation part. ;-)

> Unfortunately both 'git read-tree -h' and 'git read-tree --help' say nothing
> about this.  Ideas for wording there?

Next pach series will propose this.

> 
> Thanks and hope that helps,
> Jonathan

Re: [PATCH 2/4] usability: fix am and checkout for nevermind questions

[Jean-Noël AVILA]

Le mercredi 3 mai 2017 09:51:58 CEST, vous avez écrit :
> Jean-Noel Avila wrote:
> > Subject: usability: fix am and checkout for nevermind questions
> > 
> > Signed-off-by: Jean-Noel Avila 
> 
> Thanks for working on improving Git's UX.  I agree with the goal in
> general (we should not gratuitously surprise users) but I think I
> lack context for appreciating this particular example.
> 
> This is a good place to describe the motivation behind the patch and
> what effective change it would have.
> 
> [...]
> 
> > +++ b/builtin/am.c
> 
> [...]
> 
> > if (is_empty_file(am_path(state, "patch"))) {
> > 
> > -   printf_ln(_("Patch is empty. Was it split wrong?"));
> > +   printf_ln(_("Patch is empty. It may have been split wrong."));
> 
> [...]
> 
> > if (unmerged_cache()) {
> > 
> > printf_ln(_("You still have unmerged paths in your index.\n"
> > 
> > -   "Did you forget to use 'git add'?"));
> > +   "You might want to use 'git add' on them."));
> 
> [...]
> 
> > if (opts.new_branch && argc == 1)
> > 
> > die(_("Cannot update paths and switch to branch '%s' at 
> > the same
> > time.\n"
> > 
> > - "Did you intend to checkout '%s' which can not be 
> > resolved 
as
> > commit?"), +  "'%s' can not be resolved as 
> > commit, but it
> > should."),
> 
> In the current state I think this patch makes things worse (questions
> are not automatically a bad thing), which would make it especially
> useful to see more about the motivation so we can find out whether
> there's another way.
> 

I am not a UX designer, but for me, in the context of interaction with a 
command line program, any question that does not accept a reply is bad design. 
That also means that any command that does not run interactively should not 
ask questions. The shell interface is too informal to allow being loose on the 
program side. Comparatively to a GUI, where a label is formally informative 
and a popping-up dialog box asks for user input.

This patch should indeed be squashed with the first one. They are small changes 
in strings printed when dying.  They would share the more extended commit 
message.

Re: [PATCH 1/7] t7300: skip untracked dirs containing ignored files

[Stefan Beller]

On Wed, May 3, 2017 at 11:26 AM, Samuel Lijin  wrote:
> On Wed, May 3, 2017 at 1:19 PM, Stefan Beller  wrote:
>> On Tue, May 2, 2017 at 8:29 PM, Samuel Lijin  wrote:
>>> If git sees a directory which contains only untracked and ignored
>>> files, clean -d should not remove that directory.
>>
>> Yes that states a fact; it is not clear why we want to have this test here
>> and now. (Is it testing for a recently fixed regression?)
>
> It was recently discovered that this is not true of clean -d (and I'm
> not sure if it ever was, to be honest). See
> http://public-inbox.org/git/xmqqshkof6jd@gitster.mtv.corp.google.com/T/#mf541c06250724bb000461d210b4ed157e415a596
>
>> Are you just introducing the test to demonstrate it keeps working later on?
>> Do you plan on changing this behavior in a later patch?)
>
> The idea was to introduce the broken test and then have the rest of
> the patch series fix it;

This is valuable information for the commit message.
Also to keep git-bisect happy we want to have all tests passing on all commits,
which this does not. However to further signal your intent, you can mark it
as test_expect_failure. This is marking a test as "if Git was not buggy, this
is a test that would pass", so it actually marks success when it
fails, for example
when running t7300:

...
ok 31 - should not clean submodules
ok 32 - should avoid cleaning possible submodules
ok 33 - nested (empty) git should be kept
ok 34 - nested bare repositories should be cleaned
not ok 35 - nested (empty) bare repositories should be cleaned even
when in .git # TODO known breakage
not ok 36 - nested (non-empty) bare repositories should be cleaned
even when in .git # TODO known breakage
ok 37 - giving path in nested git work tree will remove it
ok 38 - giving path to nested .git will not remove it
ok 39 - giving path to nested .git/ will remove contents
ok 40 - force removal of nested git work tree
...

Similarly if you were to mark that test as expecting failure, we'd see:

no ok  - 45 git clean -d skips untracked dirs containing ignored files
# TODO known breakage

And then in a later patch, when you actually fix the bug, you would just flip it
to test_expect_success.

Thanks,
Stefan

Re: [PATCH 00/10] RFC Partial Clone and Fetch

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:
> On 3/8/2017 1:50 PM, g...@jeffhostetler.com wrote:

>> [RFC] Partial Clone and Fetch
>> =
>> [...]
>> E. Unresolved Thoughts
>> ==
>>
>> *TODO* The server should optionally return (in a side-band?) a list
>> of the blobs that it omitted from the packfile (and possibly the sizes
>> or sha1_object_info() data for them) during the fetch-pack/upload-pack
>> operation.  This would allow the client to distinguish from invalid
>> SHAs and missing ones.  Size information would allow the client to
>> maybe choose between various servers.
>
> Since I first posted this, Jonathan Tan has started a related
> discussion on missing blob support.
> https://public-inbox.org/git/cagf8dgk05+f4ux-8+imfvqd0n2jp6yxj18ag8udaeh6qc6s...@mail.gmail.com/T/
>
> I want to respond to both of these threads here.

Thanks much for this.

> Missing-Blob Support
> 
>
> Let me offer up an alternative idea for representing
> missing blobs.  This is differs from both of our previous
> proposals.  (I don't have any code for this new proposal,
> I just want to think out loud a bit and see if this is a
> direction worth pursuing -- or a complete non-starter.)
>
> Both proposals talk about detecting and adapting to a missing
> blob and ways to recover -- when we fail to find a blob.
> Comments on the thread asked about:
> () being able to detect missing blobs vs corrupt repos
> () being unable to detect duplicate blobs
> () expense of blob search.
>
> Suppose we store "positive" information about missing blobs?
> This would let us know that a blob is intentionally missing
> and possibly some meta-data about it.

We've discussed this a little informally but didn't go more into
it, so I'm glad you brought it up.

There are two use cases I care about.  I'll want names to refer to
them later, so describing them now:

 A. A p4 or svn style "monorepo" containing all code within a company.
We want to make git scale well when working with such a
repository.  Disk usage, network usage, index size, and object
lookup time are all issues for such a repository.

A repository can creep up in size so it starts falling into this
category even though git coped well with it before.  Another way
to end up in this category is a conversion from a version control
system like p4 or svn.

 B. A more modestly sized repository with some large blobs in it.  At
clone time we want to omit unneeded large blobs to speed up the
clone, save disk space, and save bandwidth.

For this kind of repository, the idx file already contained all
those blobs and that was not causing problems --- the only problem
was the actual blob size.

> 1. Suppose we update the .pack file format slightly.
[...]
> 2. Make a similar change in the .idx format and git-index-pack
>to include them there.  Then blob lookup operations could
>definitively determine that a blob exists and is just not
>present locally.

Some nits:

- there shouldn't be any need for the blobs to even be mentioned in
  the .pack stored locally.  The .idx file maps from sha1 to offset
  within the packfile --- a special offset could mean "this is a
  missing blob".

- Git protocol works by sending pack files over the wire.  The idx
  file is not transmitted to the client --- the client instead
  reconstructs it from the pack file.  I assume this is why you
  stored the SHA-1 of the object in the pack file, but it could
  equally well be sent through another stream (since this proposal
  involves a change to git protocol anyway).

- However, the list of missing blobs can be inferred from the existing
  pack format, without a change to the pack format used in git
  protocol.  As part of constructing the idx, "git index-pack"
  inflates every object in the pack file sent by the server.  This
  means we know what blobs they reference, so we can easily produce a
  list for the idx file without changing the pack file format.

If this is done by only changing the idx file format and not the pack
file, then it does not affect the protocol.  That is good for
experimentation --- it lets us try out different formats client-side
without having to coordinate with server authors.

In case (A), this proposal means we get back some of the per-object
overhead that we were trying to avoid.  I would like to avoid that
if possible.

In case (B), this proposal doesn't hurt.

One problem with proposals so far has been how to handle repositories
with multiple remotes.  Having a local list of missing blobs is
convenient because it can be associated to the remote --- when a blob
is referenced later, we know which remote to ask for for it.  This is
a niche feature, but it's a nice bonus.

[...]
> 3. With this, packfile-based blob-lookup operations can get a
>"missing-blob" result.
>() It should be possible to short-cut searching in other
>   packfiles (because we don't have to assume that the 

Re: Cloning a custom refspec does not work as expected

[Stefan Beller]

On Wed, May 3, 2017 at 5:16 AM, Sebastian Schuberth
 wrote:
> Hi,
>
> I'm trying to clone a custom ref, avoiding the need to init && fetch
> manually. From reading [1] which says:
>
> "Set a configuration variable in the newly-created repository; this
> takes effect immediately after the repository is initialized, but
> before the remote history is fetched or any files checked out. [...]
> This makes it safe, for example, to add additional fetch refspecs to
> the origin remote."
>
> I'd assume that this would work:
>
> $ git clone -c 
> remote.origin.fetch=+refs/changes/*:refs/remotes/origin/changes/*
> https://gerrit.googlesource.com/git-repo
>
> In fact, this *does* add the custom refs correct to .git/config, but
> they are not fetched during the clone. I can manually fix that by
> doing
>
> $ cd git-repo
> $ git fetch
>
> afterwards, but the whole point is to avoid exactly that separate step.
>
> Is this a code bug or documentation bug?

I would blame the code to be buggy, as it first writes out the config
and then doesn't read the config, but blindly performs some magic
to come up with the refspec to fetch and performs that fetch.

Thanks,
Stefan

>
> I'm using git version 2.12.2.windows.2.
>
> [1] https://git-scm.com/docs/git-clone#git-clone---configltkeygtltvaluegt
>
> --
> Sebastian Schuberth

Re: [PATCH 1/7] t7300: skip untracked dirs containing ignored files

[Samuel Lijin]

On Wed, May 3, 2017 at 1:19 PM, Stefan Beller  wrote:
> On Tue, May 2, 2017 at 8:29 PM, Samuel Lijin  wrote:
>> If git sees a directory which contains only untracked and ignored
>> files, clean -d should not remove that directory.
>
> Yes that states a fact; it is not clear why we want to have this test here
> and now. (Is it testing for a recently fixed regression?)

It was recently discovered that this is not true of clean -d (and I'm
not sure if it ever was, to be honest). See
http://public-inbox.org/git/xmqqshkof6jd@gitster.mtv.corp.google.com/T/#mf541c06250724bb000461d210b4ed157e415a596

> Are you just introducing the test to demonstrate it keeps working later on?
> Do you plan on changing this behavior in a later patch?)

The idea was to introduce the broken test and then have the rest of
the patch series fix it; Junio pointed out to me (off-list, since he
was responding from his phone, I think) that I got the convention
backwards, so I'll be changing this in the next version.

>> ---
>>  t/t7300-clean.sh | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
>> index b89fd2a6a..948a455e8 100755
>> --- a/t/t7300-clean.sh
>> +++ b/t/t7300-clean.sh
>> @@ -653,4 +653,14 @@ test_expect_success 'git clean -d respects pathspecs 
>> (pathspec is prefix of dir)
>> test_path_is_dir foobar
>>  '
>>
>> +test_expect_success 'git clean -d skips untracked dirs containing ignored 
>> files' '
>> +   echo /foo/bar >.gitignore &&
>> +   rm -rf foo &&
>> +   mkdir -p foo &&
>> +   touch foo/bar &&
>> +   git clean -df &&
>> +   test_path_is_file foo/bar &&
>> +   test_path_is_dir foo
>> +'
>
> The test makes sense, though I am wondering if we can integrate
> this test into another test e.g. "ok 15 - git clean -d".
>
> Is the -f flag needed?

Will look into both.

> Thanks,
> Stefan

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Samuel Lijin]

On Wed, May 3, 2017 at 12:14 PM, Stefan Beller  wrote:
> On Wed, May 3, 2017 at 4:31 AM, Samuel Lijin  wrote:
>>
>> Just to throw out an example, I'm relatively new to the codebase (I've
>> been lurking on the mailing list for a few months now) and for a
>> recent project (I'm an undergrad wrapping up my senior year, and one
>> of my classes' final projects was to do something that involved
>> concurrency) I took a shot at parallelizing the estimate_similarity()
>> calls in diffcore_rename(). The only way I was able to get it to work
>> was by dropping global mutexes in one or two files (the code for those
>> mutexes still makes me cringe), because of concurrent writes to global
>> data structures.
>
> That sounds like a challenge. As we have many globals, we need to be
> very careful about threading.
>
> Also an interesting discussion about threading:
> https://public-inbox.org/git/9e4733910708111412t48c1beaahfbaa2c68a02f6...@mail.gmail.com/

Thanks.

> Are the patches available for discussion?

I was planning on revisiting the patch series before sending it out -
the changes in attr.c and sha1_file.c are not pretty (and I'm pretty
sure one of them is non-portable) - but it is published at
https://github.com/sxlijin/git/commits/parallelize.thread-pool.1 (it's
based off v2.12.2).

> Thanks,
> Stefan

Re: [PATCH 1/7] t7300: skip untracked dirs containing ignored files

[Stefan Beller]

On Tue, May 2, 2017 at 8:29 PM, Samuel Lijin  wrote:
> If git sees a directory which contains only untracked and ignored
> files, clean -d should not remove that directory.

Yes that states a fact; it is not clear why we want to have this test here
and now. (Is it testing for a recently fixed regression?)
Are you just introducing the test to demonstrate it keeps working later on?
Do you plan on changing this behavior in a later patch?)

> ---
>  t/t7300-clean.sh | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index b89fd2a6a..948a455e8 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -653,4 +653,14 @@ test_expect_success 'git clean -d respects pathspecs 
> (pathspec is prefix of dir)
> test_path_is_dir foobar
>  '
>
> +test_expect_success 'git clean -d skips untracked dirs containing ignored 
> files' '
> +   echo /foo/bar >.gitignore &&
> +   rm -rf foo &&
> +   mkdir -p foo &&
> +   touch foo/bar &&
> +   git clean -df &&
> +   test_path_is_file foo/bar &&
> +   test_path_is_dir foo
> +'

The test makes sense, though I am wondering if we can integrate
this test into another test e.g. "ok 15 - git clean -d".

Is the -f flag needed?

Thanks,
Stefan

Re: [PATCH 3/7] dir: add method to check if a dir_entry lexically contains another

[Stefan Beller]

On Tue, May 2, 2017 at 8:29 PM, Samuel Lijin  wrote:
> Introduce a method that allows us to check if one dir_entry corresponds
> to a path which contains the path corresponding to another dir_entry.
> ---
>  dir.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/dir.c b/dir.c
> index 6bd0350e9..25cb9eadf 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1852,6 +1852,14 @@ static int cmp_name(const void *p1, const void *p2)
> return name_compare(e1->name, e1->len, e2->name, e2->len);
>  }
>
> +// check if *out lexically contains *in

Thanks for adding a comment to describe what the function ought to do.

However our Coding style prefers

/* comments this way */

/*
 * or in case of multi-
 * line comments,
 * this way.
 */

I think one of the ancient compilers just dislikes // as comment style.

> +static int check_contains(const struct dir_entry *out, const struct 
> dir_entry *in)
> +{
> +   return (out->len < in->len) &&
> +   (out->name[out->len - 1] == '/') &&
> +   !memcmp(out->name, in->name, out->len);
> +}
> +
>  static int treat_leading_path(struct dir_struct *dir,
>   const char *path, int len,
>   const struct pathspec *pathspec)
> --
> 2.12.2
>

Re: [PATCH 1/4] usability: don't ask questions if no reply is required

[Jean-Noël AVILA]

Le mercredi 3 mai 2017, 09:47:44 CEST Jonathan Nieder a écrit :
> Hi,
> 
> Jean-Noel Avila wrote:
> > As described in the bug report at
> > 
> > https://github.com/git/git-scm.com/issues/999
> 
> External issue tracker URLs have been known to change or disappear and
> we try to make commit messages self-contained instead of relying on
> them.  It is common to put a 'Requested-by:' footer or sentence saying
> 'Requested at  by ' near the bottom of a commit message
> for attribution and context.  Relying on the bug report more heavily
> like this example (instead of including any relevant information)
> makes it harder for a reader to understand the patch easily in
> one place.
> 
> In other words, instead of asking the reader to read the bug report,
> please include pertinent information the reader needs to
> understand the patch here so they don't have to.

Ok. Will include more context in the commit message and just provide the BT as 
an additional link.

> 
> > the user was disconcerted by the question asked by the program not
> > requiring a reply from the user. To improve the general usability of
> > the Git suite, The following rule was applied:
> > 
> > if the sentence
> > 
> >  * appears in a non-interactive session
> >  * is printed last before exit
> >  * is a question addressing the user ("you")
> > 
> > the sentence is turned into affirmative and proposes the option.
> > 
> > Signed-off-by: Jean-Noel Avila 
> > ---
> > 
> >  help.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/help.c b/help.c
> > index bc6cd19cf..4658a55c6 100644
> > --- a/help.c
> > +++ b/help.c
> > @@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
> > 
> > if (SIMILAR_ENOUGH(best_similarity)) {
> > 
> > fprintf_ln(stderr,
> > 
> > -  Q_("\nDid you mean this?",
> > - "\nDid you mean one of these?",
> > +  Q_("\nThe most approaching command is",
> > + "\nThe most approaching commands are",
> > 
> >n));
> 
> For what it's worth, I find the new text harder to understand than the
> old text.
> 
> From the bug report:
> 
>   Now git says git: 'stahs' is not a git command. See 'git --help'.
>   Did you mean this?
> 
>   stash
> 
>   Git asked if i meant git stash. and i entered yes. and git
>   printed the character y infinite times.
> 
> If I'm reading that correctly, the problem is not that questions are
> alarming but that Git did not cope well with the answer.  When I try
> to reproduce it, I get

No, I don't think that the questions are alarming. The whole point is that Git 
no longer runs when the user enters its reply. In the case of the bug report, 
the user was unlucky to type in the name of the shell command `yes` because he 
was thinking that Git was still running interactively, due to the question at 
the end of the run.

So this patch series'aim is simply to get rid of asking questions just before 
exiting. Even if a question might seem more user friendly, it's insufficiently 
formal to indicate to the user that there's no point replying. The question 
was just a hint, and it should presented as such.

To be fair, I'm not accustomed enough to the code to know exactly in which 
cases the given strings are occurring (except here). All the patch series 
tries to tackle this at different levels. Maybe squashing them all would be 
better for understanding. 

> 
>   $ git stahs
>   WARNING: You called a Git command named 'stahs', which does not exist.
>   Continuing under the assumption that you meant 'stash'
>   in 5.0 seconds automatically...
> 
> which is much clearer.  After commenting out "[help] autocorrect = 50" in my
> ~/.config/git/config, I get
> 
>   $ git stahs
>   git: 'stahs' is not a git command. See 'git --help'.
> 
>   Did you mean this?
>   stash
> 
> which does seem improvable, at least for consistency with the
> autocorrect case.  For example, would something like
> 
>   $ git stahs
>   fatal: You called a Git command named 'stahs', which does not exist.
>   hint: Did you mean 'git stash'?
> 
> work better?  And the autocorrect case could say something like

Would adding a "hint:" prefix be enough to provide context? I don't think so. 
I'd prefer to be clearer on the objectives of the printed information, even at 
the risk of being clumsy.


>
>   $ git stahs
>   warning: You called a Git command named 'stahs', which does not exist.
>   warning: Continuing under the assumption that you meant 'stash'
>   warning: in 5.0 seconds automatically...
> 
> Is contact information for the bug reporter available so we can try out
> different wordings and see what works for them?

I guess so. The discussion on github is still open and only depends on the 
willingness of the reporter to reply.


> 
> Thanks and hope that helps,
> 

Re: [PATCH v4] l10n: de.po: update German translation

[Matthias Rüster]

Hi Ralf,

thanks again for your work!


>  
>  #: ref-filter.c:565
> -#, fuzzy, c-format
> +#, c-format
>  msgid "format: %%(then) atom used after %%(else)"
> -msgstr "Format: %%(end) Atom fehlt"
> +msgstr "format: %%(then) nach %%(else) verwendet"
>  

Is there a "Atom" missing?
"format: %%(then) Atom nach %%(else) verwendet"


>  
>  #: submodule.c:1332
> -#, fuzzy, c-format
> +#, c-format
>  msgid "could not start 'git status' in submodule '%s'"
>  msgstr "Konnte 'git status' in Submodul '%s' nicht starten."
>  
>  #: submodule.c:1345
> -#, fuzzy, c-format
> +#, c-format
>  msgid "could not run 'git status' in submodule '%s'"
>  msgstr "konnte 'git status' in Submodul '%s' nicht ausführen"
>  

Maybe "Konnte 'git status' nicht in Submodul starten/ausführen" would
sound better?

In general: sometimes there is a sentence (starts with a uppercase
letter and ends with a dot) and sometimes not.

So for example also here:

 #: fetch-pack.c:1150
 #, c-format
 msgid "Server does not allow request for unadvertised object %s"
-msgstr ""
+msgstr "Der Server lehnt Anfrage nach nicht angebotenem Objekt %s ab."

I have not looked up the exact circumstances when this would show up,
though.


>  #: submodule.c:1421
> -#, fuzzy, c-format
> +#, c-format
>  msgid "submodule '%s' has dirty index"
> -msgstr "Submodul '%s' kann Alternative nicht hinzufügen: %s"
> +msgstr "Submodul '%s' hat geänderten Index"
>  

I would suggest:
"Submodul '%s' hat einen geänderten Index"


>  
>  #: builtin/tag.c:493
> -#, fuzzy
>  msgid "--contains option is only allowed in list mode"
> -msgstr "--contains Option ist nur erlaubt mit -l."
> +msgstr "--contains Option ist nur im Listenmodus erlaubt"
>  
>  #: builtin/tag.c:495
> -#, fuzzy
>  msgid "--no-contains option is only allowed in list mode"
> -msgstr "--contains Option ist nur erlaubt mit -l."
> +msgstr "Option --no-contains ist nur im Listenmodus erlaubt"
>  

Is "Option --contains ist nur im Listenmodus erlaubt" possible there? So
it would be like the second one...



Kind regards,
Matthias

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Stefan Beller]

On Wed, May 3, 2017 at 4:31 AM, Samuel Lijin  wrote:
> On Tue, May 2, 2017 at 9:05 AM, Jeff Hostetler  wrote:
>>
>>
>> On 5/2/2017 12:17 AM, Stefan Beller wrote:
>>>
>>> On Mon, May 1, 2017 at 6:36 PM, Junio C Hamano  wrote:

 Stefan Beller  writes:

> This applies to origin/master.
>
> For better readability and understandability for newcomers it is a good
> idea
> to not offer 2 APIs doing the same thing with on being the #define of
> the other.
>
> In the long run we may want to drop the macros guarded by
> NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
>>
>>
>> Thank you for bringing this up and making this proposal.
>> I started a similar effort internally last fall, but
>> stopped because of the footprint size.
>>

 Why?  Why should we keep typing _index, when most of the time we
 are given _the_ index and working on it?
>>>
>>>
>>> As someone knowledgeable with the code base you know that the cache_*
>>> and index_* functions only differ by an index argument. A newcomer may not
>>> know this, so they wonder why we have (A) so many functions [and which is
>>> the
>>> right function to use]; it is an issue of ease of use of the code base.
>>>
>>> Anything you do In submodule land today needs to spawn new processes in
>>> the submodule. This is cumbersome and not performant. So in the far future
>>> we may want to have an abstraction of a repo (B), i.e. all repository
>>> state in
>>> one struct/class. That way we can open a submodule in-process and perform
>>> the required actions without spawning a process.
>>>
>>> The road to (B) is a long road, but we have to art somewhere. And this
>>> seemed
>>> like a good place by introducing a dedicated argument for the
>>> repository. In a follow
>>> up in the future we may want to replace _index by
>>> "the_main_repo.its_index"
>>> and then could also run the commands on other (submodule) indexes. But
>>> more
>>> importantly, all these commands would operate on a repository object.
>>>
>>> In such a far future we would have functions like the cmd_* functions
>>> that would take a repository object instead of doing its setup discovery
>>> on their own.
>>>
>>> Another reason may be its current velocity (or absence of it) w.r.t. to
>>> these
>>> functions, such that fewer merge conflicts may arise.
>>
>>
>> In addition to (eventually) allowing multiple repos be open at
>> the same time for submodules, it would also help with various
>> multi-threading efforts.  For example, we have loops that do a
>> "for (k = 0, k < active_nr; k++) {...}"  There is no visual clue
>> in that code that it references "the_index" and therefore should
>> be subject to the same locking.  Granted, this is a trivial example,
>> but goes to the argument that the code has lots of subtle global
>> variables and macros that make it difficult to reason about the
>> code.
>
> Just to throw out an example, I'm relatively new to the codebase (I've
> been lurking on the mailing list for a few months now) and for a
> recent project (I'm an undergrad wrapping up my senior year, and one
> of my classes' final projects was to do something that involved
> concurrency) I took a shot at parallelizing the estimate_similarity()
> calls in diffcore_rename(). The only way I was able to get it to work
> was by dropping global mutexes in one or two files (the code for those
> mutexes still makes me cringe), because of concurrent writes to global
> data structures.

That sounds like a challenge. As we have many globals, we need to be
very careful about threading.

Also an interesting discussion about threading:
https://public-inbox.org/git/9e4733910708111412t48c1beaahfbaa2c68a02f6...@mail.gmail.com/

Are the patches available for discussion?

Thanks,
Stefan

Re: [PATCH 4/4] git-filter-branch: be assertative on dying message

[Jonathan Nieder]

Hi,

Jean-Noel Avila wrote:

> Signed-off-by: Jean-Noel Avila 

As with the previous patches, this is a good place to put the motivation
for the patch.

> ---
>  git-filter-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 2b8cdba15..dd3a605d0 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name 
> \
>  sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
>  
>  test -s "$tempdir"/heads ||
> - die "Which ref do you want to rewrite?"
> + die "You must specify a ref to rewrite"

I find both the old and the new messages pretty uncompelling.  The user
got the usage wrong but we don't know what they were trying to do ---
e.g. maybe they specified the ref to rewrite but in the wrong place.

Would e.g. a simple call to 'usage' work?

Thanks,
Jonathan

Re: [PATCH 3/4] read-tree.c: rework UI when merging no trees

[Jonathan Nieder]

Hi,

Jean-Noel Avila wrote:

> Subject: read-tree.c: rework UI when merging no trees

nit: this is about user-facing behavior, not an implementation detail,
so the part before the colon can be the command that changed
(read-tree:).

nit: the word "rework" is dangerous in a commit message in the same
way as the word "fix" --- it stands for "make better", in a vague way
that leaves the reader guessing about how.  Usually a more specific
description can work better.

> The initial test was inherited from a previous commit, but it is no
> longer needed, given the following switch case. Moreover, the question
> sentence ending the program has been replace by an assertative one.
> 
> Signed-off-by: Jean-Noel Avila 

This can have a simpler, short-and-sweet motivation:

read-tree -m: make error message for merging 0 trees less smart-alecky

"git read-tree -m" requires a tree argument to name the tree to be
merged in.  Git uses a cutesy error message to say so and why:

$ git read-tree -m
warning: read-tree: emptying the index with no arguments is 
deprecated; use --empty
fatal: just how do you expect me to merge 0 trees?
$ git read-tree -m --empty
fatal: just how do you expect me to merge 0 trees?

When lucky, that could produce an ah-hah moment for the user, but it's
more likely to irritate and distract them.

Instead, tell the user plainly that the tree argument is required. Also
document this requirement in the git-read-tree(1) manpage where there is
room to explain it in a more straightforward way.

Unfortunately both 'git read-tree -h' and 'git read-tree --help' say nothing 
about
this.  Ideas for wording there?

Thanks and hope that helps,
Jonathan

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Stefan Beller]

On Wed, May 3, 2017 at 3:27 AM, Duy Nguyen  wrote:
> On Tue, May 2, 2017 at 8:36 AM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> This applies to origin/master.
>>>
>>> For better readability and understandability for newcomers it is a good idea
>>> to not offer 2 APIs doing the same thing with on being the #define of the 
>>> other.
>>>
>>> In the long run we may want to drop the macros guarded by
>>> NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
>>
>> Why?
>
> Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k
>
>> Why should we keep typing _index, when most of the time we are given 
>> _the_ index and working on it?
>
> I attempted the same thing once or twice in the past, had the same
> impression and dropped it. But I think it's good to get rid of cache_*
> macros, at least outside builtin/ directory. It makes it clearer about
> the index dependency. Maybe one day we could libify sha1_file.c and
> finally introduce "struct repository", where the_index, object db and
> ref-store belong (hmm.. the index may belong to "struct worktree", not
> the repo one...).

+cc Brandon, who attempts to come up with a struct repository as we speak.

> So yeah it may look a bit more verbose (and probably causes a lot more
> conflicts on 'pu') but in my opinion it's a good direction. I wish
> Stefan good luck. Brave soul :D

Thanks for the encouragement! As you seem to be interested in this topic,
you may want to help out by reviewing, starting at:
https://public-inbox.org/git/2017050322.21055-1-sbel...@google.com/

Thanks,
Stefan

Re: [PATCH 1/4] usability: don't ask questions if no reply is required

[Stefan Beller]

+cc rashmipa...@gmail.com

On Wed, May 3, 2017 at 9:47 AM, Jonathan Nieder  wrote:
> Hi,
>
> Jean-Noel Avila wrote:
>
>> As described in the bug report at
>>
>> https://github.com/git/git-scm.com/issues/999
>
> External issue tracker URLs have been known to change or disappear and
> we try to make commit messages self-contained instead of relying on
> them.  It is common to put a 'Requested-by:' footer or sentence saying
> 'Requested at  by ' near the bottom of a commit message
> for attribution and context.  Relying on the bug report more heavily
> like this example (instead of including any relevant information)
> makes it harder for a reader to understand the patch easily in
> one place.
>
> In other words, instead of asking the reader to read the bug report,
> please include pertinent information the reader needs to
> understand the patch here so they don't have to.
>
>> the user was disconcerted by the question asked by the program not
>> requiring a reply from the user. To improve the general usability of
>> the Git suite, The following rule was applied:
>>
>> if the sentence
>>  * appears in a non-interactive session
>>  * is printed last before exit
>>  * is a question addressing the user ("you")
>>
>> the sentence is turned into affirmative and proposes the option.
>>
>> Signed-off-by: Jean-Noel Avila 
>> ---
>>  help.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/help.c b/help.c
>> index bc6cd19cf..4658a55c6 100644
>> --- a/help.c
>> +++ b/help.c
>> @@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
>>
>>   if (SIMILAR_ENOUGH(best_similarity)) {
>>   fprintf_ln(stderr,
>> -Q_("\nDid you mean this?",
>> -   "\nDid you mean one of these?",
>> +Q_("\nThe most approaching command is",
>> +   "\nThe most approaching commands are",
>>  n));
>
> For what it's worth, I find the new text harder to understand than the
> old text.
>
> From the bug report:
>
> Now git says git: 'stahs' is not a git command. See 'git --help'.
> Did you mean this?
>
> stash
>
> Git asked if i meant git stash. and i entered yes. and git
> printed the character y infinite times.
>
> If I'm reading that correctly, the problem is not that questions are
> alarming but that Git did not cope well with the answer.  When I try
> to reproduce it, I get
>
> $ git stahs
> WARNING: You called a Git command named 'stahs', which does not exist.
> Continuing under the assumption that you meant 'stash'
> in 5.0 seconds automatically...
>
> which is much clearer.  After commenting out "[help] autocorrect = 50" in my
> ~/.config/git/config, I get
>
> $ git stahs
> git: 'stahs' is not a git command. See 'git --help'.
>
> Did you mean this?
> stash
>
> which does seem improvable, at least for consistency with the
> autocorrect case.  For example, would something like
>
> $ git stahs
> fatal: You called a Git command named 'stahs', which does not exist.
> hint: Did you mean 'git stash'?
>
> work better?  And the autocorrect case could say something like
>
> $ git stahs
> warning: You called a Git command named 'stahs', which does not exist.
> warning: Continuing under the assumption that you meant 'stash'
> warning: in 5.0 seconds automatically...
>
> Is contact information for the bug reporter available so we can try out
> different wordings and see what works for them?

yes, cc'd.
Also see
https://public-inbox.org/git/caoqcaxsozcg8mijv+yattmc1pfgyiosqtrasdbhbp2rrhbo...@mail.gmail.com

>
> Thanks and hope that helps,
> Jonathan

Re: [PATCH 2/4] usability: fix am and checkout for nevermind questions

[Jonathan Nieder]

Jean-Noel Avila wrote:

> Subject: usability: fix am and checkout for nevermind questions
>
> Signed-off-by: Jean-Noel Avila 

Thanks for working on improving Git's UX.  I agree with the goal in
general (we should not gratuitously surprise users) but I think I
lack context for appreciating this particular example.

This is a good place to describe the motivation behind the patch and
what effective change it would have.

[...]
> +++ b/builtin/am.c
[...]
>   if (is_empty_file(am_path(state, "patch"))) {
> - printf_ln(_("Patch is empty. Was it split wrong?"));
> + printf_ln(_("Patch is empty. It may have been split wrong."));
[...]
>   if (unmerged_cache()) {
>   printf_ln(_("You still have unmerged paths in your index.\n"
> - "Did you forget to use 'git add'?"));
> + "You might want to use 'git add' on them."));
[...]
>   if (opts.new_branch && argc == 1)
>   die(_("Cannot update paths and switch to branch '%s' at 
> the same time.\n"
> -   "Did you intend to checkout '%s' which can not be 
> resolved as commit?"),
> +   "'%s' can not be resolved as commit, but it 
> should."),

In the current state I think this patch makes things worse (questions
are not automatically a bad thing), which would make it especially
useful to see more about the motivation so we can find out whether
there's another way.

Thanks,
Jonathan

Re: [PATCH 1/4] usability: don't ask questions if no reply is required

[Jonathan Nieder]

Hi,

Jean-Noel Avila wrote:

> As described in the bug report at
>
> https://github.com/git/git-scm.com/issues/999

External issue tracker URLs have been known to change or disappear and
we try to make commit messages self-contained instead of relying on
them.  It is common to put a 'Requested-by:' footer or sentence saying
'Requested at  by ' near the bottom of a commit message
for attribution and context.  Relying on the bug report more heavily
like this example (instead of including any relevant information)
makes it harder for a reader to understand the patch easily in
one place.

In other words, instead of asking the reader to read the bug report,
please include pertinent information the reader needs to
understand the patch here so they don't have to.

> the user was disconcerted by the question asked by the program not
> requiring a reply from the user. To improve the general usability of
> the Git suite, The following rule was applied:
>
> if the sentence
>  * appears in a non-interactive session
>  * is printed last before exit
>  * is a question addressing the user ("you")
>
> the sentence is turned into affirmative and proposes the option.
>
> Signed-off-by: Jean-Noel Avila 
> ---
>  help.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/help.c b/help.c
> index bc6cd19cf..4658a55c6 100644
> --- a/help.c
> +++ b/help.c
> @@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
>  
>   if (SIMILAR_ENOUGH(best_similarity)) {
>   fprintf_ln(stderr,
> -Q_("\nDid you mean this?",
> -   "\nDid you mean one of these?",
> +Q_("\nThe most approaching command is",
> +   "\nThe most approaching commands are",
>  n));

For what it's worth, I find the new text harder to understand than the
old text.

>From the bug report:

Now git says git: 'stahs' is not a git command. See 'git --help'.
Did you mean this?

stash

Git asked if i meant git stash. and i entered yes. and git
printed the character y infinite times.

If I'm reading that correctly, the problem is not that questions are
alarming but that Git did not cope well with the answer.  When I try
to reproduce it, I get

$ git stahs
WARNING: You called a Git command named 'stahs', which does not exist.
Continuing under the assumption that you meant 'stash'
in 5.0 seconds automatically...

which is much clearer.  After commenting out "[help] autocorrect = 50" in my
~/.config/git/config, I get

$ git stahs
git: 'stahs' is not a git command. See 'git --help'.

Did you mean this?
stash

which does seem improvable, at least for consistency with the
autocorrect case.  For example, would something like

$ git stahs
fatal: You called a Git command named 'stahs', which does not exist.
hint: Did you mean 'git stash'?

work better?  And the autocorrect case could say something like

$ git stahs
warning: You called a Git command named 'stahs', which does not exist.
warning: Continuing under the assumption that you meant 'stash'
warning: in 5.0 seconds automatically...

Is contact information for the bug reporter available so we can try out
different wordings and see what works for them?

Thanks and hope that helps,
Jonathan

[PATCH v4] l10n: de.po: update German translation

[Ralf Thielow]

Translate 96 new messages came from git.pot update in dfc182b (l10n:
git.pot: v2.13.0 round 1 (96 new, 37 removed)).

Signed-off-by: Ralf Thielow 
---
 po/de.po | 323 ---
 1 file changed, 142 insertions(+), 181 deletions(-)

diff --git a/po/de.po b/po/de.po
index b92c4fe11..f597e1baa 100644
--- a/po/de.po
+++ b/po/de.po
@@ -866,9 +866,9 @@ msgid "Argument not supported for format '%s': -%d"
 msgstr "Argument für Format '%s' nicht unterstützt: -%d"
 
 #: attr.c:212
-#, fuzzy, c-format
+#, c-format
 msgid "%.*s is not a valid attribute name"
-msgstr "'%s' ist kein gültiger Name für ein Remote-Repository"
+msgstr "%.*s ist kein gültiger Attributname"
 
 #: attr.c:408
 msgid ""
@@ -1260,6 +1260,8 @@ msgstr "Speicher verbraucht"
 #: config.c:191
 msgid "relative config include conditionals must come from files"
 msgstr ""
+"Bedingungen für das Einbinden von Konfigurationen aus relativen Pfaden 
müssen\n"
+"aus Dateien kommen."
 
 #: config.c:701
 #, c-format
@@ -1379,12 +1381,12 @@ msgstr "Ungültiger %s: '%s'"
 #: config.c:1952
 #, c-format
 msgid "unknown core.untrackedCache value '%s'; using 'keep' default value"
-msgstr ""
+msgstr "Unbekannter Wert '%s' in core.untrackedCache; benutze Standardwert 
'keep'"
 
 #: config.c:1978
 #, c-format
 msgid "splitIndex.maxPercentChange value '%d' should be between 0 and 100"
-msgstr ""
+msgstr "Der Wert '%d' von splitIndex.maxPercentChange sollte zwischen 0 und 
100 liegen."
 
 #: config.c:1989
 #, c-format
@@ -1645,9 +1647,9 @@ msgstr ""
 "für dieses Verzeichnis deaktiviert."
 
 #: dir.c:2776 dir.c:2781
-#, fuzzy, c-format
+#, c-format
 msgid "could not create directories for %s"
-msgstr "Konnte Verzeichnis '%s' nicht erstellen."
+msgstr "Konnte Verzeichnisse für '%s' nicht erstellen."
 
 #: dir.c:2806
 #, c-format
@@ -1655,9 +1657,9 @@ msgid "could not migrate git directory from '%s' to '%s'"
 msgstr "Konnte Git-Verzeichnis nicht von '%s' nach '%s' migrieren."
 
 #: entry.c:280
-#, fuzzy, c-format
+#, c-format
 msgid "could not stat file '%s'"
-msgstr "konnte Datei '%s' nicht erstellen"
+msgstr "konnte Datei '%s' nicht lesen"
 
 #: fetch-pack.c:249
 msgid "git fetch-pack: expected shallow list"
@@ -1827,14 +1829,14 @@ msgid "no matching remote head"
 msgstr "kein übereinstimmender Remote-Branch"
 
 #: fetch-pack.c:1147
-#, fuzzy, c-format
+#, c-format
 msgid "no such remote ref %s"
-msgstr "Kein solches Remote-Repository: %s"
+msgstr "keine solche Remote-Referenz %s"
 
 #: fetch-pack.c:1150
 #, c-format
 msgid "Server does not allow request for unadvertised object %s"
-msgstr ""
+msgstr "Der Server lehnt Anfrage nach nicht angebotenem Objekt %s ab."
 
 #: gpg-interface.c:185
 msgid "gpg failed to sign the data"
@@ -1961,31 +1963,31 @@ msgstr ""
 
 #: ident.c:367
 msgid "no email was given and auto-detection is disabled"
-msgstr ""
+msgstr "keine E-Mail angegeben und automatische Erkennung ist deaktiviert"
 
 #: ident.c:372
-#, fuzzy, c-format
+#, c-format
 msgid "unable to auto-detect email address (got '%s')"
-msgstr "Fehler: konnte keine gültige Adresse aus %s extrahieren\n"
+msgstr "Konnte die E-Mail-Adresse nicht automatisch erkennen ('%s' erhalten)"
 
 #: ident.c:382
 msgid "no name was given and auto-detection is disabled"
-msgstr ""
+msgstr "kein Name angegeben und automatische Erkennung ist deaktiviert"
 
 #: ident.c:388
-#, fuzzy, c-format
+#, c-format
 msgid "unable to auto-detect name (got '%s')"
-msgstr "konnte \"Tree\"-Objekt (%s) nicht lesen"
+msgstr "konnte Namen nicht automatisch erkennen ('%s' erhalten)"
 
 #: ident.c:396
 #, c-format
 msgid "empty ident name (for <%s>) not allowed"
-msgstr ""
+msgstr "Leerer Name in Identifikation (für <%s>) nicht erlaubt."
 
 #: ident.c:402
 #, c-format
 msgid "name consists only of disallowed characters: %s"
-msgstr ""
+msgstr "Name besteht nur aus nicht erlaubten Zeichen: %s"
 
 #: ident.c:417 builtin/commit.c:611
 #, c-format
@@ -2102,13 +2104,11 @@ msgstr ""
 "im Arbeitsbereich gelassen."
 
 #: merge-recursive.c:1097
-#, fuzzy, c-format
+#, c-format
 msgid ""
 "CONFLICT (%s/delete): %s deleted in %s and %s to %s in %s. Version %s of %s "
 "left in tree."
-msgstr ""
-"KONFLIKT (%s/löschen): %s gelöscht in %s und %s in %s. Stand %s von %s wurde "
-"im Arbeitsbereich gelassen."
+msgstr "KONFLIKT (%s/löschen): %s gelöscht in %s und %s nach %s in %s. Stand 
%s von %s wurde im Arbeitsbereich gelassen."
 
 #: merge-recursive.c:1104
 #, c-format
@@ -2120,13 +2120,11 @@ msgstr ""
 "im Arbeitsbereich bei %s gelassen."
 
 #: merge-recursive.c:1109
-#, fuzzy, c-format
+#, c-format
 msgid ""
 "CONFLICT (%s/delete): %s deleted in %s and %s to %s in %s. Version %s of %s "
 "left in tree at %s."
-msgstr ""
-"KONFLIKT (%s/löschen): %s gelöscht in %s und %s in %s. Stand %s von %s wurde "
-"im Arbeitsbereich bei %s gelassen."
+msgstr "KONFLIKT (%s/löschen): %s gelöscht in %s und %s nach %s in %s. Stand 
%s von %s wurde im Arbeitsbereich bei %s 

Re: [PATCH v3] l10n: de.po: update German translation

[Ralf Thielow]

Am 3. Mai 2017 um 14:38 schrieb Christian Brabandt :
> Hallo Ralf!
>
> Ralf Thielow schrieb am Dienstag, den 02. Mai 2017:
>
>> @@ -1260,6 +1260,8 @@ msgstr "Speicher verbraucht"
>>  #: config.c:191
>>  msgid "relative config include conditionals must come from files"
>>  msgstr ""
>> +"Bedingungen für das Einbinden von Konfigurationen aus relativen Pfaden 
>> muss\n"
>> +"aus Dateien kommen."
>
> Bedingungen [...] müssen aus Dateien kommen
>
>>  #: fetch-pack.c:1150
>>  #, c-format
>>  msgid "Server does not allow request for unadvertised object %s"
>> -msgstr ""
>> +msgstr "Der Server erlaubt keine Anfrage für nicht angebotenes Objekt %s."
>
> Double negation does not sound good. Perhaps:
> "Der Server lehnt Anfrage nach nicht angebotenem Objekt %s ab."
> or possibly (still double negation)
> "Der Server erlaubt keine Anfrage für nicht öffentliches Objekt %s."
>>  #: builtin/for-each-ref.c:46
>> -#, fuzzy
>>  msgid "print only refs which don't contain the commit"
>> -msgstr "nur Referenzen ausgeben, die diesen Commit enthalten"
>> +msgstr "nur Referenzen ausgeben, die diesen nicht Commit enthalten"
>
> "nur Referenzen ausgeben, die diesen Commit nicht enthalten"
>
> refs are here translated as Referenzen
>
>>  #: builtin/grep.c:1189
>> -#, fuzzy
>>  msgid "--no-index or --untracked cannot be used with revs"
>> -msgstr ""
>> -"Die Optionen --no-index und --untracked können nicht mit Commits verwendet 
>> "
>> -"werden."
>> +msgstr "--no-index oder --untracked können nicht mit Commits verwendet 
>> werden"
>
> refs are here translated as Commits?
>
>>  #: builtin/grep.c:1195
>> -#, fuzzy, c-format
>> +#, c-format
>>  msgid "unable to resolve revision: %s"
>> -msgstr "Konnte %s nicht nach %s verschieben"
>> +msgstr "Konnte Commit nicht auflösen: %s"
>
> same here
>

Ref or Reference is translated as "Referenz" while
Rev or Revision is translated as "Commit" so I think
the translation is correct.

Thanks for your findings and suggestions!

>>  #: builtin/tag.c:421 builtin/tag.c:423
>> -#, fuzzy
>>  msgid "print only tags that don't contain the commit"
>> -msgstr "nur Tags ausgeben, die diesen Commit beinhalten"
>> +msgstr "nur Tags ausgeben, die diesen nicht Commit enthalten"
>
> "nur Tags ausgeben, die diesen Commit nicht enthalten"
>
> Best,
> Christian
> --
> Wie man sein Kind nicht nennen sollte:
>   Anna Naß

Re: [PATCH] i18n: remove i18n from tag reflog message

[Jean-Noël AVILA]

Le dimanche 30 avril 2017, 18:45:05 CEST Junio C Hamano a écrit :
> Jean-Noel Avila  writes:
> > The building of the reflog message is using strbuf, which is not
> > friendly with internationalization frameworks. No other reflog
> > messages are translated right now and switching all the messages to
> > i18n would require a major rework of the way the messages are built.
> 
> Thanks for spotting.  Before we declare that reflog messages are for
> i18n, we'd need to either drop (or redesign the implementation of)
> the "checkout -" feature, which relies on the exact phrasing of how
> reflog entries from "git checkout" looks like.
> 
> Will queue and merge down to 'master' quickly.
> 

I didn't know this "side effect". Maybe adding a test against it would be 
requiered. Unfortunately, I don't know enough of it.

Re: [PATCH 00/10] RFC Partial Clone and Fetch

[Jeff Hostetler]

On 3/8/2017 1:50 PM, g...@jeffhostetler.com wrote:

From: Jeff Hostetler 


[RFC] Partial Clone and Fetch
=
[...]
E. Unresolved Thoughts
==

*TODO* The server should optionally return (in a side-band?) a list
of the blobs that it omitted from the packfile (and possibly the sizes
or sha1_object_info() data for them) during the fetch-pack/upload-pack
operation.  This would allow the client to distinguish from invalid
SHAs and missing ones.  Size information would allow the client to
maybe choose between various servers.


Since I first posted this, Jonathan Tan has started a related
discussion on missing blob support.
https://public-inbox.org/git/cagf8dgk05+f4ux-8+imfvqd0n2jp6yxj18ag8udaeh6qc6s...@mail.gmail.com/T/

I want to respond to both of these threads here.
-

Missing-Blob Support


Let me offer up an alternative idea for representing
missing blobs.  This is differs from both of our previous
proposals.  (I don't have any code for this new proposal,
I just want to think out loud a bit and see if this is a
direction worth pursuing -- or a complete non-starter.)

Both proposals talk about detecting and adapting to a missing
blob and ways to recover -- when we fail to find a blob.
Comments on the thread asked about:
() being able to detect missing blobs vs corrupt repos
() being unable to detect duplicate blobs
() expense of blob search.

Suppose we store "positive" information about missing blobs?
This would let us know that a blob is intentionally missing
and possibly some meta-data about it.


1. Suppose we update the .pack file format slightly.
   () We use the 5 value in "enum object_type" to mean a
  "missing-blob".
   () We update git-pack-object as I did in my RFC, but have it
  create type 5 entries for the blobs that are omitted,
  rather than nothing.
   () Hopefully, the same logic that currently keeps pack-object
  from sending unnecessary blobs on subsequent fetches can
  also be used to keep it from sending unnecessary missing-blob
  entries.
   () The type 5 missing-blob entry would contain the SHA-1 of the
  blob and some meta-data to be explained later.

2. Make a similar change in the .idx format and git-index-pack
   to include them there.  Then blob lookup operations could
   definitively determine that a blob exists and is just not
   present locally.

3. With this, packfile-based blob-lookup operations can get a
   "missing-blob" result.
   () It should be possible to short-cut searching in other
  packfiles (because we don't have to assume that the blob
  was just misplaced in another packfile).
   () Lookup can still look for the corresponding loose blob
  (in case a previous lookup already "faulted it in").

4. We can then think about dynamically fetching it.
   () Several techniques for this are currently being
  discussed on the mailing list in other threads,
  so I won't go into this here.
   () There has also been debate about whether this should
  yield a loose blob or a new packfile.  I think both
  forms have merit and depend on whether we are limited
  to asking for a single blob or can make a batch request.
   () A dynamically-fetched loose blob is placed in the normal
  loose blob directory hierarchy so that subsequent
  lookups can find it as mentioned above.
   () A dynamically-fetched packfile (with one or more blobs)
  is written to the ODB and then the lookup operation
  completes.
  {} I want to isolate these packfiles from the main
 packfiles, so that they behave like a second-stage
 lookup and don't affect the caching/LRU nature of
 the existing first-stage packfile lookup.
  {} I also don't want the ambiguity of having 2 primary
 packfiles with a blob marked as missing in 1 and
 present in the other.

5. git-repack should be updated to "do the right thing" and
   squash missing-blob entries.

6. And etc.


Missing-Blob Entry Data
===

A missing-blob entry needs to contain the SHA-1 value of
the blob (obviously).  Other fields are nice to have, but
are not necessary.  Here are a few fields to consider.

A. The SHA-1 (20 bytes)

B. The raw size of the blob (5? bytes).
   () This is the cleaned size of the file as stored.  The
  server does not (and should not) have any knowledge
  of the smudging that may happen.
   () This may be useful if whatever dynamic-fetch-hook
  wants to customize its behavior, such as individually
  fetching large blobs and batch fetching smaller ones
  from the same server.
   () GVFS found it necessary to create a custom server
  end-point to get blob size data so that "ls -l"
  could show file sizes for non-present virtualized
  files.
   () 5 bytes (uint:40) should be more than enough for this.

C. A server "hint" (20 

[PATCH 4/4] git-filter-branch: be assertative on dying message

[Jean-Noel Avila]

Signed-off-by: Jean-Noel Avila 
---
 git-filter-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 2b8cdba15..dd3a605d0 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name \
 sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
 
 test -s "$tempdir"/heads ||
-   die "Which ref do you want to rewrite?"
+   die "You must specify a ref to rewrite"
 
 GIT_INDEX_FILE="$(pwd)/../index"
 export GIT_INDEX_FILE
-- 
2.12.0

[PATCH 3/4] read-tree.c: rework UI when merging no trees

[Jean-Noel Avila]

The initial test was inherited from a previous commit, but it is no
longer needed, given the following switch case. Moreover, the question
sentence ending the program has been replace by an assertative one.

Signed-off-by: Jean-Noel Avila 
---
 builtin/read-tree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 23e212ee8..05296997c 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -226,9 +226,10 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
setup_work_tree();
 
if (opts.merge) {
-   if (stage < 2)
-   die("just how do you expect me to merge %d trees?", 
stage-1);
switch (stage - 1) {
+   case 0:
+   die("there are no trees to merge!");
+   break;
case 1:
opts.fn = opts.prefix ? bind_merge : oneway_merge;
break;
-- 
2.12.0

[PATCH 2/4] usability: fix am and checkout for nevermind questions

[Jean-Noel Avila]

Signed-off-by: Jean-Noel Avila 
---
 builtin/am.c   | 4 ++--
 builtin/checkout.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e..f5afa438d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1312,7 +1312,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
}
 
if (is_empty_file(am_path(state, "patch"))) {
-   printf_ln(_("Patch is empty. Was it split wrong?"));
+   printf_ln(_("Patch is empty. It may have been split wrong."));
die_user_resolve(state);
}
 
@@ -1940,7 +1940,7 @@ static void am_resolve(struct am_state *state)
 
if (unmerged_cache()) {
printf_ln(_("You still have unmerged paths in your index.\n"
-   "Did you forget to use 'git add'?"));
+   "You might want to use 'git add' on them."));
die_user_resolve(state);
}
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f3..05037b9b6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1287,7 +1287,7 @@ int cmd_checkout(int argc, const char **argv, const char 
*prefix)
 */
if (opts.new_branch && argc == 1)
die(_("Cannot update paths and switch to branch '%s' at 
the same time.\n"
- "Did you intend to checkout '%s' which can not be 
resolved as commit?"),
+ "'%s' can not be resolved as commit, but it 
should."),
opts.new_branch, argv[0]);
 
if (opts.force_detach)
-- 
2.12.0

[PATCH 1/4] usability: don't ask questions if no reply is required

[Jean-Noel Avila]

As described in the bug report at

https://github.com/git/git-scm.com/issues/999

the user was disconcerted by the question asked by the program not
requiring a reply from the user. To improve the general usability of
the Git suite, The following rule was applied:

if the sentence
 * appears in a non-interactive session
 * is printed last before exit
 * is a question addressing the user ("you")

the sentence is turned into affirmative and proposes the option.

Signed-off-by: Jean-Noel Avila 
---
 help.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index bc6cd19cf..4658a55c6 100644
--- a/help.c
+++ b/help.c
@@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
 
if (SIMILAR_ENOUGH(best_similarity)) {
fprintf_ln(stderr,
-  Q_("\nDid you mean this?",
- "\nDid you mean one of these?",
+  Q_("\nThe most approaching command is",
+ "\nThe most approaching commands are",
   n));
 
for (i = 0; i < n; i++)
-- 
2.12.0

Re: [PATCH v2 21/21] t1308: add a test case on open a config directory

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> We don't support config-as-a-directory (maybe someday we will?). Make
> sure we consistently fail in this case, which should happen on platforms
> where fopen() returns non-NULL if FREAD_READS_DIRECTORIES is
> defined.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 

How about saying that we make sure that this is the case by adding a test
case? Otherwise the reader might (and this reader certainly did) expect a
code change in a .c file.

I reviewed this patch series, and am happy with the overall diff.

My main problem: it should be organized in a more intuitively-graspable
way, i.e. put all the fopen() -> fopen_or_warn() changes into a single
patch, separate out changes (drive-by test fixing for Windows and, ahem,
whitespace changes) into their own commits. Then those commits should be
ordered to relate an easy-flowing story.

Other than that: well done,
Dscho

Re: [PATCH v2 20/21] config.c: handle error on failing to fopen()

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> In the first case, we already correctly return -1 if fopen() fails to
> open. But we should report something so people know what's wrong.
> 
> In the second case, config_file == NULL does not necessarily mean "no
> config file". Bail out if needed.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 

Again, it seems that your patch series tries to cut at the file boundary,
not at the "logically the same change" boundary, making it a bit more
cumbersome than necessary to follow this patch series.

I highly recommend squashing the first change into the big fopen() ->
fopen_or_warn() patch I hinted at earlier, and let the second change (with
the accompanying test case) stand on its own.

Ciao,
Dscho

Re: [PATCH v2 15/21] rerere.c: report correct errno

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/rerere.c b/rerere.c
> index 971bfedfb2..c26c29f87a 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char 
> *sha1, const char *output
>   io.input = fopen(path, "r");
>   io.io.wrerror = 0;
>   if (!io.input)
> - return error("Could not open %s", path);
> + return error_errno("Could not open %s", path);

IMO the error() -> error_errno() changes should all be part of the same
commit, as they probably share the explanation why fopen_or_warn() is not
appropriate here.

>   if (output) {
>   io.io.output = fopen(output, "w");
>   if (!io.io.output) {
> + error_errno("Could not write %s", output);
>   fclose(io.input);
> - return error("Could not write %s", output);
> + return -1;
>   }

This one is logically different from the change above, as it not only
cannot be replaced by fopen_or_warn(), but also requires the reordering
due to the different nature of the change.

Ciao,
Dscho

Re: [PATCH v2 13/21] remote.c: report error on failure to fopen()

[Johannes Schindelin]

Hi Duy,


On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> There's plenty of error() in this code to safely assume --quiet is not a
> concern.
> 
> t5512 needs update because if we check the path 'refs*master' (i.e. the
> asterisk is part of the path) then we'll get an EINVAL error.

So the first change in this patch unmasks a bug that is fixed by the
second patch?

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 94fc9be9ce..02106c9226 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -85,8 +85,15 @@ test_expect_success 'use branch..remote if possible' 
> '
>  '
>  
>  test_expect_success 'confuses pattern as remote when no remote specified' '
> - cat >exp <<-\EOF &&
> - fatal: '\''refs*master'\'' does not appear to be a git repository
> + if test_have_prereq MINGW
> + then
> + # Windows does not like asterisks in pathname
> + does_not_exist=master
> + else
> + does_not_exist="refs*master"
> + fi &&
> + cat >exp <<-EOF &&
> + fatal: '\''$does_not_exist'\'' does not appear to be a git repository
>   fatal: Could not read from remote repository.
>  
>   Please make sure you have the correct access rights
> @@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no 
> remote specified' '
>   # fetch .
>   # We could just as easily have used "master"; the "*" emphasizes its
>   # role as a pattern.
> - test_must_fail git ls-remote refs*master >actual 2>&1 &&
> + test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&
>   test_i18ncmp exp actual
>  '

Sure enough. This totally looks like it needs to be a preparatory bug fix.
Please separate it out and make sure that it comes before the
fopen_or_warn() change in the patch series.

Ciao,
Dscho

Re: [PATCH v2 10/21] log: report errno on failure to fopen() a file

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy 

I do think that this merits a one-line explanation why you do not use
fopen_or_warn() here.

> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1ed..26d6a3cf14 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -858,7 +858,8 @@ static int open_next_file(struct commit *commit, const 
> char *subject,
>   printf("%s\n", filename.buf + outdir_offset);
>  
>   if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
> - return error(_("Cannot open patch file %s"), filename.buf);
> + return error_errno(_("Cannot open patch file %s"),
> +filename.buf);
>  
>   strbuf_release();

The patches before and after this one all convert fopen() to
fopen_or_warn(). If you *must* separate those into individual patches
(which just makes this patch series unnecessarily bloated, if you ask me),
you should at least group them a bit better so that the patch series tells
more of a consistent story rather than jumping back and forth like a
hyperactive four-year-old telling you about her latest adventure.

Ciao,
Dscho

Re: [PATCH v2 09/21] blame: report error on open if graft_file is a directory

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/blame.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 07506a3e45..1648b396dc 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)
>   */
>  static int read_ancestry(const char *graft_file)
>  {
> - FILE *fp = fopen(graft_file, "r");
> + FILE *fp = fopen_or_warn(graft_file, "r");
>   struct strbuf buf = STRBUF_INIT;
> +
>   if (!fp)
>   return -1;
> +
>   while (!strbuf_getwholeline(, fp, '\n')) {

This is an excellent example to demonstrate why folding all conversions to
fopen_or_warn() into the same patch as introducing that function makes
sense: I had to close this mail, find the mail with the patch introducing
the function, verify that it returns NULL as before, close that mail, and
continue this reply.

Oh, and I do not think it is a good idea to introduce style fixes in the
middle of such a large patch series. It's a bit distracting from the real
meat here.

Ciao,
Dscho

Re: [PATCH v2 08/21] bisect: report on fopen() error

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> The main thing to catch here is when fopen() is called on a
> directory. It's safe even without this change because a few lines
> earlier we do check if "filename" is a regular file.
> 
> Regardless, let's stay on the safe side in case somebody changes those
> lines. Unconditionally printing to stderr by warn_on_inaccessible()
> should be fine, because the caller does unconditional fprintf(stderr,
> too, no checking for quiet option or anything.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 

I appreciate that you mention specifically the use case that triggered my
complaint that in turn prodded you into starting working on this.

However, I think this should be a mere side note in a single commit that
introduces and uses fopen_or_warn().

Ciao,
Dscho

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Ævar Arnfjörð Bjarmason]

[Just replying to you & Duy in the same mail, easier]

On Wed, May 3, 2017 at 11:45 AM, Johannes Schindelin
 wrote:
> Hi Ævar,
>
> On Tue, 2 May 2017, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, May 2, 2017 at 6:05 PM, Johannes Schindelin
>>  wrote:
>>
>> > [... complaint about the abrupt change that makes Git for Windows fail
>> >  to build because pcre2.h is missing ...]
>>
>> I think it's worth it to copy/paste the commit message where I made
>> this change, since we're having this discussion in this thread:
>>
>> Makefile & configure: make PCRE v2 the default PCRE implementation
>>
>> Change the USE_LIBPCRE=YesPlease & --with-libpcre flags to the
>> Makefile & configure script, respectively, to mean use PCRE v2, not
>> PCRE v1.
>>
>> The legacy library previously available via those options is still
>> available on request via USE_LIBPCRE1=YesPlease or
>> --with-libpcre1. The existing USE_LIBPCRE2=YesPlease & --with-libpcre2
>> still explicitly ask for v2.
>>
>> The v2 PCRE is stable & end-user compatible, all this change does is
>> change the default. Someone building a new git is likely to also have
>> packaged PCRE v2 sometime in the last 2 years since it was released.
>> If not they can choose to use the legacy v2 library by making the
>> trivial s/USE_LIBPCRE/USE_LIBPCRE1/ change, or package up PCRE v2.
>>
>> New releases of PCRE v2 are already faster than PCRE v1, and not only
>> is all significant development is happening on v2, but bugs in
>> reported against v1 have started getting WONTFIX'd asking users to
>> just upgrade to v2.
>>
>> So it makes sense to give our downstream distributors a nudge to
>> switch over to it.
>
> I see where you are coming from.
>
> At this point, I feel that someone should recall into our collective
> memory what happened when we made a change similar in nature that broke
> existing build setups: by requiring REG_STARTEND all of a sudden ("you can
> easily flip the NO_REGEX switch", as if everybody should know about those
> Makefile flags we have).

And as a result grep/log -G got faster by default, and more
importantly since v2.10.1 which includes your 2f8952250a and made a
REG_STARTEND engine a hard requirement nobody using git is
mysteriously going to miss grep results because of some stray \0 in
the string being matched.

I agree that I should drop the patch to make v2 the default from this
series for now. Clearly it's controversial, and can be considered on
its own merits once the supporting code is in. I'll do that in the
next submission, which I'm planning to send after v2.13.0 comes out.

But aside from that, I must say I completely disagree with this line
of argument. I think to the extent that we did anything wrong with
NO_REGEX it's that I should have noticed that we could use
REG_STARTEND after I imported gawk's engine in compat/regex &
submitted a patch like yours back in 2010. Instead we got 6 years of
git releases where

Similarly, if PCRE v2 is less buggy & faster then we're looking at man
days/weeks and possibly months/years saved given how many people use
git v.s. how many are developing it & write build scripts for it.

Leaving user benefits on the table to optimize for reducing the
one-time inconvenience of packagers is the wrong move.

> [...]
On Wed, May 3, 2017 at 1:15 PM, Duy Nguyen  wrote:
> On Wed, May 3, 2017 at 4:45 PM, Johannes Schindelin
>  wrote:
>>> So it makes sense to give our downstream distributors a nudge to
>>> switch over to it.
>
> Some contributor (i.e. me) was not happy with this nudging though. The
> other day I switched to some branch (probably 'pu') and the build
> failed because, guess what, I didn't have pcre2 installed. If I set
> USE_LIBPCRE1 then I lose pcre support when switching to other
> branches. And no, I don't want to install libpcre2, not when I'm
> forced to this way.

Assuming that we'd want to change the default to v2, can you think of
a better way to handle this transition? As pointed out in
 I
can't.

Perhaps the best way would be to migrate this change directly to
master if there's consensus to move to v2 by default, that would avoid
the pain for developers related to switching between concurrent
branches while the change is cooking.

> 188 packages on Gentoo optionally depend on libpcre, 6 packages on
> libpcre2. Chances that a Gentoo user has libpcre2 already are rather
> low.

It sounds like two things are being conflated here, surely "a Gentoo
user", i.e. someone who's not actively contributing to git.git will
just get this via `emerge git` if this change were released, since the
ebuild will depend on libpcre2?

> I'll revisit my installation when the level of libpcre2 support
> grows a bit more than that.

There being few existing reverse 

Re: [PATCH v2 06/21] attr.c: use fopen_or_warn()

[Johannes Schindelin]

Hi Duy,


On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy 

As commented about warn_on_fopen_errors(), I do not see a benefit in
separating the introduction of this small function from the individual
files' changes to use it. Let's bring it all together.

Ciao,
Dscho

Re: [PATCH v2 04/21] wrapper.c: add warn_on_fopen_errors()

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy 

This commit message is a bit empty. How about:

In many places, Git warns about an inaccessible file after a
fopen() failed. To discern these cases from other cases where we
want to warn about inaccessible files, introduce a new helper
specifically to test whether fopen() failed because the current
user lacks the permission to open file in question.

> diff --git a/dir.c b/dir.c
> index f451bfa48c..8218a24962 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -745,8 +745,7 @@ static int add_excludes(const char *fname, const char 
> *base, int baselen,
>  
>   fd = open(fname, O_RDONLY);
>   if (fd < 0 || fstat(fd, ) < 0) {
> - if (errno != ENOENT)
> - warn_on_inaccessible(fname);
> + warn_on_fopen_errors(fname);
>   if (0 <= fd)
>   close(fd);

I see you already converted one location. Why not simply fold all of them
into this patch? It's not like it would be easier to review those changes
if you separate patches addressing this class of problems.

Ciao,
Dscho

Re: [PATCH v2 02/21] clone: use xfopen() instead of fopen()

[Johannes Schindelin]

Hi Duy,

On Wed, 3 May 2017, Nguyễn Thái Ngọc Duy wrote:

> This code uses the result FILE pointer right away without the NULL
> check. Let's use xfopen() and die if we could not open the file. That's
> still better than crashing,
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 

While funny to read, maybe it would be better to spell it out clearly:

copy_alternates() called fopen() without handling errors. By
switching to xfopen(), this bug is fixed.

Ciao,
Dscho

Re: git-clone --config order & fetching extra refs during initial clone

[SZEDER Gábor]

Cc'ing Ævar because of his work on 'clone --no-tags'.


On Wed, Mar 15, 2017 at 6:08 PM, Jeff King  wrote:
> On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote:
>> > Though if I'm bikeshedding, I'd probably have written the whole thing
>> > with an argv_array and avoided counting at all.
>>
>> Yeah, I did notice that you love argv_array :)  I had to raise an
>> eyebrow recently while looking at send-pack and how it uses argv_array
>> to read refspecs from stdin into an array.  I think argv_array feels a
>> bit out of place in both cases.  Yes, it does exactly what's needed.
>> However, it's called *argv*_array, indicating that its contents is
>> destined to become the options of some command.  But that's not the
>> case in these two cases, we are not dealing with arguments to a
>> command, these are just arrays of strings.
>
> In my mind, "argv" is synonymous with "NULL-terminated array of
> strings". If the name is the only thing keeping it from wider use, I'd
> much prefer us to give it a more generic name. All I really care about
> is simplifying memory management. :)

Whether its name is the _only_ thing keeping it from wider use, I
don't know :)

All I can say is that I was aware of argv_array, but because of
its name it didn't even occur to me.  And even if I had considered it,
I still wouldn't have used it here.  Had it been called string_array,
I think I would have used it.

On a related note, we have string_list, which is not a list but an
ALLOC_GROW()-able array, and not that of strings (i.e. plan char*),
but of structs with a string and an additional data field.
Oh, well :)


>> > I do also notice that right _after_ this parsing, we use remote_get(),
>> > which is supposed to give us this config anyway. Which makes me wonder
>> > if we could just reorder this to put remote_get() first, and then read
>> > the resulting refspecs from remote->fetch.
>>
>> Though get_remote() does give us this config, at this point the
>> default fetch refspec has not been configured yet, therefore it's not
>> included in the resulting remote->fetch array.  The default refspec is
>> set in write_refspec_config(), but that's called only about two
>> screenfulls later.  So there is a bit of extra work to do in order to
>> leverage get_remote()'s parsing.
>>
>> I think the simplest way is to keep parsing the default fetch refspec
>> manually, and then append it to the remote->fetch array.  Definitely
>> shorter and simpler than that parsing in the current patch.
>> Alternatively, we could set the default fetch refspec in the
>> configuration temporarily, only for the duration of the get_remote()
>> call, but it feels a bit too hackish...
>
> Yeah, I think manually combining the two here is fine. Though I won't
> complain if you want to look into setting the config earlier. If the
> refactoring isn't too bad, it would probably provide the nicest outcome.

I did actually look into that, but don't think it's a good idea.

write_refspec_config() nicely encapsulates writing the proper fetch
refspec configuration according to the given command line options.  Of
course these options are already known right at the start, so solely
in this regard we could call this function earlier.  However, in some
cases, e.g. '--single-branch', the refspec to be written to the config
depends not only on the given options but on the refs in the remote
repository, too, so it can only be written after we got the remote's
refs.


Unfortunately, there is more to this issue.  Earlier I though that the
fetch refspec is the only config that is ignored during a clone.
However, Ævar's 'clone --no-tags' patches[1] drew my attention to the
'remote..tagOpt' config variable, that I overlooked earlier...
and apparently 'git clone' overlooks it as well, grabbing all tags
even when it's set to '--no-tags'.

The root issue is that 'git clone' calls directly into the fetch
machinery instead of running 'git fetch' (either via run_command() or
cmd_fetch()), and some of these "higher-level" config variables are
only handled in 'builtin/fetch.c' but not in 'git clone'.  By
"handle" I mean "parse and act accordingly": as it happens, these
config values are parsed alright when 'git clone' calls remote_get(),
but it never looks at the relevant fields in the resulting 'struct
remote'.

Luckily, many "lower-level" config variables are working properly even
during 'git clone', because they are handled in the transport layer,
e.g. 'git clone -c url.https://github.com/.insteadof=gh: gh:git/git'
does the right thing.


I'm not sure what the right way forward would be.

My patch deals with 'remote..refspec', i.e. 'remote->fetch'.
Apparently some extra care is necessary for 'remote..tagOpt' and
'remote->fetch_tags', too.  Perhaps there are more, I haven't checked
again, and maybe we'll add similar config variables in the future.  So
I don't think that dealing with such config variables one by one in
'git clone', too, is the right long-term 

Re: CYGWIN git cannot install python packages with pip

[Johannes Schindelin]

Hi,

On Wed, 3 May 2017, ankostis wrote:

> On 3 May 2017 11:47, Johannes Schindelin  wrote:
> > On Tue, 2 May 2017, ankostis wrote:
> >
> >> On Windows, with Cygwin-git 02.12.2-1 the python command:
> >> [...]
> >
> > You forgot to mention what python/pip you use (is it the Cygwin one,
> > or a standalone one?). Intentional?
> 
> I have tested it on using WinPython[1] 3.6.1 & 3.5.3.
> WinPython is built out of the "standard" python on Windows.

I am afraid that you run into path conversion problems here. /cygdrive/d/
is something Cygwin understands, but not WinPython nor Git for Windows.

It appears to be difficult to impossible to mix Cygwin with non-Cygwin
executables...

Ciao,
Johannes

Re: [PATCH 0/5] Abide by our own rules regarding line endings

[Johannes Schindelin]

Hi Jonathan,

On Tue, 2 May 2017, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > Over the past decade, there have been a couple of attempts to remedy the
> [...]
> 
> I'm intentionally skimming this cover letter, since anything important
> it says needs to also be in the commit messages.

Sure, makes sense. I tried to do that, too, before sending out my patch
series.

> [...]
> > Without these fixes, Git will fail to build and pass the test suite, as
> > can be verified even on Linux using this cadence:
> >
> > git config core.autocrlf true
> > rm .git/index && git stash
> > make DEVELOPER=1 -j15 test
> 
> This should go in a commit message (or perhaps in all of them).

Hmm, okay. I wanted to keep it out of them, as commit messages are (in my
mind) more about the why?, and a little less about the how? when not
obvious from the diff.

I added a variation to the first patch (because the tests would still
fail, I skipped the `test` from the `make` invocation) and the unmodified
cadence to the "Fix the remaining tests..." patch.

> [...]
> > Johannes Schindelin (5):
> >   Fix build with core.autocrlf=true
> >   git-new-workdir: mark script as LF-only
> >   completion: mark bash script as LF-only
> >   Fix the remaining tests that failed with core.autocrlf=true
> >   t4051: mark supporting files as requiring LF-only line endings
> 
> With or without that change,
> Reviewed-by: Jonathan Nieder 

Thanks.

I added that footer to the patches (not to the two new ones, though).

> The t/README bit in patch 4/5 is sad (I want to be able to open
> t/README using an old version of Notepad) but changing that test to
> use another file seems out of scope for what this series does.

Yes, it is sad. Maybe I should list the tests that do this (use files
outside any t/ directory):

t4003-diff-rename-1.sh (uses t/diff-lib/COPYING)
t4005-diff-rename-2.sh (uses t/diff-lib/COPYING)
t4007-rename-3.sh (uses t/diff-lib/COPYING)
t4008-diff-break-rewrite.sh (uses t/diff-lib/README and t/diff-lib/COPYING)
t4009-diff-rename-4.sh (uses t/diff-lib/COPYING)
t4022-diff-rewrite.sh (uses COPYING)
t4023-diff-rename-typechange.sh (uses COPYING)
t7001-mv.sh (uses README.md (!!!) and COPYING)
t7060-wtstatus.sh (uses t/README)
t7101-reset-empty-subdirs.sh (uses COPYING)

Note most of these tests may *use* those files, but do *not* assume that
they have Unix line endings! Only a couple test compare SHA-1s to
hardcoded values (which, if you ask me, is a bit fragile, given that files
outside the tests' control are used).

Interesting side note: t0022-crlf-rename.sh copies *itself* to the trash*
directory where it is then used to perform tests. So while this test uses
"an outside file", that file happens to be a .sh file which we already
have to mark LF-only for different reasons (Bash likes its input
CR/LF-free).

Another interesting side note: the convention appears to dictate that
supporting files should be either generated in the test script itself, or
be committed into t/t/ directories (where  matches the respective
test script's number, or reuses another test script's supporting files). A
notable exception is t3901 which has the supporting files t3901-8859-1.txt
and t3901-utf8.txt. I would wageer that this is just a remnant of ancient
times before the current convention, judging by the date of the commit
that added these files: a731ec5eb82 (t3901: test "format-patch | am" pipe
with i18n, 2007-01-13). The scripts t0203-gettext-setlocale-sanity.sh,
t9350-fast-export.sh and t9500-gitweb-standalone-no-errors.sh merely reuse
those files, and when those scripts started using those files, they did
not change their location. I made this move a preparatory step in this
patch series.

Back to the question what to do about those tests that make improper
assumptions about line endings of files outside the tests' realm: I think
I can do this more cleverly, as it would appear that only four test
scripts make hard assumptions about the exact byte sequence of the COPYING
file, and I simply turned those `cp` (and even `cat`!) calls into `tr -d
"\015"` calls.

I will Cc: you on v2.

Ciao,
Dscho

Re God Bless You and Happy Easter!

[Sister Grace Alain]

Hello My Dear,

This is my second mail but No response,from you Why? regarding my
Easter Donation Gift"

Reminding notice/ this is to inform you that i am still in the
hospital bed taking a medical treatments because of my Old age
illness.Please i need your urgent answer/ i want to know if you have
received my donation offer this sum of ($4.5 Million Dollars) keep by
check from the Church Bishop John Allen ? to Help a Charity work in
your country

Yes ? OR No?
Reply Back immediately so that i can have rest of mind.

Yours' faithfully,
God Bless You and Happy Easter!

Remain Bless.

>From Mrs Grace Alain
Coccody Medical Center.

my good Frend if you never hear from the pastor just go ahead now and
contact him ok

Bishop John Allen

Email/ church.past...@gmail.com

[PATCH] config.mak.uname: set NO_REGEX=NeedsStartEnd on AIX

[Ævar Arnfjörð Bjarmason]

Set the NO_REGEX=NeedsStartEnd Makefile flag by default on AIX.

Since commit 2f8952250a ("regex: add regexec_buf() that can work on a
non NUL-terminated string", 2016-09-21) git has errored out at
compile-time if the regular expression library doesn't support
REG_STARTEND.

While looking through Google search results for the use of NO_REGEX I
found a Chef recipe that set this on AIX[1], looking through the
documentation for the latest version of AIX (7.2, released October
2015) shows that its regexec() doesn't have REG_STARTEND.

1. 
https://github.com/chef/omnibus-software/commit/e247e36761#diff-3df898345d670979b74acc0bf71d8c47
2. 
https://www.ibm.com/support/knowledgecenter/ssw_aix_72/com.ibm.aix.basetrf2/regexec.htm

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index 399fe19271..192629f143 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -237,6 +237,7 @@ ifeq ($(uname_S),AIX)
NO_MKDTEMP = YesPlease
NO_STRLCPY = YesPlease
NO_NSEC = YesPlease
+   NO_REGEX = NeedsStartEnd
FREAD_READS_DIRECTORIES = UnfortunatelyYes
INTERNAL_QSORT = UnfortunatelyYes
NEEDS_LIBICONV = YesPlease
-- 
2.11.0

Re: [PATCH v3] l10n: de.po: update German translation

[Christian Brabandt]

Hallo Ralf!

Ralf Thielow schrieb am Dienstag, den 02. Mai 2017:

> @@ -1260,6 +1260,8 @@ msgstr "Speicher verbraucht"
>  #: config.c:191
>  msgid "relative config include conditionals must come from files"
>  msgstr ""
> +"Bedingungen für das Einbinden von Konfigurationen aus relativen Pfaden 
> muss\n"
> +"aus Dateien kommen."

Bedingungen [...] müssen aus Dateien kommen

>  #: fetch-pack.c:1150
>  #, c-format
>  msgid "Server does not allow request for unadvertised object %s"
> -msgstr ""
> +msgstr "Der Server erlaubt keine Anfrage für nicht angebotenes Objekt %s."

Double negation does not sound good. Perhaps:
"Der Server lehnt Anfrage nach nicht angebotenem Objekt %s ab."
or possibly (still double negation)
"Der Server erlaubt keine Anfrage für nicht öffentliches Objekt %s."
>  #: builtin/for-each-ref.c:46
> -#, fuzzy
>  msgid "print only refs which don't contain the commit"
> -msgstr "nur Referenzen ausgeben, die diesen Commit enthalten"
> +msgstr "nur Referenzen ausgeben, die diesen nicht Commit enthalten"

"nur Referenzen ausgeben, die diesen Commit nicht enthalten"

refs are here translated as Referenzen

>  #: builtin/grep.c:1189
> -#, fuzzy
>  msgid "--no-index or --untracked cannot be used with revs"
> -msgstr ""
> -"Die Optionen --no-index und --untracked können nicht mit Commits verwendet "
> -"werden."
> +msgstr "--no-index oder --untracked können nicht mit Commits verwendet 
> werden"

refs are here translated as Commits?

>  #: builtin/grep.c:1195
> -#, fuzzy, c-format
> +#, c-format
>  msgid "unable to resolve revision: %s"
> -msgstr "Konnte %s nicht nach %s verschieben"
> +msgstr "Konnte Commit nicht auflösen: %s"

same here

>  #: builtin/tag.c:421 builtin/tag.c:423
> -#, fuzzy
>  msgid "print only tags that don't contain the commit"
> -msgstr "nur Tags ausgeben, die diesen Commit beinhalten"
> +msgstr "nur Tags ausgeben, die diesen nicht Commit enthalten"

"nur Tags ausgeben, die diesen Commit nicht enthalten"

Best,
Christian
-- 
Wie man sein Kind nicht nennen sollte: 
  Anna Naß 

Antw: Re: Terrible bad performance for it blame --date=iso -C -C master --

[Ulrich Windl]

>>> Samuel Lijin  schrieb am 03.05.2017 um 09:12 in Nachricht

Cloning a custom refspec does not work as expected

[Sebastian Schuberth]

Hi,

I'm trying to clone a custom ref, avoiding the need to init && fetch
manually. From reading [1] which says:

"Set a configuration variable in the newly-created repository; this
takes effect immediately after the repository is initialized, but
before the remote history is fetched or any files checked out. [...]
This makes it safe, for example, to add additional fetch refspecs to
the origin remote."

I'd assume that this would work:

$ git clone -c remote.origin.fetch=+refs/changes/*:refs/remotes/origin/changes/*
https://gerrit.googlesource.com/git-repo

In fact, this *does* add the custom refs correct to .git/config, but
they are not fetched during the clone. I can manually fix that by
doing

$ cd git-repo
$ git fetch

afterwards, but the whole point is to avoid exactly that separate step.

Is this a code bug or documentation bug?

I'm using git version 2.12.2.windows.2.

[1] https://git-scm.com/docs/git-clone#git-clone---configltkeygtltvaluegt

-- 
Sebastian Schuberth

[no subject]

[CAROLINE EDWARD]

hs

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Samuel Lijin]

On Tue, May 2, 2017 at 9:05 AM, Jeff Hostetler  wrote:
>
>
> On 5/2/2017 12:17 AM, Stefan Beller wrote:
>>
>> On Mon, May 1, 2017 at 6:36 PM, Junio C Hamano  wrote:
>>>
>>> Stefan Beller  writes:
>>>
 This applies to origin/master.

 For better readability and understandability for newcomers it is a good
 idea
 to not offer 2 APIs doing the same thing with on being the #define of
 the other.

 In the long run we may want to drop the macros guarded by
 NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
>
>
> Thank you for bringing this up and making this proposal.
> I started a similar effort internally last fall, but
> stopped because of the footprint size.
>
>>>
>>> Why?  Why should we keep typing _index, when most of the time we
>>> are given _the_ index and working on it?
>>
>>
>> As someone knowledgeable with the code base you know that the cache_*
>> and index_* functions only differ by an index argument. A newcomer may not
>> know this, so they wonder why we have (A) so many functions [and which is
>> the
>> right function to use]; it is an issue of ease of use of the code base.
>>
>> Anything you do In submodule land today needs to spawn new processes in
>> the submodule. This is cumbersome and not performant. So in the far future
>> we may want to have an abstraction of a repo (B), i.e. all repository
>> state in
>> one struct/class. That way we can open a submodule in-process and perform
>> the required actions without spawning a process.
>>
>> The road to (B) is a long road, but we have to art somewhere. And this
>> seemed
>> like a good place by introducing a dedicated argument for the
>> repository. In a follow
>> up in the future we may want to replace _index by
>> "the_main_repo.its_index"
>> and then could also run the commands on other (submodule) indexes. But
>> more
>> importantly, all these commands would operate on a repository object.
>>
>> In such a far future we would have functions like the cmd_* functions
>> that would take a repository object instead of doing its setup discovery
>> on their own.
>>
>> Another reason may be its current velocity (or absence of it) w.r.t. to
>> these
>> functions, such that fewer merge conflicts may arise.
>
>
> In addition to (eventually) allowing multiple repos be open at
> the same time for submodules, it would also help with various
> multi-threading efforts.  For example, we have loops that do a
> "for (k = 0, k < active_nr; k++) {...}"  There is no visual clue
> in that code that it references "the_index" and therefore should
> be subject to the same locking.  Granted, this is a trivial example,
> but goes to the argument that the code has lots of subtle global
> variables and macros that make it difficult to reason about the
> code.

Just to throw out an example, I'm relatively new to the codebase (I've
been lurking on the mailing list for a few months now) and for a
recent project (I'm an undergrad wrapping up my senior year, and one
of my classes' final projects was to do something that involved
concurrency) I took a shot at parallelizing the estimate_similarity()
calls in diffcore_rename(). The only way I was able to get it to work
was by dropping global mutexes in one or two files (the code for those
mutexes still makes me cringe), because of concurrent writes to global
data structures.

Re: [PATCH v3 0/6] rebase -i: add config to abbreviate command-names

[Johannes Schindelin]

Hi Liam,

On Tue, 2 May 2017, Liam Beguin wrote:

> On Tue, 2017-05-02 at 17:48 +0200, Johannes Schindelin wrote:
> 
> > I offered a couple of comments, my biggest one probably being that
> > this patch series is crossing paths with my patch series that tries to
> > move more functionality out of the git-rebase--interactive.sh script
> > into the git-rebase--helper that is written in C, closely followed by
> > my suggestion to fold at least part of the functionality into the
> > SHA-1 collapsing/expanding.
> 
> I've seen a few messages about this migration, but I'm not yet sure I
> understand the difference between the shell and the C implementations.
> Is the C version going to replace 'git-rebase--interactive.sh'?

The C version has already started to replace the shell script, yes. In
adding and using git-rebase--helper, there are already parts of the
interactive rebase functionality that are run using C code only. The idea
is to move more and more functionality over (separating out the
--preserve-merges handling into a different shell script, as I have no
plans to convert that code to C, and as far as I can see nobody else wants
to step up to that task, either). Eventually, we may be able to finish
that gigantic task of having git-rebase be all builtin C code.

> > If your patch series "wins", I can easily forward-port your changes to
> > the rebase-i-extra branch, but it may actually make sense to build on
> > top of the rebase-i-extra branch to begin with. If you agree: I pushed
> > the proposed change to the `rebase-i-extra+abbrev` branch at
> > https://github.com/dscho/git.
> 
> If 'git-rebase--interactive.sh' is bound to be replaced, I could
> just shrink this to the Documentation cleanup (patches 4 and 5)
> and rework the rest on top of your new implementation.

I kind of hoped that Junio would chime in with his verdict. That would be
the ultimate deciding factor, I think.

Ciao,
Johannes

Re: PCRE v2 compile error, was Re: What's cooking in git.git (May 2017, #01; Mon, 1)

[Duy Nguyen]

On Wed, May 3, 2017 at 4:45 PM, Johannes Schindelin
 wrote:
>> So it makes sense to give our downstream distributors a nudge to
>> switch over to it.

Some contributor (i.e. me) was not happy with this nudging though. The
other day I switched to some branch (probably 'pu') and the build
failed because, guess what, I didn't have pcre2 installed. If I set
USE_LIBPCRE1 then I lose pcre support when switching to other
branches. And no, I don't want to install libpcre2, not when I'm
forced to this way.

188 packages on Gentoo optionally depend on libpcre, 6 packages on
libpcre2. Chances that a Gentoo user has libpcre2 already are rather
low. I'll revisit my installation when the level of libpcre2 support
grows a bit more than that. You can nudge distributors directly,
probably more efficient too.

> ...
>
> I hate to be that someone, but it has to be said: this is a disruptive
> change, and it would be a lot better to make it an opt-in at first, and
> when the dust settled about this option and many distributions have opted
> in already because of the benefits and tested this thoroughly in practice,

Agreed.
-- 
Duy

Re: RFA: untracked cache vs git reset --hard

[Duy Nguyen]

On Wed, May 3, 2017 at 5:27 PM, Johannes Schindelin
 wrote:
> Hi all,
>
> I have a problem and would like to solicit advice how to fix it.
>
> The untracked cache has made a real difference on rather large
> repositories with tons of directories, and it is really, really useful.
>
> But. One innocuous `git reset --hard` will just blow it away.
>
> How? reset_index() calls unpack_trees() which in turn tries to populate a
> new index and then discards the old one:
>
> https://github.com/git/git/blob/v2.12.2/unpack-trees.c#L1293
>
> That discard_index() unfortunately also blows away each and every index
> extension that had been read carefully before.

This is a real problem when we introduce non-optional extensions (i.e.
extension name in lower case). Dropping them is not an option because
they may contain vital/original information. We haven't any so far,
but I've been wanting to add one for years (narrow clone). So I'm all
for tackling the problem now :)

> All users of `git reset --hard` (including `git stash`) suffer this.
>
> In fact, it looks as if *any* caller of unpack_trees() would suffer the
> same problem: git-am, git-checkout, git-commit, git-merge, etc
>
> Now, I could imagine that maybe we could just "move"
> o->dst_index.untracked to o->result.untracked, and that the machinery then
> would do the right thing.

These extensions may have dependencies in the o->result.cache[] (do we
allow an extension to depend on another?). If invalidation is not
handled correctly then it's not safe to simply copy the extension
over.

For untracked cache, I think we do invalidation right and just moving
it over dst_index (and resetting NULL in o->result so it does not get
accidentally deleted) is fine.

I'd rather we have a common way of dealing with this for any extension
though. Split index needs special treatment too [1]. Maybe we can add

int migrate_index_extensions(struct index_state *dst, struct index_state *src);

in read-cache.c where it calls migrate_XXX() for each extension. In
some cases (cache-tree) we could even do more, like repair cache-tree
there to avoid hitting performance regressions.

[1] https://github.com/git/git/blob/v2.12.2/unpack-trees.c#L1165-L1167
-- 
Duy

RFA: untracked cache vs git reset --hard

[Johannes Schindelin]

Hi all,

I have a problem and would like to solicit advice how to fix it.

The untracked cache has made a real difference on rather large
repositories with tons of directories, and it is really, really useful.

But. One innocuous `git reset --hard` will just blow it away.

How? reset_index() calls unpack_trees() which in turn tries to populate a
new index and then discards the old one:

https://github.com/git/git/blob/v2.12.2/unpack-trees.c#L1293

That discard_index() unfortunately also blows away each and every index
extension that had been read carefully before.

All users of `git reset --hard` (including `git stash`) suffer this.

In fact, it looks as if *any* caller of unpack_trees() would suffer the
same problem: git-am, git-checkout, git-commit, git-merge, etc

Now, I could imagine that maybe we could just "move"
o->dst_index.untracked to o->result.untracked, and that the machinery then
would do the right thing.

However, I am far from an expert in this area, so I would appreciate all
the helpful advice I could get.

Thoughts?

Thanks,
Johannes

Re: [PATCH 0/5] Start of a journey: drop NO_THE_INDEX_COMPATIBILITY_MACROS

[Duy Nguyen]

On Tue, May 2, 2017 at 8:36 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This applies to origin/master.
>>
>> For better readability and understandability for newcomers it is a good idea
>> to not offer 2 APIs doing the same thing with on being the #define of the 
>> other.
>>
>> In the long run we may want to drop the macros guarded by
>> NO_THE_INDEX_COMPATIBILITY_MACROS. This converts a couple of them.
>
> Why?

Well.. why did you add NO_THE_INDEX_COMP... in the first place? ;-) j/k

> Why should we keep typing _index, when most of the time we are given 
> _the_ index and working on it?

I attempted the same thing once or twice in the past, had the same
impression and dropped it. But I think it's good to get rid of cache_*
macros, at least outside builtin/ directory. It makes it clearer about
the index dependency. Maybe one day we could libify sha1_file.c and
finally introduce "struct repository", where the_index, object db and
ref-store belong (hmm.. the index may belong to "struct worktree", not
the repo one...).

So yeah it may look a bit more verbose (and probably causes a lot more
conflicts on 'pu') but in my opinion it's a good direction. I wish
Stefan good luck. Brave soul :D
-- 
Duy

Re: CYGWIN git cannot install python packages with pip

[ankostis]

On 3 May 2017 11:47, Johannes Schindelin  wrote:
> On Tue, 2 May 2017, ankostis wrote:
>
>> On Windows, with Cygwin-git 02.12.2-1 the python command:
>> [...]
>
> You forgot to mention what python/pip you use (is it the Cygwin one, or a
> standalone one?). Intentional?

I have tested it on using WinPython[1] 3.6.1 & 3.5.3.
WinPython is built out of the "standard" python on Windows.

Judging by the error, the problem is on the temporary folder
used as target.  For some reason, Git uses $HOME
as a prefix to the temporary folder selected by `pip`.

My home in the above case above is this:
/cygdrive/d/Work/ALLINONES2/co2mpas_AIO-v1.6.0/CO2MPAS


Thanks,
  Kostis

[1] http://winpython.github.io/

[PATCH v2 21/21] t1308: add a test case on open a config directory

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

We don't support config-as-a-directory (maybe someday we will?). Make
sure we consistently fail in this case, which should happen on platforms
where fopen() returns non-NULL if FREAD_READS_DIRECTORIES is
defined.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1308-config-set.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 13e95561f4..e495a61616 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -183,6 +183,15 @@ test_expect_success 'proper error on non-existent files' '
test_cmp expect actual
 '
 
+test_expect_success 'proper error on directory "files"' '
+   echo "Error (-1) reading configuration file a-directory." >expect &&
+   mkdir a-directory &&
+   test_expect_code 2 test-config configset_get_value foo.bar a-directory 
2>output &&
+   grep "^warning:" output &&
+   grep "^Error" output >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
chmod -r .git/config &&
test_when_finished "chmod +r .git/config" &&
-- 
2.11.0.157.gd943d85

[PATCH v2 20/21] config.c: handle error on failing to fopen()

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

In the first case, we already correctly return -1 if fopen() fails to
open. But we should report something so people know what's wrong.

In the second case, config_file == NULL does not necessarily mean "no
config file". Bail out if needed.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 config.c  | 5 -
 t/t1308-config-set.sh | 4 +++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index b4a3205da3..e54d99d519 100644
--- a/config.c
+++ b/config.c
@@ -1422,7 +1422,7 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
int ret = -1;
FILE *f;
 
-   f = fopen(filename, "r");
+   f = fopen_or_warn(filename, "r");
if (f) {
flockfile(f);
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, 
filename, f, data);
@@ -2640,6 +2640,9 @@ int git_config_rename_section_in_file(const char 
*config_filename,
}
 
if (!(config_file = fopen(config_filename, "rb"))) {
+   ret = warn_on_fopen_errors(config_filename);
+   if (ret)
+   goto out;
/* no config file means nothing to rename, no error */
goto commit_and_out;
}
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960cca..13e95561f4 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -187,7 +187,9 @@ test_expect_success POSIXPERM,SANITY 'proper error on 
non-accessible files' '
chmod -r .git/config &&
test_when_finished "chmod +r .git/config" &&
echo "Error (-1) reading configuration file .git/config." >expect &&
-   test_expect_code 2 test-config configset_get_value foo.bar .git/config 
2>actual &&
+   test_expect_code 2 test-config configset_get_value foo.bar .git/config 
2>output &&
+   grep "^warning:" output &&
+   grep "^Error" output >actual &&
test_cmp expect actual
 '
 
-- 
2.11.0.157.gd943d85

[PATCH v2 17/21] server-info: report error on failure to fopen()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 server-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server-info.c b/server-info.c
index f6c1a3dfb0..e01ac154a8 100644
--- a/server-info.c
+++ b/server-info.c
@@ -133,7 +133,7 @@ static int read_pack_info_file(const char *infofile)
char line[1000];
int old_cnt = 0;
 
-   fp = fopen(infofile, "r");
+   fp = fopen_or_warn(infofile, "r");
if (!fp)
return 1; /* nonexistent is not an error. */
 
-- 
2.11.0.157.gd943d85

[PATCH v2 15/21] rerere.c: report correct errno

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

In the first case, we should have reported the reason fopen() fails. In
the second case, fclose() might change errno but we want to report
fopen() failure.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 rerere.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/rerere.c b/rerere.c
index 971bfedfb2..c26c29f87a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -484,13 +484,14 @@ static int handle_file(const char *path, unsigned char 
*sha1, const char *output
io.input = fopen(path, "r");
io.io.wrerror = 0;
if (!io.input)
-   return error("Could not open %s", path);
+   return error_errno("Could not open %s", path);
 
if (output) {
io.io.output = fopen(output, "w");
if (!io.io.output) {
+   error_errno("Could not write %s", output);
fclose(io.input);
-   return error("Could not write %s", output);
+   return -1;
}
}
 
-- 
2.11.0.157.gd943d85

[PATCH v2 18/21] wt-status.c: report error on failure to fopen()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 wt-status.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 0375484962..cdf9f5eed2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1065,7 +1065,8 @@ static void show_am_in_progress(struct wt_status *s,
 static char *read_line_from_git_path(const char *filename)
 {
struct strbuf buf = STRBUF_INIT;
-   FILE *fp = fopen(git_path("%s", filename), "r");
+   FILE *fp = fopen_or_warn(git_path("%s", filename), "r");
+
if (!fp) {
strbuf_release();
return NULL;
-- 
2.11.0.157.gd943d85

[PATCH v2 19/21] xdiff-interface.c: report errno on failure to stat() or fopen()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 xdiff-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index 060038c2d6..d3f78ca2a7 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -164,9 +164,9 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
size_t sz;
 
if (stat(filename, ))
-   return error("Could not stat %s", filename);
+   return error_errno("Could not stat %s", filename);
if ((f = fopen(filename, "rb")) == NULL)
-   return error("Could not open %s", filename);
+   return error_errno("Could not open %s", filename);
sz = xsize_t(st.st_size);
ptr->ptr = xmalloc(sz ? sz : 1);
if (sz && fread(ptr->ptr, sz, 1, f) != 1) {
-- 
2.11.0.157.gd943d85

[PATCH v2 14/21] rerere.c: report error on failure to fopen()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 rerere.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rerere.c b/rerere.c
index 3bd55caf3b..971bfedfb2 100644
--- a/rerere.c
+++ b/rerere.c
@@ -200,7 +200,7 @@ static struct rerere_id *new_rerere_id(unsigned char *sha1)
 static void read_rr(struct string_list *rr)
 {
struct strbuf buf = STRBUF_INIT;
-   FILE *in = fopen(git_path_merge_rr(), "r");
+   FILE *in = fopen_or_warn(git_path_merge_rr(), "r");
 
if (!in)
return;
-- 
2.11.0.157.gd943d85

[PATCH v2 16/21] sequencer.c: report error on failure to fopen()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 sequencer.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 10c3b4ff81..11b5c7c114 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -897,8 +897,8 @@ static void flush_rewritten_pending(void) {
FILE *out;
 
if (strbuf_read_file(, rebase_path_rewritten_pending(), 82) > 0 &&
-   !get_sha1("HEAD", newsha1) &&
-   (out = fopen(rebase_path_rewritten_list(), "a"))) {
+   !get_sha1("HEAD", newsha1) &&
+   (out = fopen_or_warn(rebase_path_rewritten_list(), "a"))) {
char *bol = buf.buf, *eol;
 
while (*bol) {
@@ -917,7 +917,7 @@ static void flush_rewritten_pending(void) {
 
 static void record_in_rewritten(struct object_id *oid,
enum todo_command next_command) {
-   FILE *out = fopen(rebase_path_rewritten_pending(), "a");
+   FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a");
 
if (!out)
return;
@@ -1378,7 +1378,7 @@ static int read_populate_todo(struct todo_list *todo_list,
 
if (is_rebase_i(opts)) {
struct todo_list done = TODO_LIST_INIT;
-   FILE *f = fopen(rebase_path_msgtotal(), "w");
+   FILE *f = fopen_or_warn(rebase_path_msgtotal(), "w");
 
if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
!parse_insn_buffer(done.buf.buf, ))
-- 
2.11.0.157.gd943d85

[PATCH v2 13/21] remote.c: report error on failure to fopen()

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

There's plenty of error() in this code to safely assume --quiet is not a
concern.

t5512 needs update because if we check the path 'refs*master' (i.e. the
asterisk is part of the path) then we'll get an EINVAL error.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 remote.c |  4 ++--
 t/t5512-ls-remote.sh | 13 ++---
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 801137c72e..1f2453d0f6 100644
--- a/remote.c
+++ b/remote.c
@@ -251,7 +251,7 @@ static const char *skip_spaces(const char *s)
 static void read_remotes_file(struct remote *remote)
 {
struct strbuf buf = STRBUF_INIT;
-   FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
+   FILE *f = fopen_or_warn(git_path("remotes/%s", remote->name), "r");
 
if (!f)
return;
@@ -277,7 +277,7 @@ static void read_branches_file(struct remote *remote)
 {
char *frag;
struct strbuf buf = STRBUF_INIT;
-   FILE *f = fopen(git_path("branches/%s", remote->name), "r");
+   FILE *f = fopen_or_warn(git_path("branches/%s", remote->name), "r");
 
if (!f)
return;
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 94fc9be9ce..02106c9226 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -85,8 +85,15 @@ test_expect_success 'use branch..remote if possible' '
 '
 
 test_expect_success 'confuses pattern as remote when no remote specified' '
-   cat >exp <<-\EOF &&
-   fatal: '\''refs*master'\'' does not appear to be a git repository
+   if test_have_prereq MINGW
+   then
+   # Windows does not like asterisks in pathname
+   does_not_exist=master
+   else
+   does_not_exist="refs*master"
+   fi &&
+   cat >exp <<-EOF &&
+   fatal: '\''$does_not_exist'\'' does not appear to be a git repository
fatal: Could not read from remote repository.
 
Please make sure you have the correct access rights
@@ -98,7 +105,7 @@ test_expect_success 'confuses pattern as remote when no 
remote specified' '
# fetch .
# We could just as easily have used "master"; the "*" emphasizes its
# role as a pattern.
-   test_must_fail git ls-remote refs*master >actual 2>&1 &&
+   test_must_fail git ls-remote "$does_not_exist" >actual 2>&1 &&
test_i18ncmp exp actual
 '
 
-- 
2.11.0.157.gd943d85

[PATCH v2 12/21] commit.c: report error on failure to fopen() the graft file

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

Suppressing the error (because the command requires --quiet) is not a
concern because we already call error() just a couple lines down.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 commit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 73c78c2b80..075607a6da 100644
--- a/commit.c
+++ b/commit.c
@@ -167,10 +167,12 @@ struct commit_graft *read_graft_line(char *buf, int len)
 
 static int read_graft_file(const char *graft_file)
 {
-   FILE *fp = fopen(graft_file, "r");
+   FILE *fp = fopen_or_warn(graft_file, "r");
struct strbuf buf = STRBUF_INIT;
+
if (!fp)
return -1;
+
while (!strbuf_getwholeline(, fp, '\n')) {
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
-- 
2.11.0.157.gd943d85

[PATCH v2 09/21] blame: report error on open if graft_file is a directory

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/blame.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 07506a3e45..1648b396dc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2071,10 +2071,12 @@ static int prepare_lines(struct scoreboard *sb)
  */
 static int read_ancestry(const char *graft_file)
 {
-   FILE *fp = fopen(graft_file, "r");
+   FILE *fp = fopen_or_warn(graft_file, "r");
struct strbuf buf = STRBUF_INIT;
+
if (!fp)
return -1;
+
while (!strbuf_getwholeline(, fp, '\n')) {
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line(buf.buf, buf.len);
-- 
2.11.0.157.gd943d85