Re: [PATCH 14/40] sha1_file: prepare for external odbs

[Christian Couder]

On Thu, Jan 4, 2018 at 7:00 PM, Jeff Hostetler  wrote:
>
>
> On 1/3/2018 11:33 AM, Christian Couder wrote:

>> diff --git a/sha1_file.c b/sha1_file.c
>> index 261baf800f..785e8dda03 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -322,17 +322,22 @@ static void fill_sha1_path(struct strbuf *buf, const
>> unsigned char *sha1)
>> }
>>   }
>>   -const char *sha1_file_name(const unsigned char *sha1)
>> +const char *sha1_file_name_alt(const char *objdir, const unsigned char
>> *sha1)
>>   {
>> static struct strbuf buf = STRBUF_INIT;
>
> While we are refactoring sha1_file_name() and adding
> sha1_file_name_alt(), could we also change the API and
> pass in the strbuf so we can get rid of the static buffer?
> Granted, it is a little off topic, but it will help out
> in the long run.

Ok, but I prefer to do that in a separate patch series, so I just sent:

https://public-inbox.org/git/20180116071814.19884-1-chrisc...@tuxfamily.org

>> @@ -1551,7 +1562,7 @@ static inline int directory_size(const char
>> *filename)
>>* We want to avoid cross-directory filename renames, because those
>>* can have problems on various filesystems (FAT, NFS, Coda).
>>*/
>> -static int create_tmpfile(struct strbuf *tmp, const char *filename)
>> +int create_object_tmpfile(struct strbuf *tmp, const char *filename)
>>   {
>> int fd, dirlen = directory_size(filename);
>>   @@ -1591,7 +1602,7 @@ static int write_loose_object(const unsigned char
>> *sha1, char *hdr, int hdrlen,
>> static struct strbuf tmp_file = STRBUF_INIT;
>
> Same thing here, since we are renaming the function anyway, could we
> add a strbuf arg and get rid of the static one?

I will see how it goes with the above patch to remove the static
buffer in sha1_file_name() before preparing a patch to remove the
static buffer in create_tmpfile().

Thanks,
Christian.

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

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

2018-01-16 1:09 GMT+03:00 Jeff King :
> On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:
>
>> That works, but I don't think it's where we want to end up in the long
>> run.

I absolutely agree, I want to merge current edits and then continue
migrating process. And hopefully second part of the function will also
be removed.

>> Because:
>>
>>   1. We still have the set of formats duplicated between expand_atom()
>>  and the "preparation" step. It's just that the preparation is now
>>  in ref-filter.c. What happens if ref-filter.c learns new formatting
>>  placeholders (or options for those placeholders) that cat-file.c
>>  doesn't, or vice versa? The two have to be kept in sync.
>>
>>   2. We're missing out on all of the other placeholders that ref-filter
>>  knows about. Not all of them are meaningful (e.g., %(refname)
>>  wouldn't make sense here), but part of our goal is to support the
>>  same set of placeholders as much as possible. Some obvious ones
>>  that ought to work for cat-file: %(objectname:short), %(if), and
>>  things like %(subject) when the appropriate object type is used.
>>
>> In other words, I think the endgame is that expand_atom() isn't there at
>> all, and we're calling the equivalent of format_ref_item() for each
>> object (except that in a unified formatting world, it probably doesn't
>> have the word "ref" in it, since that's just one of the items a caller
>> might pass in).

Agree! I want to merge current edits, then create format.h file and
make some renames, then finish migrating process to new format.h and
support all new meaningful tags.

>
> I read carefully up through about patch 6, and then skimmed through the
> rest. I think we really want to push this in the direction of more
> unification, as above. I know that the patches here may be a step on the
> way, but I had a hard time evaluating each patch to see if it was
> leading us in the right direction.
>
> I think what would help for reviewing this is:
>
>   1. Start with a cover letter that makes it clear what the end state of
>  the series is, and why that's the right place to end up.
>
>   2. Include a rough roadmap of the patches in the cover letter.
>  Hopefully they should group into a few obvious steps (like "in
>  patches 1-5, we're teaching ref-filter to support everything that
>  cat-file can do, then in 6-10 we're preparing cat-file for the
>  conversion, and then in 11 we do the conversion"). If it doesn't
>  form a coherent narrative, then it may be worth stepping back and
>  thinking about combining or reordering the patches in a different
>  way, so that the progression becomes more plain.
>
>  I think one of the things that makes the progression here hard to
>  understand (for me, anyway) is that it's "inside out" of what I'd
>  expect. There's a lot of code movement happening first, and then
>  refactoring and simplifying after that. So between those two steps,
>  there's a lot of interim ugliness (e.g., having to reach across
>  module boundaries to look at expand_data). It's hard to tell when
>  looking at each individual patch how necessary the ugliness is, and
>  whether and when it's going to go away later in the series.
>
> There's a lot of good work here, and you've figured out a lot about how
> the two systems function. I think we just need to rearrange things a bit
> to make sure each step is moving in the right direction.
>
> -Peff

As I said, I am sure that I will continue working on that, so this is
not the end state. So I am not sure that I am able to write finalizing
messages for now. But, if we merge current edits, it will be much
easier for me to continue working on that (next patch would be about
creating format.h and I am afraid of some merge conflicts if I will
develop absolutely all process from start to finish in my branch, it
takes time. It's not a big problem, but, if we find final goal
worthwhile, so maybe we could go to it step-by-step).

[PATCH] sha1_file: remove static strbuf from sha1_file_name()

[Christian Couder]

Using a static buffer in sha1_file_name() is error prone
and the performance improvements it gives are not needed
in most of the callers.

So let's get rid of this static buffer and, if necessary
or helpful, let's use one in the caller.
---
 cache.h   |  8 +++-
 http-walker.c |  6 --
 http.c| 16 ++--
 sha1_file.c   | 38 +-
 4 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/cache.h b/cache.h
index d8b975a571..6db565408e 100644
--- a/cache.h
+++ b/cache.h
@@ -957,12 +957,10 @@ extern void check_repository_format(void);
 #define TYPE_CHANGED0x0040
 
 /*
- * Return the name of the file in the local object database that would
- * be used to store a loose object with the specified sha1.  The
- * return value is a pointer to a statically allocated buffer that is
- * overwritten each time the function is called.
+ * Put in `buf` the name of the file in the local object database that
+ * would be used to store a loose object with the specified sha1.
  */
-extern const char *sha1_file_name(const unsigned char *sha1);
+extern void sha1_file_name(struct strbuf *buf, const unsigned char *sha1);
 
 /*
  * Return an abbreviated sha1 unique within this repository's object database.
diff --git a/http-walker.c b/http-walker.c
index 1ae8363de2..07c2b1af82 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -544,8 +544,10 @@ static int fetch_object(struct walker *walker, unsigned 
char *sha1)
} else if (hashcmp(obj_req->sha1, req->real_sha1)) {
ret = error("File %s has bad hash", hex);
} else if (req->rename < 0) {
-   ret = error("unable to write sha1 filename %s",
-   sha1_file_name(req->sha1));
+   struct strbuf buf = STRBUF_INIT;
+   sha1_file_name(, req->sha1);
+   ret = error("unable to write sha1 filename %s", buf.buf);
+   strbuf_release();
}
 
release_http_object_request(req);
diff --git a/http.c b/http.c
index 5977712712..5979305bc9 100644
--- a/http.c
+++ b/http.c
@@ -2168,7 +2168,7 @@ struct http_object_request *new_http_object_request(const 
char *base_url,
unsigned char *sha1)
 {
char *hex = sha1_to_hex(sha1);
-   const char *filename;
+   struct strbuf filename = STRBUF_INIT;
char prevfile[PATH_MAX];
int prevlocal;
char prev_buf[PREV_BUF_SIZE];
@@ -2180,14 +2180,15 @@ struct http_object_request 
*new_http_object_request(const char *base_url,
hashcpy(freq->sha1, sha1);
freq->localfile = -1;
 
-   filename = sha1_file_name(sha1);
+   sha1_file_name(, sha1);
snprintf(freq->tmpfile, sizeof(freq->tmpfile),
-"%s.temp", filename);
+"%s.temp", filename.buf);
 
-   snprintf(prevfile, sizeof(prevfile), "%s.prev", filename);
+   snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf);
unlink_or_warn(prevfile);
rename(freq->tmpfile, prevfile);
unlink_or_warn(freq->tmpfile);
+   strbuf_release();
 
if (freq->localfile != -1)
error("fd leakage in start: %d", freq->localfile);
@@ -2302,6 +2303,7 @@ void process_http_object_request(struct 
http_object_request *freq)
 int finish_http_object_request(struct http_object_request *freq)
 {
struct stat st;
+   struct strbuf filename = STRBUF_INIT;
 
close(freq->localfile);
freq->localfile = -1;
@@ -2327,8 +2329,10 @@ int finish_http_object_request(struct 
http_object_request *freq)
unlink_or_warn(freq->tmpfile);
return -1;
}
-   freq->rename =
-   finalize_object_file(freq->tmpfile, sha1_file_name(freq->sha1));
+
+   sha1_file_name(, freq->sha1);
+   freq->rename = finalize_object_file(freq->tmpfile, filename.buf);
+   strbuf_release();
 
return freq->rename;
 }
diff --git a/sha1_file.c b/sha1_file.c
index 3da70ac650..f66c21b2da 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -321,15 +321,11 @@ static void fill_sha1_path(struct strbuf *buf, const 
unsigned char *sha1)
}
 }
 
-const char *sha1_file_name(const unsigned char *sha1)
+void sha1_file_name(struct strbuf *buf, const unsigned char *sha1)
 {
-   static struct strbuf buf = STRBUF_INIT;
-
-   strbuf_reset();
-   strbuf_addf(, "%s/", get_object_directory());
+   strbuf_addf(buf, "%s/", get_object_directory());
 
-   fill_sha1_path(, sha1);
-   return buf.buf;
+   fill_sha1_path(buf, sha1);
 }
 
 struct strbuf *alt_scratch_buf(struct alternate_object_database *alt)
@@ -710,7 +706,12 @@ int check_and_freshen_file(const char *fn, int freshen)
 
 static int check_and_freshen_local(const unsigned char *sha1, int freshen)
 {
-   return check_and_freshen_file(sha1_file_name(sha1), freshen);
+   static struct strbuf buf = STRBUF_INIT;
+
+   strbuf_reset();
+   sha1_file_name(, sha1);
+
+   

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

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

2018-01-16 0:44 GMT+03:00 Jeff King :
> On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:
>
>> Need that for further reusing of formatting logic in cat-file.
>> Have plans to get rid of using expand_data in cat-file at all,
>> and use it only in ref-filter for collecting, formatting and printing
>> needed data.
>
> I think some of these will want to remain in cat-file.c. For instance,
> split_on_whitespace is not something that ref-filter.c itself would care
> about. I'd expect in the endgame for cat-file to keep its own
> split_on_whitespace flag, and to set it based on the presence of the
> %(rest) flag, which it can check by seeing if the "rest" atom is in the
> "used_atom" list after parsing the format.
>
> -Peff

Yes, maybe we will need to leave some part of expand_data variables.
But, if it would be only "split_on_whitespace", it's better to make
just one flag without any other stuff. Actually, I thought about
moving related logic to ref-filter also. Anyway, it's hard to say
exact things before we code everything. Do I need to fix commit
message and make it more soft?

Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-16 0:42 GMT+03:00 Jeff King :
> On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> I agree in the end we'd want a single valid_atom list. It doesn't look
> like we hit that end state in this series, though.
>
> I guess I'm not quite clear on why we're not adding these new atoms to
> ref-filter (and for-each-ref) right away, though. We already have the
> first three (name, type, and size), and we'd just need to support
> %(rest) and %(deltabase).
>
> I think %(rest) doesn't really make sense for for-each-ref (we're not
> reading any input), but it could expand to the empty string by default
> (or even throw an error if the caller asks us not to support it).
>
> IOW, the progression I'd expect in a series like this is:
>
>   1. Teach ref-filter.c to support everything that cat-file can do.
>
>   2. Convert cat-file to use ref-filter.c.
>
> -Peff

I agree, I even made this and it's working fine:
https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
But I decided not to add that to patch because I expand the
functionality of several commands (not only cat-file and
for-each-ref), and I need to support all new functionality in a proper
way, make these error messages, test everything and - the hardest one
- support many new commands for cat-file. As I understand, it is not
possible unless we finally move to ref-filter and print results also
there. Oh, and I also need to rewrite docs in that case. And I decided
to apply this in another patch. But, please, say your opinion, maybe
we could do that here in some way.

Olga

RE: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Randall S. Becker]

On January 15, 2018 10:01 PM, brian m. Carlson wrote:
> On Mon, Jan 15, 2018 at 09:25:37AM -0500, Randall S. Becker wrote:
> > On January 15, 2018 2:06 AM, Johannes Sixt wrote:
> > > I take "die exits with non-zero" as a piece of information for the
> > > *users* so that they can write "if perl foo.pl; then something; fi"
> > > in shell scripts. I do *not* interpret it as leeway for implementers
> > > of perl to choose any random value as exit code. Choosing 162 just
> > > to be funky would be short-sighted. [I'm saying all this without knowing
> how perl specifies 'die'
> > > beyond the paragraph you cited. Perhaps there's more about 'die'
> > > that justifies exit code 162.] I'd say that the perl port is broken.
> >
> > I agree that 162 is wrong. Its interpretation is 128+signal, which
> > clearly does not happen in this case. On the platform, if the perl
> > script is via stdin, 162 or 169 are returned. If via file (perl
> > file.pl), 255 comes back. The port has issues. I have an opened a bug
> > report with the platform developers. Usual non-Open Source timeframes
> > to fix apply. ☹
> 
> I believe the standard behavior for Perl with die is the following:
> 
> exit $! if $!;
> exit $? >> 8 if $? >> 8;
> exit 255; # otherwise
> 
> Is there an errno value on your port that matches 162?  Maybe EBADF?
> 
> On Linux, I get the following:
> 
> genre ok % printf die | perl -; echo $?
> Died at - line 1.
> 9

Nah. Worse. Assume a perl script that is simply 'die "hello world"'. If it's in 
a file, perl reports 255. If from stdin, perl reports 162/169. Doh. That's 
supposed to be 128+signum, and max sig is 31 (SIGABEND) on the platform. The 
difficulty at this point is that if I fix wait_or_whine to map either code to 
255 or 1, then many more other tests fail, so I'm stuck with either 6 
reasonably acceptable breaks or about 60 unacceptable ones, based on 
assumptions in test_must_fail or other fail detections in the git suite. I'd 
rather not mess with git code if the test breaks themselves are explainable.

Sign.

Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(2112884442)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[brian m. carlson]

On Mon, Jan 15, 2018 at 09:25:37AM -0500, Randall S. Becker wrote:
> On January 15, 2018 2:06 AM, Johannes Sixt wrote:
> > I take "die exits with non-zero" as a piece of information for the
> > *users* so that they can write "if perl foo.pl; then something; fi" in shell
> > scripts. I do *not* interpret it as leeway for implementers of perl to 
> > choose
> > any random value as exit code. Choosing 162 just to be funky would be
> > short-sighted. [I'm saying all this without knowing how perl specifies 'die'
> > beyond the paragraph you cited. Perhaps there's more about 'die' that
> > justifies exit code 162.] I'd say that the perl port is broken.
> 
> I agree that 162 is wrong. Its interpretation is 128+signal, which
> clearly does not happen in this case. On the platform, if the perl
> script is via stdin, 162 or 169 are returned. If via file (perl
> file.pl), 255 comes back. The port has issues. I have an opened a bug
> report with the platform developers. Usual non-Open Source timeframes
> to fix apply. ☹

I believe the standard behavior for Perl with die is the following:

exit $! if $!;
exit $? >> 8 if $? >> 8;
exit 255; # otherwise

Is there an errno value on your port that matches 162?  Maybe EBADF?

On Linux, I get the following:

genre ok % printf die | perl -; echo $?
Died at - line 1.
9
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [BUG] git remote prune removes local tags, depending on fetch config

[Michael Giuffrida]

Just to confirm, you're talking about the behavior of removing *all*
tags that aren't found on the remote? (Not just tags that used to be
on some remote, but have since been removed from that remote.) So,
with your proposed workflow, you would never create your own tags
locally, without pushing them to the remote before running `git fetch`
-- otherwise they would simply be deleted.

It doesn't seem like a useful feature -- you wouldn't expect `git
fetch --prune` to remove your local branches that you were developing
on, right?

On Mon, Jan 15, 2018 at 4:38 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Mon, Jan 15 2018, Michael Giuffrida jotted:
>
>> `git remote prune ` should "delete all stale remote-tracking
>> branches under ". I was surprised to discover, after some
>> troubleshooting, that it also deletes *all* local tags that don't
>> exist on the remote, if the following refspec is included in the
>> remote's fetch config:
>>
>> +refs/tags/*:refs/tags/*
>>
>> So, if `remote.origin.fetch` is configured to fetch all tags from the
>> remote, any tags I create locally will be deleted when running `git
>> remote prune origin`. This is not intuitive [1], nor is is it
>> explained in the docs [2]. Is this behavior obvious to someone with a
>> better understanding of Git internals?
>>
>> I did find a better way to automatically fetch tags (using tagopt
>> instead of adding the fetch refspec). However, the refspec doesn't
>> seem "wrong" in itself; in particular, `git fetch --tags` used to be
>> considered equivalent to specifying the refspec
>> "refs/tags/*:refs/tags/*" -- implying that this is a sensible refspec
>> [3]. So I wouldn't expect it to "break" the behavior of another
>> command.
>>
>> [1] https://stackoverflow.com/q/34687657/1327867
>> [2] https://git-scm.com/docs/git-remote.html#git-remote-empruneem
>> [3] 
>> https://github.com/git/git/commit/c5a84e92a2fe9e8748e32341c344d7a6c0f52a50
>
> These docs are really confusing, but it is working as intended, and
> really should be re-documented.
>
> The `git remote prune` subcommand just ends up piggy-backing on
> git-fetch, whose behavior is explained here:
> https://git-scm.com/docs/git-fetch.html#git-fetch---prune
>
> It's worked this way since at least v1.8.5.6, maybe at some distant
> point in the past it only did this for branches when invoked via
> git-remote as the documentation says.
>
> RELATED:
>
> I've actually had the reverse problem with this. I want some way to turn
> this behavior on without explicitly hacking the refspec, so I can do it
> globally in /etc/gitconfig or in ~/.gitconfig without screwing with the
> config of each checkout on certain machines.
>
> You can set fetch.prune=true, but that only prunes the branches, you
> need to inject remote.origin.fetch into each checkout, unless I've
> missed some way of doing this.
>
> I wanted to add fetch.pruneTags that would make it as if you had
> refs/tags/*:refs/tags/* in the fetch spec, but I haven't hacked that up
> yet, if anyone can see any inherent issue with that plan I'd like to
> know about it.

[PATCH] mru: Replace mru.[ch] with list.h implementation

[Gargi Sharma]

Replace the custom calls to mru.[ch] with calls to list.h. This patch is the
final step in removing the mru API completely and inlining the logic.

Another discussion, here
(https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/)
was on what has to be done with the next pointer of packed git type
inside the
packed_git structure. It can be removed _given_ that no one needs to
access the list in order and can be sent as another patch.

---
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.

The commit has been built on top of ot/mru-on-list branch.

 Makefile   |  1 -
 builtin/pack-objects.c | 12 ++--
 cache.h|  9 +
 list.h |  7 +++
 mru.c  | 27 ---
 mru.h  | 40 
 packfile.c | 18 +-
 sha1_file.c|  1 -
 8 files changed, 27 insertions(+), 88 deletions(-)
 delete mode 100644 mru.c
 delete mode 100644 mru.h

diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
 LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
 LIB_OBJS += name-hash.o
 LIB_OBJS += notes.o
 LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..4064e35 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
 #include "reachable.h"
 #include "sha1-array.h"
 #include "argv-array.h"
-#include "mru.h"
+#include "list.h"
 #include "packfile.h"
 
 static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
 
-   list_for_each(pos, _git_mru.list) {
-   struct mru *entry = list_entry(pos, struct mru, list);
-   struct packed_git *p = entry->item;
+   list_for_each(pos, _git_mru) {
+   struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
 
if (p == *found_pack)
@@ -1030,8 +1029,9 @@ static int want_object_in_pack(const unsigned char *sha1,
*found_pack = p;
}
want = want_found_object(exclude, p);
-   if (!exclude && want > 0)
-   mru_mark(_git_mru, entry);
+   if (!exclude && want > 0) {
+   list_move_to_front(>mru, _git_mru);
+   }
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..1a275ae 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 #include "hashmap.h"
-#include "mru.h"
+#include "list.h"
 #include "advice.h"
 #include "gettext.h"
 #include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
 
 extern struct packed_git {
struct packed_git *next;
+   struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,10 @@ extern struct packed_git {
 } *packed_git;
 
 /*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
  */
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
+
 
 struct pack_entry {
off_t offset;
diff --git a/list.h b/list.h
index eb60119..5129b0a 100644
--- a/list.h
+++ b/list.h
@@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem, struct 
list_head *head)
list_add(elem, head);
 }
 
+/* Move to the front of the list. */
+static inline void list_move_to_front(struct list_head *elem, struct list_head 
*head)
+{
+   list_del(elem);
+   list_add(elem, head);
+}
+
 /* Replace an old entry. */
 static inline void list_replace(struct list_head *old, struct list_head *newp)
 {
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
-   struct mru *cur = xmalloc(sizeof(*cur));
-   cur->item = item;
-   list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
-   /* To mark means to put at the front of the list. */
-   list_del(>list);
-   list_add(>list, >list);
-}
-
-void mru_clear(struct mru *head)
-{
-   struct list_head *pos;
-   struct list_head *tmp;
-
-   list_for_each_safe(pos, tmp, >list) {
-   free(list_entry(pos, struct 

Re: [BUG] git remote prune removes local tags, depending on fetch config

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 15 2018, Michael Giuffrida jotted:

> `git remote prune ` should "delete all stale remote-tracking
> branches under ". I was surprised to discover, after some
> troubleshooting, that it also deletes *all* local tags that don't
> exist on the remote, if the following refspec is included in the
> remote's fetch config:
>
> +refs/tags/*:refs/tags/*
>
> So, if `remote.origin.fetch` is configured to fetch all tags from the
> remote, any tags I create locally will be deleted when running `git
> remote prune origin`. This is not intuitive [1], nor is is it
> explained in the docs [2]. Is this behavior obvious to someone with a
> better understanding of Git internals?
>
> I did find a better way to automatically fetch tags (using tagopt
> instead of adding the fetch refspec). However, the refspec doesn't
> seem "wrong" in itself; in particular, `git fetch --tags` used to be
> considered equivalent to specifying the refspec
> "refs/tags/*:refs/tags/*" -- implying that this is a sensible refspec
> [3]. So I wouldn't expect it to "break" the behavior of another
> command.
>
> [1] https://stackoverflow.com/q/34687657/1327867
> [2] https://git-scm.com/docs/git-remote.html#git-remote-empruneem
> [3] https://github.com/git/git/commit/c5a84e92a2fe9e8748e32341c344d7a6c0f52a50

These docs are really confusing, but it is working as intended, and
really should be re-documented.

The `git remote prune` subcommand just ends up piggy-backing on
git-fetch, whose behavior is explained here:
https://git-scm.com/docs/git-fetch.html#git-fetch---prune

It's worked this way since at least v1.8.5.6, maybe at some distant
point in the past it only did this for branches when invoked via
git-remote as the documentation says.

RELATED:

I've actually had the reverse problem with this. I want some way to turn
this behavior on without explicitly hacking the refspec, so I can do it
globally in /etc/gitconfig or in ~/.gitconfig without screwing with the
config of each checkout on certain machines.

You can set fetch.prune=true, but that only prunes the branches, you
need to inject remote.origin.fetch into each checkout, unless I've
missed some way of doing this.

I wanted to add fetch.pruneTags that would make it as if you had
refs/tags/*:refs/tags/* in the fetch spec, but I haven't hacked that up
yet, if anyone can see any inherent issue with that plan I'd like to
know about it.

Re: [PATCH] Removed unnecessary void* from hashmap.h that caused compile warnings

[Thomas Gummerer]

On 01/15, Randall S. Becker wrote:
> On January 15, 2018 3:43 PM, Thomas Gummerer wrote:
> > Thanks for your patch!  A few nitpicks below:
> > 
> > > Subject: [PATCH] Removed unnecessary void* from hashmap.h that caused
> > > compile warnings
> > 
> > From Documentation/SubmittingPatches:
> > 
> > Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> > to do frotz", as if you are giving orders to the codebase to change
> > its behavior.
> > 
> > I liked the subject Philip suggested in the other thread: "hashmap.h:
> > remove unnecessary void*", or maybe "hashmap.h: remove unnecessary
> > variable".
> > 
> > On 01/14, randall.s.bec...@rogers.com wrote:
> > > From: "Randall S. Becker" 
> > >
> > > * hashmap.h: Revised the while loop in the
> > hashmap_enable_item_counting
> > >   to remove unneeded void* item.
> > 
> > As above, this should be described in an imperative mood, and describe why
> > this is a good change and should be merged.  Maybe something along the
> > lines of the below?
> > 
> > In 'hashmap_enable_item_counting()', item is assigned but never
> > used.  This causes a warning on HP NonStop.  As the variable is
> > never used, fix this by just removing it.
> > 
> > > Signed-off-by: Randall S. Becker 
> > >
> > > [..snip..]
> > >
> I like it. Do you need this resubmitted? Or should I just learn for next
> time?

I think it would be good if you resubmit the patch.  These rules tend
to be applied quite strictly, as you can also see when looking at the
git commit history.  So with the updated commit message Junio should
just be able to pick it up (unless there's something I missed here as
well :))

As a side note, I just noticed the two submissions both had [PATCH] in
the title, whereas new submissions should be marked as such using
[PATCH v2] etc. as prefix, so it's easier for reviewers to know which
version is the newer one.

> Cheers,
> Randall
> 

Re: [PATCH] packed_ref_cache: don't use mmap() for small files

[Jeff King]

On Tue, Jan 16, 2018 at 12:37:51AM +0100, Kim Gybels wrote:

> > This looks good to me, and since it's a recent-ish regression, I think
> > we should take the minimal fix here.
> 
> The minimal fix being a simple NULL check before munmap()?

Sorry to be unclear. I just meant that your patch is probably fine
as-is. I didn't want to hold up a regression fix with a bunch of
nit-picking or possible future work, when we could build that on top
later.

> > But it does make me wonder whether xmmap() ought to be doing this "small
> > mmap" optimization for us. Obviously that only works when we do
> > MAP_PRIVATE and never write to the result. But that's how we always use
> > it anyway, and we're restricted to that to work with the NO_MMAP wrapper
> > in compat/mmap.c.
> 
> Maybe I should have left the optimization for small files out of the patch for
> the zero length regression. After all, read() vs mmap() performance might
> depend on other factors than just size.

I'd be OK including it here, since there's prior art in the commit you
referenced. Though of course actual numbers are always good when
claiming an optimization. :)

> > If the "!size" case is just lumped in with "size <= SMALL_FILE_SIZE",
> > then we'd try to xmalloc(0), which is guaranteed to work (we fallback to
> > a 1-byte allocation if necessary). Would that make things simpler and
> > more consistent for the rest of the code to always have snapshot->buf be
> > a valid pointer (just based on seeing Michael's follow-up patches)?
> 
> Indeed, all those patches are to avoid using the NULL pointers in ways that 
> are
> undefined. We could also copy index_core's way of handling the zero length
> case:
> ret = index_mem(sha1, "", size, type, path, flags);
> 
> Point to some static memory instead of NULL, then all the pointer arithmetic 
> is defined.

Yep, that would work, too. I don't think the overhead of a
once-per-process xmalloc(0) is a big deal, though, if it keeps the code
simpler (though I admit it is not that complex either way).

-Peff

Re: [PATCH] packed_ref_cache: don't use mmap() for small files

[Kim Gybels]

On (15/01/18 16:15), Jeff King wrote:

> On Sat, Jan 13, 2018 at 05:11:49PM +0100, Kim Gybels wrote:
> 
> > Take a hint from commit ea68b0ce9f8ce8da3e360aed3cbd6720159ffbee and use
> > read() instead of mmap() for small packed-refs files.
> > 
> > This also fixes the problem[1] where xmmap() returns NULL for zero
> > length[2], for which munmap() later fails.
> > 
> > Alternatively, we could simply check for NULL before munmap(), or
> > introduce an xmunmap() that could be used together with xmmap().
> 
> This looks good to me, and since it's a recent-ish regression, I think
> we should take the minimal fix here.

The minimal fix being a simple NULL check before munmap()?

> But it does make me wonder whether xmmap() ought to be doing this "small
> mmap" optimization for us. Obviously that only works when we do
> MAP_PRIVATE and never write to the result. But that's how we always use
> it anyway, and we're restricted to that to work with the NO_MMAP wrapper
> in compat/mmap.c.

Maybe I should have left the optimization for small files out of the patch for
the zero length regression. After all, read() vs mmap() performance might
depend on other factors than just size.

> > @@ -489,21 +491,21 @@ static int load_contents(struct snapshot *snapshot)
> > die_errno("couldn't stat %s", snapshot->refs->path);
> > size = xsize_t(st.st_size);
> >  
> > -   switch (mmap_strategy) {
> > -   case MMAP_NONE:
> > +   if (!size) {
> > +   snapshot->buf = NULL;
> > +   snapshot->eof = NULL;
> > +   snapshot->mmapped = 0;
> > +   } else if (size <= SMALL_FILE_SIZE || mmap_strategy == MMAP_NONE) {
> > snapshot->buf = xmalloc(size);
> > bytes_read = read_in_full(fd, snapshot->buf, size);
> > if (bytes_read < 0 || bytes_read != size)
> > die_errno("couldn't read %s", snapshot->refs->path);
> > snapshot->eof = snapshot->buf + size;
> > snapshot->mmapped = 0;
> 
> If the "!size" case is just lumped in with "size <= SMALL_FILE_SIZE",
> then we'd try to xmalloc(0), which is guaranteed to work (we fallback to
> a 1-byte allocation if necessary). Would that make things simpler and
> more consistent for the rest of the code to always have snapshot->buf be
> a valid pointer (just based on seeing Michael's follow-up patches)?

Indeed, all those patches are to avoid using the NULL pointers in ways that are
undefined. We could also copy index_core's way of handling the zero length
case:
ret = index_mem(sha1, "", size, type, path, flags);

Point to some static memory instead of NULL, then all the pointer arithmetic is 
defined.

-Kim

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Jeff King]

On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:

> That works, but I don't think it's where we want to end up in the long
> run. Because:
> 
>   1. We still have the set of formats duplicated between expand_atom()
>  and the "preparation" step. It's just that the preparation is now
>  in ref-filter.c. What happens if ref-filter.c learns new formatting
>  placeholders (or options for those placeholders) that cat-file.c
>  doesn't, or vice versa? The two have to be kept in sync.
> 
>   2. We're missing out on all of the other placeholders that ref-filter
>  knows about. Not all of them are meaningful (e.g., %(refname)
>  wouldn't make sense here), but part of our goal is to support the
>  same set of placeholders as much as possible. Some obvious ones
>  that ought to work for cat-file: %(objectname:short), %(if), and
>  things like %(subject) when the appropriate object type is used.
> 
> In other words, I think the endgame is that expand_atom() isn't there at
> all, and we're calling the equivalent of format_ref_item() for each
> object (except that in a unified formatting world, it probably doesn't
> have the word "ref" in it, since that's just one of the items a caller
> might pass in).

I read carefully up through about patch 6, and then skimmed through the
rest. I think we really want to push this in the direction of more
unification, as above. I know that the patches here may be a step on the
way, but I had a hard time evaluating each patch to see if it was
leading us in the right direction.

I think what would help for reviewing this is:

  1. Start with a cover letter that makes it clear what the end state of
 the series is, and why that's the right place to end up.

  2. Include a rough roadmap of the patches in the cover letter.
 Hopefully they should group into a few obvious steps (like "in
 patches 1-5, we're teaching ref-filter to support everything that
 cat-file can do, then in 6-10 we're preparing cat-file for the
 conversion, and then in 11 we do the conversion"). If it doesn't
 form a coherent narrative, then it may be worth stepping back and
 thinking about combining or reordering the patches in a different
 way, so that the progression becomes more plain.

 I think one of the things that makes the progression here hard to
 understand (for me, anyway) is that it's "inside out" of what I'd
 expect. There's a lot of code movement happening first, and then
 refactoring and simplifying after that. So between those two steps,
 there's a lot of interim ugliness (e.g., having to reach across
 module boundaries to look at expand_data). It's hard to tell when
 looking at each individual patch how necessary the ugliness is, and
 whether and when it's going to go away later in the series.

There's a lot of good work here, and you've figured out a lot about how
the two systems function. I think we just need to rearrange things a bit
to make sure each step is moving in the right direction.

-Peff

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:

> Need that for further reusing of formatting logic in cat-file.
> Have plans to get rid of using expand_data in cat-file at all,
> and use it only in ref-filter for collecting, formatting and printing
> needed data.

I think some of these will want to remain in cat-file.c. For instance,
split_on_whitespace is not something that ref-filter.c itself would care
about. I'd expect in the endgame for cat-file to keep its own
split_on_whitespace flag, and to set it based on the presence of the
%(rest) flag, which it can check by seeing if the "rest" atom is in the
"used_atom" list after parsing the format.

-Peff

Re: [PATCH] Add shell completion for git remote rm

[Keith Smiley]

So it sounds like either we should deprecate rm, or I should update the patch 
to the suggested method where we just complete remotes, but not rm in the list 
of completions.

Thoughts?

-- 
Keith Smiley

On Wed, Jan 3, 2018, at 11:24, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
> > On Sat, Dec 30 2017, Todd Zullinger jotted:
> >
> >> And I think that should also apply to
> >> not offering completion for commands/subcommands/options
> >> which are only kept for backward compatibility.
> >
> > Yeah I think it makes sense to at some point stop completing things if
> > we're going to remove stuff, if we decide to remove it.
> >
> >> Here's one way to make 'git remote rm ' work without
> >> including it in the output of 'git remote ':
> >>
> >> diff --git i/contrib/completion/git-completion.bash 
> >> w/contrib/completion/git-completion.bash
> >> index 3683c772c5..aa63f028ab 100644
> >> --- i/contrib/completion/git-completion.bash
> >> +++ w/contrib/completion/git-completion.bash
> >> @@ -2668,7 +2668,9 @@ _git_remote ()
> >>add rename remove set-head set-branches
> >>get-url set-url show prune update
> >>"
> >> -  local subcommand="$(__git_find_on_cmdline "$subcommands")"
> >> +  # Don't advertise rm by including it in subcommands, but complete
> >> +  # remotes if it is used.
> >> +  local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
> >>if [ -z "$subcommand" ]; then
> >>case "$cur" in
> >>--*)
> >
> > Neat!
> 
> Yes, indeed it is nice.
> 
> 

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:

> Make valid_atom as a function parameter,
> there could be another variable further.
> Need that for further reusing of formatting logic in cat-file.c.
> 
> We do not need to allow users to pass their own valid_atom variable in
> global functions like verify_ref_format because in the end we want to
> have same set of valid atoms for all commands. But, as a first step
> of migrating, I create further another version of valid_atom
> for cat-file.

I agree in the end we'd want a single valid_atom list. It doesn't look
like we hit that end state in this series, though.

I guess I'm not quite clear on why we're not adding these new atoms to
ref-filter (and for-each-ref) right away, though. We already have the
first three (name, type, and size), and we'd just need to support
%(rest) and %(deltabase).

I think %(rest) doesn't really make sense for for-each-ref (we're not
reading any input), but it could expand to the empty string by default
(or even throw an error if the caller asks us not to support it).

IOW, the progression I'd expect in a series like this is:

  1. Teach ref-filter.c to support everything that cat-file can do.

  2. Convert cat-file to use ref-filter.c.

-Peff

Re: [PATCH v2 02/18] cat-file: reuse struct ref_format

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:

> Start using ref_format struct instead of simple char*.
> Need that for further reusing of formatting logic from ref-filter.

OK, this makes sense (though again, at some point we want this to stop
being a "ref_format" and just be a "format").

>  struct batch_options {
> + struct ref_format *format;

Does this need to be a pointer? We can just store the ref_format inside
the struct, right? And then...

> @@ -557,7 +558,8 @@ int cmd_cat_file(int argc, const char **argv, const char 
> *prefix)
>  {
>   int opt = 0;
>   const char *exp_type = NULL, *obj_name = NULL;
> - struct batch_options batch = {0};
> + struct ref_format format = REF_FORMAT_INIT;
> + struct batch_options batch = {};
>   int unknown_type = 0;

...here you would not need the extra local variable. You can initialize
it like:

  struct batch_options batch = { REF_FORMAT_INIT };

-Peff

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:

> Split expand_atom function into 2 different functions,
> expand_atom_into_fields prepares variable for further filling,
> (new) expand_atom creates resulting string.
> Need that for further reusing of formatting logic from ref-filter.

This commit puzzled me, and I had to look ahead to try to figure out why
we want this split (because on its face, the split is bad, since it
duplicates the list).

Later, the preparation step goes away, but we still are left with
expand_atom(). That's because the preparation was all moved into
ref-filter.c, where we rely on populate_value() to fill in the values,
and then we pick them out with our own formats.

That works, but I don't think it's where we want to end up in the long
run. Because:

  1. We still have the set of formats duplicated between expand_atom()
 and the "preparation" step. It's just that the preparation is now
 in ref-filter.c. What happens if ref-filter.c learns new formatting
 placeholders (or options for those placeholders) that cat-file.c
 doesn't, or vice versa? The two have to be kept in sync.

  2. We're missing out on all of the other placeholders that ref-filter
 knows about. Not all of them are meaningful (e.g., %(refname)
 wouldn't make sense here), but part of our goal is to support the
 same set of placeholders as much as possible. Some obvious ones
 that ought to work for cat-file: %(objectname:short), %(if), and
 things like %(subject) when the appropriate object type is used.

In other words, I think the endgame is that expand_atom() isn't there at
all, and we're calling the equivalent of format_ref_item() for each
object (except that in a unified formatting world, it probably doesn't
have the word "ref" in it, since that's just one of the items a caller
might pass in).

-Peff

[GIT PULL] l10n updates for 2.16.0 round 2

[Jiang Xin]

Hi Junio,

Please pull the following git l10n updates for Git 2.16.0.

The following changes since commit 36438dc19dd2a305dddebd44bf7a65f1a220075b:

  Git 2.16-rc1 (2018-01-05 13:45:17 -0800)

are available in the Git repository at:

  git://github.com/git-l10n/git-po tags/l10n-2.16.0-rnd2

for you to fetch changes up to c9741bb98e59d6580145f4d1c9628faa1a22e623:

  l10n: de.po: translate 72 new messages (2018-01-15 07:47:30 +0100)


l10n for Git 2.16.0 round 2


Alexander Shopov (2):
  l10n: bg.po: Updated Bulgarian translation (3284t)
  l10n: bg.po: Updated Bulgarian translation (3288t)

Changwoo Ryu (2):
  l10n: ko.po: Update Korean translation
  l10n: TEAMS: Add ko team members

Christopher Díaz Riveros (3):
  l10n: Update Spanish translation
  l10n: es.po: Update Spanish Translation v2.16.0
  l10n: es.po: Spanish translation 2.16.0 round 2

Dimitriy Ryazantcev (1):
  l10n: ru.po: update Russian translation

Fangyi Zhou (1):
  l10n: zh_CN translate parameter name

Jean-Noel Avila (2):
  l10n: fr.po v2.16.0 round 1
  l10n: fr.po 2.16 round 2

Jiang Xin (15):
  Merge branch '2.15.1' of https://github.com/ChrisADR/git-po into maint
  l10n: git.pot: v2.16.0 round 1 (64 new, 25 removed)
  Merge branch 'maint' of git://github.com/git-l10n/git-po
  Merge branch 'master' of git://github.com/alshopov/git-po
  Merge branch 'fr_2.16' of git://github.com/jnavila/git
  Merge branch '2.16' of https://github.com/ChrisADR/git-po
  Merge branch 'master' of git://github.com/git-l10n/git-po
  l10n: git.pot: v2.16.0 round 2 (8 new, 4 removed)
  Merge branch 'fr_2.16-rc1' of git://github.com/jnavila/git
  Merge branch '2.16' of https://github.com/ChrisADR/git-po
  Merge branch 'ko/merge-l10n' of https://github.com/git-l10n-ko/git-l10n-ko
  Merge branch 'russian-l10n' of https://github.com/DJm00n/git-po-ru
  Merge branch 'master' of git://github.com/nafmo/git-l10n-sv
  l10n: zh_CN: for git v2.16.0 l10n round 2
  l10n: TEAMS: add zh_CN team members

Jordi Mas (1):
  l10n: Fixes to Catalan translation

Louis Bettens (1):
  l10n: fr.po: "worktree list" mistranslated as prune

Peter Krefting (2):
  l10n: sv.po: Update Swedish translation (3284t0f0u)
  l10n: sv.po: Update Swedish translation (3288t0f0u)

Ralf Thielow (2):
  l10n: de.po: improve messages when a branch starts to track another ref
  l10n: de.po: translate 72 new messages

Robert Abel (1):
  l10n: fixes to German translation

Trần Ngọc Quân (1):
  l10n: vi.po(3288t): Updated Vietnamese translation for v2.16.0 round 2

Zhou Fangyi (1):
  l10n: zh_CN Fix typo

 po/TEAMS|5 +-
 po/bg.po| 4098 ---
 po/ca.po|  384 +++--
 po/de.po| 4128 +--
 po/es.po| 5136 +++
 po/fr.po| 4095 +--
 po/git.pot  | 3967 +++--
 po/ko.po| 4142 +--
 po/ru.po| 4010 --
 po/sv.po| 4082 +--
 po/vi.po| 4085 +--
 po/zh_CN.po | 4098 +--
 12 files changed, 22202 insertions(+), 20028 deletions(-)

--
Jiang Xin

[BUG] git remote prune removes local tags, depending on fetch config

[Michael Giuffrida]

`git remote prune ` should "delete all stale remote-tracking
branches under ". I was surprised to discover, after some
troubleshooting, that it also deletes *all* local tags that don't
exist on the remote, if the following refspec is included in the
remote's fetch config:

+refs/tags/*:refs/tags/*

So, if `remote.origin.fetch` is configured to fetch all tags from the
remote, any tags I create locally will be deleted when running `git
remote prune origin`. This is not intuitive [1], nor is is it
explained in the docs [2]. Is this behavior obvious to someone with a
better understanding of Git internals?

I did find a better way to automatically fetch tags (using tagopt
instead of adding the fetch refspec). However, the refspec doesn't
seem "wrong" in itself; in particular, `git fetch --tags` used to be
considered equivalent to specifying the refspec
"refs/tags/*:refs/tags/*" -- implying that this is a sensible refspec
[3]. So I wouldn't expect it to "break" the behavior of another
command.

[1] https://stackoverflow.com/q/34687657/1327867
[2] https://git-scm.com/docs/git-remote.html#git-remote-empruneem
[3] https://github.com/git/git/commit/c5a84e92a2fe9e8748e32341c344d7a6c0f52a50

Re: [PATCH] packed_ref_cache: don't use mmap() for small files

[Jeff King]

On Sat, Jan 13, 2018 at 05:11:49PM +0100, Kim Gybels wrote:

> Take a hint from commit ea68b0ce9f8ce8da3e360aed3cbd6720159ffbee and use
> read() instead of mmap() for small packed-refs files.
> 
> This also fixes the problem[1] where xmmap() returns NULL for zero
> length[2], for which munmap() later fails.
> 
> Alternatively, we could simply check for NULL before munmap(), or
> introduce an xmunmap() that could be used together with xmmap().

This looks good to me, and since it's a recent-ish regression, I think
we should take the minimal fix here.

But it does make me wonder whether xmmap() ought to be doing this "small
mmap" optimization for us. Obviously that only works when we do
MAP_PRIVATE and never write to the result. But that's how we always use
it anyway, and we're restricted to that to work with the NO_MMAP wrapper
in compat/mmap.c.

> @@ -489,21 +491,21 @@ static int load_contents(struct snapshot *snapshot)
>   die_errno("couldn't stat %s", snapshot->refs->path);
>   size = xsize_t(st.st_size);
>  
> - switch (mmap_strategy) {
> - case MMAP_NONE:
> + if (!size) {
> + snapshot->buf = NULL;
> + snapshot->eof = NULL;
> + snapshot->mmapped = 0;
> + } else if (size <= SMALL_FILE_SIZE || mmap_strategy == MMAP_NONE) {
>   snapshot->buf = xmalloc(size);
>   bytes_read = read_in_full(fd, snapshot->buf, size);
>   if (bytes_read < 0 || bytes_read != size)
>   die_errno("couldn't read %s", snapshot->refs->path);
>   snapshot->eof = snapshot->buf + size;
>   snapshot->mmapped = 0;

If the "!size" case is just lumped in with "size <= SMALL_FILE_SIZE",
then we'd try to xmalloc(0), which is guaranteed to work (we fallback to
a 1-byte allocation if necessary). Would that make things simpler and
more consistent for the rest of the code to always have snapshot->buf be
a valid pointer (just based on seeing Michael's follow-up patches)?

-Peff

RE: [PATCH] Removed unnecessary void* from hashmap.h that caused compile warnings

[Randall S. Becker]

On January 15, 2018 3:43 PM, Thomas Gummerer wrote:
> Thanks for your patch!  A few nitpicks below:
> 
> > Subject: [PATCH] Removed unnecessary void* from hashmap.h that caused
> > compile warnings
> 
> From Documentation/SubmittingPatches:
> 
> Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behavior.
> 
> I liked the subject Philip suggested in the other thread: "hashmap.h:
> remove unnecessary void*", or maybe "hashmap.h: remove unnecessary
> variable".
> 
> On 01/14, randall.s.bec...@rogers.com wrote:
> > From: "Randall S. Becker" 
> >
> > * hashmap.h: Revised the while loop in the
> hashmap_enable_item_counting
> > to remove unneeded void* item.
> 
> As above, this should be described in an imperative mood, and describe why
> this is a good change and should be merged.  Maybe something along the
> lines of the below?
> 
> In 'hashmap_enable_item_counting()', item is assigned but never
> used.  This causes a warning on HP NonStop.  As the variable is
> never used, fix this by just removing it.
> 
> > Signed-off-by: Randall S. Becker 
> > ---
> >  hashmap.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/hashmap.h b/hashmap.h
> > index 7ce79f3..d375d9c 100644
> > --- a/hashmap.h
> > +++ b/hashmap.h
> > @@ -400,7 +400,6 @@ static inline void
> hashmap_disable_item_counting(struct hashmap *map)
> >   */
> >  static inline void hashmap_enable_item_counting(struct hashmap *map)
> > {
> > -   void *item;
> > unsigned int n = 0;
> > struct hashmap_iter iter;
> >
> > @@ -408,7 +407,7 @@ static inline void
> hashmap_enable_item_counting(struct hashmap *map)
> > return;
> >
> > hashmap_iter_init(map, );
> > -   while ((item = hashmap_iter_next()))
> > +   while (hashmap_iter_next())
> > n++;
> >
> > map->do_count_items = 1;

I like it. Do you need this resubmitted? Or should I just learn for next
time?

Cheers,
Randall

Re: [PATCH] Removed unnecessary void* from hashmap.h that caused compile warnings

[Thomas Gummerer]

Thanks for your patch!  A few nitpicks below:

> Subject: [PATCH] Removed unnecessary void* from hashmap.h that caused compile 
> warnings

>From Documentation/SubmittingPatches:

Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behavior.

I liked the subject Philip suggested in the other thread: "hashmap.h:
remove unnecessary void*", or maybe "hashmap.h: remove unnecessary
variable".

On 01/14, randall.s.bec...@rogers.com wrote:
> From: "Randall S. Becker" 
> 
> * hashmap.h: Revised the while loop in the hashmap_enable_item_counting
>   to remove unneeded void* item.

As above, this should be described in an imperative mood, and describe
why this is a good change and should be merged.  Maybe something along
the lines of the below?

In 'hashmap_enable_item_counting()', item is assigned but never
used.  This causes a warning on HP NonStop.  As the variable is
never used, fix this by just removing it.

> Signed-off-by: Randall S. Becker 
> ---
>  hashmap.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hashmap.h b/hashmap.h
> index 7ce79f3..d375d9c 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -400,7 +400,6 @@ static inline void hashmap_disable_item_counting(struct 
> hashmap *map)
>   */
>  static inline void hashmap_enable_item_counting(struct hashmap *map)
>  {
> - void *item;
>   unsigned int n = 0;
>   struct hashmap_iter iter;
>  
> @@ -408,7 +407,7 @@ static inline void hashmap_enable_item_counting(struct 
> hashmap *map)
>   return;
>  
>   hashmap_iter_init(map, );
> - while ((item = hashmap_iter_next()))
> + while (hashmap_iter_next())
>   n++;
>  
>   map->do_count_items = 1;
> -- 
> 2.8.5.23.g6fa7ec3
> 

Hello

[Miss Samira Kipkalya]

My Dearest,

I am Miss Samira Kipkalya Kones, I am seeking for an investment assistance, 
relocation  and long term relationship , I am constrained to contact you 
because of the abuse which I am receiving from my step mother .My Late father 
deposited the sum of US$ 27.5 Million in one bank in Benin Republic with my 
name as the next of kin which I want to transfer into your Bank Account as my 
Trustee and Partner which is what the Bank demands now, I will give you full 
details in my next mail after receiving your acceptance mail to help me, please 
do reply me here on my private email at (ldgxv...@yahoo.com )

Yours sincerely
Miss Samira Kipkalya Kones

[PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

[René Scharfe]

Call strbuf_add_unique_abbrev() to add an abbreviated hash to a strbuf
instead of taking a detour through find_unique_abbrev() and its static
buffer.  This is shorter and a bit more efficient.

Patch generated by Coccinelle (and contrib/coccinelle/strbuf.cocci).

Signed-off-by: Rene Scharfe 
---
The changed line was added by 4dbc59a4cc (builtin/describe.c: factor
out describe_commit).

"make coccicheck" doesn't propose any other changes for current master.

 builtin/describe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 3b0b204b1e..21e37f5dae 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -380,7 +380,7 @@ static void describe_commit(struct object_id *oid, struct 
strbuf *dst)
if (!match_cnt) {
struct object_id *cmit_oid = >object.oid;
if (always) {
-   strbuf_addstr(dst, find_unique_abbrev(cmit_oid->hash, 
abbrev));
+   strbuf_add_unique_abbrev(dst, cmit_oid->hash, abbrev);
if (suffix)
strbuf_addstr(dst, suffix);
return;
-- 
2.15.1

Re: [PATCH 10/40] external-odb: implement external_odb_get_direct

[Christian Couder]

On Thu, Jan 4, 2018 at 6:44 PM, Jeff Hostetler  wrote:
>
>
> On 1/3/2018 11:33 AM, Christian Couder wrote:
>>
>> This is implemented only in the promisor remote mode
>> for now by calling fetch_object().
>>
>> Signed-off-by: Christian Couder 
>> ---
>>   external-odb.c | 15 +++
>>   external-odb.h |  1 +
>>   odb-helper.c   | 13 +
>>   odb-helper.h   |  3 ++-
>>   4 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/external-odb.c b/external-odb.c
>> index d26e63d8b1..5d0afb9762 100644
>> --- a/external-odb.c
>> +++ b/external-odb.c
>> @@ -76,3 +76,18 @@ int external_odb_has_object(const unsigned char *sha1)
>> return 1;
>> return 0;
>>   }
>> +
>> +int external_odb_get_direct(const unsigned char *sha1)
>> +{
>> +   struct odb_helper *o;
>> +
>> +   external_odb_init();
>> +
>> +   for (o = helpers; o; o = o->next) {
>> +   if (odb_helper_get_direct(o, sha1) < 0)
>> +   continue;
>> +   return 0;
>
>> + }
>
> Would this be simpler said as:
> for (o = ...)
> if (!odb_helper_get_direct(...))
> return 0;

At then end of the series the content of the loop is:

if (!(o->supported_capabilities & ODB_HELPER_CAP_GET_DIRECT))
continue;
if (odb_helper_get_direct(o, sha1) < 0)
continue;
return 0;

And I think it is fine like that, so I don't think changing this
commit is a good idea.

>> +   return -1;
>> +}
>> diff --git a/external-odb.h b/external-odb.h
>> index 9a3c2f01b3..fd6708163e 100644
>> --- a/external-odb.h
>> +++ b/external-odb.h
>> @@ -4,5 +4,6 @@
>>   extern int has_external_odb(void);
>>   extern const char *external_odb_root(void);
>>   extern int external_odb_has_object(const unsigned char *sha1);
>> +extern int external_odb_get_direct(const unsigned char *sha1);
>> #endif /* EXTERNAL_ODB_H */
>> diff --git a/odb-helper.c b/odb-helper.c
>> index 1404393807..4b70b287af 100644
>> --- a/odb-helper.c
>> +++ b/odb-helper.c
>> @@ -4,6 +4,7 @@
>>   #include "odb-helper.h"
>>   #include "run-command.h"
>>   #include "sha1-lookup.h"
>> +#include "fetch-object.h"
>> struct odb_helper *odb_helper_new(const char *name, int namelen)
>>   {
>> @@ -52,3 +53,15 @@ int odb_helper_has_object(struct odb_helper *o, const
>> unsigned char *sha1)
>> return !!odb_helper_lookup(o, sha1);
>>   }
>>   +int odb_helper_get_direct(struct odb_helper *o,
>> + const unsigned char *sha1)
>> +{
>> +   int res = 0;
>> +   uint64_t start = getnanotime();
>> +
>> +   fetch_object(o->dealer, sha1);
>> +
>> +   trace_performance_since(start, "odb_helper_get_direct");
>> +
>> +   return res;
>
>
> 'res' will always be 0, so the external_odb_get_direct() will
> only do the first helper.  i haven't looked at the rest of the
> series yet, so maybe you've already addressed this.

That's why I previously suggested in one of your or Jonathan's patch
that fetch_object() should return an int that tells the caller if the
object has been fetched instead of void.

If we make it possible at one point to have the objects fetched
fetch_object() in different remotes (and I think that's a
straightforward goal), this will actually fail but callers will have
no simple way to know that.

> Also, I put a TODO comment in the fetch_object() header to
> consider returning an error/success, so maybe that could help
> here too.

Yeah, indeed.

Re: [PATCH 01/40] Add initial external odb support

[Christian Couder]

On Thu, Jan 4, 2018 at 8:59 PM, Jeff Hostetler  wrote:
>
>> diff --git a/odb-helper.h b/odb-helper.h
>> new file mode 100644
>> index 00..9395e606ce
>> --- /dev/null
>> +++ b/odb-helper.h
>> @@ -0,0 +1,24 @@
>> +#ifndef ODB_HELPER_H
>> +#define ODB_HELPER_H
>> +
>> +struct odb_helper {
>> +   const char *name;
>> +   const char *dealer;
>> +
>> +   struct odb_helper_object {
>> +   unsigned char sha1[20];
>
>
> Should this be "struct object_id" ?

Yeah, I changed this in my current version.

I cannot change all the functions to take a struct object_id though as
some are called inside sha1_file.c where there are only sha1.

RE: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Randall S. Becker]

On January 15, 2018 2:06 AM, Johannes Sixt wrote:
> Am 15.01.2018 um 03:37 schrieb Randall S. Becker:
> > On January 14, 2018 4:33 PM, I wrote:
> >> The exotic error code coming back from perl is 162. I can muck with
> >> it, if there was a value more useful to git.
> >> *  553  } else if (WIFEXITED(status)) {
> >> *  554  code = WEXITSTATUS(status);
> >> (eInspect 3,887):p code
> >> $4 = 162
> >>
> >> The perl code uses die to terminate with a non-specific non-zero error
> code.
> >> What it seems is that there is an assumed value that the git tests
> >> depend on, but perl.org describes the following:
> >>
> >> "die raises an exception. Inside an eval the error message is stuffed
> >> into $@ and the eval is terminated with the undefined value. If the
> >> exception is outside of all enclosing evals, then the uncaught
> >> exception prints LIST to STDERR and exits with a non-zero value. If
> >> you need to exit the process with a specific exit code, see exit."
> 
> I take "die exits with non-zero" as a piece of information for the
> *users* so that they can write "if perl foo.pl; then something; fi" in shell
> scripts. I do *not* interpret it as leeway for implementers of perl to choose
> any random value as exit code. Choosing 162 just to be funky would be
> short-sighted. [I'm saying all this without knowing how perl specifies 'die'
> beyond the paragraph you cited. Perhaps there's more about 'die' that
> justifies exit code 162.] I'd say that the perl port is broken.

I agree that 162 is wrong. Its interpretation is 128+signal, which clearly does 
not happen in this case. On the platform, if the perl script is via stdin, 162 
or 169 are returned. If via file (perl file.pl), 255 comes back. The port has 
issues. I have an opened a bug report with the platform developers. Usual 
non-Open Source timeframes to fix apply. ☹

> >>
> >> So it seems that we might need to either not use die or change the
> >> tests not to care about the exit code for specific tests involving perl.
> Suggestions?
> >
> > Sadly, while changing the funky 162 completion code to 255 works fine
> > for t9001, it causes a bunch of other tests to fail (parts of
> > 1308 and most of 1404). I can't tall whether this is test suite or
> > code related but I'm caught in the middle. Going to the platform
> > developers may eventually get the fix for t9001 (fixing perl), but
> > otherwise, I'm not sure why changing 162 to 255 causes 1308 and 1404
> > to blow so badly. In any event, I'm putting this away for a few days
> > due to $DAYJOB.
> 
> Why do you not choose 1? He, on my Linux box perl -e die exits with 255.
> So...

Choosing 1 is an option. I can try that, but I'm not sure it's going to help 
the test situation more than choosing 255 to replace 162. 

Cheers,
Randall

[PATCH 2/3] create_snapshot(): exit early if the file was empty

[Michael Haggerty]

If the `packed_refs` files is entirely empty (i.e., not even a header
line), then `load_contents()` returns 1 even though `snapshot->buf`
and `snapshot->eof` both end up set to NULL. In that case, the
subsequent processing that `create_snapshot()` does is unnecessary,
and also involves computing `NULL - NULL` and `NULL + 0`, which are
probably safe in real life but are technically undefined in C.

Sidestep both issues by exiting early if `snapshot->buf` is NULL.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f20f05b4df..36796d65f0 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -613,7 +613,7 @@ static struct snapshot *create_snapshot(struct 
packed_ref_store *refs)
acquire_snapshot(snapshot);
snapshot->peeled = PEELED_NONE;
 
-   if (!load_contents(snapshot))
+   if (!load_contents(snapshot) || !snapshot->buf)
return snapshot;
 
/* If the file has a header line, process it: */
-- 
2.14.2

[PATCH 3/3] find_reference_location(): don't invoke if `snapshot->buf` is NULL

[Michael Haggerty]

If `snapshot->buf` is NULL, then `find_reference_location()` has two
problems:

1. It relies on behavior that is technically undefined in C, such as
   computing `NULL + 0`.

2. It returns NULL if the reference doesn't exist, even if `mustexist`
   is not set. This problem doesn't come up in the current code,
   because we never call this function with `snapshot->buf == NULL`
   and `mustexist` set. But it is something that future callers need
   to be aware of.

We could fix the first problem by adding some extra logic to the
function. But considering both problems together, it is more
straightforward to document that the function should only be called if
`snapshot->buf` is non-NULL.

Adjust `packed_read_raw_ref()` to return early if `snapshot->buf` is
NULL rather than calling `find_reference_location()`.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 36796d65f0..ed2b396bef 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -521,8 +521,9 @@ static int load_contents(struct snapshot *snapshot)
  * reference name; for example, one could search for "refs/replace/"
  * to find the start of any replace references.
  *
+ * This function must only be called if `snapshot->buf` is non-NULL.
  * The record is sought using a binary search, so `snapshot->buf` must
- * be sorted.
+ * also be sorted.
  */
 static const char *find_reference_location(struct snapshot *snapshot,
   const char *refname, int mustexist)
@@ -728,6 +729,12 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
 
*type = 0;
 
+   if (!snapshot->buf) {
+   /* There are no packed references */
+   errno = ENOENT;
+   return -1;
+   }
+
rec = find_reference_location(snapshot, refname, 1);
 
if (!rec) {
-- 
2.14.2

[PATCH 1/3] SQUASH? Mention that `snapshot::buf` can be NULL for empty files

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 01a13cb817..f20f05b4df 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -69,11 +69,11 @@ struct snapshot {
 
/*
 * The contents of the `packed-refs` file. If the file was
-* already sorted, this points at the mmapped contents of the
-* file. If not, this points at heap-allocated memory
-* containing the contents, sorted. If there were no contents
-* (e.g., because the file didn't exist), `buf` and `eof` are
-* both NULL.
+* already sorted and if mmapping is allowed, this points at
+* the mmapped contents of the file. If not, this points at
+* heap-allocated memory containing the contents, sorted. If
+* there were no contents (e.g., because the file didn't exist
+* or was empty), `buf` and `eof` are both NULL.
 */
char *buf, *eof;
 
-- 
2.14.2

[PATCH 0/3] Supplements to "packed_ref_cache: don't use mmap() for small files"

[Michael Haggerty]

Thanks for your patch. I haven't measured the performance difference
of `mmap()` vs. `read()` for small `packed-refs` files, but it's not
surprising that `read()` would be faster.

I especially like the fix for zero-length `packed-refs` files. (Even
though AFAIK Git never writes such files, they are totally legitimate
and shouldn't cause Git to fail.) With or without the additions
mentioned below,

Reviewed-by: Michael Haggerty 

While reviewing your patch, I realized that some areas of the existing
code use constructs that are undefined according to the C standard,
such as computing `NULL + 0` and `NULL - NULL`. This was already wrong
(and would come up more frequently after your change). Even though
these are unlikely to be problems in the real world, it would be good
to avoid them.

So I will follow up this email with three patches:

1. Mention that `snapshot::buf` can be NULL for empty files

   I suggest squashing this into your patch, to make it clear that
   `snapshot::buf` and `snapshot::eof` can also be NULL if the
   `packed-refs` file is empty.

2. create_snapshot(): exit early if the file was empty

   Avoid undefined behavior by returning early if `snapshot->buf` is
   NULL.

3. find_reference_location(): don't invoke if `snapshot->buf` is NULL

   Avoid undefined behavior and confusing semantics by not calling
   `find_reference_location()` when `snapshot->buf` is NULL.

Michael

Michael Haggerty (3):
  SQUASH? Mention that `snapshot::buf` can be NULL for empty files
  create_snapshot(): exit early if the file was empty
  find_reference_location(): don't invoke if `snapshot->buf` is NULL

 refs/packed-backend.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

-- 
2.14.2

[PATCH v5 7/7] trace.c: print new cwd in trace_run_command()

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

If a command sets a new env variable GIT_DIR=.git, we need more context
to know where that '.git' is related to.

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

diff --git a/trace.c b/trace.c
index 8db5bbf3c9..ad75d683bf 100644
--- a/trace.c
+++ b/trace.c
@@ -342,6 +342,12 @@ void trace_run_command(const struct child_process *cp)
 
strbuf_addf(, "trace: run_command:");
 
+   if (cp->dir) {
+   strbuf_addstr(, " cd ");
+   sq_quote_buf_pretty(, cp->dir);
+   strbuf_addch(, ';');
+   }
+
/*
 * The caller is responsible for initializing cp->env from
 * cp->env_array if needed. We only check one place.
-- 
2.15.1.600.g899a5f85c6

[PATCH v5 5/7] trace.c: print program 'git' when child_process.git_cmd is set

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

We normally print full command line, including the program and its
argument. When git_cmd is set, we have a special code path to run the
right "git" program and child_process.argv[0] will not contain the
program name anymore. As a result, we print just the command
arguments.

I thought it was a regression when the code was refactored and git_cmd
added, but apparently it's not. git_cmd mode was introduced before
tracing was added in 8852f5d704 (run_command(): respect GIT_TRACE -
2008-07-07) so it's more like an oversight in 8852f5d704.

Fix it, print the program name "git" in git_cmd mode. It's nice to have
now. But it will be more important later when we start to print env
variables too, in shell syntax. The lack of a program name would look
confusing then.

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

diff --git a/trace.c b/trace.c
index da3db301e7..98bee2c259 100644
--- a/trace.c
+++ b/trace.c
@@ -286,6 +286,9 @@ void trace_run_command(const struct child_process *cp)
 
strbuf_addf(, "trace: run_command:");
 
+   if (cp->git_cmd)
+   strbuf_addstr(, " git");
+
sq_quote_argv_pretty(, cp->argv);
print_trace_line(_default_key, );
strbuf_release();
-- 
2.15.1.600.g899a5f85c6

[PATCH v5 6/7] trace.c: print env vars in trace_run_command()

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

Occasionally submodule code could execute new commands with GIT_DIR set
to some submodule. GIT_TRACE prints just the command line which makes it
hard to tell that it's not really executed on this repository.

Print the env delta (compared to parent environment) in this case.

Helped-by: Junio C Hamano 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/helper/test-run-command.c |  9 +++
 t/t0061-run-command.sh  | 22 
 trace.c | 63 +
 3 files changed, 94 insertions(+)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index d24d157379..153342e44d 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -54,6 +54,15 @@ int cmd_main(int argc, const char **argv)
struct child_process proc = CHILD_PROCESS_INIT;
int jobs;
 
+   if (argc < 3)
+   return 1;
+   while (!strcmp(argv[1], "env")) {
+   if (!argv[2])
+   die("env specifier without a value");
+   argv_array_push(_array, argv[2]);
+   argv += 2;
+   argc -= 2;
+   }
if (argc < 3)
return 1;
proc.argv = (const char **)argv + 2;
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index e4739170aa..e6208316c3 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -141,4 +141,26 @@ test_expect_success 'run_command outputs ' '
test_cmp expect actual
 '
 
+test_trace() {
+   local expected="$1"
+   shift
+   GIT_TRACE=1 test-run-command "$@" run-command true 2>&1 >/dev/null | \
+   sed 's/.* run_command: //' >actual &&
+   echo "$expected true" >expected &&
+   test_cmp expected actual
+}
+
+test_expect_success 'GIT_TRACE with environment variables' '
+   test_trace "abc=1 def=2" env abc=1 env def=2 &&
+   test_trace "abc=2" env abc env abc=1 env abc=2 &&
+   test_trace "abc=2" env abc env abc=2 &&
+   abc=1 test_trace "def=1" env abc=1 env def=1 &&
+   abc=1 test_trace "def=1" env abc env abc=1 env def=1 &&
+   test_trace "def=1" env non-exist env def=1 &&
+   test_trace "abc=2" env abc=1 env abc env abc=2 &&
+   abc=1 def=2 test_trace "unset abc def;" env abc env def &&
+   abc=1 def=2 test_trace "unset def; abc=3" env abc env def env abc=3 &&
+   abc=1 test_trace "unset abc;" env abc=2 env abc
+'
+
 test_done
diff --git a/trace.c b/trace.c
index 98bee2c259..8db5bbf3c9 100644
--- a/trace.c
+++ b/trace.c
@@ -275,6 +275,62 @@ void trace_performance_fl(const char *file, int line, 
uint64_t nanos,
 
 #endif /* HAVE_VARIADIC_MACROS */
 
+static void add_env(struct strbuf *dst, const char *const *deltaenv)
+{
+   struct string_list envs = STRING_LIST_INIT_DUP;
+   const char *const *e;
+   int i;
+   int printed_unset = 0;
+
+   /* Last one wins, see run-command.c:prep_childenv() for context */
+   for (e = deltaenv; e && *e; e++) {
+   struct strbuf key = STRBUF_INIT;
+   char *equals = strchr(*e, '=');
+
+   if (equals) {
+   strbuf_add(, *e, equals - *e);
+   string_list_insert(, key.buf)->util = equals + 1;
+   } else {
+   string_list_insert(, *e)->util = NULL;
+   }
+   strbuf_release();
+   }
+
+   /* "unset X Y...;" */
+   for (i = 0; i < envs.nr; i++) {
+   const char *var = envs.items[i].string;
+   const char *val = envs.items[i].util;
+
+   if (val || !getenv(var))
+   continue;
+
+   if (!printed_unset) {
+   strbuf_addstr(dst, " unset");
+   printed_unset = 1;
+   }
+   strbuf_addf(dst, " %s", var);
+   }
+   if (printed_unset)
+   strbuf_addch(dst, ';');
+
+   /* ... followed by "A=B C=D ..." */
+   for (i = 0; i < envs.nr; i++) {
+   const char *var = envs.items[i].string;
+   const char *val = envs.items[i].util;
+   const char *oldval;
+
+   if (!val)
+   continue;
+
+   oldval = getenv(var);
+   if (oldval && !strcmp(val, oldval))
+   continue;
+
+   strbuf_addf(dst, " %s=", var);
+   sq_quote_buf_pretty(dst, val);
+   }
+   string_list_clear(, 0);
+}
 
 void trace_run_command(const struct child_process *cp)
 {
@@ -286,6 +342,13 @@ void trace_run_command(const struct child_process *cp)
 
strbuf_addf(, "trace: run_command:");
 
+   /*
+* The caller is responsible for initializing cp->env from
+* cp->env_array if needed. We only check one place.
+*/
+   if (cp->env)
+   add_env(, cp->env);
+
if (cp->git_cmd)

[PATCH v5 4/7] trace.c: introduce trace_run_command()

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

This is the same as the old code that uses trace_argv_printf() in
run-command.c. This function will be improved in later patches to
print more information from struct child_process.

A slight regression: the old code would print run-command.c:xxx as the
trace location site while the new code prints trace.c:xxx. This should
be fine until the second call site is added, then we might need a macro
wrapper named trace_run_command() to capture the right source location.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 run-command.c |  3 ++-
 trace.c   | 16 
 trace.h   |  3 +++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 31fc5ea86e..002074b128 100644
--- a/run-command.c
+++ b/run-command.c
@@ -624,7 +624,8 @@ int start_command(struct child_process *cmd)
cmd->err = fderr[0];
}
 
-   trace_argv_printf(cmd->argv, "trace: run_command:");
+   trace_run_command(cmd);
+
fflush(NULL);
 
 #ifndef GIT_WINDOWS_NATIVE
diff --git a/trace.c b/trace.c
index 7f3b08e148..da3db301e7 100644
--- a/trace.c
+++ b/trace.c
@@ -23,6 +23,7 @@
 
 #include "cache.h"
 #include "quote.h"
+#include "run-command.h"
 
 struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 };
 struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE);
@@ -275,6 +276,21 @@ void trace_performance_fl(const char *file, int line, 
uint64_t nanos,
 #endif /* HAVE_VARIADIC_MACROS */
 
 
+void trace_run_command(const struct child_process *cp)
+{
+   struct strbuf buf = STRBUF_INIT;
+
+   if (!prepare_trace_line(__FILE__, __LINE__,
+   _default_key, ))
+   return;
+
+   strbuf_addf(, "trace: run_command:");
+
+   sq_quote_argv_pretty(, cp->argv);
+   print_trace_line(_default_key, );
+   strbuf_release();
+}
+
 static const char *quote_crnl(const char *path)
 {
static struct strbuf new_path = STRBUF_INIT;
diff --git a/trace.h b/trace.h
index 88055abef7..e54c687f26 100644
--- a/trace.h
+++ b/trace.h
@@ -4,6 +4,8 @@
 #include "git-compat-util.h"
 #include "strbuf.h"
 
+struct child_process;
+
 struct trace_key {
const char * const key;
int fd;
@@ -17,6 +19,7 @@ extern struct trace_key trace_default_key;
 extern struct trace_key trace_perf_key;
 
 extern void trace_repo_setup(const char *prefix);
+extern void trace_run_command(const struct child_process *cp);
 extern int trace_want(struct trace_key *key);
 extern void trace_disable(struct trace_key *key);
 extern uint64_t getnanotime(void);
-- 
2.15.1.600.g899a5f85c6

[PATCH v5 2/7] trace: avoid unnecessary quoting

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

From: Jeff King 

Trace output which contains arbitrary strings (e.g., the
arguments to commands which we are running) is always passed
through sq_quote_buf(). That function always adds
single-quotes, even if the output consists of vanilla
characters. This can make the output a bit hard to read.

Let's avoid the quoting if there are no characters which a
shell would interpret. Trace output doesn't necessarily need
to be shell-compatible, but:

  - the shell language is a good ballpark for what humans
consider readable (well, humans versed in command line
tools)

  - the run_command bits can be cut-and-pasted to a shell,
and we'll keep that property

  - it covers any cases which would make the output
visually ambiguous (e.g., embedded whitespace or quotes)

Signed-off-by: Jeff King 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 quote.c | 26 ++
 quote.h |  8 
 trace.c |  4 ++--
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/quote.c b/quote.c
index b2970da627..37d2686865 100644
--- a/quote.c
+++ b/quote.c
@@ -43,6 +43,22 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
free(to_free);
 }
 
+void sq_quote_buf_pretty(struct strbuf *dst, const char *src)
+{
+   static const char ok_punct[] = "+,-./:=@_^";
+   const char *p;
+
+   for (p = src; *p; p++) {
+   if (!isalpha(*p) && !isdigit(*p) && !strchr(ok_punct, *p)) {
+   sq_quote_buf(dst, src);
+   return;
+   }
+   }
+
+   /* if we get here, we did not need quoting */
+   strbuf_addstr(dst, src);
+}
+
 void sq_quotef(struct strbuf *dst, const char *fmt, ...)
 {
struct strbuf src = STRBUF_INIT;
@@ -68,6 +84,16 @@ void sq_quote_argv(struct strbuf *dst, const char **argv)
}
 }
 
+void sq_quote_argv_pretty(struct strbuf *dst, const char **argv)
+{
+   int i;
+
+   for (i = 0; argv[i]; i++) {
+   strbuf_addch(dst, ' ');
+   sq_quote_buf_pretty(dst, argv[i]);
+   }
+}
+
 static char *sq_dequote_step(char *arg, char **next)
 {
char *dst = arg;
diff --git a/quote.h b/quote.h
index 48a75a416b..ea992dcc91 100644
--- a/quote.h
+++ b/quote.h
@@ -33,6 +33,14 @@ extern void sq_quote_buf(struct strbuf *, const char *src);
 extern void sq_quote_argv(struct strbuf *, const char **argv);
 extern void sq_quotef(struct strbuf *, const char *fmt, ...);
 
+/*
+ * These match their non-pretty variants, except that they avoid
+ * quoting when there are no exotic characters. These should only be used for
+ * human-readable output, as sq_dequote() is not smart enough to dequote it.
+ */
+void sq_quote_buf_pretty(struct strbuf *, const char *src);
+void sq_quote_argv_pretty(struct strbuf *, const char **argv);
+
 /* This unwraps what sq_quote() produces in place, but returns
  * NULL if the input does not look like what sq_quote would have
  * produced.
diff --git a/trace.c b/trace.c
index fa9174fc4b..9784915be1 100644
--- a/trace.c
+++ b/trace.c
@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 
strbuf_vaddf(, format, ap);
 
-   sq_quote_argv(, argv);
+   sq_quote_argv_pretty(, argv);
print_trace_line(_default_key, );
 }
 
@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)
atexit(print_command_performance_atexit);
 
strbuf_reset(_line);
-   sq_quote_argv(_line, argv);
+   sq_quote_argv_pretty(_line, argv);
command_start_time = getnanotime();
 }
-- 
2.15.1.600.g899a5f85c6

[PATCH v5 0/7] Trace env variables in run_command()

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

v5 

 - makes test-run-command safer
 - deletes a dead line
 - fixes an embarassing memory leak

Interdiff

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 0ab71f14fb..153342e44d 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -57,9 +57,14 @@ int cmd_main(int argc, const char **argv)
if (argc < 3)
return 1;
while (!strcmp(argv[1], "env")) {
+   if (!argv[2])
+   die("env specifier without a value");
argv_array_push(_array, argv[2]);
argv += 2;
+   argc -= 2;
}
+   if (argc < 3)
+   return 1;
proc.argv = (const char **)argv + 2;
 
if (!strcmp(argv[1], "start-command-ENOENT")) {
diff --git a/trace.c b/trace.c
index 4bfd3fce10..ad75d683bf 100644
--- a/trace.c
+++ b/trace.c
@@ -286,8 +286,8 @@ static void add_env(struct strbuf *dst, const char *const 
*deltaenv)
for (e = deltaenv; e && *e; e++) {
struct strbuf key = STRBUF_INIT;
char *equals = strchr(*e, '=');
+
if (equals) {
-   strbuf_reset();
strbuf_add(, *e, equals - *e);
string_list_insert(, key.buf)->util = equals + 1;
} else {
@@ -360,6 +360,7 @@ void trace_run_command(const struct child_process *cp)
 
sq_quote_argv_pretty(, cp->argv);
print_trace_line(_default_key, );
+   strbuf_release();
 }
 
 static const char *quote_crnl(const char *path)

Jeff King (2):
  sq_quote_argv: drop maxlen parameter
  trace: avoid unnecessary quoting

Nguyễn Thái Ngọc Duy (5):
  trace.c: move strbuf_release() out of print_trace_line()
  trace.c: introduce trace_run_command()
  trace.c: print program 'git' when child_process.git_cmd is set
  trace.c: print env vars in trace_run_command()
  trace.c: print new cwd in trace_run_command()

 builtin/am.c|  2 +-
 builtin/rev-parse.c |  4 +-
 quote.c | 30 --
 quote.h | 10 -
 run-command.c   |  3 +-
 t/helper/test-run-command.c |  9 +
 t/t0061-run-command.sh  | 22 ++
 trace.c | 97 +++--
 trace.h |  3 ++
 9 files changed, 169 insertions(+), 11 deletions(-)

-- 
2.15.1.600.g899a5f85c6

[PATCH v5 1/7] sq_quote_argv: drop maxlen parameter

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

From: Jeff King 

No caller passes anything but "0" for this parameter, which
requests that the function ignore it completely. In fact, in
all of history there was only one such caller, and it went
away in 7f51f8bc2b (alias: use run_command api to execute
aliases, 2011-01-07).

Signed-off-by: Jeff King 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/am.c| 2 +-
 builtin/rev-parse.c | 4 ++--
 quote.c | 4 +---
 quote.h | 2 +-
 trace.c | 4 ++--
 5 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index acfe9d3c8c..5bdd2d7578 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1061,7 +1061,7 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
}
write_state_text(state, "scissors", str);
 
-   sq_quote_argv(, state->git_apply_opts.argv, 0);
+   sq_quote_argv(, state->git_apply_opts.argv);
write_state_text(state, "apply-opt", sb.buf);
 
if (state->rebasing)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 74aa644cbb..96d06a5d01 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -516,7 +516,7 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_SHELL_EVAL);
 
strbuf_addstr(, " --");
-   sq_quote_argv(, argv, 0);
+   sq_quote_argv(, argv);
puts(parsed.buf);
return 0;
 }
@@ -526,7 +526,7 @@ static int cmd_sq_quote(int argc, const char **argv)
struct strbuf buf = STRBUF_INIT;
 
if (argc)
-   sq_quote_argv(, argv, 0);
+   sq_quote_argv(, argv);
printf("%s\n", buf.buf);
strbuf_release();
 
diff --git a/quote.c b/quote.c
index de2922ddd6..b2970da627 100644
--- a/quote.c
+++ b/quote.c
@@ -56,7 +56,7 @@ void sq_quotef(struct strbuf *dst, const char *fmt, ...)
strbuf_release();
 }
 
-void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
+void sq_quote_argv(struct strbuf *dst, const char **argv)
 {
int i;
 
@@ -65,8 +65,6 @@ void sq_quote_argv(struct strbuf *dst, const char** argv, 
size_t maxlen)
for (i = 0; argv[i]; ++i) {
strbuf_addch(dst, ' ');
sq_quote_buf(dst, argv[i]);
-   if (maxlen && dst->len > maxlen)
-   die("Too many or long arguments");
}
 }
 
diff --git a/quote.h b/quote.h
index 66f5644aa2..48a75a416b 100644
--- a/quote.h
+++ b/quote.h
@@ -30,7 +30,7 @@ struct strbuf;
  */
 
 extern void sq_quote_buf(struct strbuf *, const char *src);
-extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
+extern void sq_quote_argv(struct strbuf *, const char **argv);
 extern void sq_quotef(struct strbuf *, const char *fmt, ...);
 
 /* This unwraps what sq_quote() produces in place, but returns
diff --git a/trace.c b/trace.c
index b7530b51a9..fa9174fc4b 100644
--- a/trace.c
+++ b/trace.c
@@ -157,7 +157,7 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 
strbuf_vaddf(, format, ap);
 
-   sq_quote_argv(, argv, 0);
+   sq_quote_argv(, argv);
print_trace_line(_default_key, );
 }
 
@@ -426,6 +426,6 @@ void trace_command_performance(const char **argv)
atexit(print_command_performance_atexit);
 
strbuf_reset(_line);
-   sq_quote_argv(_line, argv, 0);
+   sq_quote_argv(_line, argv);
command_start_time = getnanotime();
 }
-- 
2.15.1.600.g899a5f85c6

[PATCH v5 3/7] trace.c: move strbuf_release() out of print_trace_line()

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

The function is about printing a trace line, not releasing the buffer it
receives too. Move strbuf_release() back outside. This makes it easier
to see how strbuf is managed.

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

diff --git a/trace.c b/trace.c
index 9784915be1..7f3b08e148 100644
--- a/trace.c
+++ b/trace.c
@@ -131,7 +131,6 @@ static void print_trace_line(struct trace_key *key, struct 
strbuf *buf)
 {
strbuf_complete_line(buf);
trace_write(key, buf->buf, buf->len);
-   strbuf_release(buf);
 }
 
 static void trace_vprintf_fl(const char *file, int line, struct trace_key *key,
@@ -144,6 +143,7 @@ static void trace_vprintf_fl(const char *file, int line, 
struct trace_key *key,
 
strbuf_vaddf(, format, ap);
print_trace_line(key, );
+   strbuf_release();
 }
 
 static void trace_argv_vprintf_fl(const char *file, int line,
@@ -159,6 +159,7 @@ static void trace_argv_vprintf_fl(const char *file, int 
line,
 
sq_quote_argv_pretty(, argv);
print_trace_line(_default_key, );
+   strbuf_release();
 }
 
 void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
@@ -171,6 +172,7 @@ void trace_strbuf_fl(const char *file, int line, struct 
trace_key *key,
 
strbuf_addbuf(, data);
print_trace_line(key, );
+   strbuf_release();
 }
 
 static void trace_performance_vprintf_fl(const char *file, int line,
@@ -190,6 +192,7 @@ static void trace_performance_vprintf_fl(const char *file, 
int line,
}
 
print_trace_line(_perf_key, );
+   strbuf_release();
 }
 
 #ifndef HAVE_VARIADIC_MACROS
-- 
2.15.1.600.g899a5f85c6

Re: [PATCH 1/1] Mark messages for translations

[Duy Nguyen]

On Mon, Jan 15, 2018 at 12:44 PM, Alexander Shopov  wrote:
> @@ -160,7 +160,7 @@ int check_filename(const char *prefix, const char *arg)
> free(to_free);
> return 0; /* file does not exist */
> }
> -   die_errno("failed to stat '%s'", arg);
> +   die_errno(_("failed to stat '%s'", arg));
>  }

The new ")" is at a wrong place. It should be %s'"), arg); not %s'", arg));
-- 
Duy

Re: [PATCH 1/1] Mark messages for translations

[Eric Sunshine]

On Mon, Jan 15, 2018 at 12:44 AM, Alexander Shopov  wrote:
> Reuse already translated messages if possible
> Do not translate messages aimed at developers of git

A couple comments beyond those from Hannes...

> Signed-off-by: Alexander Shopov 
> ---
> diff --git a/git.c b/git.c
> @@ -5,11 +5,11 @@
>  const char git_usage_string[] =
> +   N_("git [--version] [--help] [-C ] [-c name=value]\n"

Since you're touching this, perhaps take the opportunity to fix `-c
name=value` to say `-c =`?

> +  "   [--exec-path[=]] [--html-path] [--man-path] 
> [--info-path]\n"
> +  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
> [--bare]\n"
> +  "   [--git-dir=] [--work-tree=] 
> [--namespace=]\n"
> +  "[]");
> diff --git a/setup.c b/setup.c
> @@ -525,19 +525,19 @@ void read_gitfile_error_die(int error_code, const char 
> *path, const char *dir)
> case READ_GITFILE_ERR_NOT_A_REPO:
> -   die("Not a git repository: %s", dir);
> +   die(_("Not a git repository: %s"), dir);
> default:
> -   die("BUG: unknown error code");
> +   die(_("BUG: unknown error code"));

This last one is aimed at developers (indeed, "BUG" message should
never been seen by end-users). I'd leave it untranslated.

> }
>  }

Re: [PATCH 1/1] Mark messages for translations

[Alexander Shopov]

And again, sigh:
>>   const char git_usage_string[] =

>>   const char git_more_info_string[] =
> It is not obvious to me where git_usage_string is looked up in the
> message catalog. It is used that way in builtin/help.c ..

I wanted to be consistent with the current state of the file. This is the
same way const char git_more_info_string[] is defined and initialized.
Having it this way saves the lookup on each usage I guess but any performance
gains will be negligible. Is there a convention?

> We have settled with lower-case letters at the beginning of error
> messages. (See Documentation/CodingGuidelines, "Error Messages".) You
> could fix that while you are touching die, die_errno, etc, messages.

Great! It will allow for further reduction in repetition of messages.

> I notice you change past tense to present tense in some cases.
I am reading this that messages SHOULD be in present tense (unless they MUST
be in past tense). This is good. Perhaps I will look at current messages and
then fix en masse (if there is sth to fix).

> I'm not a friend of geeky abbreviations like "chdir" or "cwd" in
> user-visible messages

I agree but I would also keep in mind that using the name of the function
may alllow to parametrize the messages like:
Cannot execute "%s" on "%s" - for example. Anyway this is not that important
for me. Will wait for other opinions.

>> - die_errno("fork failed");
>> + die_errno(_("fork failed"));
>> - die_errno("setsid failed");
>> + die_errno(_("setsid failed"));

> it is useful to have the function name in  the message. Which rises the
> question:why translate them at all?
Why not eat the cake while having it - one can pass function name in  a
 message like: '"%s" failed'

Regards:
al_shopov