Re: git stash while pending merge should not be allowed
On Fri, Jun 07, 2013 at 11:47:07AM -0700, Junio C Hamano wrote: Scott McPeak smcp...@coverity.com writes: I suggest that this problem could easily have been avoided if git stash refused to run with a pending merge (present MERGE_HEAD file), since this is crucial repository state that it does not save. This seems similar to what git cherry-pick does. Sounds senslbe. What do we want to see happen in other states, in which Git gives control back to the user asking for help before moving forward (e.g. am, rebase, cherry-pick, revert)? I don't think there's any need to prevent stash running in these cases and I sometimes find it useful that I can stash during a rebase. Having said that, I wonder what happens with cherry-pick -x if you do stash changes while it is stopped. I don't think that is as serious as the merge case because it's easy to detect in the commit message. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: remove existing output-directory
On Fri, Jun 14, 2013 at 06:13:33PM +0530, Ramkumar Ramachandra wrote: The following command $ git format-patch -o outgoing master does not ensure that the output-directory outgoing doesn't already exist. As a result, it's possible for patches from two different series to get mixed up if the user is not careful. Fix the problem by unconditionally removing the output-directory before writing to it. I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. It may be better to issue a warning when this happens, or die and provide a flag to let the user bypass that. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: remove existing output-directory
On Fri, Jun 14, 2013 at 06:45:19PM +0530, Ramkumar Ramachandra wrote: John Keeping wrote: I don't think this is the correct behaviour. I can think of cases where I would want to output multiple things into the same directory. format.cleanOutputDirectory = true|false? Maybe, but I was thinking of something more like: Output directory is not empty, use --allow-non-empty-dir if you really want to proceed. Using that configuration variable lets someone shoot themselves in the foot quite badly if they forget that they have set it and set the output directory to somewhere containing important data. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] add--interactive: respect diff.algorithm
When staging hunks interactively it is sometimes useful to use an
alternative diff algorithm which splits the changes into hunks in a more
logical manner. This is not possible because the plumbing commands
called by add--interactive ignore the diff.algorithm configuration
option (as they should).
Since add--interactive is a porcelain command it should respect this
configuration variable. To do this, make it read diff.algorithm and
pass its value to the underlying diff-index and diff-files invocations.
At this point, do not add options to git add, git reset or git
checkout (all of which can call git-add--interactive). If a user want
to override the value on the command line they can use:
git -c diff.algorithm=$ALGO ...
Signed-off-by: John Keeping j...@keeping.me.uk
---
On Mon, Jun 10, 2013 at 05:56:56PM -0400, Jeff King wrote:
On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:
Overall, I think respecting diff.algorithm in add--interactive is a very
sane thing to do. I would even be tempted to say we should allow a few
other select diff options (e.g., fewer or more context lines). If you
allowed diff options like this:
git add --patch=--patience -U5
that is very flexible, but I would not want to think about what the code
does when you pass --patch=--raw or equal nonsense.
An alternative would be to permit them to be set from within the
interactive UI. I'd find it quite useful to experiment with various
diff options when I encounter a hunk that isn't as easy to pick as I'd
like. I expect it would be very hard to do that on a per-hunk basis,
although per-file doesn't seem like it would be too hard.
That's an interesting idea, for a subset of options (e.g., increase
context for this hunk). I suspect implementing it would be painful,
though, as you would have to re-run diff, and you have no guarantee of
getting the same set of hunks (e.g., the hunk might end up coalesced
with another).
I think you'd need to re-run the diff over the whole file and then skip
hunks until you reach one that overlaps with the original hunk. But I
suspect it would end up being quite a lot more complicated than that.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d2c4ce6..0b0fac2 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -44,6 +44,8 @@ my ($diff_new_color) =
my $normal_color = $repo-get_color(, reset);
+my $diff_algorithm = ($repo-config('diff.algorithm') or 'default');
+
my $use_readkey = 0;
my $use_termcap = 0;
my %term_escapes;
@@ -731,6 +733,9 @@ sub run_git_apply {
sub parse_diff {
my ($path) = @_;
my @diff_cmd = split( , $patch_mode_flavour{DIFF});
+ if ($diff_algorithm ne default) {
+ push @diff_cmd, --diff-algorithm=${diff_algorithm};
+ }
if (defined $patch_mode_revision) {
push @diff_cmd, $patch_mode_revision;
Yeah, that looks like the sane way to do it to me. As a perl style
thing, I think the usual way of spelling 'default' is 'undef'. I.e.:
my $diff_algorithm = $repo-config('diff.algorithm');
...
if (defined $diff_algorithm) {
push @diff_cmd, --diff-algorithm=$diff_algorithm;
}
OK. The default is actually the value that is equivalent to 'myers'
for diff.algorithm and I was originally going to add --diff-algorithm
to the command line unconditionally.
But I think it's better to add it only when it has been specified so
I've changed it as you suggest.
git-add--interactive.perl | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d2c4ce6..5310959 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -44,6 +44,8 @@ my ($diff_new_color) =
my $normal_color = $repo-get_color(, reset);
+my $diff_algorithm = $repo-config('diff.algorithm');
+
my $use_readkey = 0;
my $use_termcap = 0;
my %term_escapes;
@@ -731,6 +733,9 @@ sub run_git_apply {
sub parse_diff {
my ($path) = @_;
my @diff_cmd = split( , $patch_mode_flavour{DIFF});
+ if (defined $diff_algorithm) {
+ push @diff_cmd, --diff-algorithm=${diff_algorithm};
+ }
if (defined $patch_mode_revision) {
push @diff_cmd, $patch_mode_revision;
}
--
1.8.3.779.g691e267
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: * When reviewing other peoples' code, be tactful and constructive. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. I think this is 30% aimed at me (as I think I do about that much of the reviews around here). I fully agree with most of them, but the last sentence is a bit too fuzzy to be a practically useful guideline. Somebody's bare minimum is somebody else's perfection. An unqualified perfect is the enemy of good is often incorrectly used to justify It works for me. and There already are other codepaths that do it in the same wrong way., both of which make things _worse_ for the long term project health. One thing that I think is missing from these proposals so far is some clear indication that a review should not be confrontational. Consider the following two review comments (taken from a recent example that happened to stick in my mind, but I don't want to single out any one individual here): Ugh, why this roundabout-passive-past tone? Use imperative tone like this: ... vs. We normally use the imperative in commit messages, perhaps like this? ... Both say the same thing but the first immediately puts the submitter on the defensive. If I see something like that on one of my patches I have to consciously resist the urge to reply immediately and instead review what I'm about to send once I've calmed down. I realise that we shouldn't take offence to review comments, but we are all human and it is sometimes hard not to take things personally. In the examples above, the first makes it feel like the submitter is fighting to get a patch included, but the second feels like we're collaborating to get to the best result for the project. As my mother would say, politeness costs nothing ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 08:52:05PM +0200, Michael Haggerty wrote: That's a very good point (and a good illustration, too). How do you like the new second and third sentences below? * When reviewing other peoples' code, be tactful and constructive. Remember that submitting patches for public critique can be very intimidating and when mistakes are found it can be embarrassing. Do what you can to make it a positive and pleasant experience for the submitter. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. I'm not sure. I like the intent, but I'm not sure that it's clear enough that we're talking about the tone of comments rather than the type of feedback to provide. How about something like this? * Having your code reviewed should feel like a collaboration aiming for the best result for the project, not like a fight to get your patch accepted. Try to bear this in mind when reviewing other peoples' code and consider how you would feel reading the same comments if the review was the other way round. We are only human and the tone of a review can influence how the following discussion progresses. * If you do feel that a review is aggressive, don't reply immediately. Contributors are spread around the world in different timezones and it is often better to wait a few hours for others to comment before rushing to defend your patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation/CommunityGuidelines
On Wed, Jun 12, 2013 at 12:16:28AM +0530, Ramkumar Ramachandra wrote: John Keeping wrote: Ugh, why this roundabout-passive-past tone? Use imperative tone like this: ... vs. We normally use the imperative in commit messages, perhaps like this? ... As my mother would say, politeness costs nothing ;-) The review is being honest about her feelings in the first one, and being artificially diplomatic in the second one. I don't think it is artificially diplomatic, it's an attempt to convey a helpful tone in an email. As has been said elsewhere, it is easy to read an email in the wrong tone (there is an oft-cited statistic about the percentage of communication that is non-verbal, and which cannot be inferred from written text). For this reason I think it is important for reviewers to make an effort to minimise the risk that what they write can be interpreted as being aggressive. Both of them are constructive and friendly, in that they provide an example for the submitter to follow. Both provide the same advice, yes. But I disagree that they are both friendly. The top example reads (to me at least) as an attack on the submitter for not knowing better. It may sometimes be necessary to resort to strong wording if someone appears to be wilfully ignoring sensible advice but we should not expect every submitter to know the expectations of the project; the first message to someone should gently guide them in the right direction. Either way, I'm not interested in problems that have no solutions. The only solution I see here is to suffocate every contributor until they are tactful enough for the majority's liking, and remove the ones that don't conform. If you do have an alternate solution, please share it with us. I don't have a solution, only a hope that regular contributors will learn from others how they can phrase review comments less aggressively. I expect different people will read the same statement differently; people are from different cultures and what is considered acceptable in one culture can be considered rude in another. We should aim to cultivate our own culture where we try to minimise the risk that what we write will be misinterpreted by someone with a different cultural background. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/7] git send-email suppress-cc=self fixes
On Mon, Jun 10, 2013 at 09:53:24AM +0300, Michael S. Tsirkin wrote:
I vaguely remember there was some way to say
head of the remote I am tracking - but I could not find it.
Where are all the tricks like foo^{} documented?
gitrevisions(7) is what you're looking for here.
In this case I think you want '@{upstream}' or its short form '@{u}'.
I tried fgrep '{}' Documentation/*txt and it only returned
git-show-ref.txt which isn't really informative ...
'{' and '}' need to be escaped in AsciiDoc so you have to grep for
'\\{\\}'.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Different diff strategies in add --interactive
On Mon, Jun 10, 2013 at 05:11:41PM -0400, Jeff King wrote:
On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote:
John Keeping j...@keeping.me.uk writes:
I think the first thing to do is read the diff.algorithm setting in
git-add--interactive and pass its value to the underlying diff-index and
diff-files commands, but should we also have a command line parameter to
git-add to specify the diff algorithm in interactive mode? And if so,
can we simply add --diff-algorithm to git-add, or is that too
confusing?
Making git add--interactive read from diff.algorithm is probably a
good idea, because the command itself definitely is a Porcelain. We
would probably need a way to defeat the configured default for
completeness, either:
git add -p --diff-algorithm=default
git -c diff.algorithm=default add -p
but I suspect that a new option to git add that only takes effect
together with -p is probably an overkill, only in order to support
the former and not having to say the latter, but I can be persuaded
either way.
Worse than that, you would need to add such an option to checkout -p,
reset -p, stash -p, etc. I think the latter form you suggest is
probably acceptable in this case.
That's what I'm planning to do at the moment, if anyone wants to extend
it further in the future then that can be built on top.
Overall, I think respecting diff.algorithm in add--interactive is a very
sane thing to do. I would even be tempted to say we should allow a few
other select diff options (e.g., fewer or more context lines). If you
allowed diff options like this:
git add --patch=--patience -U5
that is very flexible, but I would not want to think about what the code
does when you pass --patch=--raw or equal nonsense.
An alternative would be to permit them to be set from within the
interactive UI. I'd find it quite useful to experiment with various
diff options when I encounter a hunk that isn't as easy to pick as I'd
like. I expect it would be very hard to do that on a per-hunk basis,
although per-file doesn't seem like it would be too hard.
I don't intend to investigate that though - respecting diff.algorithm is
good enough for my usage.
But I cannot off the top of my head think of other options besides -U
that would be helpful. I have never particularly wanted it for add -p,
either, though I sometimes generate patches to the list with a greater
number of context lines when I think it makes the changes to a short
function more readable.
--function-context might also be useful, but that's in the same category
as -U.
The patch I'm using is below. I'm not sure how we can test this though;
it seems fragile to invent a patch that appears different with different
diff algorithms. Any suggestions?
-- 8 --
git-add--interactive.perl | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d2c4ce6..0b0fac2 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -44,6 +44,8 @@ my ($diff_new_color) =
my $normal_color = $repo-get_color(, reset);
+my $diff_algorithm = ($repo-config('diff.algorithm') or 'default');
+
my $use_readkey = 0;
my $use_termcap = 0;
my %term_escapes;
@@ -731,6 +733,9 @@ sub run_git_apply {
sub parse_diff {
my ($path) = @_;
my @diff_cmd = split( , $patch_mode_flavour{DIFF});
+ if ($diff_algorithm ne default) {
+ push @diff_cmd, --diff-algorithm=${diff_algorithm};
+ }
if (defined $patch_mode_revision) {
push @diff_cmd, $patch_mode_revision;
}
--
1.8.3.779.g691e267
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 10:40:32AM -0500, Felipe Contreras wrote:
On Sun, Jun 9, 2013 at 10:12 AM, John Keeping j...@keeping.me.uk wrote:
On Sun, Jun 09, 2013 at 08:26:32PM +0530, Ramkumar Ramachandra wrote:
Felipe Contreras wrote:
The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.
Give this a try:
--- a/sequencer.c
+++ b/sequencer.c
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
This is a good example: yes, I'm convinced that the code does need to
be reorganized. Please resend your {sequencer.c -
builtin/sequencer.c} patch with this example as the rationale, and
let's work towards improving libgit.a.
Why should sequencer.c move into builtin/ to solve this? Why not pull
init_copy_notes_for_rewrite and finish_copy_notes_for_rewrite up into
notes.c?
Because finish_copy_notes_for_rewrite is only useful for builtin
commands, so it belongs in builtin/. If there's any meaning to the
./*.o vs. builtin/*.o divide, it's for that. Otherwise we should just
squash all objects into libgit.a and be done with it.
How is it only useful for builtin commands? By that logic everything
belongs in builtin/ because it's all only used by builtin commands (I
realise that is what you're arguing towards).
But we make a distinction between things that are specific to one
command (especially argument parsing and user interaction) and more
generally useful features. Copying notes around in the notes tree is
generally useful so why shouldn't it be in notes.c with the other note
manipulation functions? The current API may not be completely suitable
but that doesn't mean that it cannot be extracted into notes.c. In
fact, other than the commit message saying Notes added by 'git notes
copy' I don't see what's wrong with the current functions being
extracted as-is.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 11:22:06AM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 11:02 AM, John Keeping j...@keeping.me.uk wrote: But we make a distinction between things that are specific to one command (especially argument parsing and user interaction) and more generally useful features. No, we don't. Everything under ./*.o goes to libgit.a, and everything under ./builtin/*.o goes to 'git'. So builtin/commit.o can access code from builtin/notes.o, but sequencer.o can't. I would argue that it was a mistake not to extract these functions from builtin/notes.c to notes.c when builtin/commit.c started using them. Calling across from one builtin/*.c file to another is just as wrong as calling into a builtin/*.c file from a top-level file but the build system happens not to enforce the former. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] build: get rid of the notion of a git library
On Sun, Jun 09, 2013 at 12:13:41PM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 12:03 PM, Ramkumar Ramachandra artag...@gmail.com wrote: John Keeping wrote: Calling across from one builtin/*.c file to another is just as wrong as calling into a builtin/*.c file from a top-level file but the build system happens not to enforce the former. So libgit.a is a collection of everything that is shared between builtins? Does that correspond to reality? I think that's *precisely* what libgit.a is. It doesn't currently correspond exactly to reality, but that's mostly for historic reasons (see below). $ ls *.h | sed 's/.h$/.c/' | xargs file An example violation: builtin/log.c uses functions defined in builtin/shortlog.c. What is the point of all this separation, if no external scripts are ever going to use libgit.a? Why do we structure code in a certain way at all? The reason libgit.a was introduced (according to commit 0a02ce7) is: This introduces the concept of git library objects that the real programs use, and makes it easier to add such things to a libgit.a. And all the functions should be static, which doesn't seem to be the case: 03c0 T add_files_to_cache 0530 T interactive_add 0410 T run_add_interactive 1920 T textconv_object 05b0 T fmt_merge_msg 0090 T fmt_merge_msg_config 0c00 T init_db 0b40 T set_git_dir_init 0360 T overlay_tree_on_cache 0500 T report_path_error 11a0 T copy_note_for_rewrite 1210 T finish_copy_notes_for_rewrite 1060 T init_copy_notes_for_rewrite T prune_packed_objects 0510 T shortlog_add_commit 06b0 T shortlog_init 0780 T shortlog_output T stripspace A quick check with git log -S... shows that most of these have barely been touched since the builtin/ directory was created. So the reason they're not static is most likely because no one has tidied them up since the division between builtins was introduced. It is a fact of life that as we live and work with a system we realise that there may be a better way of doing something. This doesn't mean that someone needs to immediately convert everything to the new way, it is often sufficient to do new things in the new way and slowly move existing things across as and when they are touched for other reasons. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 10/45] sequencer: trivial fix
On Sun, Jun 09, 2013 at 07:33:42PM +0200, SZEDER Gábor wrote: On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor sze...@ira.uka.de wrote: On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote: We should free objects before leaving. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com A shortlog-friendlier subject could be: sequencer: free objects before leaving. I already defended my rationale for this succinct commit message: http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610 Your arguments were unconvincing. The mere fact that I raised this issue unbeknownst to the earlier posting clearly shows that there's demand for descriptive subjects. Not to mention that with your subject no body is needed, making the overall message more succinct. When reading a log, as soon as I see trivial I become suspicious that someone is trying to cover something up, much like left as an exercise for the reader. If the subject says fix memory leak then it's obvious what the patch is meant to do, and when there is no subtlety to be explained (as there isn't in this patch) there is no need for a body. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 10/45] sequencer: trivial fix
On Sun, Jun 09, 2013 at 12:53:38PM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 12:37 PM, John Keeping j...@keeping.me.uk wrote: On Sun, Jun 09, 2013 at 07:33:42PM +0200, SZEDER Gábor wrote: On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote: On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor sze...@ira.uka.de wrote: On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote: We should free objects before leaving. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com A shortlog-friendlier subject could be: sequencer: free objects before leaving. I already defended my rationale for this succinct commit message: http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610 Your arguments were unconvincing. The mere fact that I raised this issue unbeknownst to the earlier posting clearly shows that there's demand for descriptive subjects. Not to mention that with your subject no body is needed, making the overall message more succinct. It's not succinct at all, because there's no short and quick description of what the patch actually is; a trivial fix. Is it not equally succinct to say fix memory leak? When reading a log, as soon as I see trivial I become suspicious that someone is trying to cover something up, much like left as an exercise for the reader. If the subject says fix memory leak then it's obvious what the patch is meant to do, and when there is no subtlety to be explained (as there isn't in this patch) there is no need for a body. You are not a rational person then. The commit message has absolutely no bearing on the quality of the code. If you are less suspicious of a commit message that says fix memory leak, you are being completely biased. Whether the commit message says fix memory leak, or trivial fix, or foobar, the code might still be doing something wrong, and you can't decide that until you look at the code. I have a certain level of trust that commit summaries in git.git will be accurate. If I want to know what has changed, then fix memory leak implies no functional change; if I see trivial fix then how do I know what that is? It could be a whitespace change, a fix to a memory leak, a typo correction, a change to a separator in a message shown to the user, or even a small change to corner case behaviour. If you don't care about the code, but still want to know what the patch is doing, then you can look at the whole commit message, and We should free objects before leaving. explains that perfectly. The short message is what appears in What's Cooking, why should I need to break out of my mail client to find out what it means? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: What's cooking in git.git (May 2013, #09; Wed, 29)
On Tue, Jun 04, 2013 at 06:57:34PM -0400, Phil Hord wrote: On Tue, Jun 4, 2013 at 8:48 AM, John Keeping j...@keeping.me.uk wrote: The problem is that sometimes you do want to adjust the path and sometimes you don't. Reading git-submodule(1), it says: This may be either an absolute URL, or (if it begins with ./ or ../), the location relative to the superproject’s origin repository. [snip] If the superproject doesn’t have an origin configured the superproject is its own authoritative upstream and the current working directory is used instead. So I think it's quite reasonable to have a server layout that looks like this: project |- libs | |- libA | `- libB |- core.git and with only core.git on your local system do: cd core/libs git submodule add ../libs/libB expecting that to point to libB. But if we adjust the path then the user has to do: git submodule add ../../libs/libB However, it is also perfectly reasonable to have no remote configured and the library next to the repository itself. In which case we do want to specify the additional ../ so that shell completion works in the natural way. In submodule add, the leading '../' prefix on the repository url has always meant that the url is relative to the url of the current repo. The given repo-url is precisely what ends up in .gitmodules' submodule.$name.url. It works this way whether there is a remote configured or not. It does seem like we need to be cautious around this change. The only way I can see to resolve the ambiguity is to die when we hit this particular case. This should be acceptable because people shouldn't be adding new submodules anywhere near as often as they perform other submodule operations and it doesn't affect absolute URLs. I don't think I like that. But I don't know if I like anything I dreamed up, either. I've decided that I will make it die (unless anyone comes up with an obviously correct solution before I get around to sending the reroll) because it's a lot easier to loosen that in the future than to change it if we get the behaviour wrong now. I don't want to hold every other subcommand hostage to this one case. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: What's cooking in git.git (May 2013, #09; Wed, 29)
On Tue, Jun 04, 2013 at 09:17:17PM +1000, Heiko Voigt wrote: On Tue, Jun 04, 2013 at 09:10:45AM +0100, John Keeping wrote: On Tue, Jun 04, 2013 at 03:29:51PM +1000, Heiko Voigt wrote: On Mon, Jun 03, 2013 at 11:23:41PM +0100, John Keeping wrote: Sorry, I should have been more specific here. I saw that you did some changes to make submodule add do the right thing with relative paths, but the following change to t7406 does not work like I believe it should but instead makes the test fail: ---8- diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index a4ffea0..9766b9e 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -559,7 +559,9 @@ test_expect_success 'add different submodules to the same pa test_expect_success 'submodule add places git-dir in superprojects git-dir' ' (cd super mkdir deeper -git submodule add ../submodule deeper/submodule +(cd deeper + git submodule add ../../submodule submodule +) (cd deeper/submodule git log ../../expected ) ---8- Ah, ok. I think this case is problematic because the repository argument is either relative to remote.origin.url or to the top of the working tree if there is no origin remote. I wonder if we should just die when a relative path is given for the repository and we're not at the top of the working tree. Why not behave as if we are at the top of the working tree for relative paths? If there is an origin remote thats fine. If there is no origin remote you could warn that the path used is taken relative from the root of the superproject during add. What do you think? That's what the patch currently queued on pu does, which Jens wants to change, isn't it? True I did not realize this when reading it the first time. But I think we should still not die when in a subdirectory. After all this series is trying to archive that the submodule command works in subdirectories seamlessly right? So you probably want to translate a relative path without origin remote given from a subdirectory to the superproject level and use that. Then you do not have to die. The problem is that sometimes you do want to adjust the path and sometimes you don't. Reading git-submodule(1), it says: This may be either an absolute URL, or (if it begins with ./ or ../), the location relative to the superproject’s origin repository. [snip] If the superproject doesn’t have an origin configured the superproject is its own authoritative upstream and the current working directory is used instead. So I think it's quite reasonable to have a server layout that looks like this: project |- libs | |- libA | `- libB |- core.git and with only core.git on your local system do: cd core/libs git submodule add ../libs/libB expecting that to point to libB. But if we adjust the path then the user has to do: git submodule add ../../libs/libB However, it is also perfectly reasonable to have no remote configured and the library next to the repository itself. In which case we do want to specify the additional ../ so that shell completion works in the natural way. The only way I can see to resolve the ambiguity is to die when we hit this particular case. This should be acceptable because people shouldn't be adding new submodules anywhere near as often as they perform other submodule operations and it doesn't affect absolute URLs. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (May 2013, #09; Wed, 29)
On Tue, Jun 04, 2013 at 11:39:25PM +0200, Jens Lehmann wrote:
Am 04.06.2013 14:48, schrieb John Keeping:
The problem is that sometimes you do want to adjust the path and
sometimes you don't. Reading git-submodule(1), it says:
This may be either an absolute URL, or (if it begins with ./ or
../), the location relative to the superproject’s origin
repository.
[snip]
If the superproject doesn’t have an origin configured the
superproject is its own authoritative upstream and the current
working directory is used instead.
So I think it's quite reasonable to have a server layout that looks like
this:
project
|- libs
| |- libA
| `- libB
|- core.git
and with only core.git on your local system do:
cd core/libs
git submodule add ../libs/libB
expecting that to point to libB. But if we adjust the path then the
user has to do:
git submodule add ../../libs/libB
However, it is also perfectly reasonable to have no remote configured
and the library next to the repository itself. In which case we do want
to specify the additional ../ so that shell completion works in the
natural way.
Exactly.
The only way I can see to resolve the ambiguity is to die when we hit
this particular case.
Hmm, I'm not so sure about that. Don't the first three lines in
resolve_relative_url() show how to distinguish between these two
cases?
resolve_relative_url ()
{
remote=$(get_default_remote)
remoteurl=$(git config remote.$remote.url) ||
remoteurl=$(pwd) # the repository is its own authoritative
upstream
...
If it's this simple, yes. But I think there's also a third possibility
that combines both of these: what if the local directory structure is
the same as that on the origin remote? Then origin exists but we
still want to adjust for the subdirectory.
The risk is that I can't see a behaviour that doesn't seem to choose
whether to convert the given path or not arbitrarily. Even knowing the
rules, I expect that I could end up being surprised by this if I create
a new repository and haven't set up origin yet.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (May 2013, #09; Wed, 29)
On Mon, Jun 03, 2013 at 11:47:23PM +0200, Jens Lehmann wrote: Am 31.05.2013 21:40, schrieb John Keeping: The current version does make '$sm_path' relative in submodule foreach, although it's hard to spot because we have to leave doing so until right before the eval. Yes. If I read the code correctly the submodule is cd'ed in before the foreach command is executed, so $sm_path should only be used for displaying info about where the command is executed anyway. Looks like your code is doing the right thing adjusting $sm_path to be relative to the directory the user is in. But a test showing that would really be nice ;-) Agreed. I've also noticed that the legacy path variable hasn't been adjusted and the printing of the module paths does not make them relative. I'll fix them in the next version. I'm not sure what you mean about submodule add - the new version treats the path argument as relative (providing it is not an absolute path). The repository argument is not changed by running from a subdirectory but I think that's correct since it is documented as being relative to the superproject's origin repository. Sorry, I should have been more specific here. I saw that you did some changes to make submodule add do the right thing with relative paths, but the following change to t7406 does not work like I believe it should but instead makes the test fail: ---8- diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index a4ffea0..9766b9e 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -559,7 +559,9 @@ test_expect_success 'add different submodules to the same pa test_expect_success 'submodule add places git-dir in superprojects git-dir' ' (cd super mkdir deeper -git submodule add ../submodule deeper/submodule +(cd deeper + git submodule add ../../submodule submodule +) (cd deeper/submodule git log ../../expected ) ---8- Ah, ok. I think this case is problematic because the repository argument is either relative to remote.origin.url or to the top of the working tree if there is no origin remote. I wonder if we should just die when a relative path is given for the repository and we're not at the top of the working tree. submodule init is behaving in the same way as deinit - if you say submodule init . then it will only initialize submodules below the current directory. The difference is that deinit dies if it is not given any arguments whereas init will initialize everything from the top level down. I'm not sure whether to change this; given the direction git add -u is heading in for 2.0 I think the current behaviour is the most consistent with the rest of Git. I meant that both commands still print the submodule names from the top-level directory, not the one the user is in. Will fix. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Poor performance of git describe in big repos
On Fri, May 31, 2013 at 09:14:49AM +0100, Alex Bennée wrote:
On 30 May 2013 20:30, John Keeping j...@keeping.me.uk wrote:
On Thu, May 30, 2013 at 06:21:55PM +0200, Thomas Rast wrote:
Alex Bennée kernel-hac...@bennee.com writes:
On 30 May 2013 16:33, Thomas Rast tr...@inf.ethz.ch wrote:
Alex Bennée kernel-hac...@bennee.com writes:
snip
Will it be loading the blob for every commit it traverses or just ones
that hit
a tag? Why does it need to load the blob at all? Surely the commit
tree state doesn't
need to be walked down?
No, my theory is that you tagged *the blobs*. Git supports this.
Wait is this the difference between annotated and non-annotated tags?
I thought a non-annotated just acted like references to a particular
tree state?
No, this is something slightly different. In Git there are four types
of object: tag, commit, tree and blob. When you have a heavyweight tag,
the tag reference points at a tag object (which in turn points at
another object). With a lightweight tag, the tag reference typically
points at a commit object.
However, there is no restriction that says that a tag object must point
to a commit or that a lightweight tag must point at a commit - it is
equally possible to point directly at a tree or a blob (although a lot
less common).
Thomas is suggesting that you might have a tag that does not point at a
commit but instead points to a blob object.
You can see if that is the case by doing something like this:
eval $(git for-each-ref --shell --format '
test $(git cat-file -t %(objectname)^{}) = commit ||
echo %(refname);')
That will print out the name of any ref that doesn't point at a
commit.
Hmm that didn't seem to work.
You mean there was no output? In that case it's likely that all your
references do indeed point at commits.
But looking at the output by hand I
certainly have a mix of tags that are commits vs tags:
09:08 ajb@sloy/x86_64 [work.git] git for-each-ref | grep refs/tags
| grep commit | wc -l
1345
09:12 ajb@sloy/x86_64 [work.git] git for-each-ref | grep refs/tags
| grep -v commit | wc -l
66
This means that you have 1345 lightweight tags and 66 heavyweight tags,
assuming that all of the lines that don't say commit do say tag.
By the way, I don't remember if you said which version of Git you're
using. If it's an older version then it's possible that something has
changed.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Poor performance of git describe in big repos
On Fri, May 31, 2013 at 09:49:57AM +0100, Alex Bennée wrote: On 31 May 2013 09:32, John Keeping j...@keeping.me.uk wrote: Thomas is suggesting that you might have a tag that does not point at a commit but instead points to a blob object. It's looking like I just have some very heavy commits. One data point I probably should have mentioned at the beginning is this was a converted CVS repo and I'm wondering if some of the artifacts that introduced has contributed to this. You can try another for-each-ref invocation to see if that's the case: eval $(git for-each-ref --format 'printf %s %s\n \ $(git cat-file -s %(objectname)) %(refname);') | sort -n That will print the size of each object followed by the ref that points to it, sorted by size. I'm running the GIT stable PPA: 09:38 ajb@sloy/x86_64 [work.git] git --version git version 1.8.3 Although I have also tested with the latest git.git maint. I'm happy to try master if it's likely to have changed. master's still very close to 1.8.3 at the moment, so I don't think that will make a difference. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (May 2013, #09; Wed, 29)
On Thu, May 30, 2013 at 09:23:40PM +0200, Jens Lehmann wrote: Am 30.05.2013 01:58, schrieb Junio C Hamano: * jk/submodule-subdirectory-ok (2013-04-24) 3 commits (merged to 'next' on 2013-04-24 at 6306b29) + submodule: fix quoting in relative_path() (merged to 'next' on 2013-04-22 at f211e25) + submodule: drop the top-level requirement + rev-parse: add --prefix option Allow various subcommands of git submodule to be run not from the top of the working tree of the superproject. The summary and status commands are looking good in this version (they are now showing the submodule directory paths relative to the current directory). Apart from that my other remarks from gmane $221575 still seem to apply. And this series has only tests for status, summary and add (and that just with an absolute URL), I'd rather like to see a test for each submodule command (and a relative add to) to document the desired behavior. To summarize what I think are the outstanding issues from your email: * Should '$sm_path' be relative in submodule foreach? * submodule add with a relative path * submodule init initializes all submodules * Tests The current version does make '$sm_path' relative in submodule foreach, although it's hard to spot because we have to leave doing so until right before the eval. I'm not sure what you mean about submodule add - the new version treats the path argument as relative (providing it is not an absolute path). The repository argument is not changed by running from a subdirectory but I think that's correct since it is documented as being relative to the superproject's origin repository. submodule init is behaving in the same way as deinit - if you say submodule init . then it will only initialize submodules below the current directory. The difference is that deinit dies if it is not given any arguments whereas init will initialize everything from the top level down. I'm not sure whether to change this; given the direction git add -u is heading in for 2.0 I think the current behaviour is the most consistent with the rest of Git. But I'm not sure if it's better to have another iteration of this series or to address the open issues a follow-up series. Having status, summary and add - at least with absolute URLs - lose the toplevel requirement is already a huge improvement IMO. Opinions? I think the only thing outstanding is tests. I'm happy to add those as a follow-up or in a re-roll. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ls-files -i directories
On Fri, May 31, 2013 at 04:22:37PM -0400, Roland Schulz wrote: Hi, the gitignore rules work so that if a directory is ignored, all files in that directory are ignored. While that behavior isn't clearly documented in gitignore, this behavior is consistent across all git tools (status, ls-files, ...). An exception is that listing the ignored files using ls-files -i doesn't behave the same way. example: $ mkdir d $ touch d/f $ echo /d/ .gitignore $ git ls-files -o --exclude-standard .gitignore #d/f is correctly not listed $ git ls-files -i --exclude-standard #no output d/f isn't listed even though it is treated as an ignored file by all other git tools. That seems inconsistent to me. Is that behavior intentionally or is this a bug? It is listed with git ls-files -i -o --exclude-standard. The documentation says: Show only ignored files in the output. When showing files in the index, print only those matched by an exclude pattern. When showing other files, show only those matched by an exclude pattern. If you do this then it is shown: $ git add -f d/f $ git ls-files -i --exclude-standard d/f I think this is working as documented. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Poor performance of git describe in big repos
On Thu, May 30, 2013 at 11:38:32AM +0100, Alex Bennée wrote: One factor might be the size of my repo (.git is around 2.4G). Could this just be due to computational cost of searching through large packs to walk the commit chain? Is there any way to make this easier for git to do? What does git count-objects -v say for your repository? You may find that performance improves if you repack with git gc --aggressive. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-gui: fix file name handling with non-empty prefix
In the hope that the Pat Thoyts who just posted in another thread from a
GMail address is the same one that maintains git-gui, let's see if that
address works...
On Sat, May 11, 2013 at 10:03:25PM -0400, Andrew Wong wrote:
Sorry for the late reply. I was able to reproduce the problem that you
were describing a while ago. And your patch indeed fixes it. It's a much
more elegant way of dealing with the absolute vs relative path problem
that I was trying to fix.
Thanks!
As for Pat, I'm not sure wha'ts going on with his email address. It was
working back in October, and his username still seems to be active over
at SourceForge... let's see if this email reaches him.
Here's a link for his reference just in case he missed your original email:
http://thread.gmane.org/gmane.comp.version-control.git/222646
On 04/27/13 10:18, John Keeping wrote:
I got a bounce with 550 no such user for Pat's email address when
sending this. Does anyone have more up-to-date contact details? Or is
it just SourceForge being broken?
On Sat, Apr 27, 2013 at 02:24:16PM +0100, John Keeping wrote:
Commit e3d06ca (git-gui: Detect full path when parsing arguments -
2012-10-02) fixed the handling of absolute paths passed to the browser
and blame subcommands by checking whether the file exists without the
prefix before prepending the prefix and checking again. Since we have
chdir'd to the top level of the working tree before doing this, this
does not work if a file with the same name exists in a subdirectory and
at the top level (for example Makefile in git.git's t/ directory).
Instead of doing this, revert that patch and fix absolute path issue by
using file join to prepend the prefix to the supplied path. This will
correctly handle absolute paths by skipping the prefix in that case.
Signed-off-by: John Keeping j...@keeping.me.uk
---
git-gui.sh | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/git-gui.sh b/git-gui.sh
index e11..a94ad7f 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -3003,19 +3003,11 @@ blame {
set jump_spec {}
set is_path 0
foreach a $argv {
- if {[file exists $a]} {
- if {$path ne {}} usage
- set path [normalize_relpath $a]
- break
- } elseif {[file exists $_prefix$a]} {
- if {$path ne {}} usage
- set path [normalize_relpath $_prefix$a]
- break
- }
+ set p [file join $_prefix $a]
- if {$is_path} {
+ if {$is_path || [file exists $p]} {
if {$path ne {}} usage
- set path [normalize_relpath $_prefix$a]
+ set path [normalize_relpath $p]
break
} elseif {$a eq {--}} {
if {$path ne {}} {
--
1.8.3.rc0.149.g98a72f2.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Should git help respect the 'pager' setting?
On Thu, May 30, 2013 at 10:38:59PM +0530, Ramkumar Ramachandra wrote: Matthieu Moy wrote: I find it a bit weird that Git sets the configuration for external commands, but it may make sense. No strong opinion here. I don't mean a setenv() kind of thing: how would we unset it after that? Perhaps something like execvpe(), passing in the environment as an argument? Overriding PAGER might make sense, but I'd be quite annoyed if Git decided to override MANPAGER without providing some way to override it. If a user sets MANPAGER then it's because they want a specific pager when reading man pages - invoking man through git help shouldn't cause it to behave differently in this case. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Poor performance of git describe in big repos
On Thu, May 30, 2013 at 06:21:55PM +0200, Thomas Rast wrote:
Alex Bennée kernel-hac...@bennee.com writes:
On 30 May 2013 16:33, Thomas Rast tr...@inf.ethz.ch wrote:
Alex Bennée kernel-hac...@bennee.com writes:
41.58% git libcrypto.so.1.0.0 [.] sha1_block_data_order_ssse3
33.62% git libz.so.1.2.3.4 [.] inflate_fast
10.39% git libz.so.1.2.3.4 [.] adler32
2.03% git [kernel.kallsyms] [k] clear_page_c
Do you have any large blobs in the repo that are referenced directly by
a tag?
Most probably. I've certainly done a bunch of releases (which are tagged)
were
the last thing that was updated was an FPGA image.
[...]
git-describe should probably be fixed to avoid loading blobs, though I'm
not sure off hand if we have any infrastructure to infer the type of a
loose object without inflating it. (This could probably be added by
inflating only the first block.) We do have this for packed objects, so
at least for packed repos there's a speedup to be had.
Will it be loading the blob for every commit it traverses or just ones that
hit
a tag? Why does it need to load the blob at all? Surely the commit
tree state doesn't
need to be walked down?
No, my theory is that you tagged *the blobs*. Git supports this.
You can see if that is the case by doing something like this:
eval $(git for-each-ref --shell --format '
test $(git cat-file -t %(objectname)^{}) = commit ||
echo %(refname);')
That will print out the name of any ref that doesn't point at a commit.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-send-email doesn't deal with quoted names
On Tue, May 28, 2013 at 01:40:20AM +0200, Jason A. Donenfeld wrote:
My commit author name is Jason A. Donenfeld. Because this has a dot,
SMTP handling likes to put it in quotes.
git-send-email has this line:
if (defined $author and $author ne $sender) {
With my name, this always winds up false, because it's comparing
'Jason A. Donenfeld ja...@zx2c4.com' with 'Jason A. Donenfeld
ja...@zx2c4.com'.
So, the logic needs to be fixed somehow.
There was a patch for this recently, although it appears to be still
under discussion:
http://article.gmane.org/gmane.comp.version-control.git/225247
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
On Tue, May 28, 2013 at 11:06:13AM -0700, Junio C Hamano wrote:
Kenichi Saita nito...@gmail.com writes:
When deciding whether or not we should link a working tree file into
the temporary right-hand directory for a directory diff, we
currently behave differently in the --symlink and --no-symlink
cases. If using symlinks any identical files are linked across but
with --no-symlink only files that contain unstaged changes are
copied back into the working tree.
I may have missed an earlier discussion, but I do not follow the
last sentence. The former part (i.e. symlinks) talks about what is
done to populate the temporary tree (i.e. everything is linked), but
the latter part (i.e. not symlinks) only talks about what is copied
back, i.e. it is not a contrast between the behaviour of symlink vs
no-symlink case wrt how the temporary tree is populated.
Confused...
Yeah, the commit message is still quite focused on the end effect of
copying files back. But that's not what's being changed here.
In my suggested commit message I tried to make it clear that we're
changing when we decide to copy a file across to the temporary tree.
This has the beneficial (side-)effect of changing the set of files we
consider for copying back into the working tree after the diff tool has
been run.
Change this so that identical files are copied back as well. This
is beneficial because it widens the set of circumstances in which we
copy changes made by the user back into the working tree.
Ah, OK, you meant that the set of files we keep in @working_tree
array for later copying back are different between the two.
Signed-off-by: Kenichi Saita nito...@gmail.com
---
git-difftool.perl |9 ++---
t/t7800-difftool.sh | 19 +++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 8a75205..e57d3d1 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -85,13 +85,9 @@ sub exit_cleanup
sub use_wt_file
{
- my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
+ my ($repo, $workdir, $file, $sha1) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 ne $null_sha1 and not $symlinks) {
- return 0;
- }
-
if (! -e $workdir/$file) {
# If the file doesn't exist in the working tree, we cannot
# use it.
@@ -213,8 +209,7 @@ EOF
if ($rmode ne $null_mode) {
my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
- $dst_path, $rsha1,
- $symlinks);
+ $dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= $rmode $wt_sha1\t$dst_path\0;
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index d46f041..2418528 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff
--symlink without unstage
test_cmp actual expect
'
+write_script modify-right-file \EOF
+echo new content $2/file
+EOF
+
+run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged
change' '
+ test_when_finished git reset --hard
+ echo orig content file
+ git difftool -d $symlinks --extcmd $(pwd)/modify-right-file branch
+ echo new content expect
+ test_cmp expect file
+'
+
+run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged
change' '
+ test_when_finished git reset --hard
+ git difftool -d $symlinks --extcmd $(pwd)/modify-right-file branch
+ echo new content expect
+ test_cmp expect file
+'
+
write_script modify-file \EOF
echo new content file
EOF
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
On Tue, May 28, 2013 at 11:57:08AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: Yeah, the commit message is still quite focused on the end effect of copying files back. But that's not what's being changed here. In my suggested commit message I tried to make it clear that we're changing when we decide to copy a file across to the temporary tree. This has the beneficial (side-)effect of changing the set of files we consider for copying back into the working tree after the diff tool has been run. I actually think the effect of copying files back _is_ the primary motivation of this change, and stressing that end effect is a much better description. After all, if the working tree files do not have any difference from the RHS of the comparison, copying from the working tree and stuffing the $rsha1 to the RHS temporary index and running checkout -f should produce identical temporary directory for the user to start viewing. The _only_ difference in behaviour before and after this patch that matters to the end user is if that path is in @working_tree, which is returned to @worktree of dir_diff sub to be later copied back, no? I would view this change as a mere means, an implementation detail, to achieve that end of stuffing more paths in the @worktree array. I agree with this, but like you I found it confusing that the patch touched code seemingly unrelated to copying files back. I went toward describing the patch more literally and giving the motivation in the final paragraph. Your message below is better, but I think it needs to say that the set of files considered for copying back is the set that is copied across to begin with. Perhaps difftool --dir-diff: allow changing any clean working tree file The temporary directory prepared by difftool --dir-diff to show the result of a change can be modified by the user via the tree diff program, and we try hard not to lose changes to them after tree diff program returns to us. However, the set of files to be copied back is computed differently between --symlinks and --no-symlinks modes. The former checks all paths that start out as identical to the working tree file, while the latter checks paths that already had a local modification in the working tree, allowing changes made in the tree diff program to paths that did not have any local change to be lost. or something. This invites a few questions, though. - By allowing these files in the temporary directory to be modified, aren't we making the user's life harder by forcing them to handle working tree file was already modified, made different changes in the temporary directory, now these changes need to be consolidated case? - When comparing two revisions, e.g. --dir-diff HEAD^^ HEAD^, that checks out (via $rsha1 to checkout -f codepath) a blob that does not match what is in the working tree of HEAD to the temporary directory, we still allow modifications to the copy in the temporary directory, but what can the user do with these changes that are _not_ based on HEAD, short of checking out HEAD^ and apply the difference first? I still cannot shake this nagging feeling that giving a writable temporary directory might have been a mistake in the first place. Perhaps it may be a better design to make the ones that the user shouldn't touch (or will lead to the above confusion) read-only, while the ones that match the working tree read-write? My ideal scenario would be that we only allow users to edit files when they are comparing against the working tree, but that would require git-difftool to fully understand all git-diff options since it just passes through any it doesn't recognise. I don't think there's an easy way to do that, which leaves us with this confusing situation. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fetch: don't try to update unfetched tracking refs
Since commit f269048 (fetch: opportunistically update tracking refs,
2013-05-11) we update tracking refs opportunistically when fetching
remote branches. However, if a refspec is given on the command line
that does not include a configured (non-pattern) refspec a fatal error
occurs.
Fix this by setting the missing_ok flag when calling get_fetch_map.
Signed-off-by: John Keeping j...@keeping.me.uk
---
builtin/fetch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e41cc0d..d15a734 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -183,7 +183,7 @@ static struct ref *get_ref_map(struct transport *transport,
old_tail = tail;
for (i = 0; i transport-remote-fetch_refspec_nr; i++)
get_fetch_map(ref_map, transport-remote-fetch[i],
- tail, 0);
+ tail, 1);
for (rm = *old_tail; rm; rm = rm-next)
rm-fetch_head_status = FETCH_HEAD_IGNORE;
} else {
--
1.8.3.rc3.438.gb3e4ae3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fetch: don't try to update unfetched tracking refs
On Mon, May 27, 2013 at 11:42:52AM -0400, Jeff King wrote: On Mon, May 27, 2013 at 12:40:25PM +0100, John Keeping wrote: Since commit f269048 (fetch: opportunistically update tracking refs, 2013-05-11) we update tracking refs opportunistically when fetching remote branches. However, if a refspec is given on the command line that does not include a configured (non-pattern) refspec a fatal error occurs. I'm not sure I understand what the last sentence means. I tried to add a test like: diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ff43e08..02e30e1 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -422,6 +422,16 @@ test_expect_success 'configured fetch updates tracking' ' ) ' +test_expect_success 'non-configured ref does not confuse tracking update' ' + cd $D + git update-ref refs/odd/location HEAD + ( + cd three + git fetch origin refs/odd/location + git rev-parse --verify FETCH_HEAD + ) +' + test_expect_success 'pushing nonexistent branch by mistake should not segv' ' cd $D but it does not fail with the existing code. Can you give an example that fails? I have this in my .git/config for git.git: [remote origin] url = git://github.com/gitster/git fetch = +refs/heads/*:refs/remotes/origin/* fetch = +refs/notes/amlog:refs/notes/amlog Then doing git fetch origin master fails because: fatal: Couldn't find remote ref refs/notes/amlog The following test fails for me (and passes with my patch) - note that in two, remote.one.fetch is configured as refs/heads/master:refs/heads/one. -- 8 -- diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ff43e08..c540257 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -422,6 +422,19 @@ test_expect_success 'configured fetch updates tracking' ' ) ' +test_expect_success 'configured ref does not confuse tracking' ' + + cd $D + ( + cd one + git branch -f side + ) + ( + cd two + git fetch one side + ) +' + test_expect_success 'pushing nonexistent branch by mistake should not segv' ' cd $D -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] fetch: don't try to update unfetched tracking refs
Since commit f269048 (fetch: opportunistically update tracking refs,
2013-05-11) we update tracking refs opportunistically when fetching
remote branches. However, if there is a configured non-pattern refspec
that does not match any of the refspecs given on the command line then a
fatal error occurs.
Fix this by setting the missing_ok flag when calling get_fetch_map.
Test-added-by: Jeff King p...@peff.net
Signed-off-by: John Keeping j...@keeping.me.uk
Acked-by: Jeff King p...@peff.net
---
On Mon, May 27, 2013 at 12:19:34PM -0400, Jeff King wrote:
On Mon, May 27, 2013 at 05:01:29PM +0100, John Keeping wrote:
I'm not sure I understand what the last sentence means. I tried to add a
test like:
[...]
but it does not fail with the existing code. Can you give an example
that fails?
I have this in my .git/config for git.git:
[remote origin]
url = git://github.com/gitster/git
fetch = +refs/heads/*:refs/remotes/origin/*
fetch = +refs/notes/amlog:refs/notes/amlog
Ah, I see. It is not the refspec on the command-line does not match a
configured refspec, but rather there exists a configured non-pattern
refspec that does not match what was on the command-line (even if what
was on the command-line did match another refspec).
Exactly. I've changed the commit message to (hopefully) make this
clearer.
So your fix makes perfect sense. Do you mind squashing in this test
below? I think it is a little less subtle than what you posted, as it
sets up the situation explicitly in the test. It also checks that the
refs we _did_ match still get updated (master in this case).
Done.
builtin/fetch.c | 2 +-
t/t5510-fetch.sh | 16
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e41cc0d..d15a734 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -183,7 +183,7 @@ static struct ref *get_ref_map(struct transport *transport,
old_tail = tail;
for (i = 0; i transport-remote-fetch_refspec_nr; i++)
get_fetch_map(ref_map, transport-remote-fetch[i],
- tail, 0);
+ tail, 1);
for (rm = *old_tail; rm; rm = rm-next)
rm-fetch_head_status = FETCH_HEAD_IGNORE;
} else {
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ff43e08..fde6891 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -422,6 +422,22 @@ test_expect_success 'configured fetch updates tracking' '
)
'
+test_expect_success 'non-matching refspecs do not confuse tracking update' '
+ cd $D
+ git update-ref refs/odd/location HEAD
+ (
+ cd three
+ git update-ref refs/remotes/origin/master base-origin-master
+ git config --add remote.origin.fetch \
+ refs/odd/location:refs/remotes/origin/odd
+ o=$(git rev-parse --verify refs/remotes/origin/master)
+ git fetch origin master
+ n=$(git rev-parse --verify refs/remotes/origin/master)
+ test $o != $n
+ test_must_fail git rev-parse --verify refs/remotes/origin/odd
+ )
+'
+
test_expect_success 'pushing nonexistent branch by mistake should not segv' '
cd $D
--
1.8.3.rc3.438.gb3e4ae3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool --dir-diff: copy back all files matching the working tree
On Mon, May 27, 2013 at 12:00:46AM +0900, Kenichi Saita wrote:
After running the user's diff tool, git difftool --dir-dif --no-symlink
currently copied back a temporary file to working tree only when a file
contains unstaged changes in the working tree.
Change this behavior so that temporary files are copied back to working
tree whenever the right-hand side of the comparison has the same SHA1
as the file in the working tree.
Signed-off-by: Kenichi Saita nito...@gmail.com
This change looks sensible to me, but I found the commit message quite
confusing.
The code being changed here is to do with choosing whether to copy the
working tree file to the temporary directory (or symlink it) and hence
only indirectly related to whether it will be copied back. It might be
clearer to phrase it like this:
difftool --dir-diff: always use identical working tree file
When deciding whether or not we should link a working tree file into
the temporary right-hand directory for a directory diff, we
currently behave differently in the --symlink and --no-symlink
cases. If using symlinks any identical files are linked across but
with --no-symlink only files that contain unstaged changes are
copied.
Change this so that identical files are copied across as well. This
is beneficial because it widens the set of circumstances in which we
copy changes made by the user back into the working tree.
---
git-difftool.perl |9 ++---
t/t7800-difftool.sh | 19 +++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 8a75205..e57d3d1 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -85,13 +85,9 @@ sub exit_cleanup
sub use_wt_file
{
- my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
+ my ($repo, $workdir, $file, $sha1) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 ne $null_sha1 and not $symlinks) {
- return 0;
- }
-
if (! -e $workdir/$file) {
# If the file doesn't exist in the working tree, we cannot
# use it.
@@ -213,8 +209,7 @@ EOF
if ($rmode ne $null_mode) {
my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
- $dst_path, $rsha1,
- $symlinks);
+ $dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= $rmode $wt_sha1\t$dst_path\0;
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index d46f041..2418528 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff
--symlink without unstage
test_cmp actual expect
'
+write_script modify-right-file \EOF
+echo new content $2/file
+EOF
+
+run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '
+ test_when_finished git reset --hard
+ echo orig content file
+ git difftool -d $symlinks --extcmd $(pwd)/modify-right-file branch
+ echo new content expect
+ test_cmp expect file
+'
+
+run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged
change' '
+ test_when_finished git reset --hard
+ git difftool -d $symlinks --extcmd $(pwd)/modify-right-file branch
+ echo new content expect
+ test_cmp expect file
+'
+
write_script modify-file \EOF
echo new content file
EOF
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clone does not understand insteadOf URLs
On Sun, May 26, 2013 at 08:09:56PM +0200, Gioele Barabucci wrote: Il 26/05/2013 20:00, Andreas Schwab ha scritto: Simple, I keep all my projects on the same server, so I would like to refer to that server + path using 'remote-repo'. git+ssh://git.example.org//users/gioele/projects insteadOf remote-repo You can use remote-repo: instead. Do you mean I could use git+ssh://git.example.org//users/gioele/projects insteadOf remote-repo:? Yes, but now I have dozens of repositories already set up in various workstations and I do not want to go and change all of them. What really bugs me is the fact that `git clone` and `git remote add` parse the same path in different ways. Git already has many inconsistencies. This one can be easily ironed out. In what way do you think that `git remote add` handles the path? All `git remote add` does is add a new remote.name.url entry to the configuration file with the value as given on the command line. The insteadOf mapping will only be applied when you try to fetch from/push to the remote. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clone does not understand insteadOf URLs
On Sun, May 26, 2013 at 08:21:45PM +0200, Gioele Barabucci wrote:
Il 26/05/2013 20:14, John Keeping ha scritto:
On Sun, May 26, 2013 at 08:09:56PM +0200, Gioele Barabucci wrote:
Il 26/05/2013 20:00, Andreas Schwab ha scritto:
Simple, I keep all my projects on the same server, so I would like to
refer to that server + path using 'remote-repo'.
git+ssh://git.example.org//users/gioele/projects insteadOf
remote-repo
In what way do you think that `git remote add` handles the path?
All `git remote add` does is add a new remote.name.url entry to the
configuration file with the value as given on the command line. The
insteadOf mapping will only be applied when you try to fetch from/push
to the remote.
Regardless of the implementation of the commands, if I do
mkdir projectA
cd projectA
git init .
git remote add origin remote-repo/projectA.git
git pull origin master
I get a working repository. If I do
git clone remote-repo/projectA.git
all I will get is an error.
So the problem is that git clone does not seem to perform normal
remote processing if you give it something that looks like a path.
More specifically, it looks like the problem is that if you give clone
something that does not contain a colon (':') it considers it to be a
local path and dies if that path does not exist. Adding a colon as
Andreas suggested makes it look like a remote URL so it will be handled
correctly.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (May 2013, #07; Fri, 24)
On Fri, May 24, 2013 at 02:15:55PM -0700, Junio C Hamano wrote: * jk/submodule-subdirectory-ok (2013-04-24) 3 commits (merged to 'next' on 2013-04-24 at 6306b29) + submodule: fix quoting in relative_path() (merged to 'next' on 2013-04-22 at f211e25) + submodule: drop the top-level requirement + rev-parse: add --prefix option Allow various subcommands of git submodule to be run not from the top of the working tree of the superproject. What's the status of this one? As far as I'm concerned this is done. If you're re-rolling next you could squash the top two together but I don't think that's really necessary. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] for-each-ref: introduce %C(...) for color
On Sat, May 25, 2013 at 05:20:29PM +0530, Ramkumar Ramachandra wrote: Antoine Pelisse wrote: Is it not possible for color to be used uninitialized here ? My compiler didn't complain; what am I missing? Doesn't the declaration char color[COLOR_MAXLEN]; initialize an empty string? Why would it? The variable's begin allocated on the stack and the C standard only zero-initializes variables with static storage duration; Section 6.7.9 of the C11 standard says: If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. I suspect the compiler doesn't complain because there is a path through the function that initializes color before reading it (if we hit the if branch in the loop before the else branch) and the compile assumes that there is something in the function's contract that guarantees that we follow this path. But I don't think that's correct so you do need to initialize color to the empty string. More importantly, aren't there numerous instances of this in the codebase? Care to point at one? I had a quick look and all places I inspected are either static or write to the array before reading it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash deletes/drops changes of
On Fri, May 24, 2013 at 01:57:12AM +0200, Petr Baudis wrote: Just to clear up on what the best practice is, I'd imagine the setup to be something like: (a) Makefile contains inclusion of Makefile.include. (b) There is a file like Makefile.include.template containing a template to be copied over and filled by the user. (c) Makefile contains code that makes sure all variables that are supposed to be set are set and obsolete variables are not, since there is no mechanism to cause e.g. a merge conflict on change of Makefile.include.template. Is there a better way to solve this? I think the best practice would be what Git itself does ;-) The Makefile sets default values for all parameters, some of which are inferred based on the system. It then includes config.mak, which allows the user to override any of these values. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: first parent, commit graph layout, and pull merge direction
On Thu, May 23, 2013 at 06:53:36PM -0500, Felipe Contreras wrote: The alternatives are these: a) you annoy the vast majority of the user-base by making 'git pull' a dangerous operation that should be avoided, and replaced with 'git fetch'+'git rebase'. b) you annoy a minority of the user-base by making 'git pull' not do the merge the expected, so they have to do +'git merge' (which is already less of a change than a)), or configure the default (which they most likely are able to do, if they did intent to do a merge). Note that in my email that started this, I tried to be clear that I was talking about git pull *without a branch name*. If this user explicitly says git pull remote branch then I consider that a clear indication that they really do mean to perform a merge; I would not recommend changing the current behaviour in that case. If the user just says git pull then it is more likely that they are just trying to synchronise with the upstream branch, in which case they probably don't actually want a merge. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash deletes/drops changes of
On Fri, May 24, 2013 at 11:40:07AM +0200, Petr Baudis wrote: On Fri, May 24, 2013 at 09:22:53AM +0100, John Keeping wrote: On Fri, May 24, 2013 at 01:57:12AM +0200, Petr Baudis wrote: Just to clear up on what the best practice is, I'd imagine the setup to be something like: (a) Makefile contains inclusion of Makefile.include. (b) There is a file like Makefile.include.template containing a template to be copied over and filled by the user. (c) Makefile contains code that makes sure all variables that are supposed to be set are set and obsolete variables are not, since there is no mechanism to cause e.g. a merge conflict on change of Makefile.include.template. Is there a better way to solve this? I think the best practice would be what Git itself does ;-) The Makefile sets default values for all parameters, some of which are inferred based on the system. It then includes config.mak, which allows the user to override any of these values. So that's pretty similar to what I described, modulo the filenames. I'd say it's more friendly if you don't need to tweak any of the defaults in the common case, but less friendly if you always need to tweak something/everything (you really want a template file then and not covering (c) is a problem). I don't see anything wrong with having a template file documenting the parameters, but I think it's important that there are sensible defaults in place when the user's configuration file does not specify a value for a parameter. It wasn't clear to me from your definition that there were defaults to be overridden by the user's configuration file, as opposed to forcing the user to define certain values and causing an error if those are not defined. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash deletes/drops changes of
On Fri, May 24, 2013 at 12:14:16PM +0200, Petr Baudis wrote: On Fri, May 24, 2013 at 11:06:12AM +0100, John Keeping wrote: I don't see anything wrong with having a template file documenting the parameters, but I think it's important that there are sensible defaults in place when the user's configuration file does not specify a value for a parameter. It wasn't clear to me from your definition that there were defaults to be overridden by the user's configuration file, as opposed to forcing the user to define certain values and causing an error if those are not defined. That's the case in plenty of situations - when specifying usernames and passwords and server hostnames, paths to cross-compiling environments that pretty much everyone has at a different place, and so on. Yeah, I didn't mean to say that everything can have a sensible default. Going back to where this started, in the omxplayer Makefile, I would map my suggestion to a change like this: * Change most of the := in Makefile.include to = so that the order of variable definition matters less * Move Makefile.include to Makefile.defaults * Change the include Makefile.include at the top of Makefile to: include Makefile.defaults -include Makefile.config * Add Makefile.config to .gitignore So that it continues to Just Work for people using buildroot but you can create Makefile.config to override those defaults. I agree that this isn't possible in all cases, and your template approach is certainly useful for configuration files - particularly because those templates can be included in end-user documentation or the installation as they are likely to be needed in the installed application and not just development. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash deletes/drops changes of
On Fri, May 24, 2013 at 01:03:22PM +0200, Petr Baudis wrote: On Fri, May 24, 2013 at 11:40:18AM +0100, John Keeping wrote: So that it continues to Just Work for people using buildroot but you can create Makefile.config to override those defaults. Indeed, that doesn't cover some corner cases of (c), but that's not a big deal in practice I guess. My point still stands - this is extra hassle, done just for the sake of the tool; I think the tool should not get in the way. Moreover, it's not the default solution for your typical original author and therefore you will still often find yourself in a situation where you have to deal with a setup that's broken already. I think we're in violent agreement here. I can see that there are cases where an --ignore-changes option that behaves like --assume-unchanged but without ever overwriting the local file is a useful feature. I was simply trying to point at what I consider best practices for makefiles, which was relevant for the example you gave. Sorry if that was unclear. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash deletes/drops changes of
On Fri, May 24, 2013 at 03:34:26PM +, Jim Greenleaf wrote: Phil Hord phil.hord at gmail.com writes: The wording of --ignore-changes suffers the same lack of clarity that --assume-unchanged does. What's better? --sequester is probably too obscure. Maybe --hold. Or --silence. Or --shut-up. How about --freeze? I wonder if this would be better as a file rather than another option to git-update-index. We already have .git/info/exclude so we could add .git/info/freeze or .git/info/local with the same syntax as the normal .gitignore file. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git stash deletes/drops changes of
On Fri, May 24, 2013 at 03:42:37PM +, Jim Greenleaf wrote: John Keeping john at keeping.me.uk writes: I wonder if this would be better as a file rather than another option to git-update-index. We already have .git/info/exclude so we could add .git/info/freeze or .git/info/local with the same syntax as the normal .gitignore file. .git/info/freeze would be a good solution. It would avoid the need to add a new class of files for git-status, while keeping a simple, familiar record of all frozen files in a single location. Now I've thought about it a bit more, I'm not sure this does work. If an entry in the freeze list means ignore local changes in this file, we really want to be talking about local changes relative to some base. Otherwise, what happens if the upstream file is radically altered? A user probably doesn't want to keep their file unchanged when this happens. So we don't just want to store the filename, we want to store the version of the file that the user chose to ignore. One way to do this might be to mark the file as a conflict whenever a change to it comes in and ignore the freeze file when there is a conflict in the index. But then we either need to introduce a new command to manage this state or some way for the user to perform Git operations ignoring the freeze file, otherwise how can the user pull down updates? Perhaps a more user-friendly way to handle this would be to introduce auto-stash around any operation that will modify a frozen file. So we stash the user's (frozen) changes and then apply them after changing the file. If there are conflicts then these are marked in the index and must be resolved, then the unstaged changes in the file are ignored again. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: first parent, commit graph layout, and pull merge direction
On Thu, May 23, 2013 at 12:29:59PM +0200, Andreas Krey wrote: On Thu, 23 May 2013 05:48:38 +, John Szakmeister wrote: ... This is a feature of `git pull` that I really despise. I really wish `git pull` treated the remote as the first parent in its merge operation. I'd actually only like it that way when pulling from the tracking branch, not for any pull. I'll add my voice to the annoyed by this pile ;-) I've been annoyed by this at $DAYJOB recently. A lot of people seem to blindly git pull without much thought about how the history is ending up and what they actually want to do. I wonder if it would make sense for git pull (with no arguments) to pass --ff-only to git-merge, allowing this to be overridden with --rebase and --merge (which doesn't currently exist). With some suitable advice output we could hopefully educate users about how to shape their history. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: first parent, commit graph layout, and pull merge direction
On Thu, May 23, 2013 at 09:01:15AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: I've been annoyed by this at $DAYJOB recently. A lot of people seem to blindly git pull without much thought about how the history is ending up and what they actually want to do. I think these two are essentially the same thing, and having an option to flip the heads of a merge only solves a half of the problem. A merge that shows everybody else's work merged into your history means you are the integrator, the keeper of the main history. And the first-parent view of the history is useful only when the keeper of the main history takes good care of the main history. When you are using a central shared repository workflow, if you had and used an option to flip the heads of a merge to record what you have done so far as a side branch of what everybody else did to do the merge, or if you rebased your work on top of what everybody else did, the first-parent view would make a bit more sense than what you currently get. At least, everybody else's work will not appear as a side branch that does 47 unrelated things, and your work will appear as a side branch. That is a big plus. But the other half of the problem still remains, i.e. what they actually want to do. People tend to do too many pull when their work is not ready, only to catch up, and that is the real problem. ... One obvious way to solve it is to use a topic branch workflow (the first picture above; 'x's are built not on local 'master'), and you do a git pull from the shared repository while you are on your 'master', which is free of your 'x's until that 6-commit series is complete and ready. Then you locally merge that topic branch and push it back for everybody to see, which will give you the first picture in this message. Incidentally, this does not need the flip the heads option. Yes, I don't think this is as much of a problem when using a topic branch workflow, because it's clear what the history should look like and users are expected to get it right. Where I see this is when people are aiming for a linear history but don't get that because with git pull to catch up they end up with these backwards merges. In these cases, I think what users really want is git pull --rebase. I have to wonder how often git pull with no arguments actually does what users really want (even if they don't know it!) when it doesn't result in a fast-forward (and pull.rebase isn't configured). Hence my suggestion to error when git pull doesn't result in a fast-forward and no branch name is specified. We could give some advice like: Your local changes are not included in the local branch and you haven't told Git how to preserve them. If you want to rebase your changes onto the modified upstream branch, run: git pull --rebase Solving half a problem is better than solving no problem, and especially because not all changes need to be multi-commit series but can be done directly, perfectly and fully on the local 'master' (i.e. 2+3+1=6 split would not happen for such changes). For these reasons, I personally am not strongly opposed to a flip the heads option, if implemented cleanly. But people need to realize that it is not solving the other half, a more fundamental problem some people have in their workflow. Yes, but some users don't realise that their workflow is broken, and perhaps we can nudge them in the right direction. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: first parent, commit graph layout, and pull merge direction
On Thu, May 23, 2013 at 02:01:39PM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: I have to wonder how often git pull with no arguments actually does what users really want (even if they don't know it!) when it doesn't result in a fast-forward (and pull.rebase isn't configured). If you are in a totally centralized shared repository mindset without using topic branch workflow, --first-parent would not help you. In your history the second parent is more likely to be the mainline. So for them git pull that either fast-forward when it can, or makes a merge that records the then-current state of the central shared repository, is perfectly sensible. They will view gitk and see all the changes, git shortlog and git log --no-merges will give them what they expect. Yes, but for people used to a cleaner history it's confusing to see the mainline branch and one small change the wrong way round. When I see people doing this, it's normally something like: ... do some work for several hours... git commit -a git push # fails because it's not a fast forward git pull git push In this scenario, just adding --rebase to git pull actually results in a much more sensible history. Hence my suggestion to error when git pull doesn't result in a fast-forward and no branch name is specified. We could give some advice like: Your local changes are not included in the local branch and you haven't told Git how to preserve them. If you want to rebase your changes onto the modified upstream branch, run: git pull --rebase I can parse the first paragraph above, but cannot make much sense out of it. Unless you are talking about local changes that are not committed yet, that is. But in that case I fail to see what it has to do with the current discussion, or suggestion to use rebase. This isn't about swap parents, it's about helping people realise that just git pull isn't necessarily the best thing for them to do, and that they may want --rebase. So I was asking if it would be sensible (possibly in Git 2.0) to make git-pull pass --ff-only to git-merge by default. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-submodule nested subrepo bug (Segmentation fault)
I'm guessing Kirill meant to send this to the list and not just to me. It looks to me like the segfault is in MSys's mkdir.exe and not a Git process. - Forwarded message from Kirill Berezin ene...@develop-project.ru - From: Kirill Berezin ene...@develop-project.ru To: John Keeping j...@keeping.me.uk Date: Wed, 22 May 2013 09:54:47 +0400 Message-ID: caaobgf-pe_6pf3af5uba8rfkwxa43gxeg9romjf62usz7k4...@mail.gmail.com Subject: Re: git-submodule nested subrepo bug (Segmentation fault) Ok, version is: 1.8.1.msysgit.1 Segmentation fault at the git clone --recursive http://github.com/Exsul/al_server 0 [main] mkdir 6596 open_stackdumpfile: Dumping stack trace to mkdir.exe.stackdump C:\Users\\libexec\git-core\git-submodule: line 181: 6596 Segmentation fault (core dumped) mkdir -p $ditdir_base fatal: Could not switch to 's:/USER//al_server/.git/modules/': No such file or directory Clone of 'https://.../Exsul/chat.git' into submodule path 'chat' failed PS so much mails per day... 2013/5/20 Kirill Berezin ene...@develop-project.ru: This is standart github shell(Windows 7). Right now i cant access my home platform, will tell you tommorow. 2013/5/20 John Keeping j...@keeping.me.uk: On Mon, May 20, 2013 at 09:32:21AM +0400, Kirill Berezin wrote: When you trying to add submodule, that already has submodule, it craches. For example you could try: git clone --recursive http://github.com/Exsul/al_server Which version of Git were you using? I was not able to reproduce this with 1.8.3-rc3. - End forwarded message - -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] contrib/git-subtree: Use /bin/sh interpreter instead of /bin/bash
On Mon, May 20, 2013 at 03:36:58PM -0700, Junio C Hamano wrote: Dmitry Marakasov amd...@amdmi3.ru writes: Use /bin/sh interpreter instead of /bin/bash for contrib/git-subtree: it's required for systems which don't use bash by default (for example, FreeBSD), while there seem to be no bashisms in the script (confirmed by looking through the source and tesing subtree functionality with FreeBSD's /bin/sh) to require specifically bash and not the generic posix shell. Has anybody audited to make sure that the script itself is free of bash-isms? I somehow had an impression that in the past it was littered with bash-isms like function local variables and array variables and assumed that the #!/bin/bash was necessary. I did a quick eyeballing and did not see anything glaringly bash-only, but I may have missed something (the coding style is so different from the core part of Git Porcelains and distracting for me to efficiently do a good job of scanning). I ran the test suite with dash and everything passed. checkbashisms doesn't find any problems either. Signed-off-by: Dmitry Marakasov amd...@amdmi3.ru --- contrib/subtree/git-subtree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 8a23f58..5701376 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh # # git-subtree.sh: split/join git repositories in subdirectories of this one # -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Workflow Help
On Tue, May 21, 2013 at 10:59:17AM +1000, Quilkey, Tony wrote: I am looking at formulating and then documenting our vcs workflow using Git at work. I have an idea of how I want things to work, but am a little hazy on some of the details. Our basic workflow will be based around: http://nvie.com/posts/a-successful-git-branching-model, with a few exceptions. We would like to create our release-* branches from the last release tag. From there, we would like the ability to cherry pick (or take the complete diff) commits from the develop branch. So, we are after is: 1) Create topic (feature) branches from develop, and merge back into develop when complete. 2) Once it is decided we are packaging a release, make a release-* branch from the previous release tag. 3) Cherry pick/merge/whatever any commits we want from develop into the new release-* until it is complete. 4) Merge the new release-* branch into master and tag it. Repeat as necessary. At the moment I am a little stuck on how exactly we can cherry pick stuff from develop into a release-* branch. I'm not even sure this approach is exactly what we should be doing. Having been involved in a couple of projects that use cherry-pick like this, I strongly advise against doing this. It makes it much harder than it needs to be to find out which branches contain a particular commit. The workflow described in the URL above does the more sensible thing of periodically merging the release branch(es) back into master (or develop). This is similar to the workflow Junio uses to develop Git itself, which is described in gitworkflows(7). The idea is to start your topic branch from the oldest release to which a bugfix must be applied, then merge it into the appropriate release branches. Then you merge this branch upwards into the later release branches and your development branch. So your development branch always contains all release branches (not just similar commits, but the *same* commits so that each release branch tip is an ancestor of the development branch's tip). This means that you can use git branch --contains or git describe --contains to answer the question which release(s) contain this commit?, whereas with cherry picking there is no easy and reliable way to do so. Our main concern is that at this stage, there is no guarantee that all commits within develop can be pulled into a release. One advantage of starting a bugfix topic branch from the oldest release it applies to is that you are developing and testing that fix on the release code. If it doesn't apply cleanly to the development branch then you fix the conflict when merging. Of course you may start a bugfix branch from the wrong place, in which case you would have to cherry pick the commits back to an older branch, but this should be a rare occurrence and will sort itself out as you merge the fix upwards. In regards to how we can achieve the above results any input would be much appreciated. Or if there are any other better options available, I'm all ears. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] patch-ids: check modified paths before calculating diff
On Sun, May 19, 2013 at 11:36:23PM -0700, Jonathan Nieder wrote: I don't know what it should mean to try to use --cherry without --no-merges or --first-parent, so I guess this is harmless. Currently --no-merges doesn't actually get passed down this far. We do the patch ID calculations without taking that into account. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-submodule nested subrepo bug (Segmentation fault)
On Mon, May 20, 2013 at 09:32:21AM +0400, Kirill Berezin wrote: When you trying to add submodule, that already has submodule, it craches. For example you could try: git clone --recursive http://github.com/Exsul/al_server Which version of Git were you using? I was not able to reproduce this with 1.8.3-rc3. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH] patch-ids: check modified paths before calculating diff
When using git cherry or git log --cherry-pick we often have a small
number of commits on one side and a large number on the other. In
revision.c::cherry_pick_list we store the patch IDs for the small side
before comparing the large side to this.
In this case, it is likely that only a small number of paths are touched
by the commits on the smaller side of the range and by storing these we
can ignore many commits on the other side before unpacking blobs and
diffing them.
This improves performance in every example I have tried (times are best
of three, using git.git):
Before:
$ time git log --cherry master...jk/submodule-subdirectory-ok /dev/null
real0m0.373s
user0m0.341s
sys 0m0.031s
After:
$ time git log --cherry master...jk/submodule-subdirectory-ok /dev/null
real0m0.060s
user0m0.055s
sys 0m0.005s
Before:
$ time git log --cherry next...pu /dev/null
real0m0.661s
user0m0.617s
sys 0m0.044s
After:
$ time git log --cherry next...pu /dev/null
real0m0.509s
user0m0.478s
sys 0m0.030s
Signed-off-by: John Keeping j...@keeping.me.uk
---
patch-ids.c | 142
patch-ids.h | 3 ++
2 files changed, 145 insertions(+)
diff --git a/patch-ids.c b/patch-ids.c
index bc8a28f..912f40c 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -1,5 +1,6 @@
#include cache.h
#include diff.h
+#include diffcore.h
#include commit.h
#include sha1-lookup.h
#include patch-ids.h
@@ -16,6 +17,137 @@ static int commit_patch_id(struct commit *commit, struct
diff_options *options,
return diff_flush_patch_id(options, sha1);
}
+struct collect_paths_info {
+ struct string_list *paths;
+ int searching;
+};
+
+static int collect_paths_recursive(int n, struct tree_desc *t,
+ const char *base, struct pathspec *pathspec,
+ struct collect_paths_info *data);
+
+static int same_entry(struct name_entry *a, struct name_entry *b)
+{
+ if (!a-sha1 || !b-sha1)
+ return a-sha1 == b-sha1;
+ return !hashcmp(a-sha1, b-sha1)
+ a-mode == b-mode;
+}
+
+static char *traverse_path(const struct traverse_info *info,
+ const struct name_entry *n)
+{
+ char *path = xmalloc(traverse_path_len(info, n) + 1);
+ return make_traverse_path(path, info, n);
+}
+
+static int add_touched_path(struct collect_paths_info *info, const char *path)
+{
+ if (info-searching) {
+ if (!string_list_has_string(info-paths, path))
+ return -1;
+ }
+ string_list_insert(info-paths, path);
+ return 0;
+}
+
+static inline const unsigned char *dir_sha1(struct name_entry *e)
+{
+ if (S_ISDIR(e-mode))
+ return e-sha1;
+ return NULL;
+}
+
+static int collect_touched_paths_cb(int n, unsigned long mask,
+ unsigned long dirmask, struct name_entry *entry,
+ struct traverse_info *info)
+{
+ struct collect_paths_info *collect_info = info-data;
+ if (n == 1) {
+ /* We're handling a root commit - add all the paths. */
+ if (entry[0].sha1 !S_ISDIR(entry[0].mode)) {
+ if (add_touched_path(collect_info, entry[0].path))
+ return -1;
+ } else if (S_ISDIR(entry[0].mode)) {
+ char *newbase = traverse_path(info, entry);
+ struct tree_desc t[1];
+ void *buf0 = fill_tree_descriptor(t, entry[0].sha1);
+ int error = collect_paths_recursive(1, t, newbase,
+ info-pathspec, collect_info);
+ free(buf0);
+ free(newbase);
+ if (error 0)
+ return error;
+ }
+ return mask;
+ }
+
+ if (same_entry(entry+0, entry+1))
+ return mask;
+
+ if (entry[0].mode !S_ISDIR(entry[0].mode))
+ if (add_touched_path(collect_info, entry[0].path))
+ return -1;
+ if (entry[1].mode !S_ISDIR(entry[1].mode))
+ if (add_touched_path(collect_info, entry[1].path))
+ return -1;
+
+ if ((entry[0].sha1 S_ISDIR(entry[0].mode)) ||
+ (entry[1].sha1 S_ISDIR(entry[1].mode))) {
+ char *newbase = traverse_path(info,
+ S_ISDIR(entry[0].mode) ? entry+0 : entry+1);
+ struct tree_desc t[2];
+ void *buf0 = fill_tree_descriptor(t+0, dir_sha1(entry+0));
+ void *buf1 = fill_tree_descriptor(t+1, dir_sha1(entry+1));
+ int error = collect_paths_recursive(2, t, newbase,
+ info-pathspec, collect_info);
+ free(buf0);
+ free(buf1);
+ free(newbase);
+ if (error 0)
+ return
Re: .gitignore behavior on Mac
On Sat, May 18, 2013 at 08:36:42PM +0200, Peter Lauri wrote: Shouldn't this be valid? I would expect to NOT see the core/inc/config.inc.php in the git status output... Peters-MacBook-Air:dt-git plauri$ cat .gitignore .buildpath .project .settings/ web/pjotr.php core/inc/config.inc.php dt_error.log process_wrapper.sh Peters-MacBook-Air:dt-git plauri$ git status # On branch local/DT-7_gantt # Changes not staged for commit: # (use git add file... to update what will be committed) # (use git checkout -- file... to discard changes in working directory) # # modified: .gitignore # modified: core/inc/config.inc.php core/inc/config.inc.php is already in the repository. Git won't ignore files that are already tracked. If you remove the file from the index: git rm --cached core/inc/config.inc.php then you'll see it as deleted in git status but it won't appear in the untracked files section even though it's still there in the working tree. # Untracked files: # (use git add file... to include in what will be committed) # # out.ionel # tree.py no changes added to commit (use git add and/or git commit -a) Peters-MacBook-Air:dt-git plauri$ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: .gitignore behavior on Mac
On Sat, May 18, 2013 at 08:43:57PM +0200, Peter Lauri wrote: But I just don't want to see that darn file. It is a config file that I have changed, and I don't want to need to stash it for each git svn action I want to perform... Any solution for that? Read about --assume-unchanged in git-update-index(1). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: git cvsimport implications
On Fri, May 17, 2013 at 11:10:03AM +0200, Michael Haggerty wrote: On 05/15/2013 08:03 PM, Eugene Sajine wrote: My primary goal was to understand better what are the real problems that we might have with the way we use git cvsimport, so I was not asking about the guarantee of the cvsimport to import things correctly, but if there is a guarantee the import will result in completely broken history. So what are you going to do, use cvsimport whenever you cannot *prove* that it is wrong? You sure have low standards for your software. The only *useful* guarantee is that software is *correct* under defined circumstances. I don't think anybody has gone to the trouble to figure out when that claim can be made for cvsimport. If the cvsimport is that broken - is there any plan to fix it? For one-time imports, the fix is to use a tool that is not broken, like cvs2git. Alternatively, Eric Raymond claims to have developed a new version of cvsps that is not quite as broken as the old version. Presumably cvsimport would be not quite as broken if used with the new cvsps. cvsimport doesn't work with the cvsps-3 - we decided to stick with the version we have (using cvsps-2) because that is the only option that supports incremental import; those using if for that are used to its deficiencies and there is no plan to improve it. The manpage notes that it uses a deprecated version of cvsps and recommends alternatives for one-shot imports. There is a version of git-cvsimport script in the cvsps-3 repository that works with it, but it does not support incremental import in the same was as git.git's git-cvsimport so it will not replace the version in git.git. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] describe: Add --left-only option
On Fri, May 17, 2013 at 03:24:26PM +0100, Mike Crowe wrote: Only consider the first parent commit when walking the commit history. This is useful if you only wish to match tags on your branch after a merge. For consistency with git log should this be called --first-parent? In git log --left-only takes effect only when considering a symmetric range, which git describe isn't. Whereas --first-parent triggers precisely the behaviour described here. Signed-off-by: Mike Crowe m...@mcrowe.com --- Documentation/git-describe.txt | 9 - builtin/describe.c | 5 + t/t6120-describe.sh| 3 +++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt index 28e5ec0..67f7d8e 100644 --- a/Documentation/git-describe.txt +++ b/Documentation/git-describe.txt @@ -88,6 +88,11 @@ OPTIONS --always:: Show uniquely abbreviated commit object as fallback. +--left-only:: + Consider only the left-most parent of any commit with multiple + parents. This is useful when you wish to not match tags on branches + merged in the history of the target commit. + EXAMPLES @@ -149,7 +154,9 @@ is found, its name will be output and searching will stop. If an exact match was not found, 'git describe' will walk back through the commit history to locate an ancestor commit which has been tagged. The ancestor's tag will be output along with an -abbreviation of the input committish's SHA-1. +abbreviation of the input committish's SHA-1. If '--left-only' was +specified then the walk will only consider the first parent of each +commit. If multiple tags were found during the walk then the tag which has the fewest commits different from the input committish will be diff --git a/builtin/describe.c b/builtin/describe.c index 6636a68..44a4ca5 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -21,6 +21,7 @@ static int debug; /* Display lots of verbose info */ static int all; /* Any valid ref can be used */ static int tags; /* Allow lightweight tags */ static int longformat; +static int left_only; static int abbrev = -1; /* unspecified */ static int max_candidates = 10; static struct hash_table names; @@ -336,6 +337,9 @@ static void describe(const char *arg, int last_one) commit_list_insert_by_date(p, list); p-object.flags |= c-object.flags; parents = parents-next; + + if (left_only) + break; } } @@ -404,6 +408,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) OPT_BOOLEAN(0, all,all, N_(use any ref)), OPT_BOOLEAN(0, tags, tags, N_(use any tag, even unannotated)), OPT_BOOLEAN(0, long, longformat, N_(always use long format)), + OPT_BOOLEAN(0, left-only, left_only, N_(only follow left parent)), OPT__ABBREV(abbrev), OPT_SET_INT(0, exact-match, max_candidates, N_(only output exact matches), 0), diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index f67aa6f..aea7463 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -110,6 +110,9 @@ check_describe tags/e --all HEAD^^^ check_describe B-0-* --long HEAD^^2^ check_describe A-3-* --long HEAD^^2 +check_describe c-7-* --tags +check_describe e-3-* --left-only --tags + : err.expect check_describe A --all A^0 test_expect_success 'no warning was displayed for A' ' -- 1.8.3.rc2.14.g3089c4d -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] difftool: fix dir-diff when file does not exist in working tree
Commit 02c5631 (difftool --dir-diff: symlink all files matching the
working tree, 2013-03-14) does not handle the case where a file that is
being compared does not exist in the working tree. Fix this by checking
for existence explicitly before running git-hash-object.
Reported-by: Kevin Bracey ke...@bracey.fi
Signed-off-by: John Keeping j...@keeping.me.uk
---
This fixes a regression in 1.8.3-rc0.
git-difftool.perl | 9 -
t/t7800-difftool.sh | 7 +++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 6780292..0a1cb0a 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -92,7 +92,14 @@ sub use_wt_file
return 0;
}
- my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
+ my $wt_sha1;
+ if (-e $workdir/$file) {
+ $wt_sha1 = $repo-command_oneline('hash-object',
$workdir/$file);
+ } else {
+ # If the file doesn't exist in the working tree, use something
+ # that cannot match a SHA-1.
+ $wt_sha1 = '';
+ };
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
return ($use, $wt_sha1);
}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a6bd99e..d46f041 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
)
'
+run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '
+ test_when_finished git reset --hard
+ rm file2
+ git difftool --dir-diff $symlinks --extcmd ls branch master output
+ grep file2 output
+'
+
write_script .git/CHECK_SYMLINKS \EOF
for f in file file2 sub/sub
do
--
1.8.3.rc2.285.gfc18c2c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] difftool: fix dir-diff when file does not exist in working tree
Commit 02c5631 (difftool --dir-diff: symlink all files matching the
working tree, 2013-03-14) does not handle the case where a file that is
being compared does not exist in the working tree. Fix this by checking
for existence explicitly before running git-hash-object.
Reported-by: Kevin Bracey ke...@bracey.fi
Signed-off-by: John Keeping j...@keeping.me.uk
---
On Fri, May 17, 2013 at 11:10:40AM -0700, Junio C Hamano wrote:
John Keeping j...@keeping.me.uk writes:
Commit 02c5631 (difftool --dir-diff: symlink all files matching the
working tree, 2013-03-14) does not handle the case where a file that is
being compared does not exist in the working tree. Fix this by checking
for existence explicitly before running git-hash-object.
Reported-by: Kevin Bracey ke...@bracey.fi
Signed-off-by: John Keeping j...@keeping.me.uk
---
This fixes a regression in 1.8.3-rc0.
git-difftool.perl | 9 -
t/t7800-difftool.sh | 7 +++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 6780292..0a1cb0a 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -92,7 +92,14 @@ sub use_wt_file
return 0;
}
- my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
+ my $wt_sha1;
+ if (-e $workdir/$file) {
+ $wt_sha1 = $repo-command_oneline('hash-object',
$workdir/$file);
+ } else {
+ # If the file doesn't exist in the working tree, use something
+ # that cannot match a SHA-1.
+ $wt_sha1 = '';
Yuck.
that cannot match might be a good justification to say this does
not break the next line to set $use and forces it to false, but
when we return false in $use, the value of $wt_sha1 is not used
needs to be said to convince why this is a safe change.
But if $sha1 is $null_sha1, we do end up setting $use to true and
the caller would stuff the empty $wt_sha1 to form:
$wtindex .= $rmode \$dst_path\0;
Is that what we want to do here, or is it a will never happen
condition? If the latter, the reason need to be described in this
comment (and in the log).
It can't ever happen because we only call this if the mode is non-zero
in which case the SHA-1 is only null if there are unstaged changes.
However, I think this revised version is much clearer.
git-difftool.perl | 6 ++
t/t7800-difftool.sh | 7 +++
2 files changed, 13 insertions(+)
diff --git a/git-difftool.perl b/git-difftool.perl
index 6780292..8a75205 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -92,6 +92,12 @@ sub use_wt_file
return 0;
}
+ if (! -e $workdir/$file) {
+ # If the file doesn't exist in the working tree, we cannot
+ # use it.
+ return (0, $null_sha1);
+ }
+
my $wt_sha1 = $repo-command_oneline('hash-object', $workdir/$file);
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
return ($use, $wt_sha1);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index a6bd99e..d46f041 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -356,6 +356,13 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
)
'
+run_dir_diff_test 'difftool --dir-diff when worktree file is missing' '
+ test_when_finished git reset --hard
+ rm file2
+ git difftool --dir-diff $symlinks --extcmd ls branch master output
+ grep file2 output
+'
+
write_script .git/CHECK_SYMLINKS \EOF
for f in file file2 sub/sub
do
--
1.8.3.rc2.285.gfc18c2c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: is this a bug of git-diff?
On Wed, May 15, 2013 at 11:34:41AM +0200, Matthieu Moy wrote: Antoine's answer is correct. In addition, I'd say that you may want to enable color in the output to make it clearer (the @@ ... @@ part would be colored, but not the function name): git config --global color.ui auto I wonder if that should be the default. I've advised a lot of people to turn it on and it seems to me that a user is much more likely to go looking for a turn color off option than realise that color is an option at all. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] make color.ui default to 'auto'
On Wed, May 15, 2013 at 08:42:39AM -0700, Junio C Hamano wrote: Matthieu Moy matthieu@imag.fr writes: Many tutorials tell the users to set color.ui=auto as a very first step. These tutorials would benefit from skiping this step and starting the real Git manipualtions earlier. Other beginners do not know about color.ui=auto, and may not discover it by themselves, hence live with blackwhite outputs while they may have prefered colors. A few people (e.g. color-blind) prefer having no colors, but they can easily set color.ui=never for this (and googling disable colors in git already tells them how to do so). The above two paragraphs do not make a good justification [*1*]. The former can just as easily websearch for enable colours in git as the latter would for disable in order to avoid having to live with distraction while they may have preferred monochrome. The train of thought that is a sufficient justification for this change is Our document and third-party tutorials often start with setting color.ui=auto configuration. leading to Our recommendation is to enable colour on terminals. which in turn leading to Why is our default monochrome, against our own recommendation?. Saying anything more, like who are the majority or how easily the default can be overridden, is unnecessary, I think [*2*]. As this is purely a UI thing, and since daa0c3d97176 (color: delay auto-color decision until point of use, 2011-08-17), the logic to decide when auto colouring is triggered is centrary controlled (hence it is much less likely than before that color.ui=auto could misfire when it shouldn't), I agree that this does not even deserve a warning. You could even sell it as a pure bugfix (we recommend users to use auto colouring but we did not set it up for users). The default value is changed, and the documentation is reworded to mention color.ui=false first, since the primary use of color.ui after this change is to disable colors, not to enable it. Good. I'm starting to wonder why we didn't do this earlier ;-). Documentation/config.txt | 11 ++- color.c | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 1009bfc..97550be 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -913,11 +913,12 @@ color.ui:: as `color.diff` and `color.grep` that control the use of color per command family. Its scope will expand as more commands learn configuration to set a default for the `--color` option. Set it + to `false` or `never` if you prefer Git commands not to use + color unless enabled explicitly with some other configuration + or the `--color` option. Set it to `always` if you want all + output not intended for machine consumption to use color, to + `true` or `auto` (this is the default since Git 2.0) if you + want such output to use color when written to the terminal. OK, so this is planned for 2.0? I would vote for just considering this a bugfix as you say above and therefore not worthy of any special treatment, so it should end up in whatever the next release is after it hits master. The changes that are being held back for 2.0 change how commands operate and we don't provide any overrides for those; this is just a cosmetic change to the default output format. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] merge-base: add --merge-child option
On Sun, May 12, 2013 at 10:02:39PM -0700, Junio C Hamano wrote: Kevin Bracey ke...@bracey.fi writes: On 12/05/2013 19:58, John Keeping wrote: With the patch below, the --ancestry-path version drops to under 2 seconds. I'm not sure if this is a good idea though. It helps me say I know nothing that isn't on the ancestry path can be patch-identical, so don't bother checking if it is but it regresses users who want the full cherry-pick check while only limiting the output. Hmm. Should an excluded commit be a valid comparator? Is it sensible/correct to show a left commit as = to a right commit that has been excluded by the revision specifiers? Doesn't sound right to me. Neither to me. OK. I'll add some tests and send a proper patch once 1.8.3 is out of the way. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Sun, May 12, 2013 at 03:19:49PM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: But it is not a big problem. Either 3-way merge notices that there is nothing new, or you get a conflict and have chance to inspect what is going on. It's not a problem here, but false negatives would be annoying if you're looking at git log --cherry-mark. The primary thing to notice is that it is not a new problem with or without the caching layer. As Linus mentioned how patch-ids are computed by ignoring offsets and whitespaces, the filtering is done as a crude approximation and false negatives are part of design, so making the cache more complex by recording hash of the binary and/or options used to compute misses the fundamental. The caching layer could also introduce false positives though, which is more serious. If you cache patch IDs with a pathspec restriction and then run a command that uses the cache with no such restriction you could hit a patch that is the same for those paths but also touches other paths that you don't want to ignore and mark it as patch identical even though it is not. Adding a hash of the diffopts fixes that. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Mon, May 13, 2013 at 06:53:29AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: The caching layer could also introduce false positives though, which is more serious. If you cache patch IDs with a pathspec restriction ... What? What business does patch-id have with pathspec-limited diff generation? You do not rebase or cherry-pick with pathspec, so unless you are populating the patch-id cache at a wrong point (like, say whenevern git show $commit is run), I am not sure why pathspec limit becomes even an issue. revision.c::cherry_pick_list() sets the pathspec to what was specified in the revision options. It's done that since commit 36d56de (Fix --cherry-pick with given paths, 2007-07-10) and t6007 tests that it works. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Mon, May 13, 2013 at 07:46:09AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: On Mon, May 13, 2013 at 06:53:29AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: The caching layer could also introduce false positives though, which is more serious. If you cache patch IDs with a pathspec restriction ... What? What business does patch-id have with pathspec-limited diff generation? You do not rebase or cherry-pick with pathspec, so unless you are populating the patch-id cache at a wrong point (like, say whenevern git show $commit is run), I am not sure why pathspec limit becomes even an issue. revision.c::cherry_pick_list() sets the pathspec to what was specified in the revision options. It's done that since commit 36d56de (Fix --cherry-pick with given paths, 2007-07-10) and t6007 tests that it works. Then the caching should be automatically turned off when pathspec is given. That was my first thought, but since we can be affected by other diff options set in the user's config as well it ended up being simpler to include it in the options hash and use that. This has the advantage that you get the benefit of the cache if you run git log --cherry-mark with the same paths more than once. In my testing the cache is beneficial as soon as you examine more than one similar range (e.g. master...feature-A and then master...feature-B). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Mon, May 13, 2013 at 08:45:21AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: This has the advantage that you get the benefit of the cache if you run git log --cherry-mark with the same paths more than once. In my testing the cache is beneficial as soon as you examine more than one similar range (e.g. master...feature-A and then master...feature-B). OK, so perhaps the notes that are keyed with commit ID will record multiple entries, one for each invocation pattern (i.e. all pathspec given, possibly with nonstandard options)? That would be possible, but I didn't do it in the current version of the patch. git diff -- t Documentation and git diff -- Docu\* t, even though they use different pathspec, would produce the same diff; instead of pathspec you may need to key with the actual list of paths in the patch, though. Maybe, but I think that would be overkill. I'm interested to see how much of a benefit we could get by not calculating the patch ID of any commits on the larger side of a symmetric range that touch paths outside the set touched by the smaller side. (revision.c::cherry_pick_list() remembers patch IDs for the smaller side of a symmetric range and then checks if anything on the larger side matches so this fits in naturally.) In my usage I generally compare a relatively small topic branch against whatever has happened in some upstream branch so I think this could give a big improvement but I haven't had time to try it yet. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Sat, May 11, 2013 at 08:00:44PM -0700, Junio C Hamano wrote: Linus Torvalds torva...@linux-foundation.org writes: On Sat, May 11, 2013 at 2:49 PM, John Keeping j...@keeping.me.uk wrote: Hmm... I hadn't realised that. Looking a bit closer, it looks like init_patch_ids sets up its own diffopts so its not affected by the command line (except for pathspecs which would be easy to check for). Of course that still means it can be affected by settings in the user's configuration. .. and in the actual diff algorithm. As to the objection side of the argument, I already said essentially the same thing several months ago: http://thread.gmane.org/gmane.comp.version-control.git/202654/focus=202898 and do not have much to add [*1*]. However. The use of patch-id in cherry and rebase is to facilitate avoiding to replay commits that are obviously identical to the ones you have in your history. The cached patch id for an existing old commit may differ from a patch id you freshly compute for a new commit you are trying to see if it truly new, even though they may represent the same change. So we may incorrectly think such a new commit is not yet in your history and attempt to replay it. But it is not a big problem. Either 3-way merge notices that there is nothing new, or you get a conflict and have chance to inspect what is going on. It's not a problem here, but false negatives would be annoying if you're looking at git log --cherry-mark. A conceptually much larger and more problematic issue is that we may discard a truly new change that you still need as an old one you already have due to a hash collision and discard it. Because the hash space of SHA-1 is so large, however, it is not a problem in practice, and more importantly, that hash space is just as large as the hash space used by Git to reduce a patch to a patch id, the filtering done with patch-id in cherry and rebase _already_ have that exact problem with or without this additional cache layer. A stale cache may make the possibility of lost change due to such a hash collision merely twice as likely. ... it's a the patch ID actually ignores a lot of data in order to give the same ID even if lins have been added above it, and the patch is at different line numbers etc. Yes. So maybe it doesn't matter. But at the same time, I really think caching patch ID's should be something people should be aware of is fundamentally wrong, even if it might work. I do not think it is caching patch ID that people should be aware of is fundamentally wrong. What is fundamentally wrong, even if it might work, is using patch ID itself. And quite frankly, if you do rebases etc so much that you think patch ID's are so important that they need to be cached, you may be doing odd/wrong things. And that, too ;-) I've never noticed a problem with rebases, it's when I use git log --cherry master... to see if patches I've sent to a mailing list have been picked up. To take Git as an example (albeit a bad one because What's Cooking is a more useful way to track patch state here), if I compare this patch to pu I have: $ git rev-list --left-right --count pu... 234 1 and caching patch IDs takes that from ~0.6s to ~0.1s. When doing that over several branches consecutively that makes a big difference to the overall runtime, especially because most of the commits of interest will be cached during the first one. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Sat, May 11, 2013 at 06:57:02PM -0500, Johannes Schindelin wrote: On Sat, 11 May 2013, Linus Torvalds wrote: [...] I really think caching patch ID's should be something people should be aware of is fundamentally wrong, even if it might work. Given the incredible performance win, I would say it is still worth looking into. If you store also a hash of Git version and diff options (may even be the hash of the raw bytes of the diff options if you do not plan to share the ref between machines) with the patch ID, you can make it safe. That hash would be generated at patch_id init time and load_cached_patch_id() would check this hash in addition to the return value of get_sha1() (and ignore the note if the version/diff options differ). I was thinking about this overnight, glad to see someone else had the same idea :-) It's slightly annoying because the diff options can be customized after we return from init_patch_ids() so we either need a new setup_patch_ids() function to be run after init once diff options have been set or to set it lazily. I'll try introducing a setup function. If you are following git.git slavishly, maybe hashing just the major/minor Git version would be in order to avoid frequent regeneration of identical patch IDs. I think just storing the version is quite good here, and it avoids pain when a topic that affects patch IDs is working its way through pu and next. And quite frankly, if you do rebases etc so much that you think patch ID's are so important that they need to be cached, you may be doing odd/wrong things. AFAICT John actually gave a very valid scenario that validates his use case: git-gui patches are best tested in the git.git scenario but have to be contributed via git-gui.git. It's not John's fault that this typically requires a lot of rebasing between vastly divergent histories. Actually, I don't think that use case is valid. Because it's a subtree merge I can be absolutely certain that nothing on the LHS of master...git-gui/master is patch identical to anything on the RHS since all the paths are different. So doing git log --cherry-mark in that case is completely useless. I think my script should be able to learn that, which gets rid of the really horrible case I was seeing, but it would be nice to improve the fast enough cases as well if it can be done without too much effort. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH v2] patch-ids: cache patch IDs in a notes tree
This adds a new configuration variable patchid.cacheRef which controls whether (and where) patch IDs will be cached in a notes tree. The cache includes a hash of the diff options in place when the cache was generated as well as the version of Git that generated it. When the diff options hash changes we simply ignore that cache entry and replace it with the new patch ID with the new diff options. But if the Git version changes we throw away the entire cache tree on the assumption that the user is unlikely to be simply flipping between different versions of Git. Caching patch IDs in this way results in a performance improvement in every case I have tried, for example if I run a script which checks whether any of six local branches have been picked up upstream I get the following results: Without patchid.cacheRef enabled: $ time git integration --status jk/pu /dev/null real0m4.295s user0m3.927s sys 0m0.270s With patchid.cacheRef set but not yet initialised: $ time git integration --status jk/pu /dev/null real0m2.317s user0m2.036s sys 0m0.187s With patchid.cacheRef and cache primed: $ time git integration --status jk/pu /dev/null real0m1.565s user0m1.310s sys 0m0.153s The script basically does: git log --cherry pu...branch for each of six branches in turn (with some additional commands around that which are now the bottleneck). Signed-off-by: John Keeping j...@keeping.me.uk --- On Sun, May 12, 2013 at 10:08:51AM +0100, John Keeping wrote: On Sat, May 11, 2013 at 06:57:02PM -0500, Johannes Schindelin wrote: On Sat, 11 May 2013, Linus Torvalds wrote: [...] I really think caching patch ID's should be something people should be aware of is fundamentally wrong, even if it might work. Given the incredible performance win, I would say it is still worth looking into. If you store also a hash of Git version and diff options (may even be the hash of the raw bytes of the diff options if you do not plan to share the ref between machines) with the patch ID, you can make it safe. That hash would be generated at patch_id init time and load_cached_patch_id() would check this hash in addition to the return value of get_sha1() (and ignore the note if the version/diff options differ). I was thinking about this overnight, glad to see someone else had the same idea :-) It's slightly annoying because the diff options can be customized after we return from init_patch_ids() so we either need a new setup_patch_ids() function to be run after init once diff options have been set or to set it lazily. I'll try introducing a setup function. This is what that looks like. I think the way I'm hashing the diff options is sensible but another pair of eyes would be useful there. A cache entry looks like this: 9e99e9dbf5c6a717ac60f7ee425c53e87ffd821a diffopts:f8ca35c3d9d57076338dff8abf91b07157bb6329 1.8.3.rc1.45.gcb72da6.dirty Documentation/config.txt | 7 ++ builtin/log.c| 1 + patch-ids.c | 215 ++- patch-ids.h | 4 + revision.c | 1 + t/t6007-rev-list-cherry-pick-file.sh | 16 +++ 6 files changed, 243 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 6e53fc5..e36585c 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1798,6 +1798,13 @@ pager.cmd:: precedence over this option. To disable pagination for all commands, set `core.pager` or `GIT_PAGER` to `cat`. +patchid.cacheRef:: + The name of a notes ref in which to store patch IDs for commits. + The ref is taken to be in `refs/notes/` if it is not qualified. ++ +Note that the patch ID notes are never pruned automatically, so you may +wish to periodically run `git notes --ref ref prune` against this ref. + pretty.name:: Alias for a --pretty= format string, as specified in linkgit:git-log[1]. Any aliases defined here can be used just diff --git a/builtin/log.c b/builtin/log.c index 6e56a50..dd6c24d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -758,6 +758,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids) die(_(Not a range.)); init_patch_ids(ids); + setup_patch_ids(ids); /* given a range a..b get all patch ids for b..a */ init_revisions(check_rev, rev-prefix); diff --git a/patch-ids.c b/patch-ids.c index bc8a28f..73b2aaf 100644 --- a/patch-ids.c +++ b/patch-ids.c @@ -1,8 +1,12 @@ #include cache.h +#include blob.h #include diff.h #include commit.h +#include notes.h +#include refs.h #include sha1-lookup.h #include patch-ids.h +#include version.h static int commit_patch_id(struct commit *commit, struct
Re: [RFC/PATCH v2] patch-ids: cache patch IDs in a notes tree
On Sun, May 12, 2013 at 12:41:31PM +0100, John Keeping wrote: diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh index 28d4f6b..378cf3e 100755 --- a/t/t6007-rev-list-cherry-pick-file.sh +++ b/t/t6007-rev-list-cherry-pick-file.sh @@ -207,4 +207,20 @@ test_expect_success '--count --left-right' ' test_cmp expect actual ' +cat expect EOF +32 0 +EOF + +test_expect_success 'rev-list --cherry-mark caches patch ids' ' + test_config patchid.cacheref patchids + git rev-list --cherry-mark --left-right --count F...E actual + test_cmp expect actual + git notes --ref patchids show master cached_master I forgot to update this test, it needs a | sed -e 1q in the middle of this line to make sure that we're only checking the patch ID and not the diffopts hash and Git version. + git log -p -1 master | git patch-id | sed -e s/ .*// patch-id_master + test_cmp patch-id_master cached_master + # Get the patch IDs again, now they should be cached + git rev-list --cherry-mark --left-right --count F...E actual + test_cmp expect actual +' + test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] merge-base: add --merge-child option
On Sun, May 12, 2013 at 06:44:30PM +0300, Kevin Bracey wrote: On 11/05/2013 15:23, John Keeping wrote: This is helpful when examining branches with disjoint roots, for example because one is periodically merged into a subtree of the other. $ git log --left-right F...E --not $(git merge-base --merge-child F E) F E Wouldn't --left-right --ancestry-path F...E do the job? I can't immediately see how that differs. Unfortunately, that syntax doesn't currently work - I just stumbled across this problem, and my history traversal refinements series in pu fixes it, somewhat incidentally to the main subject in there. You're right (and I was wrong in my reply to Junio's parallel message) ancestry path does seem to be what I want: $ git rev-list --left-right --count master...git-gui/master 31959 5 $ git rev-list --ancestry-path --left-right --count \ master...git-gui/master 20565 $ git rev-list --ancestry-path --left-right --count \ master...git-gui/master \ --not $(git merge-base --all master git-gui/master) 20565 However, this doesn't seem to make a difference to the time taken when I add in --cherry-mark (which is why I was partially correct in the parallel thread - it doesn't have the effect on cherry-mark that I want it to): $ time git rev-list --ancestry-path --left-right --count --cherry-mark \ origin/master...git-gui/master 20565 0 real0m32.266s user0m31.522s sys 0m0.749s $ time git rev-list --left-right --count --cherry-mark \ origin/master...git-gui/master 31959 5 0 real0m32.140s user0m31.337s sys 0m0.807s This seems to be caused by the code in revision.c::limit_list() which does the cherry detection then limits to left/right and only then applies the ancestry path. I haven't looked further than that, but is there any reason not to apply the ancestry path restriction before looking for patch-identical commits? However, without that patch, the alternative way of expressing it: --left-right --ancestry path F E --not $(git merge-base --all F E) should still work. Unless --left-right is magic for ...? If it is, then my patch is more significant than I thought. Yes, --left-right only applies to symmetric differences (...). But I get the results above both on master and on pu. My series will also be better at optimising away D too, through a combination of my and Junio's efforts. Try it out. On this subject, is there any way to exclude a path from a log query? Is there a not operator for paths? Might be another way of doing this - disjoint histories probably have disjoint paths... That relates to another idea I had about optimizing the detection of patch-identical commits. If the smaller side of a symmetric difference is quite small (as it is likely to be if it's a topic branch), would it be a good idea to calculate the set of paths touched by commits on that side and then skip calculating the patch ID for any commits that touch paths outside that set. The tree comparison is a lot cheaper than doing a diff on all of the files. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] merge-base: add --merge-child option
On Sun, May 12, 2013 at 05:28:24PM +0100, John Keeping wrote: On Sun, May 12, 2013 at 06:44:30PM +0300, Kevin Bracey wrote: On 11/05/2013 15:23, John Keeping wrote: This is helpful when examining branches with disjoint roots, for example because one is periodically merged into a subtree of the other. $ git log --left-right F...E --not $(git merge-base --merge-child F E) F E Wouldn't --left-right --ancestry-path F...E do the job? I can't immediately see how that differs. Unfortunately, that syntax doesn't currently work - I just stumbled across this problem, and my history traversal refinements series in pu fixes it, somewhat incidentally to the main subject in there. You're right (and I was wrong in my reply to Junio's parallel message) ancestry path does seem to be what I want: $ git rev-list --left-right --count master...git-gui/master 31959 5 $ git rev-list --ancestry-path --left-right --count \ master...git-gui/master 20565 $ git rev-list --ancestry-path --left-right --count \ master...git-gui/master \ --not $(git merge-base --all master git-gui/master) 20565 However, this doesn't seem to make a difference to the time taken when I add in --cherry-mark (which is why I was partially correct in the parallel thread - it doesn't have the effect on cherry-mark that I want it to): $ time git rev-list --ancestry-path --left-right --count --cherry-mark \ origin/master...git-gui/master 20565 0 real0m32.266s user0m31.522s sys 0m0.749s $ time git rev-list --left-right --count --cherry-mark \ origin/master...git-gui/master 31959 5 0 real0m32.140s user0m31.337s sys 0m0.807s This seems to be caused by the code in revision.c::limit_list() which does the cherry detection then limits to left/right and only then applies the ancestry path. I haven't looked further than that, but is there any reason not to apply the ancestry path restriction before looking for patch-identical commits? However, without that patch, the alternative way of expressing it: --left-right --ancestry path F E --not $(git merge-base --all F E) should still work. Unless --left-right is magic for ...? If it is, then my patch is more significant than I thought. Yes, --left-right only applies to symmetric differences (...). But I get the results above both on master and on pu. No I didn't. I forgot to update my $PATH when I built on master - those results are from pu. master says: fatal: --ancestry-path given but there are no bottom commits My series will also be better at optimising away D too, through a combination of my and Junio's efforts. Try it out. On this subject, is there any way to exclude a path from a log query? Is there a not operator for paths? Might be another way of doing this - disjoint histories probably have disjoint paths... That relates to another idea I had about optimizing the detection of patch-identical commits. If the smaller side of a symmetric difference is quite small (as it is likely to be if it's a topic branch), would it be a good idea to calculate the set of paths touched by commits on that side and then skip calculating the patch ID for any commits that touch paths outside that set. The tree comparison is a lot cheaper than doing a diff on all of the files. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] merge-base: add --merge-child option
On Sun, May 12, 2013 at 05:28:24PM +0100, John Keeping wrote:
However, this doesn't seem to make a difference to the time taken when I
add in --cherry-mark (which is why I was partially correct in the
parallel thread - it doesn't have the effect on cherry-mark that I want
it to):
$ time git rev-list --ancestry-path --left-right --count --cherry-mark \
origin/master...git-gui/master
20565 0
real0m32.266s
user0m31.522s
sys 0m0.749s
$ time git rev-list --left-right --count --cherry-mark \
origin/master...git-gui/master
31959 5 0
real0m32.140s
user0m31.337s
sys 0m0.807s
This seems to be caused by the code in revision.c::limit_list() which
does the cherry detection then limits to left/right and only then
applies the ancestry path. I haven't looked further than that, but is
there any reason not to apply the ancestry path restriction before
looking for patch-identical commits?
With the patch below, the --ancestry-path version drops to under 2
seconds.
I'm not sure if this is a good idea though. It helps me say I know
nothing that isn't on the ancestry path can be patch-identical, so don't
bother checking if it is but it regresses users who want the full
cherry-pick check while only limiting the output.
Perhaps we need --cherry-no-uninteresting to apply the first 3 hunks of
the patch at runtime :-S
-- 8 --
diff --git a/revision.c b/revision.c
index de3b058..d721d83 100644
--- a/revision.c
+++ b/revision.c
@@ -837,7 +837,7 @@ static void cherry_pick_list(struct commit_list *list,
struct rev_info *revs)
for (p = list; p; p = p-next) {
struct commit *commit = p-item;
unsigned flags = commit-object.flags;
- if (flags BOUNDARY)
+ if (flags (BOUNDARY | UNINTERESTING))
;
else if (flags SYMMETRIC_LEFT)
left_count++;
@@ -858,7 +858,7 @@ static void cherry_pick_list(struct commit_list *list,
struct rev_info *revs)
struct commit *commit = p-item;
unsigned flags = commit-object.flags;
- if (flags BOUNDARY)
+ if (flags (BOUNDARY | UNINTERESTING))
continue;
/*
* If we have fewer left, left_first is set and we omit
@@ -879,7 +879,7 @@ static void cherry_pick_list(struct commit_list *list,
struct rev_info *revs)
struct patch_id *id;
unsigned flags = commit-object.flags;
- if (flags BOUNDARY)
+ if (flags (BOUNDARY | UNINTERESTING))
continue;
/*
* If we have fewer left, left_first is set and we omit
@@ -1103,17 +1103,18 @@ static int limit_list(struct rev_info *revs)
show(revs, newlist);
show_early_output = NULL;
}
- if (revs-cherry_pick || revs-cherry_mark)
- cherry_pick_list(newlist, revs);
-
- if (revs-left_only || revs-right_only)
- limit_left_right(newlist, revs);
if (bottom) {
limit_to_ancestry(bottom, newlist);
free_commit_list(bottom);
}
+ if (revs-cherry_pick || revs-cherry_mark)
+ cherry_pick_list(newlist, revs);
+
+ if (revs-left_only || revs-right_only)
+ limit_left_right(newlist, revs);
+
/*
* Check if any commits have become TREESAME by some of their parents
* becoming UNINTERESTING.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
git log --cherry with disjoint roots
I use my own integration branch manager[1] to manage my WIP changes to various projects, including git.git and one of the features of this is a --status option that shows whether anything that I'm tracking has been merged to the base branch I'm building on top of. Since the commit IDs will be different after being sent to the list, this uses the --cherry option to git-log. Today I realised that I wasn't tracking a git-gui change I sent to the list a couple of weeks ago [2] and so I pulled that in and added it. Getting the status for this is significantly slower than anything else because it does this: git log --cherry --oneline origin/master...git-gui-fix-subdir and although there are only 5 commits in git-gui-fix-subdir not in master there are 31964 commits in master not in git-gui-fix-subdir and --cherry seems to inspect all of those. So I get: $ time git log --oneline --cherry master...git-gui-fix-subdir \ /dev/null real0m41.378s user0m40.248s sys 0m0.986s Now I know that I don't need to check anything older than commit 8ead1bf (Merge tag 'gitgui-0.17.0' of git://repo.or.cz/git-gui, 2012-10-17) and if I tell Git that it gets significantly better: $ time git log --oneline --cherry master...git-gui-fix-subdir \ --not 8ead1bfe111085ef1ad7759e67340f074996b244 /dev/null real0m2.163s user0m2.070s sys 0m0.084s I'd like a way to automatically find the last merge that pulled in a subtree so that my script can construct the above command line without me needing to tell it the correct SHA-1 every time I have a branch that is affected by this. I guess this boils down to having a way to ask Git to list merges that have a parent in a specified range, but perhaps I'm missing an easier solution to this. I also wonder if it would be interesting to cache patch IDs rather than generating them every time... [1] http://johnkeeping.github.io/git-integration/ [2] http://article.gmane.org/gmane.comp.version-control.git/222646 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 0/2] merge-base: add --merge-child option
This is helpful when examining branches with disjoint roots, for example because one is periodically merged into a subtree of the other. With the --merge-child option, git merge-base will print a first-parent ancestor of the first revision given, where the commit printed is either a merge-base of the supplied revisions or a merge for which one of its parents (not the first) is a merge-base. For example, given the history: A---C---G \ B-D---F \ E we have: $ git merge-base F E B $ git merge-base --merge-child F E D $ git merge-base F G C $ git merge-base --merge-child F G C $ git log --left-right F...E F D C A E $ git log --left-right F...E --not $(git merge-base --merge-child F E) F E The git-log case is useful because it allows us to limit the range of commits that we are examining for patch-identical changes when using --cherry. For example with git-gui in git.git I know that anything before the last merge of git-gui is not interesting: $ time git log --cherry master...git-gui/master /dev/null real0m32.731s user0m31.956s sys 0m0.664s $ time git log --cherry master...git-gui/master --not \ $(git merge-base --merge-child master git-gui/master) \ /dev/null real0m2.296s user0m2.193s sys 0m0.092s The first commit is a small prerequisite to extract a useful function from builtin/tag.c to commit.c. The second is the main change (the commit message is identical to the text before this paragraph). I'm not convinced that '--merge-child' is the right name for this but I think the functionality itself is useful. John Keeping (2): commit: add commit_list_contains function merge-base: add --merge-child option Documentation/git-merge-base.txt | 6 builtin/merge-base.c | 61 ++-- builtin/tag.c| 10 +-- commit.c | 8 ++ commit.h | 1 + t/t6010-merge-base.sh| 25 ++-- 6 files changed, 98 insertions(+), 13 deletions(-) -- 1.8.3.rc1.289.gcb3647f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 1/2] commit: add commit_list_contains function
This is the same as the in_commit_list function already in builtin/tag.c
so we also replace that by the new commit_list_contains function.
Signed-off-by: John Keeping j...@keeping.me.uk
---
builtin/tag.c | 10 +-
commit.c | 8
commit.h | 1 +
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index af3af3f..1c24772 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -65,14 +65,6 @@ static const unsigned char *match_points_at(const char
*refname,
return NULL;
}
-static int in_commit_list(const struct commit_list *want, struct commit *c)
-{
- for (; want; want = want-next)
- if (!hashcmp(want-item-object.sha1, c-object.sha1))
- return 1;
- return 0;
-}
-
static int contains_recurse(struct commit *candidate,
const struct commit_list *want)
{
@@ -85,7 +77,7 @@ static int contains_recurse(struct commit *candidate,
if (candidate-object.flags UNINTERESTING)
return 0;
/* or are we it? */
- if (in_commit_list(want, candidate))
+ if (commit_list_contains(want, candidate))
return 1;
if (parse_commit(candidate) 0)
diff --git a/commit.c b/commit.c
index 888e02a..a8263c3 100644
--- a/commit.c
+++ b/commit.c
@@ -420,6 +420,14 @@ void commit_list_sort_by_date(struct commit_list **list)
commit_list_compare_by_date);
}
+int commit_list_contains(const struct commit_list *list, const struct commit
*item)
+{
+ for (; list; list = list-next)
+ if (!hashcmp(list-item-object.sha1, item-object.sha1))
+ return 1;
+ return 0;
+}
+
struct commit *pop_most_recent_commit(struct commit_list **list,
unsigned int mark)
{
diff --git a/commit.h b/commit.h
index 67bd509..d686ea8 100644
--- a/commit.h
+++ b/commit.h
@@ -60,6 +60,7 @@ unsigned commit_list_count(const struct commit_list *l);
struct commit_list *commit_list_insert_by_date(struct commit *item,
struct commit_list **list);
void commit_list_sort_by_date(struct commit_list **list);
+int commit_list_contains(const struct commit_list *list, const struct commit
*item);
void free_commit_list(struct commit_list *list);
--
1.8.3.rc1.289.gcb3647f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH 2/2] merge-base: add --merge-child option
This is helpful when examining branches with disjoint roots, for example
because one is periodically merged into a subtree of the other.
With the --merge-child option, git merge-base will print a
first-parent ancestor of the first revision given, where the commit
printed is either a merge-base of the supplied revisions or a merge for
which one of its parents (not the first) is a merge-base.
For example, given the history:
A---C---G
\
B-D---F
\
E
we have:
$ git merge-base F E
B
$ git merge-base --merge-child F E
D
$ git merge-base F G
C
$ git merge-base --merge-child F G
C
$ git log --left-right F...E
F
D
C
A
E
$ git log --left-right F...E --not $(git merge-base --merge-child F E)
F
E
The git-log case is useful because it allows us to limit the range of
commits that we are examining for patch-identical changes when using
--cherry. For example with git-gui in git.git I know that anything
before the last merge of git-gui is not interesting:
$ time git log --cherry master...git-gui/master /dev/null
real0m32.731s
user0m31.956s
sys 0m0.664s
$ time git log --cherry master...git-gui/master --not \
$(git merge-base --merge-child master git-gui/master) \
/dev/null
real0m2.296s
user0m2.193s
sys 0m0.092s
Signed-off-by: John Keeping j...@keeping.me.uk
---
Documentation/git-merge-base.txt | 6
builtin/merge-base.c | 61 ++--
t/t6010-merge-base.sh| 25 ++--
3 files changed, 88 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 87842e3..a1404e1 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -10,6 +10,7 @@ SYNOPSIS
[verse]
'git merge-base' [-a|--all] commit commit...
+'git merge-base' [-a|--all] --merge-child commit...
'git merge-base' [-a|--all] --octopus commit...
'git merge-base' --is-ancestor commit commit
'git merge-base' --independent commit...
@@ -39,6 +40,11 @@ As a consequence, the 'merge base' is not necessarily
contained in each of the
commit arguments if more than two commits are specified. This is different
from linkgit:git-show-branch[1] when used with the `--merge-base` option.
+--merge-child::
+ Find the first-parent ancestor of the first commit that is either
+ reachable from all of the supplied commits or which has a parent that
+ is.
+
--octopus::
Compute the best common ancestors of all supplied commits,
in preparation for an n-way merge. This mimics the behavior
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 1bc7991..0bf9f6d 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -1,7 +1,9 @@
#include builtin.h
#include cache.h
#include commit.h
+#include diff.h
#include parse-options.h
+#include revision.h
static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
{
@@ -24,12 +26,61 @@ static int show_merge_base(struct commit **rev, int rev_nr,
int show_all)
static const char * const merge_base_usage[] = {
N_(git merge-base [-a|--all] commit commit...),
+ N_(git merge-base [-a|--all] --merge-child commit...),
N_(git merge-base [-a|--all] --octopus commit...),
N_(git merge-base --independent commit...),
N_(git merge-base --is-ancestor commit commit),
NULL
};
+static int handle_merge_child(struct commit **rev, int rev_nr, const char
*prefix, int show_all)
+{
+ struct commit_list *merge_bases;
+ struct rev_info revs;
+ struct commit *commit;
+
+ merge_bases = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1, 0);
+
+ if (!merge_bases)
+ return 1;
+
+ init_revisions(revs, prefix);
+ revs.first_parent_only = 1;
+
+ clear_commit_marks(rev[0], SEEN | UNINTERESTING | SHOWN | ADDED);
+ add_pending_object(revs, rev[0]-object, rev0);
+ if (prepare_revision_walk(revs))
+ die(_(revision walk setup failed));
+
+ while ((commit = get_revision(revs)) != NULL) {
+ /*
+* If a merge base is a first-parent ancestor of rev[0] then
+* we print the commit itself instead of a merge which is its
+* child.
+*/
+ if (commit_list_contains(merge_bases, commit)) {
+ printf(%s\n, sha1_to_hex(commit-object.sha1));
+ if (!show_all)
+ return 0;
+ }
+
+ struct commit_list *parent;
+ for (parent = commit-parents; parent; parent = parent-next) {
+ /* Skip
Re: Cannot push anything via export transport helper after push fails.
On Sat, May 11, 2013 at 04:29:36PM +0400, Andrey Borzenkov wrote: I noticed that using git-remote-bzr, but as far as I can tell this is generic for all transport helpers using fast-export. What happened was git push failed due to merge conflict. So far so good - but from now on git assumes everything is up to date. bor@opensuse:/tmp/test/git git push origin master To bzr::bzr+ssh://bor@localhost/tmp/test/bzr ! [rejected]master - master (non-fast-forward) error: failed to push some refs to 'bzr::bzr+ssh://bor@localhost/tmp/test/bzr' hint: Updates were rejected because the tip of your current branch is behind hint: its remote counterpart. Merge the remote changes (e.g. 'git pull') hint: before pushing again. hint: See the 'Note about fast-forwards' in 'git push --help' for details. bor@opensuse:/tmp/test/git git push origin master Everything up-to-date bor@opensuse:/tmp/test/git The problem seems to be that git fast-export updates marks unconditionally, whether export actually applied or not. So next time it assumes everything is already exported and does nothing. Is it expected behavior? What version of Git are you using? This sounds similar to the regression fixed by commit 126aac5 (transport-helper: fix remote helper namespace regression, 2013-05-10) but that was only introduced in commit 664059f (transport-helper: update remote helper namespace, 2013-04-17) which isn't in any released versions of Git. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] merge-base: add --merge-child option
On Sat, May 11, 2013 at 10:54:12AM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: This is helpful when examining branches with disjoint roots, for example because one is periodically merged into a subtree of the other. With the --merge-child option, git merge-base will print a first-parent ancestor of the first revision given, where the commit printed is either a merge-base of the supplied revisions or a merge for which one of its parents (not the first) is a merge-base. The above two doe snot connect at least to me. The second paragraph seems to describe how this mysterious mode decides its output to a sufficient detail, but what the output _means_, and it is unclear how it relates to gitk/git-gui style merges. For example, given the history: A---C---G \ B-D---F \ E we have: ... $ git log --left-right F...E --not $(git merge-base --merge-child F E) F E The git-log case is useful because it allows us to limit the range of commits that we are examining for patch-identical changes when using --cherry. Hmph, is this reinventing ancestry-path in a different way? At the low level machinery, you are finding D to show only F and E, and your goal seems to be to ignore the side ancestry A--C--G, but it is not clear if you prefer E D F(which would be what F...E would give in a history limited to ancestry-path, ignoring C) over E F. I hadn't considered ancestry-path, but I don't think it does what I want. What I want if for LEFT to be B--D--F and RIGHT to be B--E, ignoring A--C--G because I know that none of those are patch identical to anything in B--E. So what I want is more descendant-path than ancestry path in that I don't want anything that isn't a descendant of the merge base of the supplied arguments. For example with git-gui in git.git I know that anything before the last merge of git-gui is not interesting: Can this be extended to find the second last such merge? Or is the last one always special? In this implementation it only finds the last one because that's where the merge base is. Still skeptical, but I'll let people discuss it during the feature freeze ;-). I'm not convinced this is easy to explain myself, which may make it a bad idea. Perhaps a --descendant-path argument to git-log is a better way to help with this case. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] patch-ids.c: cache patch IDs in a notes tree
This adds a new configuration variable patchid.cacheRef which controls
whether (and where) patch IDs will be cached in a notes tree.
Caching patch IDs in this way results in a performance improvement in
every case I have tried, for example when comparing the git-gui tree to
git.git (where git-gui has been merged into git.git but most git.git
commits do not appear in git-gui):
Without patch ID caching:
$ time git log --cherry master...git-gui/master /dev/null
real0m32.981s
user0m32.364s
sys 0m0.621s
Prime the cache:
$ time git -c patchid.cacheref=patchids log --cherry \
master...git-gui/master /dev/null
real0m33.860s
user0m32.832s
sys 0m0.986s
With all patch IDs cached:
$ time git -c patchid.cacheref=patchids log --cherry \
master...git-gui/master /dev/null
real0m1.041s
user0m0.679s
sys 0m0.363s
Signed-off-by: John Keeping j...@keeping.me.uk
---
This is another approach to fixing the log --cherry takes a long time
issue I encountered comparing commits built on git-gui to those in
git.git [1][2].
I think this is a useful feature even outside that use case. I measured
a small improvement (when the cache is primed) even comparing two
branches with 1 and 2 different commits respectively.
[1] http://article.gmane.org/gmane.comp.version-control.git/223959
[2] http://article.gmane.org/gmane.comp.version-control.git/223956
Documentation/config.txt | 7 +++
patch-ids.c | 91 +++-
patch-ids.h | 1 +
t/t6007-rev-list-cherry-pick-file.sh | 16 +++
4 files changed, 114 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..e36585c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1798,6 +1798,13 @@ pager.cmd::
precedence over this option. To disable pagination for all
commands, set `core.pager` or `GIT_PAGER` to `cat`.
+patchid.cacheRef::
+ The name of a notes ref in which to store patch IDs for commits.
+ The ref is taken to be in `refs/notes/` if it is not qualified.
++
+Note that the patch ID notes are never pruned automatically, so you may
+wish to periodically run `git notes --ref ref prune` against this ref.
+
pretty.name::
Alias for a --pretty= format string, as specified in
linkgit:git-log[1]. Any aliases defined here can be used just
diff --git a/patch-ids.c b/patch-ids.c
index bc8a28f..cb05eec 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -1,6 +1,9 @@
#include cache.h
+#include blob.h
#include diff.h
#include commit.h
+#include notes.h
+#include refs.h
#include sha1-lookup.h
#include patch-ids.h
@@ -34,12 +37,38 @@ struct patch_id_bucket {
struct patch_id bucket[BUCKET_SIZE];
};
+static int patch_id_config(const char *var, const char *value, void *cb)
+{
+ const char **cacheref = cb;
+
+ if (!strcmp(var, patchid.cacheref))
+ return git_config_string(cacheref, var, value);
+
+ return 0;
+}
+
int init_patch_ids(struct patch_ids *ids)
{
+ char *cacheref = NULL;
+
memset(ids, 0, sizeof(*ids));
diff_setup(ids-diffopts);
DIFF_OPT_SET(ids-diffopts, RECURSIVE);
diff_setup_done(ids-diffopts);
+
+ git_config(patch_id_config, cacheref);
+ if (cacheref) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(sb, cacheref);
+ expand_notes_ref(sb);
+
+ ids-cache = xcalloc(1, sizeof(*ids-cache));
+ init_notes(ids-cache, sb.buf, combine_notes_overwrite, 0);
+
+ strbuf_release(sb);
+ free(cacheref);
+ }
+
return 0;
}
@@ -52,9 +81,67 @@ int free_patch_ids(struct patch_ids *ids)
next = patches-next;
free(patches);
}
+
+ if (ids-cache) {
+ unsigned char notes_sha1[20];
+ if (write_notes_tree(ids-cache, notes_sha1) ||
+ update_ref(patch-id: update cache, ids-cache-ref,
+ notes_sha1, NULL, 0, QUIET_ON_ERR))
+ error(_(failed to write patch ID cache));
+
+ free_notes(ids-cache);
+ ids-cache = NULL;
+ }
+
return 0;
}
+static int load_cached_patch_id(struct commit *commit,
+ struct notes_tree *cache, unsigned char *sha1)
+{
+ const unsigned char *note_sha1;
+ char *note;
+ enum object_type type;
+ unsigned long notelen;
+ int result = 1;
+
+ if (!cache)
+ return 1;
+
+ note_sha1 = get_note(cache, commit-object.sha1);
+ if (!note_sha1)
+ return 1;
+
+ if (!(note = read_sha1_file(note_sha1, type, notelen)) || !notelen ||
+ type != OBJ_BLOB)
+ goto out;
+
+ if (get_sha1_hex(note, sha1))
+ goto out
Re: [PATCH] patch-ids.c: cache patch IDs in a notes tree
On Sat, May 11, 2013 at 02:10:01PM -0700, Linus Torvalds wrote: On Sat, May 11, 2013 at 12:54 PM, John Keeping j...@keeping.me.uk wrote: This adds a new configuration variable patchid.cacheRef which controls whether (and where) patch IDs will be cached in a notes tree. Patch ID's aren't stable wrt different diff options, so I think this is potentially a very bad idea. Hmm... I hadn't realised that. Looking a bit closer, it looks like init_patch_ids sets up its own diffopts so its not affected by the command line (except for pathspecs which would be easy to check for). Of course that still means it can be affected by settings in the user's configuration. It's a pity that this can't be done since it gives a significant performance improvement for some tasks that I perform relatively frequently. Is there a reason patch IDs couldn't be generated using fixed diff options? Since there's no way to control it from the command line it seems surprising that the results of log --cherry might be different based on what's in your config. That could go either way I suppose - is it useful to be able to change patch IDs based on command line arguments or is it wrong that as we add persistent diff settings to the configuration we've been silently changing the behaviour of git patch-id and git cherry. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/3] Makefile: avoid deprecation warnings on OS X 10.8
On Thu, May 09, 2013 at 02:13:30AM -0700, David Aguilar wrote: Mac OS X Mountain Lion prints warnings when building git: warning: 'SHA1_Init' is deprecated (declared at /usr/include/openssl/sha.h:121) Silence the warnings by disabling OpenSSH in favor of BLK_SHA1. Signed-off-by: David Aguilar dav...@gmail.com --- I know I can create config.mak, but do we prefer to have the default settings be warning-free? I do not see any other platforms that tweak NO_OPENSSL themselves, hence RFC. Is there a better way to do this? Are there any Darwin/PPC users that would be harmed by this patch? Disabling OpenSSL also has the effect of disabling SSL support in git-imap-send. Does enabling BLK_SHA1 instead also remove the warnings? Alternatively, it seems that the recommended update is to use Apple's CommonCrypto library, as in this patch: https://gist.github.com/anonymous/4466305 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/3] mergetools/kdiff3: allow opting-out of auto-merges
On Thu, May 09, 2013 at 09:10:51AM -0700, Junio C Hamano wrote: David Aguilar dav...@gmail.com writes: Marked RFC because I am kinda against adding more configuration variables. Just like git merge has -Xoption escape hatch to allow us to pass backend-specific options, perhaps you can add a mechanism to git mergetool to let the user pass --no-auto from the command line? We already have mergetool.tool.cmd which allows a completely custom command line to be specified. IIUC this can be used to override the built-in command for tool names that already exist. I'm not sure an extra -Xoption buys us much on top of this, but perhaps it would be useful for one-off usage. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor correction to Git book
On Wed, May 08, 2013 at 11:24:56AM +0100, Robin Messer wrote: I'm just learning Git so I don't yet know how to submit this as a patch, but I'm reading the Git Book to get myself started and I think there is a mistake on the page at: http://git-scm.com/book/en/Git-Basics-Recording-Changes-to-the-Repository It says: For another example, if you stage the benchmarks.rb file and then edit it, you can use git diff to see the changes in the file that are staged and the changes that are unstaged: I believe this should say git status rather than git diff. I think the text is correct as it stands. git status shows you that there are changes that are staged and unstaged, git diff (and git diff --cached) shows you what those changes are. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Minor correction to Git book
On Wed, May 08, 2013 at 12:01:00PM +0100, Robin Messer wrote: I think the text is correct as it stands. git status shows you that there are changes that are staged and unstaged, git diff (and git diff --cached) shows you what those changes are. Thanks, but the command line which follows that text does actually use git status to show which files (staged and unstaged) have changes. The text I quoted is introducing that command. Then the next example shows you how to use git diff to see what the actual unstaged changes to those files are. Ah, OK. I read your email without looking at the context. If this is not the appropriate place to report such things, please point me at the correct one. The ProGit book is maintained at https://github.com/progit/progit -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Segfault in git merge-tree (1.8.2.2)
On Mon, May 06, 2013 at 03:02:10PM +0200, Andreas Jacobsen wrote: I'm getting a segfault in git merge-tree using v1.8.2.2 on MacOS 10.8.3. I can't share the repo, but I can build patches and check if they fix the problem :) Can you rebuild with debugging information and try the backtrace again? Something like: make CFLAGS='-O0 -g' Then use the git in bin-wrappers/. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Segfault in git merge-tree (1.8.2.2)
On Mon, May 06, 2013 at 04:13:28PM +0200, Andreas Jacobsen wrote:
Sure, here you go, this time built from the HEAD I found on github
(7d3ccdffb5d28970dd7a4d177cfcca690ccd0c22) with:
NO_GETTEXT=1 make prefix=/usr/local/Cellar/git/HEAD CC=cc CFLAGS='-O0
-g' install (this is homebrew's setup, but I built it manually rather
than using the recipe.)
And the gdb output:
(gdb) set args merge-tree 027058e6ac8d03e029c4e1455bf90f63cd20e65b
FETCH_HEAD master
(gdb) run
Starting program: /usr/local/bin/git merge-tree
027058e6ac8d03e029c4e1455bf90f63cd20e65b FETCH_HEAD master
Reading symbols for shared libraries +.. done
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x
0x00010006f1a3 in add_merge_entry (entry=0x100432ba0) at
builtin/merge-tree.c:24
24 *merge_result_end = entry;
Thanks. I have an idea of what's going on, but the set up is in an
earlier pass and it only fails the next time it gets into
add_merge_entry.
Can you try adding the following patch on top? Hopefully the added
debug is in the right caller, otherwise the new assert at the top will
point us to the right one.
-- 8 --
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ec49917..8eebab7 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -23,6 +23,7 @@ static void add_merge_entry(struct merge_list *entry)
{
*merge_result_end = entry;
merge_result_end = entry-next;
+ assert(merge_result_end);
}
static void merge_trees_recursive(struct tree_desc t[3], const char *base, int
df_conflict);
@@ -267,6 +268,12 @@ static void unresolved(const struct traverse_info *info,
struct name_entry n[3])
if (n[0].mode !S_ISDIR(n[0].mode))
entry = link_entry(1, info, n + 0, entry);
+ if (!entry) {
+ fprintf(stderr, n[0].mode = %d\nn[1].mode = %d\nn[2].mode =
%d\n,
+ n[0].mode, n[1].mode, n[2].mode);
+ assert(FALSE);
+ }
+
add_merge_entry(entry);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Segfault in git merge-tree (1.8.2.2)
On Mon, May 06, 2013 at 03:29:23PM +0100, John Keeping wrote: On Mon, May 06, 2013 at 04:13:28PM +0200, Andreas Jacobsen wrote: Sure, here you go, this time built from the HEAD I found on github (7d3ccdffb5d28970dd7a4d177cfcca690ccd0c22) with: NO_GETTEXT=1 make prefix=/usr/local/Cellar/git/HEAD CC=cc CFLAGS='-O0 -g' install (this is homebrew's setup, but I built it manually rather than using the recipe.) And the gdb output: (gdb) set args merge-tree 027058e6ac8d03e029c4e1455bf90f63cd20e65b FETCH_HEAD master (gdb) run Starting program: /usr/local/bin/git merge-tree 027058e6ac8d03e029c4e1455bf90f63cd20e65b FETCH_HEAD master Reading symbols for shared libraries +.. done Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x 0x00010006f1a3 in add_merge_entry (entry=0x100432ba0) at builtin/merge-tree.c:24 24 *merge_result_end = entry; Thanks. I have an idea of what's going on, but the set up is in an earlier pass and it only fails the next time it gets into add_merge_entry. Can you try adding the following patch on top? Hopefully the added debug is in the right caller, otherwise the new assert at the top will point us to the right one. Actually, don't worry about it. I've tracked down the problem and have a failing test case for t4300. Patch to follow shortly. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] merge-tree: handle directory/empty conflict correctly
git-merge-tree causes a null pointer dereference when a directory
entry exists in only one or two of the three trees being compared with
no corresponding entry in the other tree(s).
When this happens, we want to handle the entry as a directory and not
attempt to mark it as a file merge. Do this by setting the entries bit
in the directory mask when the entry is missing or when it is a
directory, only performing the file comparison when we know that a file
entry exists.
Reported-by: Andreas Jacobsen andr...@andreasjacobsen.com
Signed-off-by: John Keeping j...@keeping.me.uk
---
Andreas, can you try this patch and see if it fixes your test case?
builtin/merge-tree.c | 6 +-
t/t4300-merge-tree.sh | 51 +++
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ec49917..61cbde4 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -251,7 +251,11 @@ static void unresolved(const struct traverse_info *info,
struct name_entry n[3])
for (i = 0; i 3; i++) {
mask |= (1 i);
- if (n[i].mode S_ISDIR(n[i].mode))
+ /*
+* Treat missing entries as directories so that we return
+* after unresolved_directory has handled this.
+*/
+ if (!n[i].mode || S_ISDIR(n[i].mode))
dirmask |= (1 i);
}
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 2defb42..9015e47 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -259,6 +259,57 @@ EXPECTED
test_cmp expected actual
'
+test_expect_success 'tree add A, B (same)' '
+ cat expect -\EOF
+ EOF
+ git reset --hard initial
+ mkdir sub
+ test_commit add sub/file sub/file file add-tree-A
+ git merge-tree initial add-tree-A add-tree-A actual
+ test_cmp expect actual
+'
+
+test_expect_success 'tree add A, B (different)' '
+ cat expect -\EOF
+ added in both
+ our100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/file
+ their 100644 ba629238ca89489f2b350e196ca445e09d8bb834 sub/file
+ @@ -1 +1,5 @@
+ + .our
+AAA
+ +===
+ +BBB
+ + .their
+ EOF
+ git reset --hard initial
+ mkdir sub
+ test_commit add sub/file sub/file AAA add-tree-a-b-A
+ git reset --hard initial
+ mkdir sub
+ test_commit add sub/file sub/file BBB add-tree-a-b-B
+ git merge-tree initial add-tree-a-b-A add-tree-a-b-B actual
+ test_cmp expect actual
+'
+
+test_expect_success 'tree unchanged A, removed B' '
+ cat expect -\EOF
+ removed in remote
+ base 100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/file
+ our100644 43d5a8ed6ef6c00ff775008633f95787d088285d sub/file
+ @@ -1 +0,0 @@
+ -AAA
+ EOF
+ git reset --hard initial
+ mkdir sub
+ test_commit add sub/file sub/file AAA tree-remove-b-initial
+ git rm sub/file
+ test_tick
+ git commit -m remove sub/file
+ git tag tree-remove-b-B
+ git merge-tree tree-remove-b-initial tree-remove-b-initial
tree-remove-b-B actual
+ test_cmp expect actual
+'
+
test_expect_success 'turn file to tree' '
git reset --hard initial
rm initial-file
--
1.8.3.rc0.149.g98a72f2.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] contrib/subtree: don't delete remote branches if split fails
When using git subtree push to split out a subtree and push it to a remote repository, we do not detect if the split command fails which causes the LHS of the refspec to be empty, deleting the remote branch. Fix this by pulling the result of the split command into a variable so that we can die if the command fails. Reported-by: Steffen Jaeckel steffen.jaec...@stzedn.de Signed-off-by: John Keeping j...@keeping.me.uk --- contrib/subtree/git-subtree.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 8a23f58..10daa8b 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -715,7 +715,8 @@ cmd_push() repository=$1 refspec=$2 echo git push using: $repository $refspec - git push $repository $(git subtree split --prefix=$prefix):refs/heads/$refspec + localrev=$(git subtree split --prefix=$prefix) || die + git push $repository $localrev:refs/heads/$refspec else die '$dir' must already exist. Try 'git subtree add'. fi -- 1.8.3.rc0.149.g98a72f2.dirty -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git grep parallelism question
On Mon, Apr 29, 2013 at 03:22:24PM -0700, Junio C Hamano wrote: John Keeping j...@keeping.me.uk writes: No, I was the one missing something (--root to be precise). But with TEST_OUTPUT_DIRECTORY you also get the result files in your temporary location, not just the trash directory. With your patch, doesn't t-*.sh --root $there automatically use the fast $there temporary location as the result depot, too? No, the current code uses: $TEST_OUTPUT_DIRECTORY/$root/trash\ directory.t where we don't prepend $TEST_OUTPUT_DIRECTORY/ if $root is absolute. If it doesn't with the current code, shouldn't it? I think the current behaviour is fine and the two options complement each other. TEST_OUTPUT_DIRECTORY is something you set once and forget about which says all of the test output should go over here, whereas --root is passed to a specific test and says put your output here but does not affect the result aggregation which is not specific to that test. Note that setting TEST_OUTPUT_DIRECTORY in config.mak affects all tests no matter how you run them (via make or as ./t-.sh) whereas setting --root=... in GIT_TEST_OPTS only affect tests run via make. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git grep parallelism question
On Tue, Apr 30, 2013 at 11:59:39AM -0400, Jeff King wrote: On Tue, Apr 30, 2013 at 09:08:49AM +0100, John Keeping wrote: With your patch, doesn't t-*.sh --root $there automatically use the fast $there temporary location as the result depot, too? No, the current code uses: $TEST_OUTPUT_DIRECTORY/$root/trash\ directory.t where we don't prepend $TEST_OUTPUT_DIRECTORY/ if $root is absolute. If it doesn't with the current code, shouldn't it? I think the current behaviour is fine and the two options complement each other. TEST_OUTPUT_DIRECTORY is something you set once and forget about which says all of the test output should go over here, whereas --root is passed to a specific test and says put your output here but does not affect the result aggregation which is not specific to that test. The original intent of --root (and how I use it) is to set and forget it, too, via GIT_TEST_OPTS. I intentionally didn't move test results with it, because to me the point was a pure optimization: put the trash directories on a faster disk, and leave everything else identical. With --root, any scripts which later want to look at test-results will find them in the usual place. Your patch updates all of the in-tree spots which look at the results, but any third-party scripts would need to take it into account, too (though I have no idea if any such scripts even exist). I'm curious if there is a good reason to want to move the results. Some possibilities I can think of are: 1. More optimization, as results are written to the faster filesystem. I doubt this is noticeable, though, as the amount of data written is relatively small compared to the tests themselves (which are constantly creating and deleting repos). 2. You can run tests in a read-only git checkout. I'm not sure how useful that is, though, since you would already need to compile git. 3. You could have multiple sets of test results to keep or compare. I'd think you'd want to keep the built versions of git around, too, though. Which would mean that a full checkout like git-new-workdir would be a much simpler way to accomplish the same thing. So I'm not against TEST_OUTPUT_DIRECTORY as a concept, but I'm having trouble seeing how it is more useful than --root. I think the original intent of TEST_OUTPUT_DIRECTORY was to allow other users of the test framework (in contrib/ or the performance tests) to put their output in a sensible place for those tests, like you describe below. The patch being discussed here [1] just makes sure that it applies to everything - previously it was applied to test-results/ inconsistently; test-lib.sh used TEST_OUTPUT_DIRECTORY but the makefile didn't. So we haven't actually changed where test-results/ live as a result of this change, just where the makefile looks in order to display the aggregate results and clean them up. Note that setting TEST_OUTPUT_DIRECTORY in config.mak affects all tests no matter how you run them (via make or as ./t-.sh) whereas setting --root=... in GIT_TEST_OPTS only affect tests run via make. I actually consider that a feature of --root. When I run make test everything happens fast. When I run the script manually (which is usually because I'm debugging), the trash directory appears in the current directory, so I can easily investigate it. And if you are running a single test, the performance impact is usually negligible (where you really notice it is when running make -j32 test). This confirms to me that the patch as it currently stands is correct: we have made TEST_OUTPUT_DIRECTORY consistent and --root still works as before. [1] http://article.gmane.org/gmane.comp.version-control.git/222555 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git grep parallelism question
On Mon, Apr 29, 2013 at 07:35:01PM +0530, Ramkumar Ramachandra wrote: On a related note, one place that IO parallelism can provide massive benefits is in executing shell scripts. Accordingly, I always use the following commands to compile and test git respectively: make -j 8 CFLAGS=-g -O0 -Wall make -j 8 DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=-j 16 test i.e. always use 8 threads when the task is known to be CPU intensive, and always use 16 threads when the task is known to be IO intensive. On this tangent, I recently added a TEST_OUTPUT_DIRECTORY line to my config.mak which points into a tmpfs mount. Keeping all of the test repositories in RAM makes the tests significantly faster for me and works nicely when you have the patches in jk/test-output (without those patches the individual tests work but the reporting of aggregate results doesn't). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git grep parallelism question
On Mon, Apr 29, 2013 at 08:04:10PM +0200, Thomas Rast wrote: John Keeping j...@keeping.me.uk writes: On Mon, Apr 29, 2013 at 07:35:01PM +0530, Ramkumar Ramachandra wrote: On a related note, one place that IO parallelism can provide massive benefits is in executing shell scripts. Accordingly, I always use the following commands to compile and test git respectively: make -j 8 CFLAGS=-g -O0 -Wall make -j 8 DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=-j 16 test i.e. always use 8 threads when the task is known to be CPU intensive, and always use 16 threads when the task is known to be IO intensive. On this tangent, I recently added a TEST_OUTPUT_DIRECTORY line to my config.mak which points into a tmpfs mount. Keeping all of the test repositories in RAM makes the tests significantly faster for me and works nicely when you have the patches in jk/test-output (without those patches the individual tests work but the reporting of aggregate results doesn't). But that's been possible for quite some time now, using --root, or am I missing something? No, I was the one missing something (--root to be precise). But with TEST_OUTPUT_DIRECTORY you also get the result files in your temporary location, not just the trash directory. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2 v2] test output: respect $TEST_OUTPUT_DIRECTORY
Most test results go in $TEST_OUTPUT_DIRECTORY, but the output files for
tests run with --tee or --valgrind just use bare test-results.
Changes these so that they do respect $TEST_OUTPUT_DIRECTORY.
As a result of this, the valgrind/analyze.sh script may no longer
inspect the correct files so it is also updated to respect
$TEST_OUTPUT_DIRECTORY by adding it to GIT-BUILD-OPTIONS. This may be a
regression for people who have TEST_OUTPUT_DIRECTORY in their config.mak
but want to override it in the environment, but this change merely
brings it into line with GIT_TEST_OPTS which already cannot be
overridden if it is specified in config.mak.
Signed-off-by: John Keeping j...@keeping.me.uk
---
On Mon, Apr 29, 2013 at 08:00:27PM +0200, Thomas Rast wrote:
John Keeping j...@keeping.me.uk writes:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ca6bdef..70ad085 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,8 +54,8 @@ done,*)
# do not redirect again
;;
*' --tee '*|*' --va'*)
- mkdir -p test-results
- BASE=test-results/$(basename $0 .sh)
+ mkdir -p $(TEST_OUTPUT_DIRECTORY)/test-results
+ BASE=$(TEST_OUTPUT_DIRECTORY)/test-results/$(basename $0 .sh)
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} $0 $@ 21;
echo $? $BASE.exit) | tee $BASE.out
test $(cat $BASE.exit) = 0
Hmm, I initially was too lazy to review this change, and now it's biting
me. The above is Makefile-quoted, which to the shell reads like a
command substitution.
Yeah, that's completely wrong - clearly it was too late on Friday
evening when I wrote this. There was another case of the same in
t/valgrind/analyze.sh.
All fixed in this version.
Makefile | 3 +++
t/test-lib.sh | 4 ++--
t/valgrind/analyze.sh | 8 ++--
3 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/Makefile b/Makefile
index 598d631..ef5be0f 100644
--- a/Makefile
+++ b/Makefile
@@ -2153,6 +2153,9 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' $@
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' $@
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' $@
+ifdef TEST_OUTPUT_DIRECTORY
+ @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' $@
+endif
ifdef GIT_TEST_OPTS
@echo GIT_TEST_OPTS=\''$(subst ','\'',$(subst
','\'',$(GIT_TEST_OPTS)))'\' $@
endif
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 657b0bd..e7d169c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,8 +54,8 @@ done,*)
# do not redirect again
;;
*' --tee '*|*' --va'*)
- mkdir -p test-results
- BASE=test-results/$(basename $0 .sh)
+ mkdir -p $TEST_OUTPUT_DIRECTORY/test-results
+ BASE=$TEST_OUTPUT_DIRECTORY/test-results/$(basename $0 .sh)
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} $0 $@ 21;
echo $? $BASE.exit) | tee $BASE.out
test $(cat $BASE.exit) = 0
diff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh
index d8105d9..2ffc80f 100755
--- a/t/valgrind/analyze.sh
+++ b/t/valgrind/analyze.sh
@@ -1,6 +1,10 @@
#!/bin/sh
-out_prefix=$(dirname $0)/../test-results/valgrind.out
+# Get TEST_OUTPUT_DIRECTORY from GIT-BUILD-OPTIONS if it's there...
+. $(dirname $0)/../../GIT-BUILD-OPTIONS
+# ... otherwise set it to the default value.
+: ${TEST_OUTPUT_DIRECTORY=$(dirname $0)/..}
+
output=
count=0
total_count=0
@@ -115,7 +119,7 @@ handle_one () {
finish_output
}
-for test_script in $(dirname $0)/../test-results/*.out
+for test_script in $TEST_OUTPUT_DIRECTORY/test-results/*.out
do
handle_one $test_script
done
--
1.8.3.rc0.149.g98a72f2.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] test output: respect $TEST_OUTPUT_DIRECTORY
On Mon, Apr 29, 2013 at 11:17:00AM -0700, Junio C Hamano wrote:
Thomas Rast tr...@inf.ethz.ch writes:
John Keeping j...@keeping.me.uk writes:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ca6bdef..70ad085 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,8 +54,8 @@ done,*)
# do not redirect again
;;
*' --tee '*|*' --va'*)
- mkdir -p test-results
- BASE=test-results/$(basename $0 .sh)
+ mkdir -p $(TEST_OUTPUT_DIRECTORY)/test-results
+ BASE=$(TEST_OUTPUT_DIRECTORY)/test-results/$(basename $0 .sh)
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} $0 $@ 21;
echo $? $BASE.exit) | tee $BASE.out
test $(cat $BASE.exit) = 0
Hmm, I initially was too lazy to review this change, and now it's biting
me. The above is Makefile-quoted, which to the shell reads like a
command substitution.
Heh, when I let my eyes coast over it I didn't notice them either.
Everything in that patch is bad.
This squashed in?
This is identical to the interdiff of what I posted at the same time, so
it obviously looks good to me.
t/test-lib.sh | 4 ++--
t/valgrind/analyze.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1b1e843..e7d169c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,8 +54,8 @@ done,*)
# do not redirect again
;;
*' --tee '*|*' --va'*)
- mkdir -p $(TEST_OUTPUT_DIRECTORY)/test-results
- BASE=$(TEST_OUTPUT_DIRECTORY)/test-results/$(basename $0 .sh)
+ mkdir -p $TEST_OUTPUT_DIRECTORY/test-results
+ BASE=$TEST_OUTPUT_DIRECTORY/test-results/$(basename $0 .sh)
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} $0 $@ 21;
echo $? $BASE.exit) | tee $BASE.out
test $(cat $BASE.exit) = 0
diff --git a/t/valgrind/analyze.sh b/t/valgrind/analyze.sh
index 7b58f01..2ffc80f 100755
--- a/t/valgrind/analyze.sh
+++ b/t/valgrind/analyze.sh
@@ -119,7 +119,7 @@ handle_one () {
finish_output
}
-for test_script in $(TEST_OUTPUT_DIRECTORY)/test-results/*.out
+for test_script in $TEST_OUTPUT_DIRECTORY/test-results/*.out
do
handle_one $test_script
done
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unexpected behavior of git-subtree
On Mon, Apr 29, 2013 at 07:34:27PM +0200, Steffen Jaeckel wrote: lately I used git-subtree to integrate a submodule directly into a repository. Now I wanted to push the changes back to the original repository of the submodule and I was a bit surprised by what happened... snip sjaeckel@T7400-003 /h/projects/my_project (develop) $ GIT_TRACE=2 git subtree push --prefix=lib/com_lib/ git@git.local:com_lib develop -b develop trace: exec: 'git-subtree' 'push' '--prefix=lib/com_lib/' 'git@git.local:com_lib' 'develop' '-b' 'develop' trace: run_command: 'git-subtree' 'push' '--prefix=lib/com_lib/' 'git@git.local:com_lib' 'develop' '-b' 'develop' trace: built-in: git 'rev-parse' '--parseopt' '--' 'push' '--prefix=lib/com_lib/' 'git@git.local:com_lib' 'develop' '-b' 'develop' trace: built-in: git 'rev-parse' '--git-dir' trace: built-in: git 'rev-parse' '--show-cdup' git push using: git@git.local:com_lib develop trace: exec: 'git-subtree' 'split' '--prefix=lib/com_lib/' trace: run_command: 'git-subtree' 'split' '--prefix=lib/com_lib/' trace: built-in: git 'rev-parse' '--parseopt' '--' 'split' '--prefix=lib/com_lib/' trace: built-in: git 'rev-parse' '--git-dir' trace: built-in: git 'rev-parse' '--show-cdup' trace: built-in: git 'rev-parse' '--default' 'HEAD' '--revs-only' trace: built-in: git 'rev-parse' '--no-revs' '--no-flags' trace: built-in: git 'log' '--grep=^git-subtree-dir: lib/com_lib/*$' '--pretty=format:START %H%n%s%n%n%b%nEND%n' '8068a810709f6284b04a18ff38dbb72c36b8d9c6' trace: built-in: git 'rev-list' '--topo-order' '--reverse' '--parents' '8068a810709f6284b04a18ff38dbb72c36b8d9c6' trace: built-in: git 'rev-list' '--topo-order' '--reverse' '--parents' '8068a810709f6284b04a18ff38dbb72c36b8d9c6' 1/102 (0)trace: built-in: git 'ls-tree' 'f6e1457d345029cf4d376ff3cf780cbb8c3080b4' '--' 'lib/com_lib' . loads of more git 'ls-tree'... 71/102 (70)72/102 (70)trace: built-in: git 'ls-tree' '48dc0efb9a148b1146b013554f8bf4635adf7a0d' '--' 'lib/com_lib' trace: built-in: git 'log' '-1' '--pretty=format:%T' '6299b48765e11302aca48cc9fca88735aeab7c54' '--' fatal: bad object 6299b48765e11302aca48cc9fca88735aeab7c54 Without knowing much about git-subtree, this looks like the culprit. A quick look at cmd_push in git-subtree.sh indicates that it doesn't check for an error return from 'git subtree split --prefix=$prefix', so if that goes wrong you end up doing: git push $repository :refs/heads/$refspec which deletes the branch. Can you try running this: git subtree split --prefix=lib/com_lib/ and see if that gives the same fatal: bad object message as above? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] Highly inconsistent diff UI
On Mon, Apr 29, 2013 at 09:32:51AM -0700, Junio C Hamano wrote: *1* Instead, you have a separate integration branch for testing that merges other's work and your topic. shameless-plug I wrote a script to help manage this [1]. It doesn't do everything I want it to yet but I'm using it on a daily basis to maintain a Git version consisting of pu plus a few changes that I'm working on which aren't ready for submission yet as well as for maintaining integration branches in a couple of other projects. You can create an integration branch on top of pu quickly with: git integration --create my-integration-branch pu \ --add my-feature-branch --rebuild And then whenever you want to rebuild with your latest changes (or on top of the latest pu): git integration --rebuild my-integration-branch [1] http://johnkeeping.github.io/git-integration/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v1.8.3-rc0
On Fri, Apr 26, 2013 at 05:22:22PM -0700, Junio C Hamano wrote: * git difftool allows the user to write into the temporary files being shown; if the user makes changes to the working tree at the same time, one of the changes has to be lost in such a case, but it tells the user what happened and refrains from overwriting the copy in the working tree. This feels slightly misleading to me, perhaps something like this would be clearer? git difftool allows the user to write into the temporary files being shown; if the user makes changes to the working tree at the same time, it now refrains from overwriting the copy in the working tree and leaves the temporary file so that changes can be merged manually. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
