Skip to content
This repository was archived by the owner on Nov 9, 2017. It is now read-only.

Maybe merge branch pt/version-resource #10

Closed
wants to merge 1 commit into from

Conversation

patthoyts
Copy link
Member

This branch provides an implementation for issue #5 in adding a git.rc file and the necessary Makefile changes to get this linked into the git executable on Windows with a sensible version number generated from the current GIT-VERSION-GEN file.
This only handles MINGW building - I didn't deal with the Windows build (ie: msvc) nor cygwin.
This also serves as a test of the merge request system over just posting the patch for me.

Embeds the git version and description into the git executable thus
implementing the request in issue #5.

Signed-off-by: Pat Thoyts <[email protected]>
@ghost ghost assigned patthoyts May 22, 2012
@buildhive
Copy link

@dscho
Copy link
Member

dscho commented May 23, 2012

I like it. Let's just do it and let's let people caring about MSVC provide patches to scratch their itch.

BTW bummer on BuildHive. There has been a new problem: apparently, Makefile-based projects created last Sunday cannot be edited anymore. So I deleted the BuildHive "project" and recreated it, with -j5 settings to circumvent the timeout. Let's see how that goes...

BTW you might want to mention in the commit message that this uses two GNU Make-specific functions, join and wordlist (at least AFAICT they are GNU Make specific).

@patthoyts
Copy link
Member Author

I'll check but I figured the use of $(patsubst) all over the place meant the whole makefile was GNU specific. I'll probably get a chance to check the MSVC build rules sometime today - should just be a matter of calling rc appropriately.

@sschuberth
Copy link

Very nice Pat, thank you. I'm also OK with merging this and add MSVC support later. As we're not shipping MSVC binaries this effectively addresses issue #5, so all is fine.

@hvoigt
Copy link
Member

hvoigt commented May 23, 2012

Nice stuff Pat. I also like it.

There is a small whitespace change in the Makefile you probably want to remove before merging.

@dscho
Copy link
Member

dscho commented May 23, 2012

@patthoyts oh, you're probably correct. I just thought that I'd remember vaguely that git.git tries to support SunOS, too, where there is definitely no GNU Make available. Thanks for checking, my concerns have been addressed completely!

@patthoyts
Copy link
Member Author

Merged to devel

@kusma
Copy link
Member

kusma commented May 31, 2012

Shouldn't we have something like this on top also?

diff --git a/Makefile b/Makefile
index c09a679..9b45f60 100644
--- a/Makefile
+++ b/Makefile
@@ -2104,7 +2104,7 @@ configure: configure.ac
    $(RM) $<+

 # These can record GIT_VERSION
-git.o git.spec \
+git.o git.spec git.res \
    $(patsubst %.sh,%,$(SCRIPT_SH)) \
    $(patsubst %.perl,%,$(SCRIPT_PERL)) \
    : GIT-VERSION-FILE

@dscho
Copy link
Member

dscho commented May 31, 2012

@kusma you mean that git.res has to be updated whenever the version changes? If so, yes, I think you're right.

@patthoyts
Copy link
Member Author

I don't think so. I looks to me like git.spec is used as a dependency for things that might record the git version. git.o doesn't need to depend on git.res, but maybe git.res needs to depend on git.spec instead. (ie: git.res: git.rc git.spec) which should catch the version change I think

@kusma
Copy link
Member

kusma commented May 31, 2012

@dscho: Exactly.

@patthoyts: git.spec is needed to build an RPM from a tarball, and it embeds GIT_VERSION as well. But it's not built at all in our setup, so making git.res depend on it makes very little sense. Why not do the same thing as tge rest of files that embed GIT_VERSION does, which is to depend on GIT-VERSION-FILE like my patch above does...?

@patthoyts
Copy link
Member Author

As far as I can see that won't make git.res be rebuilt when the version changes which is needed. So git.res: git.rc GIT-VERSION-FILE then?

@dscho
Copy link
Member

dscho commented May 31, 2012

@patthoyts I read @kusma's patch that way too, at first. But it is not "git.o: ... git.res ..." (note the colon) but "git.o ... git.res ... :". Having said that, it looks clearer to me indeed if the dependency GIT-VERSION-FILE is added to the git.res rule itself.

@kusma
Copy link
Member

kusma commented May 31, 2012

@dscho: Both approaches work, but my patch is kind-of more in the current Makefile-style. But that might be simply because the other files that embeds GIT_VERSION doesn't have explicit rules. So perhaps @patthoyts' suggestion is better, then.

@kusma
Copy link
Member

kusma commented May 31, 2012

Oh, and another reason @patthoyts' approach probably is better is that I very recently submitted an upstream-patch that added http.o to that same line. That's going to cause a conflict with my approach for sure.

@patthoyts
Copy link
Member Author

Ah. I see. I missed the significance of the colon position. Doh.

dscho referenced this pull request in dscho/git Oct 14, 2014
t-b added a commit to t-b/git that referenced this pull request Nov 27, 2014
dscho referenced this pull request in dscho/git Feb 22, 2015
dscho referenced this pull request in dscho/git Jan 25, 2017
We generate the squash commit message incrementally running
a sed script once for each commit. It parses "This is
a combination of <N> commits" from the first line of the
existing message, adds one to <N>, and uses the result as
the number of our current message.

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

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

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

  This is a combination of 10 commits.

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

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

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

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

Reported-by: Brandon Tolsch <[email protected]>
Signed-off-by: Jeff King <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants