git version statistics

git version statistics

[Jeff King]

Just for fun, I've assembled a few statistics on git client versions
that hit github.com. These are collected from the http user-agent
strings provided by smart-http requests (we don't allow dumb http at all
these days, and the git protocol does not provide any version
information). It includes only the git/* entries (which are the vast
majority of clients hitting "info/refs?service=.*"; the second biggest
is JGit/*). And I counted each version only a single time from each IP
(so this would skew if you had a lot of clients behind a single IP;
their version would be counted only once).

Here are the results from the last few days:

  git/1.7.11.x |  0.1%   (0%) | 
  git/1.7.10.x | 21.1%  (21%) | **********************************
  git/1.7.9.x  | 11.4%  (32%) | ******************
  git/1.7.8.x  | 24.1%  (56%) | ****************************************
  git/1.7.7.x  | 12.2%  (68%) | ********************
  git/1.7.6.x  |  1.8%  (70%) | **
  git/1.7.5.x  |  8.2%  (78%) | *************
  git/1.7.4.x  |  7.2%  (86%) | ***********
  git/1.7.3.x  |  1.9%  (87%) | ***
  git/1.7.2.x  |  3.0%  (90%) | ****
  git/1.7.1.x  |  2.1%  (93%) | ***
  git/1.7.0.x  |  7.0%  (99%) | ***********
  git/1.6.6.x  |  0.0% (100%) | 

In this table, I've collapsed all of the x.y.z.* versions into a single
string to get an overview (a more detailed table is below). The first
percentage is the portion of the total requests; the second is a
cumulative portion (so, for example, 90% of clients are running v1.7.2.x
or higher, though only 3% are running v1.7.2.x itself).

Note that these numbers are skewed away from git versions lower than
v1.6.6, because that is when we added smart http support. Requests over
git:// or ssh are not included at all in these results.

For comparison, here's the same table from mid-September of 2011 (v1.7.7
was in -rc1 at the time):

  git/1.7.7.x |  0.2%   (0%) | 
  git/1.7.6.x | 19.3%  (19%) | *****************
  git/1.7.5.x |  8.0%  (27%) | *******
  git/1.7.4.x | 43.8%  (71%) | ****************************************
  git/1.7.3.x | 10.7%  (82%) | *********
  git/1.7.2.x |  4.9%  (86%) | ****
  git/1.7.1.x |  4.7%  (91%) | ****
  git/1.7.0.x |  8.2%  (99%) | *******
  git/1.6.6.x |  0.1% (100%) | 

So it seems that a large proportion of git users (or at least github
users) stay within a few versions of the most current. In both cases,
70% are within 4 major releases of the most recent version.

Here's a more detailed table (from recent data), showing individual
w.x.y.z versions:

  git/1.7.10.3 |  3.6%   (3%) | *******
  git/1.7.10.2 |  4.4%   (8%) | *********
  git/1.7.10.1 |  1.5%   (9%) | ***
  git/1.7.10   |  2.4%  (11%) | *****
  git/1.7.9.6  |  0.2%  (12%) | 
  git/1.7.9.5  |  9.2%  (21%) | *******************
  git/1.7.9.4  |  1.2%  (22%) | **
  git/1.7.9.3  |  0.3%  (22%) | 
  git/1.7.9.2  |  0.4%  (23%) | 
  git/1.7.9.1  |  0.4%  (23%) | 
  git/1.7.9    |  1.1%  (24%) | **
  git/1.7.8.6  |  0.3%  (25%) | 
  git/1.7.8.5  |  0.0%  (25%) | 
  git/1.7.8.4  |  0.7%  (25%) | *
  git/1.7.8.3  | 12.2%  (38%) | *************************
  git/1.7.8.2  | 18.8%  (56%) | ****************************************
  git/1.7.8.1  |  0.1%  (56%) | 
  git/1.7.8    |  0.3%  (57%) | 
  git/1.7.7.6  |  0.5%  (57%) | *
  git/1.7.7.4  |  0.2%  (57%) | 
  git/1.7.7.3  |  0.4%  (58%) | 
  git/1.7.7.2  |  0.1%  (58%) | 
  git/1.7.7.1  |  0.0%  (58%) | 
  git/1.7.7    |  0.7%  (59%) | *
  git/1.7.6.5  |  0.1%  (59%) | 
  git/1.7.6.4  |  0.2%  (59%) | 
  git/1.7.6.3  |  0.0%  (59%) | 
  git/1.7.6.1  |  0.5%  (59%) | *
  git/1.7.6    |  0.7%  (60%) | *
  git/1.7.5.4  | 10.9%  (71%) | ***********************
  git/1.7.5.3  |  0.1%  (71%) | 
  git/1.7.5.2  |  0.1%  (71%) | 
  git/1.7.5.1  |  0.2%  (71%) | 
  git/1.7.5    |  0.1%  (71%) | 
  git/1.7.4.5  |  0.4%  (72%) | 
  git/1.7.4.4  |  3.3%  (75%) | *******
  git/1.7.4.3  |  0.0%  (75%) | 
  git/1.7.4.2  |  0.0%  (75%) | 
  git/1.7.4.1  |  5.3%  (81%) | ***********
  git/1.7.4    |  0.2%  (81%) | 
  git/1.7.3.5  |  0.2%  (81%) | 
  git/1.7.3.4  |  1.2%  (82%) | **
  git/1.7.3.3  |  0.1%  (82%) | 
  git/1.7.3.2  |  0.4%  (83%) | 
  git/1.7.3.1  |  0.1%  (83%) | 
  git/1.7.3    |  0.1%  (83%) | 
  git/1.7.2.5  |  3.6%  (86%) | *******
  git/1.7.2.3  |  0.3%  (87%) | 
  git/1.7.2.2  |  0.1%  (87%) | 
  git/1.7.2.1  |  0.0%  (87%) | 
  git/1.7.2    |  0.1%  (87%) | 
  git/1.7.1.1  |  0.1%  (87%) | 
  git/1.7.1    |  2.8%  (90%) | ******
  git/1.7.0.6  |  0.0%  (90%) | 
  git/1.7.0.5  |  0.0%  (90%) | 
  git/1.7.0.4  |  7.0%  (97%) | **************
  git/1.7.0    |  2.6%  (99%) | *****
  git/1.6.6    |  0.0% (100%) | 

The interesting thing to me is how spiky it is, and where the spikes
fall. I would expect to see a spike around the highest maint release of
each major version (so v1.7.8.6, for example, with many fewer installs
of v1.7.8.5, v1.7.8.4, and so forth). But that's not what happens. The
most popular v1.7.8.x versions are .3 and .2, and hardly anybody
bothered to move to v1.7.8.6.

I can only assume these are skewed by some widely-used binary
distribution being locked onto particular versions (e.g., the spike at
v1.7.2.5 represents Debian stable).

If anybody has suggestions for other interesting analyses to perform,
let me know.

-Peff

Re: git version statistics

[Jeff King]

On Thu, May 31, 2012 at 07:48:01AM -0400, Jeff King wrote:

> Just for fun, I've assembled a few statistics on git client versions
> that hit github.com. These are collected from the http user-agent
> strings provided by smart-http requests (we don't allow dumb http at all
> these days, and the git protocol does not provide any version
> information).

We have capabilities, and I looked into trying to fingerprint client
versions based on the capabilities. Unfortunately, it's extremely
coarse-grained, because we just don't add capabilities very often.

I came up with this list of fetch-pack capabilities, and when they were
introduced:

  - multi_ack v0.99.9
  - thin-pack v1.3.0
  - side-band v1.4.1
  - side-band-64k v1.4.3
  - ofs-delta v1.4.4
  - shallow v1.5.0
  - no-progress v1.5.1
  - include-tag v1.5.5
  - multi_ack_detailed v1.6.6
  - no-done v1.7.5

But note that "shallow" is not requested explicitly by the client; you
can detect it by the client sending "shallow" commands, but then only if
it is a shallow repository. Similarly, "no-progress" is only requested
if the client is run with "--no-progress" or without a tty.

The "no-done" capability is only requested in stateless-rpc mode (i.e.,
over smart-http). So we can use it only for smart-http, in which case we
have more accurate numbers already (because git puts the version in the
http user-agent string).

So realistically you are looking at identifying whether a client version
is >= v1.6.6 (if it has multi_ack_detailed) or >= v1.5.5 (if it has
include-tag). Beyond that it gets uselessly old, and that's really not
very fine-grained at all.

I'm really tempted to do something like the patch below, which adds an
agent field to the capability string. It wouldn't help with identifying
older versions, but eventually all versions of git would send it (and
those that didn't could be stuck in the "wow, that's old" bin).

-Peff

-- >8 --
Subject: include agent identifier in capability string

Instead of having the client advertise a particular version
number in the git protocol, we have managed extensions and
backwards compatibility by having clients and servers
advertise capabilities that they support. This is far more
robust than having each side consult a table of
known versions, and provides sufficient information for the
protocol interaction to complete.

However, it does not allow servers to keep statistics on
which client versions are being used. This information is
not necessary to complete the network request (the
capabilities provide enough information for that), but it
may be helpful to conduct a general survey of client
versions in use.

We already send the client version in the user-agent header
for http requests; adding it here would allow us to gather
similar statistics for non-http requests.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch-pack.c | 1 +
 builtin/send-pack.c  | 6 ++++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 149db88..f3b8422 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -327,6 +327,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
+			strbuf_addf(&c, " agent=git/%s", git_version_string);
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
 			strbuf_release(&c);
 		} else
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index d5d7105..3d87c71 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -306,11 +306,13 @@ int send_pack(struct send_pack_args *args,
 			int quiet = quiet_supported && (args->quiet || !args->progress);
 
 			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
-				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
+				packet_buf_write(&req_buf,
+						 "%s %s %s%c%s%s%s agent=git/%s",
 						 old_hex, new_hex, ref->name, 0,
 						 status_report ? " report-status" : "",
 						 use_sideband ? " side-band-64k" : "",
-						 quiet ? " quiet" : "");
+						 quiet ? " quiet" : "",
+						 git_version_string);
 			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
-- 
1.7.11.rc0.35.ga99aee0

Re: git version statistics

[Stephen Bash]

----- Original Message -----
> From: "Jeff King" <peff@peff.net>
> Sent: Thursday, May 31, 2012 7:48:02 AM
> Subject: git version statistics
> 
> Just for fun, I've assembled a few statistics on git client versions
> that hit github.com.

Certainly an interesting read...  Thanks!

> The interesting thing to me is how spiky it is, and where the spikes
> fall. I would expect to see a spike around the highest maint release
> of each major version (so v1.7.8.6, for example, with many fewer
> installs of v1.7.8.5, v1.7.8.4, and so forth). But that's not what
> happens.  The most popular v1.7.8.x versions are .3 and .2, and hardly
> anybody bothered to move to v1.7.8.6.

I wonder if the spikes correlate with time between releases?  For open source projects I tend to download the most recent when setting up a new machine (or after encountering a nasty bug), and then not upgrade for a while.  So in that mode of operation, releases that are "the newest" for the longest would get more users... (though at least on my Mac the homebrew project is helping me break the habit and stay more up-to-date)
 
> I can only assume these are skewed by some widely-used binary
> distribution being locked onto particular versions (e.g., the spike at
> v1.7.2.5 represents Debian stable).

... but then again, that's probably a better reason for large swaths of users to have a fixed version...

Thanks again for the interesting read.

Stephen

Re: git version statistics

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 149db88..f3b8422 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -327,6 +327,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
>  			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
>  			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
>  			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
> +			strbuf_addf(&c, " agent=git/%s", git_version_string);
>  			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
>  			strbuf_release(&c);

Even though the version string GIT-VERSION-GEN script deduces from
the repository version is designed to be safe, in general "version"
file can contain a string with whitespaces.  You may want to be
careful about that in the above.

Do we want a similar identifier string on the other side of the
connection?

Re: git version statistics

[Jeff King]

On Thu, May 31, 2012 at 11:20:46AM -0400, Stephen Bash wrote:

> > The interesting thing to me is how spiky it is, and where the spikes
> > fall. I would expect to see a spike around the highest maint release
> > of each major version (so v1.7.8.6, for example, with many fewer
> > installs of v1.7.8.5, v1.7.8.4, and so forth). But that's not what
> > happens.  The most popular v1.7.8.x versions are .3 and .2, and hardly
> > anybody bothered to move to v1.7.8.6.
> 
> I wonder if the spikes correlate with time between releases?  For open
> source projects I tend to download the most recent when setting up a
> new machine (or after encountering a nasty bug), and then not upgrade
> for a while.  So in that mode of operation, releases that are "the
> newest" for the longest would get more users... (though at least on my
> Mac the homebrew project is helping me break the habit and stay more
> up-to-date)

Good point. If you assume that users pick a random day to upgrade or
install and choose the latest version, then you will get a non-uniform
distribution. Because the release dates are non-uniform, their time
spent as the latest is not even. There may also be natural variations in
installations over time (e.g., over holidays).

Here are the release dates for the v1.7.8.x series, as well as the
adjacent master releases:

  v1.7.8   2011-12-02
  v1.7.8.1 2011-12-21
  v1.7.8.2 2011-12-28
  v1.7.8.3 2012-01-06
  v1.7.8.4 2012-01-18
  v1.7.9   2012-01-27
  v1.7.8.5 2012-02-26
  v1.7.10  2012-04-06
  v1.7.8.6 2012-04-26

So .2 and .3 were latest for 9 and 12 days, respectively. However, .4
was also the latest for 9 days (until v1.7.9 came out), but does not
have as many users. So why did nobody bother upgrading to v1.7.8.4?
And why wouldn't v1.7.8 have a spike, since it was at the top for 19
days?

I can see why v1.7.8.5 and v1.7.8.6 are the way they are (they were
never latest, and most people would just install v1.7.9 or v1.7.10
instead).

So I think your theory probably explains some of the data, but not all
(and it seems that most people don't really seem to care about old maint
releases once a new master release is out).

-Peff

Re: git version statistics

[Jeff King]

On Thu, May 31, 2012 at 12:35:20PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index 149db88..f3b8422 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -327,6 +327,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
> >  			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
> >  			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
> >  			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
> > +			strbuf_addf(&c, " agent=git/%s", git_version_string);
> >  			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
> >  			strbuf_release(&c);
> 
> Even though the version string GIT-VERSION-GEN script deduces from
> the repository version is designed to be safe, in general "version"
> file can contain a string with whitespaces.  You may want to be
> careful about that in the above.

Yeah, I agree. I should have been more clear that this patch was an RFC
about the idea, not the implementation.

We could also strip off junk like ".dirty" which is unlikely to be
interesting for statistical reporting. On the other hand, it could be
useful for somebody debugging, and it can always be stripped later.

I don't know if anybody cares about the security or privacy implications
of advertising your client version. Maybe it should be configurable?

> Do we want a similar identifier string on the other side of the
> connection?

We could. I don't see much point, unless you were going to conduct a
similar survey by hitting random IPs looking for git ports (but even
then, you're not likely to turn up much, because you have to know a repo
name before you can convince git to show a capability string). I suppose
it could also help with debugging if your client is having trouble
talking to a server that is not under your control.

Some traditional security advice I have heard is that servers should not
advertise their versions, as it makes it more obvious what holes they
have. Personally, I find that argument to be mostly security through
obscurity. If I have an exploit for version X, it's generally just as
easy to try it as it is to check the version (unless the exploit
requires a lot of effort, like guessing a value that might take
thousands of tries).

-Peff

Re: git version statistics

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't know if anybody cares about the security or privacy implications
> of advertising your client version. Maybe it should be configurable?

I do not think it is worth it.

My initial reaction to the patch was a bit of trouble with the word
"agent", as we do not call Git acting on behalf of the end user "an
agent" in general. But it could be used as an excuse for not giving
an extra knob to tweak, as you generally do not muck with User-Agent
strings, either ;-).

>> Do we want a similar identifier string on the other side of the
>> connection?
>
> We could. I don't see much point, unless you were going to conduct a
> similar survey by hitting random IPs looking for git ports (but even
> then, you're not likely to turn up much, because you have to know a repo
> name before you can convince git to show a capability string). I suppose
> it could also help with debugging if your client is having trouble
> talking to a server that is not under your control.

The latter use case was exactly what I had in mind.

> Some traditional security advice I have heard is that servers should not
> advertise their versions, as it makes it more obvious what holes they
> have. Personally, I find that argument to be mostly security through
> obscurity.

I do, too, but shipping with a configuration knob to optionally turn
it off would be sufficient.

Re: git version statistics

[Jeff King]

On Fri, Jun 01, 2012 at 07:49:17AM -0700, Junio C Hamano wrote:

> My initial reaction to the patch was a bit of trouble with the word
> "agent", as we do not call Git acting on behalf of the end user "an
> agent" in general.

Yeah, I don't especially like the term "agent". I had initially called
it "version", but rejected that for two reasons:

  1. It is not just a version, but also telling what software is in use
     (so I would expect git to write git/v1.7.10, and other
     implementations to write write dulwich/1.2.3 or whatever).

  2. I didn't want it to be confused as a protocol version.

But maybe those are non-issues. It should be fairly obvious what it is
when you see even one example of the value.

> > Some traditional security advice I have heard is that servers should not
> > advertise their versions, as it makes it more obvious what holes they
> > have. Personally, I find that argument to be mostly security through
> > obscurity.
> 
> I do, too, but shipping with a configuration knob to optionally turn
> it off would be sufficient.

I think the most sensible thing is to just add a Makefile variable
that defaults to $(GIT_VERSION), and let people override it if they want
privacy. The http user-agent variable actually respects an environment
variable, but I don't see much point in going that far.

I'll cook up a new version of the patch.

-Peff

Re: git version statistics

[Tomas Carnecky]

On Sat, 02 Jun 2012 12:32:48 -0400, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 01, 2012 at 07:49:17AM -0700, Junio C Hamano wrote:
> 
> > My initial reaction to the patch was a bit of trouble with the word
> > "agent", as we do not call Git acting on behalf of the end user "an
> > agent" in general.

Wikipedia defines User agent as 'a software [...] that is acting on behalf of
a user'. That fits this situation quite well, don't you think?

Re: git version statistics

[Jeff King]

On Sat, Jun 02, 2012 at 12:32:48PM -0400, Jeff King wrote:

> I'll cook up a new version of the patch.

The refactoring ended up expanding this into a few patches:

  [1/4]: move git_version_string into version.c
  [2/4]: version: add git_user_agent function
  [3/4]: http: get default user-agent from git_user_agent
  [4/4]: include agent identifier in capability string

-Peff

[PATCH 1/4] move git_version_string into version.c

[Jeff King]

The global git_version_string currently lives in git.c, but
doesn't have anything to do with the git wrapper. Let's move
it into its own file, where it will be more appropriate to
build more version-related functions.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile      | 8 ++++++--
 builtin.h     | 1 -
 builtin/log.c | 1 +
 git.c         | 2 --
 help.c        | 1 +
 version.c     | 4 ++++
 version.h     | 6 ++++++
 7 files changed, 18 insertions(+), 5 deletions(-)
 create mode 100644 version.c
 create mode 100644 version.h

diff --git a/Makefile b/Makefile
index 4592f1f..b394f85 100644
--- a/Makefile
+++ b/Makefile
@@ -799,6 +799,7 @@ LIB_OBJS += usage.o
 LIB_OBJS += userdiff.o
 LIB_OBJS += utf8.o
 LIB_OBJS += varint.o
+LIB_OBJS += version.o
 LIB_OBJS += walker.o
 LIB_OBJS += wrapper.o
 LIB_OBJS += write_or_die.o
@@ -1962,7 +1963,7 @@ strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
 git.o: common-cmds.h
-git.sp git.s git.o: EXTRA_CPPFLAGS = -DGIT_VERSION='"$(GIT_VERSION)"' \
+git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
@@ -1979,6 +1980,9 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+version.sp version.s version.o: EXTRA_CPPFLAGS = \
+	'-DGIT_VERSION="$(GIT_VERSION)"'
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \
@@ -2089,7 +2093,7 @@ configure: configure.ac
 	$(RM) $<+
 
 # These can record GIT_VERSION
-git.o git.spec http.o \
+version.o git.spec http.o \
 	$(patsubst %.sh,%,$(SCRIPT_SH)) \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
diff --git a/builtin.h b/builtin.h
index 338f540..dea1643 100644
--- a/builtin.h
+++ b/builtin.h
@@ -9,7 +9,6 @@
 
 #define DEFAULT_MERGE_LOG_LEN 20
 
-extern const char git_version_string[];
 extern const char git_usage_string[];
 extern const char git_more_info_string[];
 
diff --git a/builtin/log.c b/builtin/log.c
index 906dca4..4f1b42a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -21,6 +21,7 @@
 #include "parse-options.h"
 #include "branch.h"
 #include "streaming.h"
+#include "version.h"
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
diff --git a/git.c b/git.c
index d232de9..4da3db5 100644
--- a/git.c
+++ b/git.c
@@ -256,8 +256,6 @@ static int handle_alias(int *argcp, const char ***argv)
 	return ret;
 }
 
-const char git_version_string[] = GIT_VERSION;
-
 #define RUN_SETUP		(1<<0)
 #define RUN_SETUP_GENTLY	(1<<1)
 #define USE_PAGER		(1<<2)
diff --git a/help.c b/help.c
index 6012c07..662349d 100644
--- a/help.c
+++ b/help.c
@@ -6,6 +6,7 @@
 #include "common-cmds.h"
 #include "string-list.h"
 #include "column.h"
+#include "version.h"
 
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
diff --git a/version.c b/version.c
new file mode 100644
index 0000000..ca68653
--- /dev/null
+++ b/version.c
@@ -0,0 +1,4 @@
+#include "git-compat-util.h"
+#include "version.h"
+
+const char git_version_string[] = GIT_VERSION;
diff --git a/version.h b/version.h
new file mode 100644
index 0000000..8d6c413
--- /dev/null
+++ b/version.h
@@ -0,0 +1,6 @@
+#ifndef VERSION_H
+#define VERSION_H
+
+extern const char git_version_string[];
+
+#endif /* VERSION_H */
-- 
1.7.7.7.32.g4b73117

[PATCH 2/4] version: add git_user_agent function

[Jeff King]

This is basically a fancy way of saying "git/$GIT_VERSION",
except that it is overridable at build-time and through the
environment. Which means that people who don't want to
advertise their git version (for privacy or security
reasons) can tweak it.

Signed-off-by: Jeff King <peff@peff.net>
---
The next patch adapts http.c to use this, and of course the one after
that adds support for the git protocol itself. There are a few other
places where the git version leaks publicly, including at least:

  1. in the x-mailer header of send-email

  2. at the bottom of format-patch emails

  3. in the mime boundaries (!) of format-patch emails

Since the default here is functionally identical, and the only people
who would care are those interested in masking their version, and nobody
has actually come forward and said they want to do that, I am inclined
not to worry about it. I'd consider the masking to be more important (if
it is important at all) on the server side.  Perhaps this series will
catch the attention of people who do care, and they can decide if they
would like to take it farther.

 Makefile  | 11 +++++++++++
 version.c | 13 +++++++++++++
 version.h |  2 ++
 3 files changed, 26 insertions(+)

diff --git a/Makefile b/Makefile
index b394f85..e6e65ca 100644
--- a/Makefile
+++ b/Makefile
@@ -296,6 +296,9 @@ all::
 # the diff algorithm.  It gives a nice speedup if your processor has
 # fast unaligned word loads.  Does NOT work on big-endian systems!
 # Enabled by default on x86_64.
+#
+# Define GIT_USER_AGENT if you want to change how git identifies itself during
+# network interactions.  The default is "git/$(GIT_VERSION)".
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -905,6 +908,8 @@ BUILTIN_OBJS += builtin/write-tree.o
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 EXTLIBS =
 
+GIT_USER_AGENT = git/$(GIT_VERSION)
+
 #
 # Platform specific tweaks
 #
@@ -1916,6 +1921,11 @@ SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
 BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
 endif
 
+GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
+GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
+GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
+BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
@@ -2000,6 +2010,7 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+    -e 's|@@GIT_USER_AGENT@@|$(GIT_USER_AGENT_SQ)|g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
diff --git a/version.c b/version.c
index ca68653..f98d5a6 100644
--- a/version.c
+++ b/version.c
@@ -2,3 +2,16 @@
 #include "version.h"
 
 const char git_version_string[] = GIT_VERSION;
+
+const char *git_user_agent(void)
+{
+	static const char *agent = NULL;
+
+	if (!agent) {
+		agent = getenv("GIT_USER_AGENT");
+		if (!agent)
+			agent = GIT_USER_AGENT;
+	}
+
+	return agent;
+}
diff --git a/version.h b/version.h
index 8d6c413..fd9cdd6 100644
--- a/version.h
+++ b/version.h
@@ -3,4 +3,6 @@
 
 extern const char git_version_string[];
 
+const char *git_user_agent(void);
+
 #endif /* VERSION_H */
-- 
1.7.7.7.32.g4b73117

[PATCH 3/4] http: get default user-agent from git_user_agent

[Jeff King]

This means we will respect the GIT_USER_AGENT build-time
configuration and run-time environment variable.

Signed-off-by: Jeff King <peff@peff.net>
---
Note that this needs to be applied on the recent "http.o depends
on GIT-VERSION-FILE" fix by Erik, which just went into master. It
actually undoes that commit, but that is because the dependency no
longer exists.

 Makefile | 5 +----
 http.c   | 3 ++-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index e6e65ca..62de0b4 100644
--- a/Makefile
+++ b/Makefile
@@ -2104,7 +2104,7 @@ configure: configure.ac
 	$(RM) $<+
 
 # These can record GIT_VERSION
-version.o git.spec http.o \
+version.o git.spec \
 	$(patsubst %.sh,%,$(SCRIPT_SH)) \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
@@ -2274,9 +2274,6 @@ attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
 	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
 
-http.sp http.s http.o: EXTRA_CPPFLAGS = \
-	-DGIT_HTTP_USER_AGENT='"git/$(GIT_VERSION)"'
-
 ifdef NO_EXPAT
 http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
 endif
diff --git a/http.c b/http.c
index 5cb87f1..b61ac85 100644
--- a/http.c
+++ b/http.c
@@ -4,6 +4,7 @@
 #include "run-command.h"
 #include "url.h"
 #include "credential.h"
+#include "version.h"
 
 int active_requests;
 int http_is_verbose;
@@ -299,7 +300,7 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
 
 	curl_easy_setopt(result, CURLOPT_USERAGENT,
-		user_agent ? user_agent : GIT_HTTP_USER_AGENT);
+		user_agent ? user_agent : git_user_agent());
 
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
-- 
1.7.7.7.32.g4b73117

[PATCH 4/4] include agent identifier in capability string

[Jeff King]

Instead of having the client advertise a particular version
number in the git protocol, we have managed extensions and
backwards compatibility by having clients and servers
advertise capabilities that they support. This is far more
robust than having each side consult a table of
known versions, and provides sufficient information for the
protocol interaction to complete.

However, it does not allow servers to keep statistics on
which client versions are being used. This information is
not necessary to complete the network request (the
capabilities provide enough information for that), but it
may be helpful to conduct a general survey of client
versions in use.

We already send the client version in the user-agent header
for http requests; adding it here allows us to gather
similar statistics for non-http requests.

Signed-off-by: Jeff King <peff@peff.net>
---
Two important changes from the previous round:

  1. We sanitize the agent string to remove non-printable characters and
     whitespace (they are replaced with '.'). This generally shouldn't
     happen, but is a defensive measure against breaking the protocol.

  2. The server half of the connection will advertise their versions in
     the capability strings, too.

 builtin/fetch-pack.c   |  2 ++
 builtin/receive-pack.c |  6 ++++--
 builtin/send-pack.c    |  7 +++++--
 upload-pack.c          |  7 +++++--
 version.c              | 21 +++++++++++++++++++++
 version.h              |  1 +
 6 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 149db88..fe56596 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -10,6 +10,7 @@
 #include "remote.h"
 #include "run-command.h"
 #include "transport.h"
+#include "version.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -327,6 +328,7 @@ static int find_common(int fd[2], unsigned char *result_sha1,
 			if (args.no_progress)   strbuf_addstr(&c, " no-progress");
 			if (args.include_tag)   strbuf_addstr(&c, " include-tag");
 			if (prefer_ofs_delta)   strbuf_addstr(&c, " ofs-delta");
+			strbuf_addf(&c, " agent=%s", git_user_agent_sanitized());
 			packet_buf_write(&req_buf, "want %s%s\n", remote_hex, c.buf);
 			strbuf_release(&c);
 		} else
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..fbfa128 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -12,6 +12,7 @@
 #include "string-list.h"
 #include "sha1-array.h"
 #include "connected.h"
+#include "version.h"
 
 static const char receive_pack_usage[] = "git receive-pack <git-dir>";
 
@@ -121,10 +122,11 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	if (sent_capabilities)
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
 	else
-		packet_write(1, "%s %s%c%s%s\n",
+		packet_write(1, "%s %s%c%s%s agent=%s\n",
 			     sha1_to_hex(sha1), path, 0,
 			     " report-status delete-refs side-band-64k quiet",
-			     prefer_ofs_delta ? " ofs-delta" : "");
+			     prefer_ofs_delta ? " ofs-delta" : "",
+			     git_user_agent_sanitized());
 	sent_capabilities = 1;
 }
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index d5d7105..c4d4211 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -8,6 +8,7 @@
 #include "send-pack.h"
 #include "quote.h"
 #include "transport.h"
+#include "version.h"
 
 static const char send_pack_usage[] =
 "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
@@ -306,11 +307,13 @@ int send_pack(struct send_pack_args *args,
 			int quiet = quiet_supported && (args->quiet || !args->progress);
 
 			if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
-				packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
+				packet_buf_write(&req_buf,
+						 "%s %s %s%c%s%s%s agent=%s",
 						 old_hex, new_hex, ref->name, 0,
 						 status_report ? " report-status" : "",
 						 use_sideband ? " side-band-64k" : "",
-						 quiet ? " quiet" : "");
+						 quiet ? " quiet" : "",
+						 git_user_agent_sanitized());
 			}
 			else
 				packet_buf_write(&req_buf, "%s %s %s",
diff --git a/upload-pack.c b/upload-pack.c
index bb08e2e..2e90ccb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -11,6 +11,7 @@
 #include "list-objects.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "version.h"
 
 static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
 
@@ -734,9 +735,11 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	}
 
 	if (capabilities)
-		packet_write(1, "%s %s%c%s%s\n", sha1_to_hex(sha1), refname_nons,
+		packet_write(1, "%s %s%c%s%s agent=%s\n",
+			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
-			     stateless_rpc ? " no-done" : "");
+			     stateless_rpc ? " no-done" : "",
+			     git_user_agent_sanitized());
 	else
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
 	capabilities = NULL;
diff --git a/version.c b/version.c
index f98d5a6..6106a80 100644
--- a/version.c
+++ b/version.c
@@ -1,5 +1,6 @@
 #include "git-compat-util.h"
 #include "version.h"
+#include "strbuf.h"
 
 const char git_version_string[] = GIT_VERSION;
 
@@ -15,3 +16,23 @@ const char *git_user_agent(void)
 
 	return agent;
 }
+
+const char *git_user_agent_sanitized(void)
+{
+	static const char *agent = NULL;
+
+	if (!agent) {
+		struct strbuf buf = STRBUF_INIT;
+		int i;
+
+		strbuf_addstr(&buf, git_user_agent());
+		strbuf_trim(&buf);
+		for (i = 0; i < buf.len; i++) {
+			if (buf.buf[i] <= 32 || buf.buf[i] >= 127)
+				buf.buf[i] = '.';
+		}
+		agent = buf.buf;
+	}
+
+	return agent;
+}
diff --git a/version.h b/version.h
index fd9cdd6..6911a4f 100644
--- a/version.h
+++ b/version.h
@@ -4,5 +4,6 @@
 extern const char git_version_string[];
 
 const char *git_user_agent(void);
+const char *git_user_agent_sanitized(void);
 
 #endif /* VERSION_H */
-- 
1.7.7.7.32.g4b73117

Re: [PATCH 2/4] version: add git_user_agent function

[Thomas Rast]

I'm terribly late to the party, seeing as this is already in next, but:

Jeff King <peff@peff.net> writes:

> This is basically a fancy way of saying "git/$GIT_VERSION",
> except that it is overridable at build-time and through the
> environment. Which means that people who don't want to
> advertise their git version (for privacy or security
> reasons) can tweak it.
...
> +GIT_USER_AGENT = git/$(GIT_VERSION)
...
> +GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
> +GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
> +GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
> +BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'

Unless the user manually sets GIT_USER_AGENT, This forces a full rebuild
due to changed CFLAGS whenever the version changes.  Can you make it so
that only version.o needs to be rebuilt, as with the normal git version
string?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 2/4] version: add git_user_agent function

[Jeff King]

On Tue, Jun 19, 2012 at 08:40:04PM +0200, Thomas Rast wrote:

> I'm terribly late to the party, seeing as this is already in next, but:

It's never too late.

> > +GIT_USER_AGENT = git/$(GIT_VERSION)
> ...
> > +GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
> > +GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
> > +GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
> > +BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
> 
> Unless the user manually sets GIT_USER_AGENT, This forces a full rebuild
> due to changed CFLAGS whenever the version changes.  Can you make it so
> that only version.o needs to be rebuilt, as with the normal git version
> string?

Ick, yeah. I had though this was already the case, but it turns out that
it was a peculiarity of my personal config.mak (I set a custom $prefix
based on the branch, and it has a similar problem). I'll see if I can
fix both.

-Peff

Re: [PATCH 2/4] version: add git_user_agent function

[Jeff King]

On Tue, Jun 19, 2012 at 02:59:16PM -0400, Jeff King wrote:

> > Unless the user manually sets GIT_USER_AGENT, This forces a full rebuild
> > due to changed CFLAGS whenever the version changes.  Can you make it so
> > that only version.o needs to be rebuilt, as with the normal git version
> > string?
> 
> Ick, yeah. I had though this was already the case, but it turns out that
> it was a peculiarity of my personal config.mak (I set a custom $prefix
> based on the branch, and it has a similar problem). I'll see if I can
> fix both.

Here it is.

  [1/3]: Makefile: apply dependencies consistently to sparse/asm targets
  [2/3]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
  [3/3]: Makefile: split prefix flags from GIT-CFLAGS

Patches go on top of jk/version-string.

-Peff

[PATCH 1/3] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

When a C file includes a header file or depends on a
command-line "-D" macro, we note it in the Makefile like:

  git.o: common-cmds.h

However, other targets built from the C file should also
know about this dependency (in particular, .sp and .s files
that are not part of the usual build process). We sometimes
noted these and sometimes not; let's make sure they are
always included.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 62de0b4..537d2ea 100644
--- a/Makefile
+++ b/Makefile
@@ -1972,7 +1972,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.o: common-cmds.h
+git.sp git.s git.o: common-cmds.h
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -1982,9 +1982,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-help.sp help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-- 
1.7.11.rc3.5.g201460b

[PATCH 2/3] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jeff King]

The default user-agent depends on the GIT_VERSION, which
means that anytime you switch versions, it causes a full
rebuild. Instead, let's split it out into its own file and
restrict the dependency to version.o.

Signed-off-by: Jeff King <peff@peff.net>
---
This can't just depend on GIT-VERSION-FILE, since the user may have set
GIT_USER_AGENT independently, and we would want to trigger a rebuild in
that case.

I'm not happy about adding an extra built file and 4 lines of ugly shell
script per variable, but I don't see a good way around it. It would be
slightly nicer if we actually just built auto/user-agent.h with the
value, #included it from version.c, and then let the automatic
dependency checker pick it up. But we'd still have to do the "only
update it if the value is actually changed" dance.

I've worked on systems in the past which use a file per variable, which
lets make handle the dependencies normally. Something like:

  $ echo my-value >config/foo
  $ cat Makefile
  auto/foo.o: config/foo
          ./build-cstr $<

but that is a radical departure from the current config procedure. It
also doesn't play all that well with version control (are config/* files
versioned? If not, where do the defaults come from? If so, how do you
override without the vcs wanting to commit changes?).

We are pretty well tied to GNU make. So there might be some GNU-specific
way of templating GIT-CFLAGS, GIT-LDFLAGS, GIT-USER-AGENT etc.

 .gitignore |  1 +
 Makefile   | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index bf66648..7329cfe 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git
diff --git a/Makefile b/Makefile
index 537d2ea..42ce2dd 100644
--- a/Makefile
+++ b/Makefile
@@ -1924,7 +1924,11 @@ endif
 GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
 GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
 GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
-BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
+GIT-USER-AGENT: FORCE
+	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+		echo >&2 "    * new user-agent flag"; \
+		echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
+	fi
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
@@ -1990,8 +1994,10 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+version.sp version.s version.o: GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	'-DGIT_VERSION="$(GIT_VERSION)"'
+	'-DGIT_VERSION="$(GIT_VERSION)"' \
+	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
-- 
1.7.11.rc3.5.g201460b

[PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.

Signed-off-by: Jeff King <peff@peff.net>
---
We could break this down even further and save a few compilations (e.g.,
config.o does not care about bindir, only sysconfdir). But given that
most people just set prefix anyway, I don't think it's worth the extra
lines of Makefile (and this patch will never generate a _wrong_ answer;
it is too conservative in doing extra compiles).

 .gitignore |  1 +
 Makefile   | 29 +++++++++++++++++++++--------
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7329cfe..c60c5a3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-PREFIX
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index 42ce2dd..9761ae8 100644
--- a/Makefile
+++ b/Makefile
@@ -1976,7 +1976,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.sp git.s git.o: common-cmds.h
+git.sp git.s git.o: common-cmds.h GIT-PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -1988,7 +1988,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -2036,7 +2036,7 @@ $(SCRIPT_LIB) : % : %.sh
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
-perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
@@ -2080,7 +2080,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 endif # NO_PERL
 
 ifndef NO_PYTHON
-$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
+$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
@@ -2263,20 +2263,25 @@ xdiff-interface.o $(XDIFF_OBJS): $(XDIFF_H)
 $(VCSSVN_OBJS) $(VCSSVN_TEST_OBJS): $(LIB_H) $(VCSSVN_H)
 endif
 
+exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
+builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
+config.sp config.s config.o: GIT-PREFIX
 config.sp config.s config.o: EXTRA_CPPFLAGS = \
 	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.sp attr.s attr.o: GIT-PREFIX
 attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 	-DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
 
+gettext.sp gettext.s gettext.o: GIT-PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
 	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
 
@@ -2400,14 +2405,22 @@ cscope:
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
 ### Detect prefix changes
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
-             $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
-             $(localedir_SQ):$(USE_GETTEXT_SCHEME)
+TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
+		$(localedir_SQ)
+
+GIT-PREFIX: FORCE
+	@FLAGS='$(TRACK_PREFIX)'; \
+	if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
+		echo 1>&2 "    * new prefix flags"; \
+		echo "$$FLAGS" >GIT-PREFIX; \
+	fi
+
+TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)
 
 GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
-		echo 1>&2 "    * new build flags or prefix"; \
+		echo 1>&2 "    * new build flags"; \
 		echo "$$FLAGS" >GIT-CFLAGS; \
             fi
 
-- 
1.7.11.rc3.5.g201460b

Re: [PATCH 1/3] Makefile: apply dependencies consistently to sparse/asm targets

[Junio C Hamano]

Makes sense.  Thanks.

Re: [PATCH 2/3] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The default user-agent depends on the GIT_VERSION, which
> means that anytime you switch versions, it causes a full
> rebuild. Instead, let's split it out into its own file and
> restrict the dependency to version.o.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This can't just depend on GIT-VERSION-FILE, since the user may have set
> GIT_USER_AGENT independently, and we would want to trigger a rebuild in
> that case.

OK.  Will queue.

Re: [PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Most of the build targets do not care about the setting of $prefix
> (or its derivative variables), but will be rebuilt if the prefix
> changes. For most setups this doesn't matter (they set prefix once
> and never change it), but for a setup which puts each branch or
> version in its own prefix, this unnecessarily causes a full
> rebuild whenever the branc is changed.

s/branc /branch /.

I have to wonder if is this something we care about that much.

The damage is not too bad from the point of view of linecount, but
this embeds the implicit knowledge of dependencies from $prefix to
various path variables to selected object files that embed these
paths variables by scattering dependencies on GIT-PREFIX in the
Makefile, which does not seem to scale very well.  I wonder if it
makes sense to have a single default-paths.o file that holds these
strings and recompile only that file when any of the paths change,
to localize the damage.

Of course, the current users of GIT_HTML_PATH that expect they can
do sizeof(GIT_HTML_PATH)-1 in place of strlen(GIT_HTML_PATH) may
need to be adjusted if we go that route.

Will queue, but we might want to rethink this a bit more.

Thanks.

Re: [PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

On Tue, Jun 19, 2012 at 01:51:14PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Most of the build targets do not care about the setting of $prefix
> > (or its derivative variables), but will be rebuilt if the prefix
> > changes. For most setups this doesn't matter (they set prefix once
> > and never change it), but for a setup which puts each branch or
> > version in its own prefix, this unnecessarily causes a full
> > rebuild whenever the branc is changed.
> [...]
> 
> I have to wonder if is this something we care about that much.

It does speed up my build a fair bit, but I admit I have a somewhat
uncommon setup.

> The damage is not too bad from the point of view of linecount, but
> this embeds the implicit knowledge of dependencies from $prefix to
> various path variables to selected object files that embed these
> paths variables by scattering dependencies on GIT-PREFIX in the
> Makefile, which does not seem to scale very well.  I wonder if it
> makes sense to have a single default-paths.o file that holds these
> strings and recompile only that file when any of the paths change,
> to localize the damage.
> 
> Of course, the current users of GIT_HTML_PATH that expect they can
> do sizeof(GIT_HTML_PATH)-1 in place of strlen(GIT_HTML_PATH) may
> need to be adjusted if we go that route.

Yeah, I think that would be nicer overall. If we move to a link-time
dependency, then we can even put _all_ of the Makefile-based strings in
there without ever having to care about who uses it. Of course, it won't
work for things that truly need to be preprocessor macros (for
conditional compilation), so we'd still be stuck with those (most of
them just end up in CFLAGS and trigger a full rebuild, which is probably
OK).

-Peff

Re: [PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 19, 2012 at 01:51:14PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Most of the build targets do not care about the setting of $prefix
>> > (or its derivative variables), but will be rebuilt if the prefix
>> > changes. For most setups this doesn't matter (they set prefix once
>> > and never change it), but for a setup which puts each branch or
>> > version in its own prefix, this unnecessarily causes a full
>> > rebuild whenever the branc is changed.
>> [...]
>> 
>> I have to wonder if is this something we care about that much.
>
> It does speed up my build a fair bit, but I admit I have a somewhat
> uncommon setup.

Oh, I wouldn't question the "speeding up" part.  I simply expected
that people who use separate prefix depending on branch would have
separate checkout for these branches they build and install
regularly to their own prefixes, so recompilation due to prefix
change would not be an issue, because by definition in such a set-up
you won't change branch in a checkout.

> Yeah, I think that would be nicer overall. If we move to a link-time
> dependency, then we can even put _all_ of the Makefile-based strings in
> there without ever having to care about who uses it. Of course, it won't
> work for things that truly need to be preprocessor macros (for
> conditional compilation), so we'd still be stuck with those (most of
> them just end up in CFLAGS and trigger a full rebuild, which is probably
> OK).

Yeah, I didn't go through the list of Make variables to see how much
damage I was talking about ;-).

Re: [PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

On Tue, Jun 19, 2012 at 05:04:26PM -0400, Jeff King wrote:

> > The damage is not too bad from the point of view of linecount, but
> > this embeds the implicit knowledge of dependencies from $prefix to
> > various path variables to selected object files that embed these
> > paths variables by scattering dependencies on GIT-PREFIX in the
> > Makefile, which does not seem to scale very well.  I wonder if it
> > makes sense to have a single default-paths.o file that holds these
> > strings and recompile only that file when any of the paths change,
> > to localize the damage.
> > 
> > Of course, the current users of GIT_HTML_PATH that expect they can
> > do sizeof(GIT_HTML_PATH)-1 in place of strlen(GIT_HTML_PATH) may
> > need to be adjusted if we go that route.
> 
> Yeah, I think that would be nicer overall. If we move to a link-time
> dependency, then we can even put _all_ of the Makefile-based strings in
> there without ever having to care about who uses it. Of course, it won't
> work for things that truly need to be preprocessor macros (for
> conditional compilation), so we'd still be stuck with those (most of
> them just end up in CFLAGS and trigger a full rebuild, which is probably
> OK).

I started on this, but it is a little bit trickier than that. We can
cover C compilation with one strategy, but make variables end up going
lots of other places, too, like shell scripts, the perl Makefile
generation, etc. Those places all need to individually respect a
separate sentinel-file dependency. So I don't think there is an easy way
out of the sprinkling of dependencies.

At the very least, I tried to keep the dependency close to the
point-of-use, like:

  foo.o: GIT-PREFIX
  foo.o: EXTRA_CPPFLAGS = \
          '-DPREFIX=$(prefix_SQ)'

even if the actual build rules for foo.o are found elsewhere (and
typically they are, as they are part of a big pattern-based rule).

Some of the existing locations did not do a great job of that, and
instead it like this:

  version.o git.spec \
          $(patsubst %.sh,%,$(SCRIPT_SH)) \
          $(patsubst %.perl,%,$(SCRIPT_PERL)) \
          : GIT-VERSION-FILE

  [... 500 lines later ...]

  git.spec: git.spec.in
          sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' <$< >$@+

If you read the latter hunk and wanted to emulate it, there is nothing
to indicate that you must also modify the earlier hunk. I think putting
them together makes more sense (even though it technically takes more
lines).

-Peff

[PATCHv2 0/8] makefile cleanups

[Jeff King]

On Tue, Jun 19, 2012 at 05:43:08PM -0400, Jeff King wrote:

> Some of the existing locations did not do a great job of that, and
> instead it like this:
> 
>   version.o git.spec \
>           $(patsubst %.sh,%,$(SCRIPT_SH)) \
>           $(patsubst %.perl,%,$(SCRIPT_PERL)) \
>           : GIT-VERSION-FILE
> 
>   [... 500 lines later ...]
> 
>   git.spec: git.spec.in
>           sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' <$< >$@+
> 
> If you read the latter hunk and wanted to emulate it, there is nothing
> to indicate that you must also modify the earlier hunk. I think putting
> them together makes more sense (even though it technically takes more
> lines).

Hmm. The deeper I dig into the Makefile, the more problems I find. I
think I'm ready to come out of the rabbit hole for today, and I ended up
with the series below. Some of it is cleanup and some of it fixes real
problems.

My philosophy in doing the series was:

  1. Dependencies should be inclusive. That is, re-running "make" in an
     already-built tree with different parameters should produce the
     same result as running in a clean tree.

  2. Dependencies do not have to be precise. That is, it's OK to re-run
     a build step even if it will produce the same output. Of course,
     it's nice to be more precise because it saves time, but for the
     sake of simplicity, we compartmentalize groups of changes (e.g.,
     GIT-CFLAGS depends on SHELL_PATH even though only run-command.c
     needs it; that's OK because SHELL_PATH doesn't get changed often).

  3. Dependencies on variables that change regularly _should_ be
     precise. GIT_VERSION (and anything that depends on it) is a good
     example, because you don't want to rebuild every time you check out
     a new version.

  [1/8]: Makefile: apply dependencies consistently to sparse/asm targets
  [2/8]: Makefile: do not replace @@GIT_USER_AGENT@@ in scripts
  [3/8]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
  [4/8]: Makefile: split prefix flags from GIT-CFLAGS
  [5/8]: Makefile: do not replace @@GIT_VERSION@@ in shell scripts
  [6/8]: Makefile: update scripts when build-time parameters change
  [7/8]: Makefile: build instaweb similar to other scripts
  [8/8]: Makefile: move GIT-VERSION-FILE dependencies closer to use

This replaces the series I sent earlier today, and goes on top of
jk/version-string.  A lot of it could be a separate topic, but it's
textually pretty tangled. If jk/version-string is soon to graduate, then
I'd just as soon leave it as-is rather than go to the work of
disentangling it. Even though there are fixes here, none of them is so
critical that they can't be held hostage for a few days or a week.

-Peff

[PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

When a C file includes a header file or depends on a
command-line "-D" macro, we note it in the Makefile like:

  git.o: common-cmds.h

However, other targets built from the C file should also
know about this dependency (in particular, .sp and .s files
that are not part of the usual build process). We sometimes
noted these and sometimes not; let's make sure they are
always included.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as v1.

 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 62de0b4..537d2ea 100644
--- a/Makefile
+++ b/Makefile
@@ -1972,7 +1972,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.o: common-cmds.h
+git.sp git.s git.o: common-cmds.h
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -1982,9 +1982,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-help.sp help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 2/8] Makefile: do not replace @@GIT_USER_AGENT@@ in scripts

[Jeff King]

No scripts actually care about this replacement. This was
erroneously added by d937411.

Signed-off-by: Jeff King <peff@peff.net>
---
I think I started that series by just replacing GIT_USER_AGENT
everywhere that GIT_VERSION was before realizing that I didn't need to
do that at all, and this was leftover cruft.

Technically I could re-roll jk/version-string with this squashed in,
since you are probably about to do the post-release rewind of next, but
I don't know that it's worth the trouble unless we are re-rolling the
whole thing (to put the Makefile fixups first, and then build
version-string stuff on top).

 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index 537d2ea..23c289d 100644
--- a/Makefile
+++ b/Makefile
@@ -2010,7 +2010,6 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-    -e 's|@@GIT_USER_AGENT@@|$(GIT_USER_AGENT_SQ)|g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 3/8] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jeff King]

The default user-agent depends on the GIT_VERSION, which
means that anytime you switch versions, it causes a full
rebuild. Instead, let's split it out into its own file and
restrict the dependency to version.o.

Signed-off-by: Jeff King <peff@peff.net>
---
v1 forgot to add a line to "make clean" to remove the meta-file.

 .gitignore |  1 +
 Makefile   | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index bf66648..7329cfe 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git
diff --git a/Makefile b/Makefile
index 23c289d..8ff61c5 100644
--- a/Makefile
+++ b/Makefile
@@ -1924,7 +1924,11 @@ endif
 GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
 GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
 GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
-BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
+GIT-USER-AGENT: FORCE
+	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+		echo >&2 "    * new user-agent flag"; \
+		echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
+	fi
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
@@ -1990,8 +1994,10 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+version.sp version.s version.o: GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	'-DGIT_VERSION="$(GIT_VERSION)"'
+	'-DGIT_VERSION="$(GIT_VERSION)"' \
+	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -2736,6 +2742,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
+	$(RM) GIT-USER-AGENT
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 4/8] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.

Signed-off-by: Jeff King <peff@peff.net>
---
v1 forgot to remove the file during "make clean".

 .gitignore |  1 +
 Makefile   | 31 ++++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7329cfe..c60c5a3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-PREFIX
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index 8ff61c5..c95a70c 100644
--- a/Makefile
+++ b/Makefile
@@ -1976,7 +1976,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.sp git.s git.o: common-cmds.h
+git.sp git.s git.o: common-cmds.h GIT-PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -1988,7 +1988,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -2035,7 +2035,7 @@ $(SCRIPT_LIB) : % : %.sh
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
-perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
@@ -2079,7 +2079,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 endif # NO_PERL
 
 ifndef NO_PYTHON
-$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
+$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
@@ -2262,20 +2262,25 @@ xdiff-interface.o $(XDIFF_OBJS): $(XDIFF_H)
 $(VCSSVN_OBJS) $(VCSSVN_TEST_OBJS): $(LIB_H) $(VCSSVN_H)
 endif
 
+exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
+builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
+config.sp config.s config.o: GIT-PREFIX
 config.sp config.s config.o: EXTRA_CPPFLAGS = \
 	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.sp attr.s attr.o: GIT-PREFIX
 attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 	-DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
 
+gettext.sp gettext.s gettext.o: GIT-PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
 	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
 
@@ -2399,14 +2404,22 @@ cscope:
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
 ### Detect prefix changes
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
-             $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
-             $(localedir_SQ):$(USE_GETTEXT_SCHEME)
+TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
+		$(localedir_SQ)
+
+GIT-PREFIX: FORCE
+	@FLAGS='$(TRACK_PREFIX)'; \
+	if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
+		echo 1>&2 "    * new prefix flags"; \
+		echo "$$FLAGS" >GIT-PREFIX; \
+	fi
+
+TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)
 
 GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
-		echo 1>&2 "    * new build flags or prefix"; \
+		echo 1>&2 "    * new build flags"; \
 		echo "$$FLAGS" >GIT-CFLAGS; \
             fi
 
@@ -2742,7 +2755,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
-	$(RM) GIT-USER-AGENT
+	$(RM) GIT-USER-AGENT GIT-PREFIX
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 5/8] Makefile: do not replace @@GIT_VERSION@@ in shell scripts

[Jeff King]

No shell script actually uses the replacement (it is used in
some perl scripts, but cmd_munge_script only handles shell
scripts). We can also therefore drop the dependency on
GIT-VERSION-FILE.

Signed-off-by: Jeff King <peff@peff.net>
---
Probably not a big deal, since building scripts is relatively fast, but
it should make the output much shorter. :)

 Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index c95a70c..c0bf3ad 100644
--- a/Makefile
+++ b/Makefile
@@ -2015,7 +2015,6 @@ $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
-    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
@@ -2061,7 +2060,6 @@ gitweb:
 git-instaweb: git-instaweb.sh gitweb
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
 	    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
 	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
@@ -2110,7 +2108,6 @@ configure: configure.ac
 
 # These can record GIT_VERSION
 version.o git.spec \
-	$(patsubst %.sh,%,$(SCRIPT_SH)) \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
 
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 6/8] Makefile: update scripts when build-time parameters change

[Jeff King]

Currently, running:

  make SHELL_PATH=/bin/bash &&
  make SHELL_PATH=/bin/sh

will not rebuild any shell scripts in the second command,
leading to incorrect results when building from an unclean
working directory.

This patch introduces a new dependency meta-file to notice
the change.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspect nobody complained because the parameters replace in shell
files tend not to change. I think perl scripts have a similar problem,
but I decided not to tackle them today.

 .gitignore |  1 +
 Makefile   | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/.gitignore b/.gitignore
index c60c5a3..6535cd7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@
 /GIT-LDFLAGS
 /GIT-GUI-VARS
 /GIT-PREFIX
+/GIT-SCRIPT-DEFINES
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index c0bf3ad..979ba31 100644
--- a/Makefile
+++ b/Makefile
@@ -2010,6 +2010,8 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
+SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
+	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -2022,12 +2024,20 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     $@.sh >$@+
 endef
 
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
+GIT-SCRIPT-DEFINES: FORCE
+	@FLAGS='$(SCRIPT_DEFINES)'; \
+	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
+		echo 1>&2 "    * new script parameters"; \
+		echo "$$FLAGS" >$@; \
+            fi
+
+
+$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
-$(SCRIPT_LIB) : % : %.sh
+$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
@@ -2752,7 +2762,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
-	$(RM) GIT-USER-AGENT GIT-PREFIX
+	$(RM) GIT-USER-AGENT GIT-PREFIX GIT-SCRIPT-DEFINES
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 7/8] Makefile: build instaweb similar to other scripts

[Jeff King]

Instaweb would not properly rebuild if the build-time
parameters changed. Fix this by depending on the
GIT-SCRIPT-DEFINES meta-file and using $(cmd_munge_script)
like all the other shell scripts. This requires adding a few
new parametres to cmd_munge_script, but that doesn't hurt
existing scripts.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 979ba31..ad183d9 100644
--- a/Makefile
+++ b/Makefile
@@ -2011,7 +2011,8 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
-	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ)
+	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
+	$(gitwebdir_SQ):$(PERL_PATH_SQ)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -2021,6 +2022,8 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
     -e $(BROKEN_PATH_FIX) \
+    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
+    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
     $@.sh >$@+
 endef
 
@@ -2067,13 +2070,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 gitweb:
 	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
 
-git-instaweb: git-instaweb.sh gitweb
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
-	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
-	    $@.sh > $@+ && \
+git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES
+	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PERL
-- 
1.7.11.rc3.5.g201460b

[PATCHv2 8/8] Makefile: move GIT-VERSION-FILE dependencies closer to use

[Jeff King]

There is a list of all of the targets which depend on
GIT-VERSION-FILE, but it can be quite far from the actual
point where the targets actually use $(GIT_VERSION). This
can make it hard to verify that each use of $(GIT_VERSION)
has a matching dependency.

This patch moves the dependency closer to the actual build
instructions, which makes verification easier.  This also
fixes the generation of "configure", which did not properly
mark the dependency.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the patch I was trying to get to when I started. :)

I do think this pattern will make patches easier to review; you should
typically see the dependency in the same hunk as the point-of-use.

 Makefile | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index ad183d9..d21702c 100644
--- a/Makefile
+++ b/Makefile
@@ -1994,7 +1994,7 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
-version.sp version.s version.o: GIT-USER-AGENT
+version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
 	'-DGIT_VERSION="$(GIT_VERSION)"' \
 	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
@@ -2050,7 +2050,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
-$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
+$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	sed -e '1{' \
@@ -2107,18 +2107,13 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PYTHON
 
-configure: configure.ac
+configure: configure.ac GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $<+ && \
 	sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    $< > $<+ && \
 	autoconf -o $@ $<+ && \
 	$(RM) $<+
 
-# These can record GIT_VERSION
-version.o git.spec \
-	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
-	: GIT-VERSION-FILE
-
 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
 GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
 	git.o
@@ -2676,7 +2671,7 @@ quick-install-html:
 
 ### Maintainer's dist rules
 
-git.spec: git.spec.in
+git.spec: git.spec.in GIT-VERSION-FILE
 	sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > $@+
 	mv $@+ $@
 
-- 
1.7.11.rc3.5.g201460b

Re: [PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

On Tue, Jun 19, 2012 at 02:39:20PM -0700, Junio C Hamano wrote:

> >> I have to wonder if is this something we care about that much.
> >
> > It does speed up my build a fair bit, but I admit I have a somewhat
> > uncommon setup.
> 
> Oh, I wouldn't question the "speeding up" part.  I simply expected
> that people who use separate prefix depending on branch would have
> separate checkout for these branches they build and install
> regularly to their own prefixes, so recompilation due to prefix
> change would not be an issue, because by definition in such a set-up
> you won't change branch in a checkout.

I dunno if other people do that. For me, separate checkouts would be
overkill. I use a single working tree, and I don't mind switching
between topic branches, but I want to keep installed versions from
polluting each other (e.g., via cruft left in the exec-path or in
templates), which has caused oddness with testing in the past.

It's also nice that I can do:

  $ git checkout v1.7.10.5
  $ make install

and now I need only point my PATH to $HOME/local/git/v1.7.10.5/bin
to test that version forever. It saves lots of compile time when I'm
investigating bug reports.

Of course, I am probably one of the few people in the world who actually
wants to have 50 built versions of git on hand.

-Peff

Re: [PATCH 3/3] Makefile: split prefix flags from GIT-CFLAGS

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Of course, I am probably one of the few people in the world who actually
> wants to have 50 built versions of git on hand.

I have this script as "rungit" on my PATH and "rungit -l" shows 110+
variants.  So at least there are two ;-)

-- >8 --
#!/bin/sh
# Run various vintage of git

variant="${0##*/}" &&
: ${RUNGIT_BASE=$HOME/g} &&
case "$variant" in
rungit)
	case $# in 
	0)
		echo >&2 "which version?"
		exit 1
		;;
	esac
	variant=$1
	shift
	;;
esac &&
case "$variant" in
-l)
	for d in "$RUNGIT_BASE/"git-*/bin/git
	do
		d=$(basename ${d%/bin/git})
		d=${d#git-}
		d=${d#snap-}
		echo "$d"
	done
	exit
	;;
git-*)
	variant=${variant#git-} ;;
v[0-9]*)
	variant=snap-$variant ;;
esac &&
d="$RUNGIT_BASE/git-$variant" &&
if test -f "$d/bin/git"
then
	exec "$d/bin/git" "$@"
else
	echo >&2 "$variant: No such variant for $a"
	exit 1
fi

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder]

Hi,

Jeff King wrote:

> When a C file includes a header file or depends on a
> command-line "-D" macro, we note it in the Makefile like:
>
>   git.o: common-cmds.h
>
> However, other targets built from the C file should also
> know about this dependency (in particular, .sp and .s files
> that are not part of the usual build process). We sometimes
> noted these and sometimes not; let's make sure they are
> always included.

First reactions:

This improves consistency.  Yay!

Making .sp and .s targets depend on generated .h files like
common-cmds.h is very important.  Otherwise, I would not be able to
generate my git.s assembler listing or sparse-check git.c unless
common-cmds.h has already been generated as a side-effect of some
earlier build process.

On the other hand, making .sp and .s targets depend on preexisting .h
files and files like GIT-CFLAGS would not have any effect at all,
because:

 - .sp targets are phony --- there is no stamp file that certifies
   a given file has been checked by a "make sparse" run.  Maybe that
   will change some day.

 - .s targets are rebuilt every time.  Maybe I am just weird, but I
   find myself upgrading my compiler pretty often, so when I manually
   ask for an assembler listing I am happy to see it regenerated
   unconditionally using the new code generation rules.

It turns out that this patch is only about common-cmds.h, which was
the straightforward case.  Why not say so and save the reader from
having to think so hard? ;)

Hope that helps,
Jonathan

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Tue, Jun 19, 2012 at 10:50:15PM -0500, Jonathan Nieder wrote:

> Making .sp and .s targets depend on generated .h files like
> common-cmds.h is very important.  Otherwise, I would not be able to
> generate my git.s assembler listing or sparse-check git.c unless
> common-cmds.h has already been generated as a side-effect of some
> earlier build process.

I suspect in most cases that the earlier build process has happened, and
that's why nobody really complained.

> On the other hand, making .sp and .s targets depend on preexisting .h
> files and files like GIT-CFLAGS would not have any effect at all,
> because:
> 
>  - .sp targets are phony --- there is no stamp file that certifies
>    a given file has been checked by a "make sparse" run.  Maybe that
>    will change some day.
> 
>  - .s targets are rebuilt every time.  Maybe I am just weird, but I
>    find myself upgrading my compiler pretty often, so when I manually
>    ask for an assembler listing I am happy to see it regenerated
>    unconditionally using the new code generation rules.

I don't have a strong opinion, as I don't use either feature. I noticed
the generated header file was a problem, and didn't realize that we
force .s builds.

My counters to the above points (and again, I don't really care
that much) would be:

  1. The .sp and .s targets _do_ need the same -D macros that the .o
     targets get. So it ends up being very obvious that you are omitting
     them in something like:

       foo.o: GIT-VERSION-FILE
       foo.o foo.sp foo.s: EXTRA_CPPFLAGS = \
              -DGIT_VERSION='$(GIT_VERSION)'

     I tend to think it is more readable to simply specify the
     dependencies fully (even if they end up being irrelevant because we
     force-build) than to confuse a reader who is not aware of our
     force-build '.s' rule that is 500 lines away (I was not aware of it
     until you mentioned it).

  2. You describe the behavior now, and I certainly have no plans to
     change it. But there also plausible reasons for both cases to stop
     force-building, in which case these dependencies would become
     relevant.

In other words, I think relying on the force-build is a bit of a
layering violation. Of course, it is a Makefile, which is not exactly
structured programming, but I like to try.

> It turns out that this patch is only about common-cmds.h, which was
> the straightforward case.  Why not say so and save the reader from
> having to think so hard? ;)

Because I didn't realize it was the case at all. :) My intent was
actually to clean up these lines so that they would be correct when I
added GIT-VERSION-FILES and the like to them later.

So I think my preference would be to tack on a note to the commit
message saying "yeah, this doesn't do anything for meta-dependencies,
but it doesn't hurt either". OK?

-Peff

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder]

Jeff King wrote:

>   1. The .sp and .s targets _do_ need the same -D macros that the .o
>      targets get.

Ah, you mean EXTRA_CPPFLAGS.  Yeah, that's also important, though the
patch doesn't have anything to do with it.

Some circuit in my mind missed that you meant EXTRA_CPPFLAGS and not a
file like GIT-CFLAGS.

[...]
>> It turns out that this patch is only about common-cmds.h, which was
>> the straightforward case.  Why not say so and save the reader from
>> having to think so hard? ;)
>
> Because I didn't realize it was the case at all. :) My intent was
> actually to clean up these lines so that they would be correct when I
> added GIT-VERSION-FILES and the like to them later.
>
> So I think my preference would be to tack on a note to the commit
> message saying "yeah, this doesn't do anything for meta-dependencies,
> but it doesn't hurt either". OK?

What is a meta-dependency?  I would find that even more confusing.

This change could be motivated more simply by saying that it prevents
"make git.sp", "make git.s", "make help.s", and "make builtin/help.s"
from failing when common-cmds.h doesn't exist yet, no?

The commit message could also say that it is improving consistency,
which is certainly valuable.

And a mention of EXTRA_CPPFLAGS and generated header files vs.
pre-existing header files could help explain that consistency.

But suggesting that we are supposed to ignore the FORCE just leaves
the reader wondering why the same patch does not also urgently need
to make additional changes such as the following, no?

	builtin/branch.o builtin/checkout.o builtin/clone.o \
	builtin/reset.o branch.o transport.o: branch.h

to

	builtin/branch.sp builtin/branch.o builtin/branch.s \
	builtin/checkout.sp builtin/checkout.o builtin/checkout.s \
	builtin/clone.sp builtin/clone.o builtin/clone.s \
	builtin/reset.sp builtin/reset.o builtin/reset.s \
	branch.sp branch.o branch.s \
	transport.sp transport.o transport.s: branch.h

Hoping that clarifies,
Jonathan

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Wed, Jun 20, 2012 at 05:27:50AM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> >   1. The .sp and .s targets _do_ need the same -D macros that the .o
> >      targets get.
> 
> Ah, you mean EXTRA_CPPFLAGS.  Yeah, that's also important, though the
> patch doesn't have anything to do with it.
> 
> Some circuit in my mind missed that you meant EXTRA_CPPFLAGS and not a
> file like GIT-CFLAGS.

No, I meant GIT-CFLAGS. But the point of my series is that the two are
intimately paired: if you are setting EXTRA_CPPFLAGS to mention a make
variable, then you should have a dependency on a file that changes if
that make variable changes.

> > So I think my preference would be to tack on a note to the commit
> > message saying "yeah, this doesn't do anything for meta-dependencies,
> > but it doesn't hurt either". OK?
> 
> What is a meta-dependency?  I would find that even more confusing.

It is my term for things like GIT-CFLAGS; they are not really
dependencies in the sense that the build process even looks at them, but
they are a marker whose timestamp changes when things which we _do_
actually depend on change. Better name suggestions are welcome.

> This change could be motivated more simply by saying that it prevents
> "make git.sp", "make git.s", "make help.s", and "make builtin/help.s"
> from failing when common-cmds.h doesn't exist yet, no?

More simply, perhaps, but that was not the entire motivation when
writing the patch. It is connected with patches later in the series
which update those lines.

> But suggesting that we are supposed to ignore the FORCE just leaves
> the reader wondering why the same patch does not also urgently need
> to make additional changes such as the following, no?
> 
> 	builtin/branch.o builtin/checkout.o builtin/clone.o \
> 	builtin/reset.o branch.o transport.o: branch.h
> 
> to
> 
> 	builtin/branch.sp builtin/branch.o builtin/branch.s \
> [...]

Those lines were not updated because I did not notice them, as I was
keeping the scope of the updates to generated headers and files like
GIT-CFLAGS. IOW, my patch is a step in what I think is the right
direction, but it does not remove all issues, only one class of them.

As a side note, I have to wonder if those lines are really worthwhile.
Everything already depends on LIB_H (when computed header dependencies
are not used). Headers like "branch.h" seem to be split out of LIB_H to
avoid causing a full rebuild when uncommon headers are updated. But it
is a half-hearted attempt; LIB_H has plenty of infrequently used
headers, and a solution which requires manually updating the target
lists seems doomed to staleness. These days COMPUTE_HEADER_DEPENDENCIES
is on by default, and I expect most developers use it.

Can we just fold these few headers into LIB_H, let people without "gcc
-MMD" deal with the extra compilation, and drop MISC_H and these extra
manual dependencies entirely? And note that "extra compilation" there
only happens when you are trying to rebuild a new version of git in a
working tree containing an older version (so probably bisection would be
the only place people would see it, and even then, only when jumping
between versions that update one of the header files listed in MISC_H,
but _not_ any of the ones listed in LIB_H).

-Peff

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Wed, Jun 20, 2012 at 12:37:14PM -0400, Jeff King wrote:

> > But suggesting that we are supposed to ignore the FORCE just leaves
> > the reader wondering why the same patch does not also urgently need
> > to make additional changes such as the following, no?
> > 
> > 	builtin/branch.o builtin/checkout.o builtin/clone.o \
> > 	builtin/reset.o branch.o transport.o: branch.h
> > 
> > to
> > 
> > 	builtin/branch.sp builtin/branch.o builtin/branch.s \
> > [...]
> 
> Those lines were not updated because I did not notice them, as I was
> keeping the scope of the updates to generated headers and files like
> GIT-CFLAGS. IOW, my patch is a step in what I think is the right
> direction, but it does not remove all issues, only one class of them.
> 
> As a side note, I have to wonder if those lines are really worthwhile.
> [...]

Here's an updated series that drops these lines and I hope will address
the commit message issues you brought up:

  [01/11]: Makefile: sort LIB_H list
  [02/11]: Makefile: fold MISC_H into LIB_H

New in this iteration to get rid of these largely pointless manual
dependencies.

  [03/11]: Makefile: do not have git.o depend on common-cmds.h

New in this iteration.  I noticed while double-checking that this
dependency is pointless.

  [04/11]: Makefile: apply dependencies consistently to sparse/asm targets

Updated based on earlier patches, and with a new commit message
explaining a little more of what's going on.

  [05/11]: Makefile: do not replace @@GIT_USER_AGENT@@ in scripts
  [06/11]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
  [07/11]: Makefile: split prefix flags from GIT-CFLAGS
  [08/11]: Makefile: do not replace @@GIT_VERSION@@ in shell scripts
  [09/11]: Makefile: update scripts when build-time parameters change
  [10/11]: Makefile: build instaweb similar to other scripts
  [11/11]: Makefile: move GIT-VERSION-FILE dependencies closer to use

The rest are largely the same, but with a few minor textual updates to
accomodate the earlier changes.

-Peff

[PATCHv3 01/11] Makefile: sort LIB_H list

[Jeff King]

This was mostly sorted already, but put things like
"cache-tree.h" after "cache.h", even though "-" comes before
"." (at least in the C locale). This will make it easier to
keep the list sorted later by piping it through "sort".

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 62de0b4..72cdb56 100644
--- a/Makefile
+++ b/Makefile
@@ -595,8 +595,8 @@ LIB_H += attr.h
 LIB_H += blob.h
 LIB_H += builtin.h
 LIB_H += bulk-checkin.h
-LIB_H += cache.h
 LIB_H += cache-tree.h
+LIB_H += cache.h
 LIB_H += color.h
 LIB_H += commit.h
 LIB_H += compat/bswap.h
@@ -636,13 +636,13 @@ LIB_H += mailmap.h
 LIB_H += merge-file.h
 LIB_H += merge-recursive.h
 LIB_H += mergesort.h
-LIB_H += notes.h
 LIB_H += notes-cache.h
 LIB_H += notes-merge.h
+LIB_H += notes.h
 LIB_H += object.h
-LIB_H += pack.h
 LIB_H += pack-refs.h
 LIB_H += pack-revindex.h
+LIB_H += pack.h
 LIB_H += parse-options.h
 LIB_H += patch-ids.h
 LIB_H += pkt-line.h
@@ -668,8 +668,8 @@ LIB_H += submodule.h
 LIB_H += tag.h
 LIB_H += thread-utils.h
 LIB_H += transport.h
-LIB_H += tree.h
 LIB_H += tree-walk.h
+LIB_H += tree.h
 LIB_H += unpack-trees.h
 LIB_H += userdiff.h
 LIB_H += utf8.h
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 02/11] Makefile: fold MISC_H into LIB_H

[Jeff King]

We keep a list of most of the header files in LIB_H, but
some are split out into MISC_H. The original point
of LIB_H was that it would force recompilation of C files
when any of the library headers changed. It was
over-encompassing, since not all C files included all of the
library headers; this made it simple to maintain, but meant
that we sometimes recompiled when it was not necessary.

Over time, some new headers were omitted from LIB_H, and
rules were added to the Makefile for a few specific targets
to explicitly depend on them. This avoided some unnecessary
recompilation at the cost of having to maintain the
dependency list of those targets manually (e.g., d349a03).

Later, we needed a complete list of headers from which we
should extract strings to localized. Thus 1b8b2e4 introduced
MISC_H to mention all header files not included in LIB_H,
and the concatenation of the two lists is fed to xgettext.
Headers mentioned as dependencies must also be manually
added to MISC_H to receive the benefits of localization.

Having to update multiple locations manually is a pain and
has led to errors. For example, see "git log -Swt-status.h
Makefile" for some back-and-forth between the two locations.
Or the fact that column.h was never added to MISC_H, and
therefore was not localized (which is fixed by this patch).
Moreover, the benefits of keeping these few headers out of
LIB_H is not that great, for two reasons:

  1. The better way to do this is by auto-computing the
     dependencies, which is more accurate and less work to
     maintain. If your compiler supports it, we turn on
     computed header dependencies by default these days. So
     these manual dependencies are used only for people who
     do not have gcc at all (which increases the chance of
     them becoming stale, as many developers will never even
     use them).

  2. Even if you do not have gcc, the manual header
     dependencies do not help all that much.  They obviously
     cannot help with an initial compilation (since their
     purpose is to avoid unnecessary recompilation when a
     header changes), which means they are only useful when
     building a new version of git in the working tree that
     held an existing build (e.g., after checkout or during a
     bisection). But since a change of a header in LIB_H
     will force recompilation, and given that the vast
     majority of headers are in LIB_H, most version changes
     will result in a full rebuild anyway.

Let's just fold MISC_H into LIB_H and get rid of these
manual rules. The worst case is some extra compilation, but
even that is unlikely to matter due to the reasons above.

The one exception is that we should keep common-cmds.h
separate. Because it is generated, the computed dependencies
do not handle it properly, and we must keep separate
individual dependencies on it. Let's therefore rename MISC_H
to GENERATED_H to make it more clear what should go in it.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 45 ++++++++++++++++-----------------------------
 1 file changed, 16 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index 72cdb56..500966b 100644
--- a/Makefile
+++ b/Makefile
@@ -397,7 +397,7 @@ XDIFF_OBJS =
 VCSSVN_H =
 VCSSVN_OBJS =
 VCSSVN_TEST_OBJS =
-MISC_H =
+GENERATED_H =
 EXTRA_CPPFLAGS =
 LIB_H =
 LIB_OBJS =
@@ -574,30 +574,22 @@ VCSSVN_H += vcs-svn/fast_export.h
 VCSSVN_H += vcs-svn/svndiff.h
 VCSSVN_H += vcs-svn/svndump.h
 
-MISC_H += bisect.h
-MISC_H += branch.h
-MISC_H += bundle.h
-MISC_H += common-cmds.h
-MISC_H += fetch-pack.h
-MISC_H += reachable.h
-MISC_H += send-pack.h
-MISC_H += shortlog.h
-MISC_H += tar.h
-MISC_H += thread-utils.h
-MISC_H += url.h
-MISC_H += walker.h
-MISC_H += wt-status.h
+GENERATED_H += common-cmds.h
 
 LIB_H += advice.h
 LIB_H += archive.h
 LIB_H += argv-array.h
 LIB_H += attr.h
+LIB_H += bisect.h
 LIB_H += blob.h
+LIB_H += branch.h
 LIB_H += builtin.h
 LIB_H += bulk-checkin.h
+LIB_H += bundle.h
 LIB_H += cache-tree.h
 LIB_H += cache.h
 LIB_H += color.h
+LIB_H += column.h
 LIB_H += commit.h
 LIB_H += compat/bswap.h
 LIB_H += compat/cygwin.h
@@ -618,6 +610,7 @@ LIB_H += diff.h
 LIB_H += diffcore.h
 LIB_H += dir.h
 LIB_H += exec_cmd.h
+LIB_H += fetch-pack.h
 LIB_H += fmt-merge-msg.h
 LIB_H += fsck.h
 LIB_H += gettext.h
@@ -627,6 +620,7 @@ LIB_H += graph.h
 LIB_H += grep.h
 LIB_H += hash.h
 LIB_H += help.h
+LIB_H += http.h
 LIB_H += kwset.h
 LIB_H += levenshtein.h
 LIB_H += list-objects.h
@@ -649,6 +643,7 @@ LIB_H += pkt-line.h
 LIB_H += progress.h
 LIB_H += prompt.h
 LIB_H += quote.h
+LIB_H += reachable.h
 LIB_H += reflog-walk.h
 LIB_H += refs.h
 LIB_H += remote.h
@@ -656,9 +651,11 @@ LIB_H += rerere.h
 LIB_H += resolve-undo.h
 LIB_H += revision.h
 LIB_H += run-command.h
+LIB_H += send-pack.h
 LIB_H += sequencer.h
 LIB_H += sha1-array.h
 LIB_H += sha1-lookup.h
+LIB_H += shortlog.h
 LIB_H += sideband.h
 LIB_H += sigchain.h
 LIB_H += strbuf.h
@@ -666,14 +663,18 @@ LIB_H += streaming.h
 LIB_H += string-list.h
 LIB_H += submodule.h
 LIB_H += tag.h
+LIB_H += tar.h
 LIB_H += thread-utils.h
 LIB_H += transport.h
 LIB_H += tree-walk.h
 LIB_H += tree.h
 LIB_H += unpack-trees.h
+LIB_H += url.h
 LIB_H += userdiff.h
 LIB_H += utf8.h
 LIB_H += varint.h
+LIB_H += walker.h
+LIB_H += wt-status.h
 LIB_H += xdiff-interface.h
 LIB_H += xdiff/xdiff.h
 
@@ -2237,20 +2238,6 @@ else
 # gcc detects!
 
 $(GIT_OBJS): $(LIB_H)
-builtin/branch.o builtin/checkout.o builtin/clone.o builtin/reset.o branch.o transport.o: branch.h
-builtin/bundle.o bundle.o transport.o: bundle.h
-builtin/bisect--helper.o builtin/rev-list.o bisect.o: bisect.h
-builtin/clone.o builtin/fetch-pack.o transport.o: fetch-pack.h
-builtin/index-pack.o builtin/grep.o builtin/pack-objects.o transport-helper.o thread-utils.o: thread-utils.h
-builtin/send-pack.o transport.o: send-pack.h
-builtin/log.o builtin/shortlog.o: shortlog.h
-builtin/prune.o builtin/reflog.o reachable.o: reachable.h
-builtin/commit.o builtin/revert.o wt-status.o: wt-status.h
-builtin/tar-tree.o archive-tar.o: tar.h
-connect.o transport.o url.o http-backend.o: url.h
-builtin/branch.o builtin/commit.o builtin/tag.o column.o help.o pager.o: column.h
-http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
-http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h url.h
 
 xdiff-interface.o $(XDIFF_OBJS): $(XDIFF_H)
 
@@ -2347,7 +2334,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 	--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(XDIFF_H) $(VCSSVN_H) $(MISC_H)
+LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(XDIFF_H) $(VCSSVN_H) $(GENERATED_H)
 LOCALIZED_SH := $(SCRIPT_SH)
 LOCALIZED_PERL := $(SCRIPT_PERL)
 
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 03/11] Makefile: do not have git.o depend on common-cmds.h

[Jeff King]

This dependency has been stale since 70827b1 (Split up
builtin commands into separate files from git.c, 2006-04-21).

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index 500966b..58e2099 100644
--- a/Makefile
+++ b/Makefile
@@ -1973,7 +1973,6 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
-git.o: common-cmds.h
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 04/11] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

When a C file "foo.c" depends on a generated header file, we
note the dependency for the "foo.o" target. However, we
should also note it for other targets that are built from
foo.c, like "foo.sp" and "foo.s". These tend to be missed
because the latter two are not part of the default build,
and are typically built after a regular build which will
generate the header.  Let's be consistent about including
them in dependencies.

This also makes us more consistent with nearby lines which
tack on EXTRA_CPPFLAGS when building certain files.  These
flags may sometimes require extra dependencies to be added
(e.g., like GIT-VERSION-FILE; this is not the case for any
of the updated lines in this patch, but it is establishing a
style that will be used in later patches). Technically the
".sp" and ".s" targets do not care about these dependencies,
because they are force-built (".sp" because it is a phony
target, and ".s" because we explicitly force a rebuild).

Since the blocks in question are about communicating "things
built from foo.c depend on these flags", it frees the reader
from having to know or care more about how those targets are
implemented, and why it is OK for only "foo.o" to depend on
GIT-VERSION-FILE while "foo.sp" and "foo.s" both are
impacted by $(GIT_VERSION). And it helps future-proof us if
those force-build details should ever change.

This patch explicitly does not update the static header
dependencies used when COMPUTED_HEADER_DEPENDENCIES is off.
They are similar to the GIT-VERSION-FILE case above, in that
technically "foo.s" would depend on its included headers,
but it is irrelevant because we force-build it anyway. So it
would be tempting to update them in the same way (for
readability and future-proofing). However, those rules are
meant as a fallback to the computed header dependencies,
which do not handle ".s" and ".sp" at all (and are a much
harder problem to solve, as gcc is the one generating those
dependency lists).

So let's leave that harder problem until (and if) somebody
wants to change the ".sp" and ".s" rules, and keep the
static header dependencies consistent with the computed
ones.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 58e2099..d2112f8 100644
--- a/Makefile
+++ b/Makefile
@@ -1982,9 +1982,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-help.sp help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 05/11] Makefile: do not replace @@GIT_USER_AGENT@@ in scripts

[Jeff King]

No scripts actually care about this replacement. This was
erroneously added by d937411.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Makefile b/Makefile
index d2112f8..336cfc4 100644
--- a/Makefile
+++ b/Makefile
@@ -2010,7 +2010,6 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-    -e 's|@@GIT_USER_AGENT@@|$(GIT_USER_AGENT_SQ)|g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 06/11] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jeff King]

The default user-agent depends on the GIT_VERSION, which
means that anytime you switch versions, it causes a full
rebuild. Instead, let's split it out into its own file and
restrict the dependency to version.o.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore |  1 +
 Makefile   | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index bf66648..7329cfe 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git
diff --git a/Makefile b/Makefile
index 336cfc4..7518ad7 100644
--- a/Makefile
+++ b/Makefile
@@ -1925,7 +1925,11 @@ endif
 GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
 GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
 GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
-BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
+GIT-USER-AGENT: FORCE
+	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+		echo >&2 "    * new user-agent flag"; \
+		echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
+	fi
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
@@ -1990,8 +1994,10 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+version.sp version.s version.o: GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	'-DGIT_VERSION="$(GIT_VERSION)"'
+	'-DGIT_VERSION="$(GIT_VERSION)"' \
+	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -2722,6 +2728,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
+	$(RM) GIT-USER-AGENT
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 07/11] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

Most of the build targets do not care about the setting of
$prefix (or its derivative variables), but will be rebuilt
if the prefix changes. For most setups this doesn't matter
(they set prefix once and never change it), but for a setup
which puts each branch or version in its own prefix, this
unnecessarily causes a full rebuild whenever the branc is
changed.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore |  1 +
 Makefile   | 30 ++++++++++++++++++++++--------
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7329cfe..c60c5a3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-PREFIX
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index 7518ad7..957b6a6 100644
--- a/Makefile
+++ b/Makefile
@@ -1977,6 +1977,7 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
+git.sp git.s git.o: GIT-PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -1988,7 +1989,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
@@ -2035,7 +2036,7 @@ $(SCRIPT_LIB) : % : %.sh
 ifndef NO_PERL
 $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 
-perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
+perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
@@ -2079,7 +2080,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh
 endif # NO_PERL
 
 ifndef NO_PYTHON
-$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
+$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX
 $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
@@ -2248,20 +2249,25 @@ xdiff-interface.o $(XDIFF_OBJS): $(XDIFF_H)
 $(VCSSVN_OBJS) $(VCSSVN_TEST_OBJS): $(LIB_H) $(VCSSVN_H)
 endif
 
+exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
 	'-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
 	'-DBINDIR="$(bindir_relative_SQ)"' \
 	'-DPREFIX="$(prefix_SQ)"'
 
+builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
 	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
 
+config.sp config.s config.o: GIT-PREFIX
 config.sp config.s config.o: EXTRA_CPPFLAGS = \
 	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
 
+attr.sp attr.s attr.o: GIT-PREFIX
 attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
 	-DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
 
+gettext.sp gettext.s gettext.o: GIT-PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
 	-DGIT_LOCALE_PATH='"$(localedir_SQ)"'
 
@@ -2385,14 +2391,22 @@ cscope:
 	$(FIND_SOURCE_FILES) | xargs cscope -b
 
 ### Detect prefix changes
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
-             $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
-             $(localedir_SQ):$(USE_GETTEXT_SCHEME)
+TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
+		$(localedir_SQ)
+
+GIT-PREFIX: FORCE
+	@FLAGS='$(TRACK_PREFIX)'; \
+	if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
+		echo 1>&2 "    * new prefix flags"; \
+		echo "$$FLAGS" >GIT-PREFIX; \
+	fi
+
+TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)
 
 GIT-CFLAGS: FORCE
 	@FLAGS='$(TRACK_CFLAGS)'; \
 	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
-		echo 1>&2 "    * new build flags or prefix"; \
+		echo 1>&2 "    * new build flags"; \
 		echo "$$FLAGS" >GIT-CFLAGS; \
             fi
 
@@ -2728,7 +2742,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
-	$(RM) GIT-USER-AGENT
+	$(RM) GIT-USER-AGENT GIT-PREFIX
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 08/11] Makefile: do not replace @@GIT_VERSION@@ in shell scripts

[Jeff King]

No shell script actually uses the replacement (it is used in
some perl scripts, but cmd_munge_script only handles shell
scripts). We can also therefore drop the dependency on
GIT-VERSION-FILE.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Makefile b/Makefile
index 957b6a6..b977903 100644
--- a/Makefile
+++ b/Makefile
@@ -2016,7 +2016,6 @@ $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
     -e 's|@@DIFF@@|$(DIFF_SQ)|' \
-    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
     -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
@@ -2062,7 +2061,6 @@ gitweb:
 git-instaweb: git-instaweb.sh gitweb
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
 	    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
 	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
@@ -2111,7 +2109,6 @@ configure: configure.ac
 
 # These can record GIT_VERSION
 version.o git.spec \
-	$(patsubst %.sh,%,$(SCRIPT_SH)) \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
 
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 09/11] Makefile: update scripts when build-time parameters change

[Jeff King]

Currently, running:

  make SHELL_PATH=/bin/bash &&
  make SHELL_PATH=/bin/sh

will not rebuild any shell scripts in the second command,
leading to incorrect results when building from an unclean
working directory.

This patch introduces a new dependency meta-file to notice
the change.

Signed-off-by: Jeff King <peff@peff.net>
---
 .gitignore |  1 +
 Makefile   | 16 +++++++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/.gitignore b/.gitignore
index c60c5a3..6535cd7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@
 /GIT-LDFLAGS
 /GIT-GUI-VARS
 /GIT-PREFIX
+/GIT-SCRIPT-DEFINES
 /GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
diff --git a/Makefile b/Makefile
index b977903..ea55179 100644
--- a/Makefile
+++ b/Makefile
@@ -2011,6 +2011,8 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
+SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
+	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -2023,12 +2025,20 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     $@.sh >$@+
 endef
 
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
+GIT-SCRIPT-DEFINES: FORCE
+	@FLAGS='$(SCRIPT_DEFINES)'; \
+	    if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
+		echo 1>&2 "    * new script parameters"; \
+		echo "$$FLAGS" >$@; \
+            fi
+
+
+$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
-$(SCRIPT_LIB) : % : %.sh
+$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
 	$(QUIET_GEN)$(cmd_munge_script) && \
 	mv $@+ $@
 
@@ -2739,7 +2749,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
-	$(RM) GIT-USER-AGENT GIT-PREFIX
+	$(RM) GIT-USER-AGENT GIT-PREFIX GIT-SCRIPT-DEFINES
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 10/11] Makefile: build instaweb similar to other scripts

[Jeff King]

Instaweb would not properly rebuild if the build-time
parameters changed. Fix this by depending on the
GIT-SCRIPT-DEFINES meta-file and using $(cmd_munge_script)
like all the other shell scripts. This requires adding a few
new parametres to cmd_munge_script, but that doesn't hurt
existing scripts.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index ea55179..592e6cb 100644
--- a/Makefile
+++ b/Makefile
@@ -2012,7 +2012,8 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
-	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ)
+	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
+	$(gitwebdir_SQ):$(PERL_PATH_SQ)
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -2022,6 +2023,8 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
     -e $(BROKEN_PATH_FIX) \
+    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
+    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
     $@.sh >$@+
 endef
 
@@ -2068,13 +2071,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 gitweb:
 	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
 
-git-instaweb: git-instaweb.sh gitweb
-	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
-	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    -e 's|@@GITWEBDIR@@|$(gitwebdir_SQ)|g' \
-	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
-	    $@.sh > $@+ && \
+git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES
+	$(QUIET_GEN)$(cmd_munge_script) && \
 	chmod +x $@+ && \
 	mv $@+ $@
 else # NO_PERL
-- 
1.7.11.5.gc0eeaa8

[PATCHv3 11/11] Makefile: move GIT-VERSION-FILE dependencies closer to use

[Jeff King]

There is a list of all of the targets which depend on
GIT-VERSION-FILE, but it can be quite far from the actual
point where the targets actually use $(GIT_VERSION). This
can make it hard to verify that each use of $(GIT_VERSION)
has a matching dependency.

This patch moves the dependency closer to the actual build
instructions, which makes verification easier.  This also
fixes the generation of "configure", which did not properly
mark the dependency.

Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 592e6cb..6b8bfa4 100644
--- a/Makefile
+++ b/Makefile
@@ -1995,7 +1995,7 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
-version.sp version.s version.o: GIT-USER-AGENT
+version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
 	'-DGIT_VERSION="$(GIT_VERSION)"' \
 	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
@@ -2051,7 +2051,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
-$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
+$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
 	sed -e '1{' \
@@ -2108,18 +2108,13 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : unimplemented.sh
 	mv $@+ $@
 endif # NO_PYTHON
 
-configure: configure.ac
+configure: configure.ac GIT-VERSION-FILE
 	$(QUIET_GEN)$(RM) $@ $<+ && \
 	sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    $< > $<+ && \
 	autoconf -o $@ $<+ && \
 	$(RM) $<+
 
-# These can record GIT_VERSION
-version.o git.spec \
-	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
-	: GIT-VERSION-FILE
-
 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
 GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
 	git.o
@@ -2663,7 +2658,7 @@ quick-install-html:
 
 ### Maintainer's dist rules
 
-git.spec: git.spec.in
+git.spec: git.spec.in GIT-VERSION-FILE
 	sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > $@+
 	mv $@+ $@
 
-- 
1.7.11.5.gc0eeaa8

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder]

Jeff King wrote:

> Here's an updated series that drops these lines and I hope will address
> the commit message issues you brought up:
>
>  [01/11]: Makefile: sort LIB_H list
>  [02/11]: Makefile: fold MISC_H into LIB_H

Please no.  Progress would be teaching the remaining compilers to
autocompute header dependencies so there would be no need to maintain
a master header list at all. I would understand if the headers had
been falling out of date and this were a way of saying "we give up",
but given that that is not happening, why would this change be a good
thing?

My comments were only about explaining what I found hard to understand
in the commit message. The patch was good. Please don't retaliate this
way. :)

Hope that helps,
Jonathan

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Wed, Jun 20, 2012 at 02:30:40PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Here's an updated series that drops these lines and I hope will address
> > the commit message issues you brought up:
> >
> >  [01/11]: Makefile: sort LIB_H list
> >  [02/11]: Makefile: fold MISC_H into LIB_H
> 
> Please no.  Progress would be teaching the remaining compilers to
> autocompute header dependencies so there would be no need to maintain
> a master header list at all. I would understand if the headers had
> been falling out of date and this were a way of saying "we give up",
> but given that that is not happening, why would this change be a good
> thing?

Did you read the argument in patch 2? They are almost certainly not
helping anyone, anyway. Forward progress would be to split all of LIB_H
out into specific targets. But keeping that up to date manually is
insanity, so we have this silly half-hearted attempt.

I would much rather "include header-deps.mk", and then periodically
rebuild and commit it using "gcc -M" (and cleaning up and canonicalizing
the result) for the benefit of those without access to a compiler that
can do header dependencies.

In an ideal world, all compilers could do it, but I am not volunteering
to patch MSVC. ;)

> My comments were only about explaining what I found hard to understand
> in the commit message. The patch was good. Please don't retaliate this
> way. :)

:) The revised commit message in patch 4 should make sense with or
without these patches, so it really is a separate issue.

-Peff

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder]

Jeff King wrote:

> Did you read the argument in patch 2? They are almost certainly not
> helping anyone, anyway.

Yes, I read patch 2. I hacked on git from time to time in the days
before COMPUTE_HEADER_DEPENDENCIES, and it sometimes involved changing
header files. When they were not in LIB_H, the experience was much
nicer.

Is that called "not helping"? I'm afraid I don't follow this line of
argument at all.

On the other hand, if someone were proposing adding a simple awk
script to implement a "make dep" fallback, I would understand that.

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Wed, Jun 20, 2012 at 02:45:31PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Did you read the argument in patch 2? They are almost certainly not
> > helping anyone, anyway.
> 
> Yes, I read patch 2. I hacked on git from time to time in the days
> before COMPUTE_HEADER_DEPENDENCIES, and it sometimes involved changing
> header files. When they were not in LIB_H, the experience was much
> nicer.
> 
> Is that called "not helping"? I'm afraid I don't follow this line of
> argument at all.

I just assumed that people who are actively hacking on individual header
files in git actually have a compiler that can do COMPUTE_HEADER_DEPENDENCIES.
Maybe that is not the case. If it were such a big deal, then why is
everything in LIB_H? Why don't people use these manual rules, or convert
existing LIB_H entries to use them?

For people who are not actively hacking on header files in git, the
arguments from that patch apply (namely that LIB_H is so gigantic that
you are unlikely to hit a specific change where one of the few manual
rules is triggered, but LIB_H is not).

> On the other hand, if someone were proposing adding a simple awk
> script to implement a "make dep" fallback, I would understand that.

I'd be OK with that. Do you have one in mind, or do we need to write it
from scratch? Surely somebody else has solved this problem before.

-Peff

Re: [PATCHv3 01/11] Makefile: sort LIB_H list

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This was mostly sorted already, but put things like
> "cache-tree.h" after "cache.h", even though "-" comes before
> "." (at least in the C locale). This will make it easier to
> keep the list sorted later by piping it through "sort".

I agree this would make it easier to blindly run "sort", but I think
the result hurts scannability.  Is it a good change?

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 62de0b4..72cdb56 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -595,8 +595,8 @@ LIB_H += attr.h
>  LIB_H += blob.h
>  LIB_H += builtin.h
>  LIB_H += bulk-checkin.h
> -LIB_H += cache.h
>  LIB_H += cache-tree.h
> +LIB_H += cache.h
>  LIB_H += color.h
>  LIB_H += commit.h
>  LIB_H += compat/bswap.h
> @@ -636,13 +636,13 @@ LIB_H += mailmap.h
>  LIB_H += merge-file.h
>  LIB_H += merge-recursive.h
>  LIB_H += mergesort.h
> -LIB_H += notes.h
>  LIB_H += notes-cache.h
>  LIB_H += notes-merge.h
> +LIB_H += notes.h
>  LIB_H += object.h
> -LIB_H += pack.h
>  LIB_H += pack-refs.h
>  LIB_H += pack-revindex.h
> +LIB_H += pack.h
>  LIB_H += parse-options.h
>  LIB_H += patch-ids.h
>  LIB_H += pkt-line.h
> @@ -668,8 +668,8 @@ LIB_H += submodule.h
>  LIB_H += tag.h
>  LIB_H += thread-utils.h
>  LIB_H += transport.h
> -LIB_H += tree.h
>  LIB_H += tree-walk.h
> +LIB_H += tree.h
>  LIB_H += unpack-trees.h
>  LIB_H += userdiff.h
>  LIB_H += utf8.h

Re: [PATCHv3 01/11] Makefile: sort LIB_H list

[Jeff King]

On Wed, Jun 20, 2012 at 01:00:01PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This was mostly sorted already, but put things like
> > "cache-tree.h" after "cache.h", even though "-" comes before
> > "." (at least in the C locale). This will make it easier to
> > keep the list sorted later by piping it through "sort".
> 
> I agree this would make it easier to blindly run "sort", but I think
> the result hurts scannability.  Is it a good change?

I don't personally see a difference, but if you do, I don't care that
much about this patch. The main motive was that the next patch involved
adding 10 new entries, and I didn't want to have to insert them
manually into the correct sorting spots.

-Peff

Re: [PATCHv3 02/11] Makefile: fold MISC_H into LIB_H

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Let's just fold MISC_H into LIB_H and get rid of these
> manual rules. The worst case is some extra compilation, but
> even that is unlikely to matter due to the reasons above.

Yay.  Thanks.

Re: [PATCHv3 05/11] Makefile: do not replace @@GIT_USER_AGENT@@ in scripts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> No scripts actually care about this replacement. This was
> erroneously added by d937411.

d937411???

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Makefile | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index d2112f8..336cfc4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2010,7 +2010,6 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
>      -e 's|@@DIFF@@|$(DIFF_SQ)|' \
>      -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
> -    -e 's|@@GIT_USER_AGENT@@|$(GIT_USER_AGENT_SQ)|g' \
>      -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
>      -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
>      -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \

Re: [PATCHv3 05/11] Makefile: do not replace @@GIT_USER_AGENT@@ in scripts

[Jeff King]

On Wed, Jun 20, 2012 at 01:06:20PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > No scripts actually care about this replacement. This was
> > erroneously added by d937411.
> 
> d937411???

Whoops. That's the version in my local repo (from which I sent a patch
that you applied). The version in your repo is 42dcbb7.

-Peff

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's an updated series that drops these lines and I hope will address
> the commit message issues you brought up:
>
>   [01/11]: Makefile: sort LIB_H list
>   [02/11]: Makefile: fold MISC_H into LIB_H
>
> New in this iteration to get rid of these largely pointless manual
> dependencies.
>
>   [03/11]: Makefile: do not have git.o depend on common-cmds.h
>
> New in this iteration.  I noticed while double-checking that this
> dependency is pointless.
>
>   [04/11]: Makefile: apply dependencies consistently to sparse/asm targets
>
> Updated based on earlier patches, and with a new commit message
> explaining a little more of what's going on.
>
>   [05/11]: Makefile: do not replace @@GIT_USER_AGENT@@ in scripts
>   [06/11]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
>   [07/11]: Makefile: split prefix flags from GIT-CFLAGS
>   [08/11]: Makefile: do not replace @@GIT_VERSION@@ in shell scripts
>   [09/11]: Makefile: update scripts when build-time parameters change
>   [10/11]: Makefile: build instaweb similar to other scripts
>   [11/11]: Makefile: move GIT-VERSION-FILE dependencies closer to use
>
> The rest are largely the same, but with a few minor textual updates to
> accomodate the earlier changes.

Looks good modulo a minor nit.  Will queue.

Thanks.

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder]

Jeff King wrote:

> I just assumed that people who are actively hacking on individual header
> files in git actually have a compiler that can do COMPUTE_HEADER_DEPENDENCIES.

That's probably true.  And it is presumably possible to implement
COMPUTE_HEADER_DEPENDENCIES for Solaris cc and MSVC, so people using
those compilers would just have an incentive to do that sooner.

So it's not all that bad.

> Maybe that is not the case. If it were such a big deal, then why is
> everything in LIB_H? Why don't people use these manual rules, or convert
> existing LIB_H entries to use them?

Once a header is included by cache.h (like most headers in LIB_H),
there is not much hope for avoiding recompilations when it changes.

> For people who are not actively hacking on header files in git, the
> arguments from that patch apply (namely that LIB_H is so gigantic that
> you are unlikely to hit a specific change where one of the few manual
> rules is triggered, but LIB_H is not).

Unless they are bisecting, it would not be so bad for such people to
effectively have to run "make clean" between compiles, as you've
hinted.  They are not the people it is possible to easily improve
build performance for.

>> On the other hand, if someone were proposing adding a simple awk
>> script to implement a "make dep" fallback, I would understand that.
>
> I'd be OK with that. Do you have one in mind, or do we need to write it
> from scratch? Surely somebody else has solved this problem before.

There are lots of "make dep" implementations out there, but it's hard
to care enough to choose between them. :)  No one who actually doesn't
use gcc has spoken up as caring.  So if we're really feeling the pain
of maintaining the detailed COMPUTE_HEADER_DEPENDENCIES=no fallback
dependencies, let's just say so and drop them like you've suggested.

Before I thought you were saying "nobody is going to notice".  And I'm
pretty sure that's not true.  What I missed before is that a different
statement holds, namely "sure, some people will notice, but they have
an easy way to move forward and the outcome would be much better than
the status quo".

Sorry to be so dense before.

Jonathan

Re: [PATCHv3 02/11] Makefile: fold MISC_H into LIB_H

[Jonathan Nieder]

Jeff King wrote:

> Let's just fold MISC_H into LIB_H and get rid of these
> manual rules. The worst case is some extra compilation, but
> even that is unlikely to matter due to the reasons above.

Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?

Re: [PATCHv3 03/11] Makefile: do not have git.o depend on common-cmds.h

[Jonathan Nieder]

Jeff King wrote:

> This dependency has been stale since 70827b1 (Split up
> builtin commands into separate files from git.c, 2006-04-21).

Good catch, thanks.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCHv3 04/11] Makefile: apply dependencies consistently to sparse/asm targets

[Jonathan Nieder]

Jeff King wrote:

>                                                      These
> flags may sometimes require extra dependencies to be added
> (e.g., like GIT-VERSION-FILE; this is not the case for any
> of the updated lines in this patch, but it is establishing a
> style that will be used in later patches).
[...] 
> This patch explicitly does not update the static header
> dependencies used when COMPUTED_HEADER_DEPENDENCIES is off.

I think you are asking the commit message to do more work than it
needs to, and to answer questions that no one just trying to
understand the patch would ask. :)

Wouldn't it be simpler to put the ground rules in a comment or a
document somewhere under Documentation/ where they can be easily
found?

Hope that helps,
Jonathan

Re: [PATCHv3 06/11] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jonathan Nieder]

Jeff King wrote:

> +	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
> +		echo >&2 "    * new user-agent flag"; \

Micronit: the term "new user-agent string" would be clearer.

Re the lack of _HTTP: interesting.  Is the plan to use this for
other protocols, too?

Looks good.
Jonathan

Re: [PATCHv3 07/11] Makefile: split prefix flags from GIT-CFLAGS

[Jonathan Nieder]

Jeff King wrote:

> Most of the build targets do not care about the setting of
> $prefix (or its derivative variables), but will be rebuilt
> if the prefix changes. For most setups this doesn't matter
> (they set prefix once and never change it), but for a setup
> which puts each branch or version in its own prefix, this
> unnecessarily causes a full rebuild whenever the branc is
> changed.

Micronit: s/branc/branch/.

[...]
> @@ -2385,14 +2391,22 @@ cscope:
>  	$(FIND_SOURCE_FILES) | xargs cscope -b
>  
>  ### Detect prefix changes
> -TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
> -             $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
> -             $(localedir_SQ):$(USE_GETTEXT_SCHEME)
> +TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
> +		$(localedir_SQ)
> +
> +GIT-PREFIX: FORCE
> +	@FLAGS='$(TRACK_PREFIX)'; \
> +	if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
> +		echo 1>&2 "    * new prefix flags"; \
> +		echo "$$FLAGS" >GIT-PREFIX; \
> +	fi

Hmm, nothing to do with this particular patch but the above list
includes gitexecdir and not htmldir.  Is there any particular logic
behind the list of variables?

Nit: I think it would be easier to understand a name like GIT-PATHS
(making the absence of htmldir a bug, if not a particularly important
one).  No other complaints. :)

Thanks.
Jonathan

Re: [PATCHv3 11/11] Makefile: move GIT-VERSION-FILE dependencies closer to use

[Jonathan Nieder]

Jeff King wrote:

> There is a list of all of the targets which depend on
> GIT-VERSION-FILE, but it can be quite far from the actual
> point where the targets actually use $(GIT_VERSION). This
> can make it hard to verify that each use of $(GIT_VERSION)
> has a matching dependency.
>
> This patch moves the dependency closer to the actual build
> instructions, which makes verification easier.  This also
> fixes the generation of "configure", which did not properly
> mark the dependency.

Very nice.

Regards,
Jonathan

Re: [PATCHv3 02/11] Makefile: fold MISC_H into LIB_H

[Jeff King]

On Wed, Jun 20, 2012 at 04:07:30PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Let's just fold MISC_H into LIB_H and get rid of these
> > manual rules. The worst case is some extra compilation, but
> > even that is unlikely to matter due to the reasons above.
> 
> Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?

I stopped short of that, but I'd be tempted to do so. I don't think
those variables have any special meaning beyond the recompilation
dependencies.

-Peff

Re: [PATCHv3 04/11] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Wed, Jun 20, 2012 at 04:12:25PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> >                                                      These
> > flags may sometimes require extra dependencies to be added
> > (e.g., like GIT-VERSION-FILE; this is not the case for any
> > of the updated lines in this patch, but it is establishing a
> > style that will be used in later patches).
> [...] 
> > This patch explicitly does not update the static header
> > dependencies used when COMPUTED_HEADER_DEPENDENCIES is off.
> 
> I think you are asking the commit message to do more work than it
> needs to, and to answer questions that no one just trying to
> understand the patch would ask. :)

Yeah, when writing out the full discussion I was awfully tempted to go
with your simplified explanation. ;)

In fact, it's the later commits that really make use of this
explanation when they add lines.

> Wouldn't it be simpler to put the ground rules in a comment or a
> document somewhere under Documentation/ where they can be easily
> found?

I think a comment in the Makefile might make sense (especially if it
introduces the section as "and this is the place to put weird
target-specific cppflags and dependencies"). Would you mind taking a
stab at writing it? I feel like the explanation I wrote in the commit
message ended up quite dense and possibly not very informative, and a
fresh brain and fingers might turn out something a little more
reasonable.

-Peff

Re: [PATCHv3 06/11] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jeff King]

On Wed, Jun 20, 2012 at 04:21:35PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > +	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
> > +		echo >&2 "    * new user-agent flag"; \
> 
> Micronit: the term "new user-agent string" would be clearer.

Probably. I am tempted to get rid of the informative message altogether.
For CFLAGS, you might be confused why everything is being rebuilt, since
the dependency chain is not straightforward. For this, it's kind of
obvious.

> Re the lack of _HTTP: interesting.  Is the plan to use this for
> other protocols, too?

Look at the jk/version-string topic that this is based on. :)

-Peff

Re: [PATCHv3 06/11] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jonathan Nieder]

Jeff King wrote:
>> Jeff King wrote:

>>> +	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
>>> +		echo >&2 "    * new user-agent flag"; \
[...]
>           I am tempted to get rid of the informative message altogether.
> For CFLAGS, you might be confused why everything is being rebuilt, since
> the dependency chain is not straightforward. For this, it's kind of
> obvious.

Makes a lot of sense.

Re: [PATCHv3 07/11] Makefile: split prefix flags from GIT-CFLAGS

[Jeff King]

On Wed, Jun 20, 2012 at 04:28:33PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Most of the build targets do not care about the setting of
> > $prefix (or its derivative variables), but will be rebuilt
> > if the prefix changes. For most setups this doesn't matter
> > (they set prefix once and never change it), but for a setup
> > which puts each branch or version in its own prefix, this
> > unnecessarily causes a full rebuild whenever the branc is
> > changed.
> 
> Micronit: s/branc/branch/.

The worst part is that Junio already corrected that in v1 and I didn't
propagate it into my re-roll. <sigh>

> >  ### Detect prefix changes
> > -TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):\
> > -             $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
> > -             $(localedir_SQ):$(USE_GETTEXT_SCHEME)
> > +TRACK_PREFIX = $(bindir_SQ):$(gitexecdir_SQ):$(template_dir_SQ):$(prefix_SQ):\
> > +		$(localedir_SQ)
> > +
> > +GIT-PREFIX: FORCE
> > +	@FLAGS='$(TRACK_PREFIX)'; \
> > +	if test x"$$FLAGS" != x"`cat GIT-PREFIX 2>/dev/null`" ; then \
> > +		echo 1>&2 "    * new prefix flags"; \
> > +		echo "$$FLAGS" >GIT-PREFIX; \
> > +	fi
> 
> Hmm, nothing to do with this particular patch but the above list
> includes gitexecdir and not htmldir.  Is there any particular logic
> behind the list of variables?

The list came from what was in CFLAGS before. But looking at it again, I
think it is not right (e.g., git.o builds with $(htmldir_SQ), but nobody
actually depends on it). IOW, it was broken before, and I have
propagated that breakage. But nobody has noticed because they usually
set prefix and not the htmldir separately.

I probably need to go through the whole Makefile again and make sure
everything is in the list.  What a royal pain. I can't help but feel
that there is a better way to do this. GNU make supports user-defined
functions via the "call" function. Maybe we can turn this into a
single-line of Makefile per variable, and get per-variable resolution of
rebuilding, and still end up with fewer lines.

I'll take a look.

> Nit: I think it would be easier to understand a name like GIT-PATHS
> (making the absence of htmldir a bug, if not a particularly important
> one).  No other complaints. :)

Yeah, I hate the name GIT-PREFIX. But I was worried people might
misinterpret GIT-PATHS. Maybe these should all go into a subdir full of
auto-built file magic, which will make it more obvious what they are
(and we would want to do that anyway if we start having a larger number
of them).

-Peff

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>>   [01/11]: Makefile: sort LIB_H list
>>   [02/11]: Makefile: fold MISC_H into LIB_H
>>   [03/11]: Makefile: do not have git.o depend on common-cmds.h
>>   [04/11]: Makefile: apply dependencies consistently to sparse/asm targets
>>   [05/11]: Makefile: do not replace @@GIT_USER_AGENT@@ in scripts
>>   [06/11]: Makefile: split GIT_USER_AGENT from GIT-CFLAGS
>>   [07/11]: Makefile: split prefix flags from GIT-CFLAGS
>>   [08/11]: Makefile: do not replace @@GIT_VERSION@@ in shell scripts
>>   [09/11]: Makefile: update scripts when build-time parameters change
>>   [10/11]: Makefile: build instaweb similar to other scripts
>>   [11/11]: Makefile: move GIT-VERSION-FILE dependencies closer to use
>
> Looks good modulo a minor nit.  Will queue.

I hate saying "me too", so instead I will say: I agree!

And thanks for this massive cleanup in response to a small nit...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Jeff King]

On Wed, Jun 20, 2012 at 01:10:19PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Here's an updated series that drops these lines and I hope will address
> > the commit message issues you brought up:
> [...]
> 
> Looks good modulo a minor nit.  Will queue.

You probably noticed a lot of nice review from Jonathan. I had hoped to
do another round exploring some alternatives we discussed, but didn't
get to it tonight, and now I'm going to be away from email for about 5
days.

I don't think there is anything _wrong_ in the series as it is, and
certainly the things we talked about changing could be built on top. So
if you want to hold it in pu for a week, I can revisit it next week. But
I'd also be just as happy to see it move to next, and revisit the topic
later by building on top.

-Peff

Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You probably noticed a lot of nice review from Jonathan. I had hoped to
> do another round exploring some alternatives we discussed, but didn't
> get to it tonight, and now I'm going to be away from email for about 5
> days.
>
> I don't think there is anything _wrong_ in the series as it is, and
> certainly the things we talked about changing could be built on top. So
> if you want to hold it in pu for a week, I can revisit it next week. But
> I'd also be just as happy to see it move to next, and revisit the topic
> later by building on top.

I'd keep it in 'pu'; it may be replaced with Jonathan's reroll if it
comes.

Have fun, whereever you are.

Automatic dependency tracking in the Git build system (was: Re: [PATCHv2 1/8] Makefile: apply dependencies consistently to sparse/asm targets)

[Stefano Lattarini]

[Adding the Automake list in CC:]

On 06/20/2012 09:57 PM, Jeff King wrote:
> On Wed, Jun 20, 2012 at 02:45:31PM -0500, Jonathan Nieder wrote:
> 
>> Jeff King wrote:
>>
>>> Did you read the argument in patch 2? They are almost certainly not
>>> helping anyone, anyway.
>>
>> Yes, I read patch 2. I hacked on git from time to time in the days
>> before COMPUTE_HEADER_DEPENDENCIES, and it sometimes involved changing
>> header files. When they were not in LIB_H, the experience was much
>> nicer.
>>
>> Is that called "not helping"? I'm afraid I don't follow this line of
>> argument at all.
> 
> I just assumed that people who are actively hacking on individual header
> files in git actually have a compiler that can do COMPUTE_HEADER_DEPENDENCIES.
> Maybe that is not the case. If it were such a big deal, then why is
> everything in LIB_H? Why don't people use these manual rules, or convert
> existing LIB_H entries to use them?
> 
> For people who are not actively hacking on header files in git, the
> arguments from that patch apply (namely that LIB_H is so gigantic that
> you are unlikely to hit a specific change where one of the few manual
> rules is triggered, but LIB_H is not).
> 
>> On the other hand, if someone were proposing adding a simple awk
>> script to implement a "make dep" fallback, I would understand that.
> 
> I'd be OK with that. Do you have one in mind, or do we need to write it
> from scratch? Surely somebody else has solved this problem before.
>

[begin shameless plug]

Have you taken a look at the 'depcomp' script that comes with Automake?
 <http://git.savannah.gnu.org/cgit/automake.git/tree/lib/depcomp>
Once you get past some of its idiosyncrasies and few historical warts,
it has a lot of built-in knowledge about automatic dependency tracking
for a lot of different compilers.

[end shameless plug]

HTH,
  Stefano

Re: [PATCHv3 02/11] Makefile: fold MISC_H into LIB_H

[Jonathan Nieder]

Hi,

I finally found some moments to revisit this series, so I'm starting
here.  I think the justification for this patch is something like
this:

  Keeping track of what files include each header is an error-prone
  chore.  On top of that, for l10n, these days we have to keep a master
  list of all headers, too, which is double work when adding a new
  header that adds insult to injury.

  Active Makefile hackers tend to use compilers like gcc that support
  automatic dependency generation with -MMD.  The precise header deps
  aren't even used when building with these compilers, so the people
  maintaining the precise header deps do not benefit from them at all.
  Unfair!

  Non-developers who can't fend for themselves do not rebuild after a
  small header change very often, so they would not be hurt much by a
  "change one header, rebuild everything" rule when automatic
  dependency generation is disabled, either.

  That leaves at least one important category of people to be hurt by
  this change: the glorious MSVC hackers.  MSVC supports the
  appropriate magic to compute header dependencies, but no one's
  gotten around to teaching the Makefile to use it yet.  So let's stop
  delaying the inevitable and drop the detailed dependencies.  If
  anyone complains then we can work with them to finish support for
  computing header dependencies for the relevant compiler.

Fair enough.  

Two details puzzle me:

Jeff King wrote:

>                                 The original point
> of LIB_H was that it would force recompilation of C files
> when any of the library headers changed.

LIB_H was introduced by commit e590d694 (Add more header dependencies,
2005-04-18).  It only lists

	cache.h
	object.h

even though some translation units included tree.h, commit.h, or
blob.h already.  So at least back then, it seems to have been about
library headers and not about all headers (and "all headers" was
puzzlingly not worth worrying about at all).

So isn't this a fundamentally new thing, rather than a return to the
state of nature?

The other remaining question is why we don't use something like
$(wildcard *.h) and avoid listing individual headers altogether.
Is the fear that some stray non-git header will find its way into
the cwd and poison the translation files?  (If so, I'd like to
document that as well to help readers understand why we keep doing
the work we do.)

Ciao,
Jonathan

[PATCH 02.5/11] Makefile: fold XDIFF_H and VCSSVN_H into LIB_H

[Jonathan Nieder]

Just like MISC_H (see previous commit), there is no reason to track
xdiff and vcs-svn headers separately from the rest of the headers.
The only purpose of these variables is to keep track of recompilation
dependencies.

As a pleasant side effect, folding these into LIB_H lets us stop
tracking GIT_OBJS and VCSSVN_TEST_OBJS separately from the list of all
OBJECTS.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:
> On Wed, Jun 20, 2012 at 04:07:30PM -0500, Jonathan Nieder wrote:

>> Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?
>
> I stopped short of that, but I'd be tempted to do so.

Here goes.

 Makefile |   60 ++++++++++++++++++++++++------------------------------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/Makefile b/Makefile
index 500966b1..b24ca20d 100644
--- a/Makefile
+++ b/Makefile
@@ -392,11 +392,8 @@ BUILTIN_OBJS =
 BUILT_INS =
 COMPAT_CFLAGS =
 COMPAT_OBJS =
-XDIFF_H =
 XDIFF_OBJS =
-VCSSVN_H =
 VCSSVN_OBJS =
-VCSSVN_TEST_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
 LIB_H =
@@ -558,21 +555,21 @@ LIB_FILE=libgit.a
 XDIFF_LIB=xdiff/lib.a
 VCSSVN_LIB=vcs-svn/lib.a
 
-XDIFF_H += xdiff/xinclude.h
-XDIFF_H += xdiff/xmacros.h
-XDIFF_H += xdiff/xdiff.h
-XDIFF_H += xdiff/xtypes.h
-XDIFF_H += xdiff/xutils.h
-XDIFF_H += xdiff/xprepare.h
-XDIFF_H += xdiff/xdiffi.h
-XDIFF_H += xdiff/xemit.h
+LIB_H += xdiff/xinclude.h
+LIB_H += xdiff/xmacros.h
+LIB_H += xdiff/xdiff.h
+LIB_H += xdiff/xtypes.h
+LIB_H += xdiff/xutils.h
+LIB_H += xdiff/xprepare.h
+LIB_H += xdiff/xdiffi.h
+LIB_H += xdiff/xemit.h
 
-VCSSVN_H += vcs-svn/line_buffer.h
-VCSSVN_H += vcs-svn/sliding_window.h
-VCSSVN_H += vcs-svn/repo_tree.h
-VCSSVN_H += vcs-svn/fast_export.h
-VCSSVN_H += vcs-svn/svndiff.h
-VCSSVN_H += vcs-svn/svndump.h
+LIB_H += vcs-svn/line_buffer.h
+LIB_H += vcs-svn/sliding_window.h
+LIB_H += vcs-svn/repo_tree.h
+LIB_H += vcs-svn/fast_export.h
+LIB_H += vcs-svn/svndiff.h
+LIB_H += vcs-svn/svndump.h
 
 GENERATED_H += common-cmds.h
 
@@ -2110,13 +2107,6 @@ version.o git.spec \
 	$(patsubst %.perl,%,$(SCRIPT_PERL)) \
 	: GIT-VERSION-FILE
 
-TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
-GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
-	git.o
-ifndef NO_CURL
-	GIT_OBJS += http.o http-walker.o remote-curl.o
-endif
-
 XDIFF_OBJS += xdiff/xdiffi.o
 XDIFF_OBJS += xdiff/xprepare.o
 XDIFF_OBJS += xdiff/xutils.o
@@ -2132,9 +2122,14 @@ VCSSVN_OBJS += vcs-svn/fast_export.o
 VCSSVN_OBJS += vcs-svn/svndiff.o
 VCSSVN_OBJS += vcs-svn/svndump.o
 
-VCSSVN_TEST_OBJS += test-line-buffer.o
-
-OBJECTS := $(GIT_OBJS) $(XDIFF_OBJS) $(VCSSVN_OBJS)
+TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
+OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
+	$(XDIFF_OBJS) \
+	$(VCSSVN_OBJS) \
+	git.o
+ifndef NO_CURL
+	OBJECTS += http.o http-walker.o remote-curl.o
+endif
 
 dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
 dep_dirs := $(addsuffix .depend,$(sort $(dir $(OBJECTS))))
@@ -2233,15 +2228,8 @@ else
 # Dependencies on automatically generated headers such as common-cmds.h
 # should _not_ be included here, since they are necessary even when
 # building an object for the first time.
-#
-# XXX. Please check occasionally that these include all dependencies
-# gcc detects!
 
-$(GIT_OBJS): $(LIB_H)
-
-xdiff-interface.o $(XDIFF_OBJS): $(XDIFF_H)
-
-$(VCSSVN_OBJS) $(VCSSVN_TEST_OBJS): $(LIB_H) $(VCSSVN_H)
+$(OBJECTS): $(LIB_H)
 endif
 
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
@@ -2334,7 +2322,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 	--keyword=_ --keyword=N_ --keyword="Q_:1,2"
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
-LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(XDIFF_H) $(VCSSVN_H) $(GENERATED_H)
+LOCALIZED_C := $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH := $(SCRIPT_SH)
 LOCALIZED_PERL := $(SCRIPT_PERL)
 
-- 
1.7.10.4

[PATCH/RFC] Makefile: document ground rules for target-specific dependencies

[Jonathan Nieder]

When a source file makes use of a makefile variable, there should be a
corresponding dependency on a file that changes when that variable
changes to ensure the build output is not left stale when the variable
changes.

Document this, even though we are not following the rule perfectly
yet.  Based on an explanation from Jeff King.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:
> On Wed, Jun 20, 2012 at 04:12:25PM -0500, Jonathan Nieder wrote:

>> Wouldn't it be simpler to put the ground rules in a comment or a
>> document somewhere under Documentation/ where they can be easily
>> found?
>
> I think a comment in the Makefile might make sense (especially if it
> introduces the section as "and this is the place to put weird
> target-specific cppflags and dependencies").

How about something like this?

 Makefile |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Makefile b/Makefile
index 3f82b51b..542856f0 100644
--- a/Makefile
+++ b/Makefile
@@ -1970,6 +1970,40 @@ shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
+
+### Target-specific flags and dependencies
+
+# The generic compilation pattern rule and automatically
+# computed header dependencies (falling back to a dependency on
+# LIB_H) are enough to describe how most targets should be built,
+# but some targets are special enough to need something a little
+# different.
+#
+# - When a source file "foo.c" #includes a generated header file,
+#   we need to list that dependency for the "foo.o" target.
+#
+#   We also list it from other targets that are built from foo.c
+#   like "foo.sp" and "foo.s", even though that is easy to forget
+#   to do because the generated header is already present around
+#   after a regular build attempt.
+#
+# - Some code depends on configuration kept in makefile
+#   variables. The target-specific variable EXTRA_CPPFLAGS can
+#   be used to convey that information to the C preprocessor
+#   using -D options.
+#
+#   The "foo.o" target should have a corresponding dependency on
+#   a file that changes when the value of the makefile variable
+#   changes.  For example, targets making use of the
+#   $(GIT_VERSION) variable depend on GIT-VERSION-FILE.
+#
+#   Technically the ".sp" and ".s" targets do not need this
+#   dependency because they are force-built, but they get the
+#   same dependency for consistency. This way, you do not have to
+#   know how each target is implemented. And it means the
+#   dependencies here will not need to change if the force-build
+#   details change some day.
+
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
-- 
1.7.10.4

[RFC/PATCH v4 06/11] Makefile: split GIT_USER_AGENT from GIT-CFLAGS

[Jonathan Nieder]

The default user-agent depends on the GIT_VERSION, which means that
anytime you switch versions, it causes a full rebuild. Instead, let's
split it out into its own file and restrict the dependency to
version.o.

To avoid noise during builds, unlike the GIT-CFLAGS rule which prints
"* new build flags or prefix" so the operator knows why all files are
being rebuilt when it changes, GIT-USER-AGENT generation is silent.

If this code breaks and a target depending on GIT-USER-AGENT ends up
being rebuilt when it shouldn't be, the full dependency chain can be
retrieved with "make --debug=b".

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

>           I am tempted to get rid of the informative message altogether.

Like this?

 .gitignore |    1 +
 Makefile   |   10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index bf66648e..7329cfe5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 /GIT-CFLAGS
 /GIT-LDFLAGS
 /GIT-GUI-VARS
+/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git
diff --git a/Makefile b/Makefile
index 7148cadd..2a84cd8b 100644
--- a/Makefile
+++ b/Makefile
@@ -1922,7 +1922,10 @@ endif
 GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
 GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
 GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
-BASIC_CFLAGS += -DGIT_USER_AGENT='$(GIT_USER_AGENT_CQ_SQ)'
+GIT-USER-AGENT: FORCE
+	@if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 2>/dev/null`"; then \
+		echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
+	fi
 
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
@@ -2021,8 +2024,10 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+version.sp version.s version.o: GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-	'-DGIT_VERSION="$(GIT_VERSION)"'
+	'-DGIT_VERSION="$(GIT_VERSION)"' \
+	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -2744,6 +2749,7 @@ ifndef NO_TCLTK
 	$(MAKE) -C git-gui clean
 endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-LDFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
+	$(RM) GIT-USER-AGENT
 
 .PHONY: all install profile-clean clean strip
 .PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
-- 
1.7.10.4

Re: [PATCH 02.5/11] Makefile: fold XDIFF_H and VCSSVN_H into LIB_H

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Just like MISC_H (see previous commit), there is no reason to track
> xdiff and vcs-svn headers separately from the rest of the headers.
> The only purpose of these variables is to keep track of recompilation
> dependencies.
>
> As a pleasant side effect, folding these into LIB_H lets us stop
> tracking GIT_OBJS and VCSSVN_TEST_OBJS separately from the list of all
> OBJECTS.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jeff King wrote:
>> On Wed, Jun 20, 2012 at 04:07:30PM -0500, Jonathan Nieder wrote:
>
>>> Should XDIFF_H and VCSSVN_H be folded into STATIC_HEADERS, too?
>>
>> I stopped short of that, but I'd be tempted to do so.
>
> Here goes.

Very tempting.  Will try squeezing this in between 2/11 and 3/11 and
see how the result looks.