[PATCH] remote-hg: set stdout to binary mode on win32
From: Amit Bakshi ambak...@gmail.com
git clone hangs on windows, and file.write would return errno 22 inside
of mercurial's windows.winstdout wrapper class. This patch sets stdout's
mode to binary, fixing both issues.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
contrib/remote-helpers/git-remote-hg | 4
1 file changed, 4 insertions(+)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index beb864b..01555dc 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -954,6 +954,10 @@ def main(args):
marks_path = os.path.join(dirname, 'marks-hg')
marks = Marks(marks_path)
+if sys.platform == 'win32':
+import msvcrt
+msvcrt.setmode(sys.stdout.fileno(), os.O_BINARY)
+
parser = Parser(repo)
for line in parser:
if parser.check('capabilities'):
--
1.8.3.rc3.286.g3d43083
--
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 3/9] for-each-ref: avoid printing each element directly to stdout
On Sun, May 19, 2013 at 6:17 PM, Ramkumar Ramachandra artag...@gmail.com wrote: [PATCH 3/9] for-each-ref: avoid printing each element directly to stdout Why did you do this? Atoms are designed to be independent of each other. I'll keep reading: might find out in a later patch. Sorry for the lack of explanation in the message. I wanted to discuss about syntax more than code. As you see later down the series, we'll need to process an atom for all refs, then output all refs in the end. We can't do that wihout buffering. -- Duy -- 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: Random thoughts on upstream
On Sun, May 19, 2013 at 6:54 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Felipe Contreras wrote: You can't represent push.default = single either. Right. And I propose that we extend the refspec to be able to represent it, instead of having single sticking out like a sore thumb (and possibly introducing more sore thumbs like this in the future). Yeah, go ahead and have fun making refspecs Turing complete. Why? Because you decided to call something a sore thumb. Calling something a sore thumb doesn't make it so. We, the sane people, will people will keep using simple configuration options. Hopefully some day there won't be much need for many of the current configurations, including push.default. -- Felipe Contreras -- 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] remote-hg: set stdout to binary mode on win32
On Sun, May 19, 2013 at 6:53 AM, Felipe Contreras felipe.contre...@gmail.com wrote: From: Amit Bakshi ambak...@gmail.com git clone hangs on windows, and file.write would return errno 22 inside of mercurial's windows.winstdout wrapper class. This patch sets stdout's mode to binary, fixing both issues. Forgot: [fc: cleaned up] Signed-off-by: Felipe Contreras felipe.contre...@gmail.com -- Felipe Contreras -- 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 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info
On Sun, May 19, 2013 at 6:48 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Duy Nguyen wrote: Exactly. I already explained why %(upstream) can't be used in 00/09. tracking may not be perfect. Somebody might want tracking:upstream:short. It does not look quite nice. Which is why I suggested keeping upstream, upstream:short, and introducing upstream:diff and upstream:shortdiff (or :tracking if you prefer that) in [0/9]. Yeah, but there won't be any upstream in %(tracking). Besides, if we manage to get downstream, we could do %(tracking:downstream). I think %(tracking) and %(short:tracking) make sense. -- Felipe Contreras -- 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] for-each-ref: introduce color format
On Fri, May 17, 2013 at 9:55 PM, Ramkumar Ramachandra artag...@gmail.com wrote: You can now do something like $ git for-each-ref --format='%C(red)%(refname:short)%C(reset) %C(blue)%(upstream:diff)%C(reset)' --count 5 --sort='-committerdate' refs/heads To get output that's much more customizable 'git branch' output. Future patches will attempt unify the semantics of 'git branch' and 'git for-each-ref'. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- So my evil plan is to keep extending this format until it's on par with pretty-formats. Then, we can move towards unifying 'git branch' and 'git for-each-ref'. This will involve deprecating badly thought-out options like '-v', and replacing it with the more powerful '--format'. I just have one major doubt: in the above output, how do I align all the upstream branches to the same column? How can I achieve it with pretty-formats? Something like %*d? But * is already taken to mean deref in for-each-ref's --format. By the way, the main motivation for all this comes from the fact that git for-each-ref is very nicely written :) Look at how it breaks everything up into atoms and lazily gets the information it needs to display. If you can put energy into this, I suggest you improve pretty.c a bit, adding new format_ref_message(), similar to format_commit_message(), except that it takes a ref. You can extend struct format_commit_context to contain what struct refinfo does. I think pretty.c only lacks a few specifiers that for-each-ref has. I think I mentioned it in my for-each-ref series already, but we need to think how to specify align to the left with the best width and how format_ref_message() can figure the width out. A callback function might do. -- Duy -- 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/WIP 0/9] for-each-ref format improvements
Duy Nguyen wrote: Hmm.. I missed that mail (or I wouldn't have worked on this already). Do you want to take over? Oh, we can collaborate on one beautiful series :) branch -vv shows [upstream: ahead x, behind y]. We need a syntax to cover that too. Can't we construct that using [%(upstream:short): %(upstream:diff)]? It's nothing fundamental. pretty and for-each-ref format seem to be on the opposite: one is terse, one verbose. Unless you are going to introduce a lot of new specifiers (and in the worst case, bring all pretty specifiers over, unify underlying code), I think we should stick with %(xx) convention. We can stick to using the existing %(...) atoms: there's no need to go as far as %an versus %aN. The atoms cannot be consistent with pretty-formats anyway, because pretty-formats has completely different atoms. For the _new_ stuff like color and alignment, we can be consistent with pretty-formats, no? Why should we deviate from the pretty case? What is different here? Laziness plays a big factor :) So again, you want to take over? ;) It's just a matter of modifying the parsing/printing layer, instead of introducing new atoms in the current parser. Doesn't $gmane/224692 demonstrate that the former can, in fact, be easier? it uses builtin/branch.c:branch_use_color. Eventually fill_tracking_info() should be moved to for-each-ref.c and pass branch_use_color in as an argument. But for now, I just leave it broken. Auto-color can come later: it's not that urgent. What's more important is that we give users the flexibility to set their own colors now. Can you give me a pull URL to your series, so we can start collaborating? Mine's at gh:artagnon/git (branch: hot-branch). -- 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: add new fetch.default configuration
On Sun, May 19, 2013 at 6:51 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Felipe Contreras wrote:
% git checkout fc/remote/hg-next
% git rebase -i # rebase to master
% git checkout fc/remote/hg-notes
% git rebase -i # rebase to fc/remote/hg-next
% git checkout fc/remote/hg-gitifyhg-compat
% git rebase -i # rebase to fc/remote/hg-notes
So it is rebase, but rebase.defaultUpstream = origin won't suffice.
You want to specify a custom base. What I don't understand is why
you're abusing @{u},
There's no abuse; 'upstream' is meant *exactly* for this: rebase, and pull.
It's called upstream because that's where eventually all the code must
go, so semantically it's *exactly* correct: 'origin/master' is the
upstream of 'master' which is the upstream of 'fc/remote/hg-next',
which is the upstream of 'fc/remote/hg-notes', and so on.
And if I where to merge a branch from 'master', it make sense that the
default is 'origin/master', because that's the upstream.
Literally each and every way in which 'upstream' is used remains the
same for local branches, except for 'git push'.
and crippling remote (by hard-interpreting it as
origin) to achieve this.
More false rhetoric; origin is *ALREADY* the default.
Show me the output of these:
%git clone --origin foobar git://git.kernel.org/pub/scm/git/git.git /tmp/git
%cd /tmp/git
%git fetch -v
%git checkout --no-track -b test master
%git fetch -v
For the record, I didn't think your
branch.name.base was a bad idea at all (re-visiting now).
It makes sens for rebase, but what happens when you do 'git pull' and
you don't have an upstream configured, only a base? Why would you want
'git pull' to fail? In which instances would you want to have a
'base', but not an 'upstream'?
Think about it.
--
Felipe Contreras
--
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: add new fetch.default configuration
My itch is very simple. Felipe Contreras wrote: % git checkout fc/remote/hg-next % git rebase -i # rebase to master % git pull # I want: pull from origin % git checkout fc/remote/hg-notes % git rebase -i # rebase to fc/remote/hg-next % git pull # I want: pull from ram % git checkout fc/remote/hg-gitifyhg-compat % git rebase -i # rebase to fc/remote/hg-notes % git pull # I want: pull from felipe With your series, rebase works and I like that. But, by specifying branch.name.remote as '.', I've essentially lost a way to specify a remote I want. Why can't I have both? -- 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/WIP 0/9] for-each-ref format improvements
On Sun, May 19, 2013 at 7:08 PM, Ramkumar Ramachandra artag...@gmail.com wrote: branch -vv shows [upstream: ahead x, behind y]. We need a syntax to cover that too. Can't we construct that using [%(upstream:short): %(upstream:diff)]? It's nothing fundamental. If there is no upstream, [] should not be shown. We don't have conditional specifiers so [] should be produced by one of the specifiers. But yes it's nothing fundamental. pretty and for-each-ref format seem to be on the opposite: one is terse, one verbose. Unless you are going to introduce a lot of new specifiers (and in the worst case, bring all pretty specifiers over, unify underlying code), I think we should stick with %(xx) convention. We can stick to using the existing %(...) atoms: there's no need to go as far as %an versus %aN. The atoms cannot be consistent with pretty-formats anyway, because pretty-formats has completely different atoms. For the _new_ stuff like color and alignment, we can be consistent with pretty-formats, no? I don't think you can easily borrow parsing code from pretty-formats (but I may be wrong). Anyway new stuff with new syntax would look alien in for-each-ref format lines. Either we bring --pretty to for-each-ref, leaving all for-each-ref atoms behind in --format, or we should follow %(..) convention if we add new stuff to --format. Why should we deviate from the pretty case? What is different here? Laziness plays a big factor :) So again, you want to take over? ;) It's just a matter of modifying the parsing/printing layer, instead of introducing new atoms in the current parser. Doesn't $gmane/224692 demonstrate that the former can, in fact, be easier? Yes it looks so. it uses builtin/branch.c:branch_use_color. Eventually fill_tracking_info() should be moved to for-each-ref.c and pass branch_use_color in as an argument. But for now, I just leave it broken. Auto-color can come later: it's not that urgent. What's more important is that we give users the flexibility to set their own colors now. Can you give me a pull URL to your series, so we can start collaborating? Mine's at gh:artagnon/git (branch: hot-branch). https://github.com/pclouds/git/tree/for-each-ref-pretty I merged 07/09 and 08/09 together as they should be one. -- Duy -- 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: add new fetch.default configuration
On Sun, May 19, 2013 at 7:26 AM, Ramkumar Ramachandra artag...@gmail.com wrote: My itch is very simple. Felipe Contreras wrote: % git checkout fc/remote/hg-next % git rebase -i # rebase to master % git pull # I want: pull from origin Then do 'git pull origin', 'fc/remote/hg-next' has *nothing* to do with origin. % git checkout fc/remote/hg-notes % git rebase -i # rebase to fc/remote/hg-next % git pull # I want: pull from ram Ditto. % git checkout fc/remote/hg-gitifyhg-compat % git rebase -i # rebase to fc/remote/hg-notes % git pull # I want: pull from felipe Ditto. With your series, rebase works and I like that. But, by specifying branch.name.remote as '.', I'm not doing that *GIT* is doing that. What does this throw? % git checkout --track -b foo master % git config branch.foo.remote I've essentially lost a way to specify a remote I want. Why can't I have both? You haven't lost anything. The upstream is 'branch.x.remote'+'branch.x.merge', and it remains so before and after this patch. You can set 'branch.x.remote' to whatever you want. -- Felipe Contreras -- 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/WIP 0/9] for-each-ref format improvements
Duy Nguyen wrote: I don't think you can easily borrow parsing code from pretty-formats (but I may be wrong). Anyway new stuff with new syntax would look alien in for-each-ref format lines. Either we bring --pretty to for-each-ref, leaving all for-each-ref atoms behind in --format, or we should follow %(..) convention if we add new stuff to --format. Why so extremist? pretty-formats has %(...), %C(...) as well as %..., so why shouldn't we? Our format is undocumented, and I doubt anyone even uses it; we're not breaking anyone's expectations. I'm just saying that our format can be a little reminiscent of pretty-formats, nothing more. There's no need to borrow parsing code: we can do it ourselves, I think. There is no need to go to the other extreme and throw out the existing --format and start out with a --pretty from scratch either: the current code isn't so bad that we can't build on top of it. Sure, we can eventually deprecate --format and move to --pretty for consistency (but that's a long-term goal). -- 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: About overzealous compatibility
On Sun, May 19, 2013 at 2:56 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Fri, May 17, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com wrote: So when the user is running git fetch on mywork branch that happens to be forked from a local master,... we still need to have FETCH_HEAD updated to point at what we would be merging if she did a git pull. No, we don't need that. That is only needed by 'git pull', and in fact, it should be possible to reimplement 'git pull' so that it skips FETCH_HEAD when the remote is local. These are mere implementation details. You seem to be incapable to understand what backward compatibility is. Really? Do you even remember the time when you changed out of nowhere all the 'git-foo' commands with 'git foo' and all hell broke loose? I remember some lonely voice of reason shouting for clear deprecation warnings: http://article.gmane.org/gmane.comp.version-control.git/94262 After re-reading this old thread, I noticed that you didn't get a straight answer, nor was there a neat conclusion, and the good replies might have been lost in the noise. So this is what you should have done: 1) Fix all the tests and documentation to use the 'git foo' form, so everything is consistent and the proper form of the commands is explained. 2) Release v1.6.0 turning on annoying deprecation warnings telling people to stop using 'git-foo'. This wouldn't have had the same bad effect that v1.6.0 had, because you added at the same time a configuration to turn the annoying message off, so users, and administrators can choose to ignore the warning, and then they couldn't complain when the 'git-foo' links get removed for real. 3) To be absolutely sure that people get the message that there's a big change, name the release v2.0. I think 3) would have been overkill; v1.7.0 would be OK, but 2) was definitely needed, and 1) would have been great. I think it's extremely cheap that you accuse me of not understanding backwards compatibility, when I did for zsh's completion[1] exactly what you should have done for v1.6.0: add an annoying deprecation warning. Cheers. [1] http://article.gmane.org/gmane.comp.version-control.git/210024 -- Felipe Contreras -- 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 v5 01/15] Add new git-related helper to contrib
Okay, let's look at this part.
Felipe Contreras wrote:
diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 000..4f31482
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,124 @@
+#!/usr/bin/env ruby
+
+# This script finds people that might be interested in a patch
+# usage: git related file
+
+$since = '5-years-ago'
+$min_percent = 10
+
+def fmt_person(name, email)
+ name ? '%s %s' % [name, email] : email
+end
+
+class Commit
+
+ attr_reader :persons
+
+ def initialize(id)
+@id = id
+@persons = []
+ end
Okay, although I'm wondering what id is.
+ def parse(data)
+msg = nil
+data.each_line do |line|
+ if not msg
+case line
+when /^author ([^]+) (\S+) (.+)$/
+ @persons fmt_person($1, $2)
+when /^$/
+ msg = true
+end
When there's a blank line, flip the switch and start looking in the
commit message. Why though? You're worried that the commit message
will contain a line matching /^author ([^]+) (\S+) (.+)$/? If so,
why don't you split('\n', 2), look for the author in the $1 and
Whatevered-by in $2?
+ else
+if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^]+) (\S+?)$/
elif? Can this be more generic to include Whatevered-by?
+ @persons fmt_person($2, $3)
Will $2 ever be nil (from fmt_person)? ie. Why are you checking for
the special case \S+?$?
+end
+ end
+end
+@persons.uniq!
So you don't care if the person has appeared twice or thrice in the message.
+ end
+
+end
+
+class Commits
+
+ def initialize
+@items = {}
+ end
Okay.
+ def size
+@items.size
+ end
#size is reminiscent of Array#size and Hash#size.
+ def each(block)
+@items.each(block)
+ end
I can see how you iterate over commits from the code below. However,
using #each like this is confusing, because the reader expects #each
to be invoked on an Enumerable. So, I have two suggestions:
1. Use block_given? to make sure that #each works even without a block
(just like Enumerator#each).
2. Mixin Enumerable by putting in an 'include Enumerable' after the
class line. Aside from clarity, you get stuff like #find for free.
+ def import
From reading the code below, your calling semantics are: first set
each @items[id] to a new Commit object. import is meant to invoke
Commit#parse and set @persons there. Okay.
+return if @items.empty?
+File.popen(%w[git cat-file --batch], 'r+') do |p|
+ p.write(@items.keys.join(\n))
+ p.close_write
Okay.
+ p.each do |l|
+if l =~ /^(\h{40}) commit (\d+)/
s/l/line/?
+ id, len = $1, $2
+ data = p.read($2.to_i)
+ @items[id].parse(data)
+end
+ end
+end
+ end
+
+ def get_blame(source, start, len, from)
+return if len == 0
+len ||= 1
Please don't use ||=. It is notorious for causing confusion in
Ruby-land. Hint: it's not exactly equivalent to either len = len || 1
or len || len = 1.
+File.popen(['git', 'blame', '--incremental', '-C',
Still no -CCC?
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+if line =~ /^(\h{40})/
+ id = $1
Use $0 and remove the parens: you're matching the whole line.
+ @items[id] = Commit.new(id)
Okay.
+end
+ end
+end
+ end
+
+ def from_patch(file)
+from = source = nil
+File.open(file) do |f|
+ f.each do |line|
+case line
+when /^From (\h+) (.+)$/
+ from = $1
Okay.
+when /^---\s+(\S+)/
+ source = $1 != '/dev/null' ? $1[2..-1] : nil
Okay.
+when /^@@ -(\d+)(?:,(\d+))?/
+ get_blame(source, $1, $2, from)
Okay.
+end
+ end
+end
+ end
+
+end
+
+exit 1 if ARGV.size != 1
Okay.
+commits = Commits.new
+commits.from_patch(ARGV[0])
+commits.import
The calling semantics could be better, but it's not a big complaint.
+count_per_person = Hash.new(0)
Initializing all keys to 0. Okay.
+commits.each do |id, commit|
Cute.
+ commit.persons.each do |person|
+count_per_person[person] += 1
Okay.
+ end
+end
+
+count_per_person.each do |person, count|
+ percent = count.to_f * 100 / commits.size
+ next if percent $min_percent
+ puts person
Not going to print percentage as well?
Overall, significant improvement over v3 which used all kinds of
unnecessarily complex data structures and convoluted logic. Looks
like something I'd want to use: not blindly as a cc-cmd, but just to
get a quick idea. I also wish for depth most times: a working
shortlog -L --follow would be really nice.
--
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
Re: [PATCH v5 01/15] Add new git-related helper to contrib
On Sun, May 19, 2013 at 9:40 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Okay, let's look at this part.
Felipe Contreras wrote:
diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 000..4f31482
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,124 @@
+#!/usr/bin/env ruby
+
+# This script finds people that might be interested in a patch
+# usage: git related file
+
+$since = '5-years-ago'
+$min_percent = 10
+
+def fmt_person(name, email)
+ name ? '%s %s' % [name, email] : email
+end
+
+class Commit
+
+ attr_reader :persons
+
+ def initialize(id)
+@id = id
+@persons = []
+ end
Okay, although I'm wondering what id is.
The commit's unique identifier. How is that not clear?
+ def parse(data)
+msg = nil
+data.each_line do |line|
+ if not msg
+case line
+when /^author ([^]+) (\S+) (.+)$/
+ @persons fmt_person($1, $2)
+when /^$/
+ msg = true
+end
When there's a blank line, flip the switch and start looking in the
commit message. Why though? You're worried that the commit message
will contain a line matching /^author ([^]+) (\S+) (.+)$/? If so,
why don't you split('\n', 2), look for the author in the $1 and
Whatevered-by in $2?
That's not efficient, it's more efficient to parse line by line
lazily, and it's not that complicated.
+ else
+if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^]+) (\S+?)$/
elif?
It will get bigger soon enough.
Can this be more generic to include Whatevered-by?
That's done in later patches. This is a minimal version.
+ @persons fmt_person($2, $3)
Will $2 ever be nil (from fmt_person)? ie. Why are you checking for
the special case \S+?$?
Yes, 'email' was valid in earlier versions of git.
+end
+ end
+end
+@persons.uniq!
So you don't care if the person has appeared twice or thrice in the message.
Yes I do, otherwise they be counted multiple times, and their
participation calculation would go beyond 100%.
+ def size
+@items.size
+ end
#size is reminiscent of Array#size and Hash#size.
Precisely. I would even include Array if it made sense.
+ def each(block)
+@items.each(block)
+ end
I can see how you iterate over commits from the code below. However,
using #each like this is confusing, because the reader expects #each
to be invoked on an Enumerable. So, I have two suggestions:
We can include Enumerable, it won't make a difference.
1. Use block_given? to make sure that #each works even without a block
(just like Enumerator#each).
It already works just like Enumerator#each; commits.each returns an
Enumerator because @items.each returns an Enumerator.
2. Mixin Enumerable by putting in an 'include Enumerable' after the
class line. Aside from clarity, you get stuff like #find for free.
Stuff we don't need or want.
+ def import
From reading the code below, your calling semantics are: first set
each @items[id] to a new Commit object. import is meant to invoke
Commit#parse and set @persons there. Okay.
+return if @items.empty?
+File.popen(%w[git cat-file --batch], 'r+') do |p|
+ p.write(@items.keys.join(\n))
+ p.close_write
Okay.
+ p.each do |l|
+if l =~ /^(\h{40}) commit (\d+)/
s/l/line/?
Fine.
+ id, len = $1, $2
+ data = p.read($2.to_i)
+ @items[id].parse(data)
+end
+ end
+end
+ end
+
+ def get_blame(source, start, len, from)
+return if len == 0
+len ||= 1
Please don't use ||=. It is notorious for causing confusion in
Ruby-land. Hint: it's not exactly equivalent to either len = len || 1
or len || len = 1.
How exactly is it not equivalent to len = len || 1?
This is what I want:
len = 1 if not len
And I think the above does exactly that.
+File.popen(['git', 'blame', '--incremental', '-C',
Still no -CCC?
I forgot.
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+if line =~ /^(\h{40})/
+ id = $1
Use $0 and remove the parens: you're matching the whole line.
No, I'm not matching the whole line, but you are right; there's no
need for groups.
+ end
+end
+
+count_per_person.each do |person, count|
+ percent = count.to_f * 100 / commits.size
+ next if percent $min_percent
+ puts person
Not going to print percentage as well?
Later. This does the minimum.
Overall, significant improvement over v3 which used all kinds of
unnecessarily complex data structures and convoluted logic.
It's coming.
--
Felipe Contreras
--
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 v5 01/15] Add new git-related helper to contrib
Felipe Contreras wrote: How exactly is it not equivalent to len = len || 1? Here, I dug up an article for you on the issue: http://www.rubyinside.com/what-rubys-double-pipe-or-equals-really-does-5488.html Although it's fine in this case, I wouldn't recommend using ||= because of the potential confusion. -- 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 v5 01/15] Add new git-related helper to contrib
Felipe Contreras wrote: Will $2 ever be nil (from fmt_person)? ie. Why are you checking for the special case \S+?$? Yes, 'email' was valid in earlier versions of git. There's a non-optional space before the email in your regex, which is what I was pointing out. -- 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 v5 01/15] Add new git-related helper to contrib
Ramkumar Ramachandra wrote: There's a non-optional space before the email in your regex, which is what I was pointing out. Er, scratch that. It's the space after the Whatevered-by: -- 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 v5 01/15] Add new git-related helper to contrib
On Sun, May 19, 2013 at 10:13 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Felipe Contreras wrote: How exactly is it not equivalent to len = len || 1? Here, I dug up an article for you on the issue: http://www.rubyinside.com/what-rubys-double-pipe-or-equals-really-does-5488.html Although it's fine in this case, I wouldn't recommend using ||= because of the potential confusion. I don't see the confusion, 'len ||= 1' is *exactly* the same as 'len = 1 if not len', which is what I expected. -- Felipe Contreras -- 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 v5 01/15] Add new git-related helper to contrib
On Sun, May 19, 2013 at 10:17 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Ramkumar Ramachandra wrote: There's a non-optional space before the email in your regex, which is what I was pointing out. Er, scratch that. It's the space after the Whatevered-by: It doesn't really matter. We can operate under the assumption that there will always be a name and remove the extra code in fmt_person(), IIRC it was because of the muttrc simplification which I don't think makes sense any more. It's still possible that the name will be missing, but I don't personally care, it would probably work for 99.99% of the cases. -- Felipe Contreras -- 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 v5 01/15] Add new git-related helper to contrib
On Sun, May 19, 2013 at 10:05 AM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Sun, May 19, 2013 at 9:40 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+if line =~ /^(\h{40})/
+ id = $1
Use $0 and remove the parens: you're matching the whole line.
No, I'm not matching the whole line, but you are right; there's no
need for groups.
Actually $.
--
Felipe Contreras
--
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 v6] Add new git-related helper to contrib
This script find people that might be interested in a patch, by going
back through the history for each single hunk modified, and finding
people that reviewed, acknowledge, signed, or authored the code the
patch is modifying.
It does this by running 'git blame' incrementally on each hunk, and then
parsing the commit message. After gathering all the relevant people, it
groups them to show what exactly was their role when the participated in
the development of the relevant commit, and on how many relevant commits
they participated. They are only displayed if they pass a minimum
threshold of participation.
For example:
% git related 0001-remote-hg-trivial-cleanups.patch
Felipe Contreras felipe.contre...@gmail.com
Jeff King p...@peff.net
Max Horn m...@quendi.de
Junio C Hamano gits...@pobox.com
Thus it can be used for 'git send-email' as a cc-cmd.
There might be some other related functions to this script, not just to
be used as a cc-cmd.
Comments-by: Ramkumar Ramachandra artag...@gmail.com
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
Sames as v5, with a few tiny modifications. I'm tired of sending the whole
series multiple times over the years, only to get stuck at the first patch, so
I'll send only the first one.
contrib/related/git-related | 124
1 file changed, 124 insertions(+)
create mode 100755 contrib/related/git-related
diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 000..b96dcdd
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,124 @@
+#!/usr/bin/env ruby
+
+# This script finds people that might be interested in a patch
+# usage: git related file
+
+$since = '5-years-ago'
+$min_percent = 10
+
+def fmt_person(name, email)
+ '%s %s' % [name, email]
+end
+
+class Commit
+
+ attr_reader :persons
+
+ def initialize(id)
+@id = id
+@persons = []
+ end
+
+ def parse(data)
+msg = nil
+data.each_line do |line|
+ if not msg
+case line
+when /^author ([^]+) (\S+) (.+)$/
+ @persons fmt_person($1, $2)
+when /^$/
+ msg = true
+end
+ else
+if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^]+) (\S+?)$/
+ @persons fmt_person($2, $3)
+end
+ end
+end
+@persons.uniq!
+ end
+
+end
+
+class Commits
+
+ def initialize
+@items = {}
+ end
+
+ def size
+@items.size
+ end
+
+ def each(block)
+@items.each(block)
+ end
+
+ def import
+return if @items.empty?
+File.popen(%w[git cat-file --batch], 'r+') do |p|
+ p.write(@items.keys.join(\n))
+ p.close_write
+ p.each do |line|
+if line =~ /^(\h{40}) commit (\d+)/
+ id, len = $1, $2
+ data = p.read($2.to_i)
+ @items[id].parse(data)
+end
+ end
+end
+ end
+
+ def get_blame(source, start, len, from)
+return if len == 0
+len ||= 1
+File.popen(['git', 'blame', '--incremental', '-CCC',
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+if line =~ /^(\h{40})/
+ id = $
+ @items[id] = Commit.new(id)
+end
+ end
+end
+ end
+
+ def from_patch(file)
+from = source = nil
+File.open(file) do |f|
+ f.each do |line|
+case line
+when /^From (\h+) (.+)$/
+ from = $1
+when /^---\s+(\S+)/
+ source = $1 != '/dev/null' ? $1[2..-1] : nil
+when /^@@ -(\d+)(?:,(\d+))?/
+ get_blame(source, $1, $2, from)
+end
+ end
+end
+ end
+
+end
+
+exit 1 if ARGV.size != 1
+
+commits = Commits.new
+commits.from_patch(ARGV[0])
+commits.import
+
+count_per_person = Hash.new(0)
+
+commits.each do |id, commit|
+ commit.persons.each do |person|
+count_per_person[person] += 1
+ end
+end
+
+count_per_person.each do |person, count|
+ percent = count.to_f * 100 / commits.size
+ next if percent $min_percent
+ puts person
+end
--
1.8.3.rc3.286.g3d43083
--
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: English/German terminology, git.git's de.po, and pro-git
2013/5/16 Holger Hellmuth (IKS) hellm...@ira.uka.de: +bare repository= bloßes Repository Since bloßes Rep. does not convey any sensible meaning to a german reader (at least it doesn't to me) it might as well be bare. Also bare is used as parameter to commands +remote tracking branch = externer Übernahmezweig Anyone used to the english client will switch as soon as he has to read this. No idea how to improve that though except to just use the english terms like the pro git translation does. +upstream branch= -||- Use upstream as it is used as parameter to commands +fetch = anfordern fetch = fetch +pull = zusammenführen pull = pull +push = versenden push = push established vocabulary used in stack programming as well as in vcs. Should not be translated. I think the messages would become a bit too G+E when we'd say something like Das Fetchen in den Branch..., Fetche von %s. Some for merge as a verb. +clean(verb)= clean(verb) = säubern/aufräumen +clean(noun)= clean(noun) = Säuberung aufräumen is the better verb but there is no noun for it. +whitespace = Leerzeichen (FIXME?) (maybe Leerraum) whitespace = whitespace There is no german word for whitespace +Still being worked out: + +prune = veraltete(n) Zweig(e) entfernen +checkout(verb) = auschecken + +git add = hinzufügen mittels git add hinzufügen if you want to emphasize that you add something with the command + +merge conflict = Merge-Konflikt +3-way merge= 3-Wege-Merge +paths = Pfade + +symbolic link = symbolische Verknüfung +path = Pfad +link = Verknüpfung + +reflog = Referenzprotokoll +partial commit = teilweise committen, partiell committen As a noun, Teil-Commit + +reset = neu setzen (maybe umsetzen?) zurücksetzen I'll send a new version to the list later. Thanks -- 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: English/German terminology, git.git's de.po, and pro-git
2013/5/16 Thomas Rast tr...@inf.ethz.ch:
Ralf Thielow ralf.thie...@gmail.com writes:
Hi,
I think the discussion might work better via ML than GitHub.
This is the full glossary of git's de.po as it would look
like with (hopefully) all the changes included that have been
discussed here. Still without any reasoning for decisions
(except HEAD).
[...]
+remote branch = externer Zweig
+remote tracking branch = externer Übernahmezweig
Hrm, before we (erm, you) do any extensive work on redoing the glossary,
I think we should step back and agree on a direction.
Remember what this thread started with:
} However, an unfortunate and unsatisfactory situation has developed:
} Christian Stimming's git-gui de.po uses a Ger translation, and Ralf
} Thielow built core git's de.po on top of it, so it's also Ger.
}
} Meanwhile, and independently, Sven Fuchs and Ralph Haussmann wrote a
} translation of pro-git (which is also quite mature at this point, having
} apparently begun in 2009), and as you probably guessed by now, it's G+E.
}
} So that leaves us at a point where the libre Git book (and also the
} one that happens to be hosted on git-scm.com, the official site) does
} not match the terminology used by German git.
}
} Like, at all. They're not even remotely near each other.
My thinly veiled opinion in the thread starter was that we should redo
git's de.po from scratch using a translation similar to pro-git.
I can accept that discussion takes a different turn, and thus the
translation does something else. But my impression in the thread so far
was that:
* Everyone voted for G+E.
* The thread went of on a tangent, bikeshedding on some Ger
translations.
Please tell me I'm wrong...
Otherwise, assuming any agreement can be reached, IMHO the first step
must be to write/complete a glossary that matches *current usage* in
pro-git. We can perhaps bikeshed about some glaring issues in the
result, but remember that -- again assuming G+E is the conclusion -- any
such change again either means a divergence between book and git (bad!)
or a lot of work for the book translators.
Well, that's what I'm trying to do, writing a new glossary. But I took
the current git's de.po glossay as the base, because it's the biggest one
and easier to apply to de.po instead of using a complete new one.
I tried to merge [1] (link is dead) to match ProGit-Book where it's possible.
IMO it's OK if we don't match the ProGit-book in all terms (I didn't do it with
intention), but it's not OK if the translations are so far away from each other
that it becomes a problem to the users because they're using totally different
languages.
What I'm doing now is collecting objections and suggestions from others (ML, GH)
and apply them to the glossary in order to get a version where everybody
more or less agree with.
[1] https://github.com/progit/progit/blob/master/de/NOTES
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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: English/German terminology, git.git's de.po, and pro-git
Hi, here's an updated version of the glossary. Comments are appreciated. Basic repository objects: blob = Blob tree = Baum, Baum-Objekt (bevorzugt), Tree-Objekt submodule = Submodul pack(noun) = Pack-Datei pack(verb) = packen (ggf. Pack-Datei erstellen) ancestor = Vorfahre, Vorgänger, Vorgänger-Commit (bevorzugt) Content in a repository: file(s)= Datei(en) tracked file = beobachtete Datei track file = beobachte Datei untracked file = unbeobachtete Datei directory = Verzeichnis Repositories / tracking concepts: clone (verb) = klonen clone (noun) = der Klon repository = Repository bare repository= Bare Repository working directory = Arbeitsverzeichnis working tree = -||- remote branch = Remote-Branch remote-tracking branch = Remote-Tracking-Branch upstream branch= Upstream-Branch remote repository = Remote-Repository remote(noun) = -||- remote(adj)= extern, entfernt liegend Authorship: author= Autor committer = Commit-Ersteller tagger= Tag-Ersteller Commits, tags and other references: HEAD = HEAD Konzept aus der Git-Welt, daher nicht zu übersetzen. detached HEAD = losgelöster HEAD commit(noun) = Commit commit(verb) = committen commit the result = das Ergebnis committen parent commit = Eltern-Commit child commit = Kind-Commit commit message= Commit-Beschreibung stash(noun) = der Stash stash(verb) = stashen, stash benutzen (bevorzugt) unstash(verb) = unstashen, zurückladen, aus 'stash' zurückladen (bevorzugt) reference = Referenz revision = Commit branch = Branch tag(noun) = Tag tag(verb) = taggen, Tag erstellen annotated tag = annotierter Tag tag message= Tag-Beschreibung orphan commit= orphan reference = boundary commit = Grenz-Commit root commit = Ursprungs-Commit, Wurzel-Commit stage/index (noun) = Staging-Area, Index stage/index (verb) = (für einen | zum) Commit vormerken (bevorzugt), zur Staging Area hinzufügen, dem Index hinzufügen unstage (verb) = aus Staging Area entfernen, aus Index entfernen The DAG: commit graph = Commit-Graph merge = Merge References in relation to other references: branches that have diverged = Branches sind divergiert diverging references= divergierte Referenzen your branch is ahead= Ihr Branch ist voraus your branch is behind = Ihr Branch ist hinterher Moving data around: fetch = anfordern pull = zusammenführen push = versenden fast-forward = vorspulen non-fast-forward = nicht vorspulen Commands: log= Log interactive commit = interaktiver Commit cherry-pick= cherry-pick benutzen rebase(verb) = rebase benutzen rebase(noun) = rebase archive= archivieren revert = zurücknehmen clean(verb)= säubern/aufräumen clean(noun)= Säuberung merge = zusammenführen bundle(noun) = Paket bundle(verb) = Paket erstellen unbundle(verb) = Paket entpacken bisect = binäre Suche bisecting = bei einer binären Suche sein, binäre Suche durchführen Diff/patch related: diff = Differenz delta = Differenz (or Delta) patch = Patch apply = anwenden diffstat = (leave it as it is) hunk = Bereich whitespace = Whitespace Still being worked out: prune = veraltete(n) Branch(es) entfernen checkout(verb) = auschecken git add = hinzufügen merge conflict = Merge-Konflikt 3-way merge= 3-Wege-Merge paths = Pfade symbolic link = symbolische Verknüfung path = Pfad link = Verknüpfung reflog = Referenzprotokoll partial commit (verb) = teilweise committen, partiell committen partial commit (noun) = Teil-Commit reset = neu setzen (maybe umsetzen?) register = in die Konfiguration eintragen unregister = aus der Konfiguration austragen -- 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: English/German terminology, git.git's de.po, and pro-git
2013/5/16 Holger Hellmuth (IKS) hellm...@ira.uka.de: [...] +reset = neu setzen (maybe umsetzen?) zurücksetzen reset can be used with every existing commit. zurücksetzen would imply that it have to be a recent commit, no? -- 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/2] doc: command line interface (cli) dot-repository dwimmery
Hi, Philip Oakley wrote: The Git cli will generally accept dot '.' (period) as equivalent to the current repository when appropriate. Tell the reader of this 'do what I mean' (dwim)mery action. [...] --- a/Documentation/gitcli.txt +++ b/Documentation/gitcli.txt @@ -59,6 +59,10 @@ working tree. After running `git add hello.c; rm hello.c`, you will _not_ see `hello.c` in your working tree with the former, but with the latter you will. +Just as, by convention, the filesystem '.' refers to the current directory, +using a '.' (period) as a repository name in Git (a dot-repository) refers +to your local repository. Good idea, but I fear that no one would find it there. Would it make sense to put this in Documentation/urls.txt (aka the GIT URLS section of git-fetch(1) and git-clone(1)), where other URL schemes are documented? Thanks, Jonathan -- 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 1/2] config doc: add dot-repository note
Philip Oakley wrote: --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -734,6 +734,8 @@ branch.name.remote:: overridden by `branch.name.pushremote`. If no remote is configured, or if you are not on any branch, it defaults to `origin` for fetching and `remote.pushdefault` for pushing. + Additionally, a `.` (period) means the current local repository + (a dot-repository), see `branch.name.merge`'s final note below. Does this accept an arbitrary git URL, or only remote names? The current documentation makes it sound like the latter, which makes this exception seem very weird. Is it actually the former? I think a cross-reference to the git urls or git remotes documentation would be a better way to go, instead of having to repeat the rules of URL syntax everywhere they are used. Thanks, Jonathan -- 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
John Keeping wrote:
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.
I like this idea a lot.
--- 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);
Can you say a little about how this function works (e.g., in a
comment)? What are the preconditions and postconditions? How does
the state evolve (e.g, when is searching set)?
+
+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;
Style: unusual whitespace (the tab after return). I'd just do
if (!a-sha1 || ...)
return ...
return !hashcmp(a-sha1, b-sha1) a-mode == b-mode;
since it is not too long.
[...]
+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);
+}
This function seems to be the same as one of the same name in
builtin/merge-tree.c. Should it be put somewhere more public, for
example as part of the tree-walk API? Who is responsible for freeing
path? Would it make sense to use a strbuf here?
strbuf_setlen(buf, traverse_path_len(info, n));
make_traverse_path(buf.buf, 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;
+}
Same question about internal API: when I see
add_touched_path(info, path)
what should I expect it to do?
In the info-searching case, string_list_insert(info-paths, path)
will always be a no-op, right? What does it mean that this is adding
a touched path in that case?
[...]
+static int collect_touched_paths_cb(int n, unsigned long mask,
+ unsigned long dirmask, struct name_entry *entry,
+ struct traverse_info *info)
+{
Same question about contracts. Ideally a reader in a rush should be
able to read an opening comment about what the function does and
then return to the caller without delving into the details of how
it does its work.
+ 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)) ||
+ (entry[1].sha1 S_ISDIR(entry[1].mode))) {
At this point I'm not sure what two trees are being traversed in
parallel, so it's not obvious to me what the code's about. Are
they the before and after trees for commits being rebased?
[...]
+static int collect_touched_paths(struct commit *commit, struct patch_ids
*ids,
+ int searching)
+{
+ struct tree_desc trees[2];
+ struct collect_paths_info info = { ids-touched_paths, searching };
+ void *commitbuf;
+ int result;
+
+ commitbuf = fill_tree_descriptor(trees + 1, commit-object.sha1);
+ if (commit-parents) {
+ void *parentbuf = fill_tree_descriptor(trees + 0,
+ commit-parents-item-object.sha1);
Looks like yes.
What should happen for commits with more than 1 parent? If they're
supposed to not enter this codepath because of a previous check, a
die(BUG: ...) could be a good way to make reading easier.
[...]
@@ -40,6 +172,7 @@ int init_patch_ids(struct patch_ids *ids)
diff_setup(ids-diffopts);
DIFF_OPT_SET(ids-diffopts, RECURSIVE);
diff_setup_done(ids-diffopts);
+ ids-touched_paths.strdup_strings = 1;
Good.
[...]
@@ -64,6 +199,13 @@ static struct patch_id *add_commit(struct commit *commit,
unsigned char sha1[20];
int pos;
+ if (no_add) {
+ if (collect_touched_paths(commit, ids, 1) 0)
+ return NULL;
Ah, so this is what the searching does.
The existing no_add argument is very confusing (what does it mean to
add a commit without adding?), but at least the confusion is
self-contained in a small, simple set of functions:
static struct patch_id *add_commit(struct commit
[PATCH 00/17] Remove assumptions about refname lifetimes
Prior to this patch series, the refs API said nothing about the lifetime of the refname parameter passed to each_ref_fn callbacks by the for_each_ref()-style iteration functions. De facto, the refnames usually had long lives because they were pointers into the ref_cache data structures, and those are only invalidated under rare circumstances. And some callers were assuming a long lifetime, for example storing references to the refname string instead of copying it. But it has long been the case that ref caches could be invalidated, for example when a packed ref is deleted. AFAIK there was never much clarity about what that might mean for callers. Recently a number of race conditions related to references have been discovered. There is likely to be a two-pronged solution to the races: * For traditional, filesystem-based references, there will have to be more checks that the ref caches are still up-to-date at the time of their use (see, for example, [1]). If not, the ref cache will have to be invalidated and reloaded. Assuming that the invalidation of the old cache includes freeing its memory, such an invalidation will cause lots of refname strings to be freed even though callers might still hold references to them. * For server-class installations, filesystem-based references might not be robust enough for 100% reliable operation, because the reading of the complete set of references is not an atomic operation. If another reference storage mechanism is developed, there is no reason to expect the refnames strings to have long lifetimes. A prerequisite for either of these approaches is to harmonize what callers assume and what the API guarantees. The purpose of this patch series is to track down callers who assume that the refnames that they receive via a each_ref_fn callback have lifetimes beyond the duration of the callback invocation and to rewrite them to work without that assumption. The final patch documents explicitly that callers should not retain references to the refnames. A word about how I audited the code: To find callers making unwarranted assumptions, I (temporarily) changed do_one_ref() to do a xstrdup() of the refname, pass the copy to the callback function, then free() the copy. This caused ill-behaved callers to access freed memory, which could be detected by running the testsuite under valgrind. There were indeed a number of such errors. All of them are fixed by this patch series, and the test just described now runs without errors. I plan to do a second audit by hand to see if the test suite and/or valgrind missed anything. The last two patches are RFCs. I would like some input on the second to last because I am not very familiar with how the object array entry names are used, how many might be created, etc. The last patch is an illustration of how I would like to change the API docs, but it will only be ready when all of the code has been audited and adapted. Please see especially my comments on these two patches for more information. [1] http://thread.gmane.org/gmane.comp.version-control.git/223299 Michael Haggerty (17): describe: make own copy of refname fetch: make own copies of refnames add_rev_cmdline(): make a copy of the name argument builtin_diff_tree(): make it obvious that function wants two entries cmd_diff(): use an object_array for holding trees cmd_diff(): rename local variable list - entry cmd_diff(): make it obvious which cases are exclusive of each other revision: split some overly-long lines gc_boundary(): move the check alloc = nr to caller get_revision_internal(): make check less mysterious object_array: add function object_array_filter() object_array_remove_duplicates(): rewrite to reduce copying fsck: don't put a void*-shaped peg in a char*-shaped hole find_first_merges(): initialize merges variable using initializer find_first_merges(): remove unnecessary code object_array_entry: copy name before storing in name field refs: document the lifetime of the refname passed to each_ref_fn builtin/describe.c | 6 +++-- builtin/diff.c | 68 ++ builtin/fetch.c| 4 ++-- builtin/fsck.c | 2 +- object.c | 50 +++ object.h | 23 -- refs.h | 22 +- revision.c | 61 +--- revision.h | 32 - submodule.c| 6 ++--- 10 files changed, 172 insertions(+), 102 deletions(-) -- 1.8.2.3 -- 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 02/17] fetch: make own copies of refnames
Do not retain references to refnames passed to the each_ref_fn
callback add_existing(), because their lifetime is not guaranteed.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/fetch.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4b6b1df..f949115 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -590,7 +590,7 @@ static void find_non_local_tags(struct transport *transport,
struct ref **head,
struct ref ***tail)
{
- struct string_list existing_refs = STRING_LIST_INIT_NODUP;
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list remote_refs = STRING_LIST_INIT_NODUP;
const struct ref *ref;
struct string_list_item *item = NULL;
@@ -693,7 +693,7 @@ static int truncate_fetch_head(void)
static int do_fetch(struct transport *transport,
struct refspec *refs, int ref_count)
{
- struct string_list existing_refs = STRING_LIST_INIT_NODUP;
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct string_list_item *peer_item = NULL;
struct ref *ref_map;
struct ref *rm;
--
1.8.2.3
--
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 03/17] add_rev_cmdline(): make a copy of the name argument
Instead of assuming that the memory pointed to by the name argument will live forever, make a local copy of it before storing it in the ref_cmdline_info. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- revision.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index a67b615..25e424c 100644 --- a/revision.c +++ b/revision.c @@ -898,6 +898,10 @@ static int limit_list(struct rev_info *revs) return 0; } +/* + * Add an entry to refs-cmdline with the specified information. + * *name is copied. + */ static void add_rev_cmdline(struct rev_info *revs, struct object *item, const char *name, @@ -909,7 +913,7 @@ static void add_rev_cmdline(struct rev_info *revs, ALLOC_GROW(info-rev, nr + 1, info-alloc); info-rev[nr].item = item; - info-rev[nr].name = name; + info-rev[nr].name = xstrdup(name); info-rev[nr].whence = whence; info-rev[nr].flags = flags; info-nr++; -- 1.8.2.3 -- 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 08/17] revision: split some overly-long lines
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
revision.c | 20 ++--
revision.h | 32 +---
2 files changed, 35 insertions(+), 17 deletions(-)
diff --git a/revision.c b/revision.c
index 25e424c..8ac88d6 100644
--- a/revision.c
+++ b/revision.c
@@ -70,7 +70,8 @@ static int show_path_truncated(FILE *out, const struct
name_path *path)
return ours || emitted;
}
-void show_object_with_name(FILE *out, struct object *obj, const struct
name_path *path, const char *component)
+void show_object_with_name(FILE *out, struct object *obj,
+ const struct name_path *path, const char *component)
{
struct name_path leaf;
leaf.up = (struct name_path *)path;
@@ -186,7 +187,9 @@ void mark_parents_uninteresting(struct commit *commit)
}
}
-static void add_pending_object_with_mode(struct rev_info *revs, struct object
*obj, const char *name, unsigned mode)
+static void add_pending_object_with_mode(struct rev_info *revs,
+struct object *obj,
+const char *name, unsigned mode)
{
if (!obj)
return;
@@ -209,7 +212,8 @@ static void add_pending_object_with_mode(struct rev_info
*revs, struct object *o
add_object_array_with_mode(obj, name, revs-pending, mode);
}
-void add_pending_object(struct rev_info *revs, struct object *obj, const char
*name)
+void add_pending_object(struct rev_info *revs,
+ struct object *obj, const char *name)
{
add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
}
@@ -226,7 +230,9 @@ void add_head_to_pending(struct rev_info *revs)
add_pending_object(revs, obj, HEAD);
}
-static struct object *get_reference(struct rev_info *revs, const char *name,
const unsigned char *sha1, unsigned int flags)
+static struct object *get_reference(struct rev_info *revs, const char *name,
+ const unsigned char *sha1,
+ unsigned int flags)
{
struct object *object;
@@ -247,7 +253,8 @@ void add_pending_sha1(struct rev_info *revs, const char
*name,
add_pending_object(revs, object, name);
}
-static struct commit *handle_commit(struct rev_info *revs, struct object
*object, const char *name)
+static struct commit *handle_commit(struct rev_info *revs,
+ struct object *object, const char *name)
{
unsigned long flags = object-flags;
@@ -368,7 +375,8 @@ static void file_change(struct diff_options *options,
DIFF_OPT_SET(options, HAS_CHANGES);
}
-static int rev_compare_tree(struct rev_info *revs, struct commit *parent,
struct commit *commit)
+static int rev_compare_tree(struct rev_info *revs,
+ struct commit *parent, struct commit *commit)
{
struct tree *t1 = parent-tree;
struct tree *t2 = commit-tree;
diff --git a/revision.h b/revision.h
index 01bd2b7..9628465 100644
--- a/revision.h
+++ b/revision.h
@@ -195,19 +195,23 @@ struct setup_revision_opt {
};
extern void init_revisions(struct rev_info *revs, const char *prefix);
-extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
struct setup_revision_opt *);
+extern int setup_revisions(int argc, const char **argv, struct rev_info *revs,
+ struct setup_revision_opt *);
extern void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t
*ctx,
-const struct option *options,
-const char * const usagestr[]);
+ const struct option *options,
+ const char * const usagestr[]);
#define REVARG_CANNOT_BE_FILENAME 01
#define REVARG_COMMITTISH 02
-extern int handle_revision_arg(const char *arg, struct rev_info *revs, int
flags, unsigned revarg_opt);
+extern int handle_revision_arg(const char *arg, struct rev_info *revs,
+ int flags, unsigned revarg_opt);
extern void reset_revision_walk(void);
extern int prepare_revision_walk(struct rev_info *revs);
extern struct commit *get_revision(struct rev_info *revs);
-extern char *get_revision_mark(const struct rev_info *revs, const struct
commit *commit);
-extern void put_revision_mark(const struct rev_info *revs, const struct commit
*commit);
+extern char *get_revision_mark(const struct rev_info *revs,
+ const struct commit *commit);
+extern void put_revision_mark(const struct rev_info *revs,
+ const struct commit *commit);
extern void mark_parents_uninteresting(struct commit *commit);
extern void mark_tree_uninteresting(struct tree *tree);
@@ -220,15 +224,19 @@ struct name_path {
char *path_name(const struct name_path *path, const char *name);
-extern void show_object_with_name(FILE *, struct object *, const
[PATCH 13/17] fsck: don't put a void*-shaped peg in a char*-shaped hole
The source of this nonsense was 04d3975937 fsck: reduce stack footprint , which wedged a pointer to parent into the object_array_entry's name field. The parent pointer was passed to traverse_one_object(), even though that function *didn't use it*. The useless code has been deleted over time. Commit a1cdc25172 fsck: drop unused parameter from traverse_one_object() removed the parent pointer from traverse_one_object()'s signature. Commit c0aa335c95 Remove unused variables removed the code that read the parent pointer back out of the name field. This commit takes the last step: don't write the parent pointer into the name field in the first place. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- I thought that this misuse of the name field was going to be a showstopper for changing how the name's memory is managed, but then I noticed that the value stored here is never used. builtin/fsck.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index bb9a2cd..9909b6d 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -112,7 +112,7 @@ static int mark_object(struct object *obj, int type, void *data) return 1; } - add_object_array(obj, (void *) parent, pending); + add_object_array(obj, NULL, pending); return 0; } -- 1.8.2.3 -- 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 15/17] find_first_merges(): remove unnecessary code
No names are ever set for the object_array_entries in merges, so there is no need to pretend to copy them to the result array. Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- submodule.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/submodule.c b/submodule.c index b837c04..ad476ce 100644 --- a/submodule.c +++ b/submodule.c @@ -893,8 +893,7 @@ static int find_first_merges(struct object_array *result, const char *path, } if (!contains_another) - add_object_array(merges.objects[i].item, -merges.objects[i].name, result); + add_object_array(merges.objects[i].item, NULL, result); } free(merges.objects); -- 1.8.2.3 -- 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 16/17] object_array_entry: copy name before storing in name field
Change object_array and object_array_entry to copy the name before
storing it in the name field, and free it when an entry is deleted
from the array. This is useful because some of the name strings
passed to add_object_array() or add_object_array_with_mode() are
refnames whose lifetime is not defined by the refs API (and which we
want to shorten).
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
This is the culmination of the last few commits. Since some callers
want to store refnames in the name field of object_array elements, but
we don't want those callers to assume that the refnames that they got
from for_each_ref() have infinite lifetime, the easiest thing to do is
have object_array make a copy of the names before writing them in the
entries, and to free the names for entries that are no longer in use.
This change fixes the problem, but has some disadvantages:
* It requires extra copies to be made of strings that are already
copies, for example when the results of path_name(path, name) are
used as a name in revision.c:add_object(). This might be rare
enough that it can be ignored (though the original result of
path_name() would have to be freed, which this patch doesn't do so
there is a memory leak).
* Many callers store the empty string () as the name; for example,
most of the entries created during a run of rev-list have as
their name. This means that lots of needless copies of are being
made. I think that the best solution to this problem would be to
store NULL rather than for such entries, but I haven't figured
out all of the places where the name is used.
The alternative would be to have callers make the copies if necessary
*before* passing the names into add_object_array(), and themselves
ensure that those copies get freed sometime. This would be more work:
effectively each object_array would have to have its own memory
ownership policy and we would have to figure out exactly where in the
code entries are added and removed from particular lists.
Since I'm not too clear on what these names are used for, how many
object_array entries are created in different scenarios, etc., I
decided to submit this patch as an RFC to get some feedback before I
work on a final solution.
object.c | 6 +-
object.h | 6 +-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/object.c b/object.c
index 10b5349..a678c1b 100644
--- a/object.c
+++ b/object.c
@@ -273,7 +273,7 @@ void add_object_array_with_mode(struct object *obj, const
char *name, struct obj
array-objects = objects;
}
objects[nr].item = obj;
- objects[nr].name = name;
+ objects[nr].name = name ? xstrdup(name) : NULL;
objects[nr].mode = mode;
array-nr = ++nr;
}
@@ -289,6 +289,8 @@ void object_array_filter(struct object_array *array,
if (src != dst)
objects[dst] = objects[src];
dst++;
+ } else {
+ free(objects[src].name);
}
}
array-nr = dst;
@@ -319,6 +321,8 @@ void object_array_remove_duplicates(struct object_array
*array)
if (src != array-nr)
objects[array-nr] = objects[src];
array-nr++;
+ } else {
+ free(objects[src].name);
}
}
}
diff --git a/object.h b/object.h
index 6c1c27f..f2c503a 100644
--- a/object.h
+++ b/object.h
@@ -11,7 +11,11 @@ struct object_array {
unsigned int alloc;
struct object_array_entry {
struct object *item;
- const char *name;
+ /*
+* name or NULL. If non-NULL, the memory pointed to
+* is owned by this object.
+*/
+ char *name;
unsigned mode;
} *objects;
};
--
1.8.2.3
--
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 06/17] cmd_diff(): rename local variable list - entry
It's not a list, it's an array entry.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/diff.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/diff.c b/builtin/diff.c
index 72d99c0..7cac6de 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -338,9 +338,9 @@ int cmd_diff(int argc, const char **argv, const char
*prefix)
}
for (i = 0; i rev.pending.nr; i++) {
- struct object_array_entry *list = rev.pending.objects+i;
- struct object *obj = list-item;
- const char *name = list-name;
+ struct object_array_entry *entry = rev.pending.objects[i];
+ struct object *obj = entry-item;
+ const char *name = entry-name;
int flags = (obj-flags UNINTERESTING);
if (!obj-parsed)
obj = parse_object(obj-sha1);
@@ -359,7 +359,7 @@ int cmd_diff(int argc, const char **argv, const char
*prefix)
die(_(more than two blobs given: '%s'), name);
hashcpy(blob[blobs].sha1, obj-sha1);
blob[blobs].name = name;
- blob[blobs].mode = list-mode;
+ blob[blobs].mode = entry-mode;
blobs++;
continue;
--
1.8.2.3
--
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 14/17] find_first_merges(): initialize merges variable using initializer
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
submodule.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index e728025..b837c04 100644
--- a/submodule.c
+++ b/submodule.c
@@ -846,7 +846,7 @@ static int find_first_merges(struct object_array *result,
const char *path,
struct commit *a, struct commit *b)
{
int i, j;
- struct object_array merges;
+ struct object_array merges = OBJECT_ARRAY_INIT;
struct commit *commit;
int contains_another;
@@ -856,7 +856,6 @@ static int find_first_merges(struct object_array *result,
const char *path,
struct rev_info revs;
struct setup_revision_opt rev_opts;
- memset(merges, 0, sizeof(merges));
memset(result, 0, sizeof(struct object_array));
memset(rev_opts, 0, sizeof(rev_opts));
--
1.8.2.3
--
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 10/17] get_revision_internal(): make check less mysterious
The condition under which gc_boundary() is called was previously
if (alloc = nr)
. But by construction, nr can never exceed alloc, so the check looks
unnecessarily mysterious. In fact, the purpose of the check is to try
to avoid a realloc() call by shrinking the array if possible if it is
at its allocation limit when a new element is about to be added. So
change the check to
if (nr == alloc)
and add a comment to explain what's going on.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
Please check that I have properly described the purpose of this check.
The way the code is written, it looks like a bad pattern of growth and
shrinkage of the array (namely, just under the resize limit) could
cause gc_boundary() to be called over and over again with (most of)
the same data. I hope that the author had some reason to believe that
such a pattern is unlikely.
revision.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 2e0992b..19c59f4 100644
--- a/revision.c
+++ b/revision.c
@@ -2573,8 +2573,10 @@ static struct commit *get_revision_internal(struct
rev_info *revs)
if (p-flags (CHILD_SHOWN | SHOWN))
continue;
p-flags |= CHILD_SHOWN;
- if (revs-boundary_commits.alloc = revs-boundary_commits.nr)
+ if (revs-boundary_commits.nr == revs-boundary_commits.alloc) {
+ /* Try to make space and thereby avoid a realloc(): */
gc_boundary(revs-boundary_commits);
+ }
add_object_array(p, NULL, revs-boundary_commits);
}
--
1.8.2.3
--
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 v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
On Sat, May 18, 2013 at 11:26 PM, Junio C Hamano gits...@pobox.com wrote: David Aguilar dav...@gmail.com writes: Thanks Eric and Junio. I looked over the patches and they look good. Are you sure about that? It seemed to me that it was breaking everybody that is not on MacOS X --- did I misread the patch? Gah, correct. I've now tested v8 which Eric just sent out. It worked fine for me, with and without NO_APPLE_COMMON_CRYPTO. Thanks, -- David -- 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/2] doc: command line interface (cli) dot-repository dwimmery
From: Jonathan Nieder jrnie...@gmail.com Sent: Sunday, May 19, 2013 6:39 PM Hi, Philip Oakley wrote: The Git cli will generally accept dot '.' (period) as equivalent to the current repository when appropriate. Tell the reader of this 'do what I mean' (dwim)mery action. [...] --- a/Documentation/gitcli.txt +++ b/Documentation/gitcli.txt @@ -59,6 +59,10 @@ working tree. After running `git add hello.c; rm hello.c`, you will _not_ see `hello.c` in your working tree with the former, but with the latter you will. +Just as, by convention, the filesystem '.' refers to the current directory, +using a '.' (period) as a repository name in Git (a dot-repository) refers +to your local repository. Good idea, but I fear that no one would find it there. Would it make sense to put this in Documentation/urls.txt (aka the GIT URLS section of git-fetch(1) and git-clone(1)), where other URL schemes are documented? Thanks, Jonathan Sounds an interesting idea. I'll have a look. Philip -- 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 1/2] Doc rebase: Describe rebase as excluding merge commits
Jonathan Nieder jrnie...@gmail.com writes: Philip Oakley wrote: Describe rebase in the description section. It already does that. :) I think you mean start with a summary, which is a valuable improvement. It indeed is a good idea to give the high-level introduction at the very beginning, but I do not think it should describe only one of the three major modes of git rebase (i.e. no -m, no -i), like the proposed patch text does. We should instead say what it is used for and why the user would want to use it that is common across these modes at a very high level. DESCRIPTION --- brief description of the purpose of the command, including some token mention of *why* a user would want to use it (e.g., so that the patches apply cleanly to their new base). Exactly. -- 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
Jonathan Nieder jrnie...@gmail.com writes:
@@ -64,6 +199,13 @@ static struct patch_id *add_commit(struct commit *commit,
unsigned char sha1[20];
int pos;
+if (no_add) {
+if (collect_touched_paths(commit, ids, 1) 0)
+return NULL;
Ah, so this is what the searching does.
The existing no_add argument is very confusing (what does it mean to
add a commit without adding?),
Such a mode of operation is usually called dry-run, no?
--
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-diff-index man page
Albert Netymk albertnet...@gmail.com writes: On Sun, May 19, 2013 at 9:46 AM, Junio C Hamano gits...@pobox.com wrote: Albert Netymk albertnet...@gmail.com writes: Hello, The man page of git-diff-index: http://git-scm.com/docs/git-diff-index states that `git-diff-index - Compares content and mode of blobs between the index and repository`. However, in fact this command compares between files on disk and repository by default. It's explained clearly in here: http://git-scm.com/docs/gitdiffcore Maybe the man page could be updated to reflect this in NAME section: `git-diff-index - Compares content and mode of blobs between files on disk and repository or between the index and repository` Surely; we need to keep it fit on a single line, though. Here's one excerpt from man page of git-diff-index: (this is how one line looks like) Compares the content and mode of the blobs found via a tree object with the content of the current index and, optionally ignoring the stat state of `git-diff-index - Compares content and mode of blobs between files on disk and repository or between the index and repository` It seems that this one is not longer than one line. I would say git-diff-index - Compare a tree and the working tree or the index should be sufficiently short and much more accurate than the current text. I don't know how to submit patches. Besides, the only part that is a bit misleading is the NAME and DESCRIPTION section. Could someone just update them? How about something like this? -- 8 -- Subject: [PATCH] Documentation/diff-index: mention two modes of operation diff-index can be used to compare a tree with the tracked working tree files (when used without the --index option), or with the index (when used with the --index option). The text however did not say anything about the comparision with the working tree at all. Fix this. Reported-by: Albert Netymk albertnet...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/git-diff-index.txt | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt index c0b7c58..58308e1 100644 --- a/Documentation/git-diff-index.txt +++ b/Documentation/git-diff-index.txt @@ -3,7 +3,7 @@ git-diff-index(1) NAME -git-diff-index - Compares content and mode of blobs between the index and repository +git-diff-index - Compare a tree and the working tree or the index SYNOPSIS @@ -13,11 +13,12 @@ SYNOPSIS DESCRIPTION --- -Compares the content and mode of the blobs found via a tree -object with the content of the current index and, optionally -ignoring the stat state of the file on disk. When paths are -specified, compares only those named paths. Otherwise all -entries in the index are compared. + +Compare the content and mode of the blobs found in a tree object +with the corresponding tracked file in the working tree, or with the +corresponding paths in the index. When paths are specified, +compares only those named paths. Otherwise all entries in the index +are compared. OPTIONS --- -- 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-submodule nested subrepo bug (Segmentation fault)
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 Its happens because git dont create `modules` directory. al_server/.git/modules Workaround for it wont work to: git submodule update --init It init only first level of subrepos. When you do: cd chat git submodule update --init It crashes cause missed `modules` directory for submodule: al_server/.git/modules/chat/modules Final workaround is create this directory and try last command 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: git-diff-index man page
On Mon, May 20, 2013 at 1:01 AM, Junio C Hamano gits...@pobox.com wrote: Subject: [PATCH] Documentation/diff-index: mention two modes of operation diff-index can be used to compare a tree with the tracked working tree files (when used without the --index option), or with the index (when used with the --index option). The text however did not say anything about the comparision with the s/comparision/comparison/ working tree at all. Fix this. Reported-by: Albert Netymk albertnet...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com -- 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] fetch: add --allow-local option
Philip Oakley philipoak...@iee.org writes: From: Junio C Hamano gits...@pobox.com Sent: Friday, May 17, 2013 7:30 PM Subject: Re: [PATCH 1/3] fetch: add --allow-local option [...] So when the user is running git fetch on mywork branch that happens to be forked from a local master, i.e. her configuration is set as [branch mywork] remote = . merge = refs/heads/master Was the '.' example illustrative rather than exact. It is exactly spelled like so. Just like '.' in the filesystem refers to our current directory, a dot-repository used there refers to our local repository. Git is distributed and no repository is special principle extends to our own local repository in that you can use it just like you can use anybody else's repository as the source of fetch or the destination of push. -- 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] New kind of upstream branch: base branch
Kevin Bracey ke...@bracey.fi writes: I found myself thinking the same thing. It's really convenient being able to set your topic branch's upstream to another local branch, so What is that another local branch? ... And if that is your workflow, setting push.default to current (and setting remote.pushdefault to your publishing repository) should be a sufficient interim solution, and you do not need to set branch.$name.push to each and every branch you intend to push out, I think. I agree that using push.default current covers some cases - I hadn't really considered it - tended to just stick with upstream. current nearly does the job, but I will sometimes be wanting different names. What I'll often be doing is creating a topic branch based on master or origin/master. (I would hardly ever be updating master or pushing to origin/master myself, so I probably should be just doing origin/master, but I tend to create a local master just to save typing on all those git rebase origin/master). Do you mean, by save typing, instead of origin/master, I can type master? If you are using the checkout -t -b topic origin/master, git rebase without any other argument should know that you meant to rebase against 'origin/master', so you do not even have to type 'master'. During work, to give others visibility, and the possibility to tinker with the topic branch during development (as we don't have full inter-site sharing of work areas), I would push the topic branch up to the central origin server, often with a kbracey/ prefix, partially for namespacing, and partially to indicate it's currently private work and subject to rebasing. I guess I could create the topic branch as kbracey/topic locally, but I'd rather not have to. Yup, that is sensible, and I think with Felipe's proposed change, you can spell it like this: [branch topic] remote = origin merge = refs/heads/master push = refs/heads/kbracey/topic Note that the above assumes you did checkout -t -b topic origin/master as a typesaver for git rebase. So I'd like git rebase (-i) to move my topic branch up (origin/)master. And I'd like git push (-f) to send it to origin/kbracey/topic. Understood. And I think the [branch topic] configuration above would cover that use case. And by extension, I suppose git pull --rebase to update origin/master and rebase. (Although I'm not much of a puller - I tend to fetch then rebase manually). git pull --rebase would be git fetch followed by git rebase, and again the [branch topic] configuration above would cover that use case, I think. And it would be ideal if the initial base and push tracking information could be set up automatically on the first git checkout -b/git branch and git push. I think checkout and branch is already covered with -t. There may even be a configuration option to implicitly add -t to them (I didn't check). (For one, I've always found it odd that there's an asymmetry - if you check out a topic branch from the server to work on or use it, you get a local copy with upstream set by default. But if you create a topic branch yourself then push it, the upstream isn't set by default - you need the -u flag. This seems odd to me, and I've seen others confused by this). Yeah, I would imagine that it would be trivial to add an option to cause git push to do that, and it would be useful if you push to and pull from the same place (I haven't thought about ramifications such an option would have on the triangular workflows, though). Thanks. -- 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/2] remote-helpers: tests: use python directly
David Aguilar dav...@gmail.com writes: On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano gits...@pobox.com wrote: So it is a right thing to do in that sense. I however am having this nagging feeling that I may be missing something subtle here. Comments from others are very much welcome. Yes, this is correct. Another way to skin this cat would be to do search/replace in a Makefile to burn in the PYTHON_PATH similar to how we do for the .sh scripts and other .py files in the main Makefile. The remote helpers are in contrib/ so they do not go through the main Makefile, which is the root cause. Longer-term, it would be good to treat these uniformly, but this is no worse for now. Ahh, so my nagging feeling was that remote-helpers could in theory be updated to follow the PYTHON_PATH like the rest of the system and matching these two in that direction is the better longer-term fix? Ok, with that in mind, I still think the patch under discussion is an immediate solution that is fine as-is. I somehow thought that there were valid reasons that we could not do so for some technical reason (e.g. the instalation of python used to run hg and/or bzr via these remote helpers and the installation of python we use may need to be different?). As long as the right longer-term direction is not we cannot fundamentally unify them, then I am very happy. I was worried that we might end up having to define random fine gained knobs like PYTHON_FOR_HG_PATH to allow tuning these. Thanks for a clarification. -- 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 v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
David Aguilar dav...@gmail.com writes: Thanks Eric and Junio. I looked over the patches and they look good. Are you sure about that? It seemed to me that it was breaking everybody that is not on MacOS X --- did I misread the 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 2/2] diffcore-pickaxe doc: document -S and -G properly
Junio C Hamano gits...@pobox.com writes: Ramkumar Ramachandra artag...@gmail.com writes: ... -Gregex:: -Look for differences whose added or removed line matches -the given regex. +Grep through the patch text of commits for added/removed lines +that match regex. `--pickaxe-regex` is implied in this +mode. The same comment on differences vs commits apply to this. ... it will _not_ apply to users of -G. s/.$/ unless they say --pickaxe-regex./; so -G does not imply it at all. grep through, if the reader knows grep, with match regex, it is crystal clear that this expects a regular expression. And that is the only thing that makes -G and --pickaxe-regex superficially related. s/^The description begins with /; Sorry, but I couldn't write complete sentences on a bus ;-) -This transformation is used to find filepairs that represent -changes that touch a specified string, and is controlled by the --S option and the `--pickaxe-all` option to the 'git diff-*' -commands. - -When diffcore-pickaxe is in use, it checks if there are -filepairs whose result side and whose origin side have -different number of specified string. Such a filepair represents -the string appeared in this changeset. It also checks for the -opposite case that loses the specified string. - -When `--pickaxe-all` is not in effect, diffcore-pickaxe leaves -only such filepairs that touch the specified string in its -output. When `--pickaxe-all` is used, diffcore-pickaxe leaves all -filepairs intact if there is such a filepair, or makes the -output empty otherwise. The latter behaviour is designed to -make reviewing of the changes in the context of the whole -changeset easier. This part is impossible to review on a bus, so I won't comment in this message. Why did you even have to touch the paragraph for --pickaxe-all? That applies to both -S and -G. I thought it would be just the matter of slightly tweaking the introductory paragraph (which was written back when there was only -S), keeping the second paragraph for -S as-is, and insert an additional paragraph for -G before --pickaxe-all. Now I see that the paragraph for --pickaxe-all needs to be touched; the original talks about touch the specified string, which only applies to -S and needs to be adjusted. So here is my attempt of clarifying it. This transformation is used to find filepairs that represent two kinds of changes, and is controlled by the -S, -G and --pickaxe-all options. The -Sblock of text option tells Git to consider that a filepair has differences only if the number of occurrences of the specified block of text is different between its preimage and its postimage, and treat other filepairs as if they did not have any change. This is meant to be used with a block of text that is unique enough to occur only once (so expected the number of occurences is 1 vs 0 or 0 vs 1) to use with git log to find a commit that touched the block of text the last time. When used with the --pickaxe-regex option, the block of text is used as a POSIX extended regular expression to match, instead of a literal string. The -Gregular expression option tells Git to consider that a filepair has differences only if a textual diff between its preimage and postimage would indicate a line that matches the given regular expression is changed, and treat other filepairs as if they did not have any change. When -S or -G option is used without --pickaxe-all option, only filepairs that match their respective criterion are kept in the output. When `--pickaxe-all` is used, all filepairs intact if there is such a filepair, or makes the output empty otherwise. This behaviour is designed to make reviewing of the changes in the context of the whole changeset easier. -- 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 v5 01/15] Add new git-related helper to contrib
Felipe Contreras felipe.contre...@gmail.com writes: On Sat, May 18, 2013 at 6:46 AM, Felipe Contreras felipe.contre...@gmail.com wrote: contrib/related/git-related | 124 1 file changed, 124 insertions(+) create mode 100755 contrib/related/git-related I tried everything and I don't think it's physically possible to make this script any simpler without severely crippling it's main goal. Hmm, I haven't read these patches yet (I just came back a few hours ago to a state in which I am well enough to read and write e-mails), but did anybody complain that it is too complex? -- 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-diff-index man page
Albert Netymk albertnet...@gmail.com writes: Hello, The man page of git-diff-index: http://git-scm.com/docs/git-diff-index states that `git-diff-index - Compares content and mode of blobs between the index and repository`. However, in fact this command compares between files on disk and repository by default. It's explained clearly in here: http://git-scm.com/docs/gitdiffcore Maybe the man page could be updated to reflect this in NAME section: `git-diff-index - Compares content and mode of blobs between files on disk and repository or between the index and repository` Surely; we need to keep it fit on a single line, though. In DESCRIPTION section: `Compares the content and mode of the blobs found via a tree object with the content of the files on disk by default, and could be compared with index only using cached option. ...` Yes, mentioning both mode is a good change. The above exact text may give a false impression (with that by default) that it is somehow more preferrable to compare tree with working tree through the index, and only weirdos would use --cached to compare tree and the index, though. Patches welcome. Thanks. -- 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
Peter Lauri peterla...@gmail.com writes: Great, I have gotten the concept now :) My workaround for my problem is to rename the file to default and then all will work out well :) Copy the file then and locally modify it, but it will be in .gitignore so not tracked :) I think it is not even an workaround but is a solid software configuration practice that is recommended. -- 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-remote-hg: set stdout to binary mode on win32
Felipe Contreras felipe.contre...@gmail.com writes:
Sorry Amit, I assumed this patch made it to the list, but I just
realized it didn't; it doesn't allow HTML, and mails and silently
dropped (I hate that).
So I'm sending it so the list can see it:
It seems OK for me, but I would like to try it, and so far I haven't
managed to access Mercurial libraries at all from python scripts in
Windows. What steps did you follow?
Thanks for keeping an eye on this part of the system. It seems that
having an extra -rc cycle turned out to be not so bad an idea.
On Mon, Jan 28, 2013 at 4:13 PM, Amit Bakshi ambak...@gmail.com wrote:
git clone hangs on windows (msysgit/cygwin), and
file.write would return errno 22 inside of mercurial's
windows.winstdout wrapper class. This patch sets
stdout's mode to binary, fixing both issues.
---
contrib/remote-helpers/git-remote-hg | 21 +
1 file changed, 21 insertions(+)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index 328c2dc..95f4c1f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -62,6 +62,24 @@ def get_config(config):
output, _ = process.communicate()
return output
+#
+# On Windows (msysgit/cygwin) have to set stdout to binary
+# mode (_O_BINARY is 32768). Otherwise clone hangs, and pushing
+# to remote fails when doing a write to mercurial's wrapper
+# windows.winstdout wrapper class.
+#
+def set_binmode(fd):
+try:
+if sys.platform == win32:
+import msvcrt
+msvcrt.setmode(fd, os.O_BINARY)
+elif sys.platform == 'cygwin':
+import ctypes
+msvcrt = ctypes.CDLL('msvcrt.dll')
+msvcrt._setmode(fd, 32768) # On Cygwin os.O_BINARY is different
+except OSError:
+pass
+
class Marks:
def __init__(self, path):
@@ -764,6 +782,9 @@ def main(args):
else:
is_tmp = False
+if sys.platform in ['win32','cygwin']:
+set_binmode(sys.stdout.fileno())
+
gitdir = os.environ['GIT_DIR']
dirname = os.path.join(gitdir, 'hg', alias)
branches = {}
--
1.8.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: [PATCH 1/3] fetch: add --allow-local option
Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Fri, May 17, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: This is irrelevant, it's an implementation detail of 'git pull'. *THE USER* is not running 'git fetch .' To those who fear running git pull, the following has worked as a quick way to preview what they would be getting. git fetch git log ..FETCH_HEAD and then they can git merge FETCH_HEAD to conclude it, or run a git pull for real. We teach the more explicit form to end users in our tutorial, That tutorial is mostly irrelevant; it has not been properly updated in years, and it doesn't do it's job properly. Nowadays most people use the Pro Git book, which doesn't mention FETCH_HEAD even once. And why would it? Because the book was written by those who did not know about its use, and I do not necessarily think it is their fault. That's an _assumption_, and I don't think it's correct; the authors might very well know about FETCH_HEAD, but made a conscious decision to avoid explaining such low-level concepts to people trying to learn Git; they are irrelevant to them. I'm sure they skipped pack formats, remote-helper interfaces, and a lot of other implementation details users don't need to know about. FETCH_HEAD is simply another implementation detail users don't need to know, so it doesn't belong in Pro Git, a book designed for people trying to learn Git. Our own documentation can use updates. Yeah, sure. I've tried to fix the tutorial, it's not an experience I would wish to anybody, especially not people unfamiliar with Git, who are the people better suited to understand what is confusing about Git. IOW, the documentation is destined to oblivion, because the only people that could point out the inconsistencies and irrelevancies, are precisely the people that don't contribute to Git. And the people who do contribute to Git, and have enough empathy to put themselves in the shoes of users, are shun for 'rocking the boat'. Yes, the documentation *can* use updates, but won't. IIUC, ProGit is maintained in public and you can send patches to it, too. Yes, and I will send patches, but about issues that *matter*. Why would I send a patch to add text about a feature *nobody* needs to know? Users of stackoverflow seem to know about and how to use FETCH_HEAD: http://stackoverflow.com/questions/9237348/what-does-fetch-head-in-git-mean http://stackoverflow.com/questions/11478558/fetch-head-not-updating-after-git-fetch This is a cheeky use of rhetoric, it implies that *all* users of stackoverflow know about this, and it's not true. Counting the amount of votes, the best we can say is at most 20 people 'know' about this. A more specific description of the situation would be that at most 20 people _knew_ about this, at the time they read it; they might have forgotten already. But more importantly, only 14 people voted for this question, and only 7 people starred it. The top question 'How do I edit an incorrect commit message in Git?' about Git has 2871 votes and 822 stars, which is more than 200 times. Can we say that stackoverflows know how to amend a commit? Well, if they knew why it continues to receive votes? So if one question has 2871 votes, and the other one has 14 votes, does it mean they 'know' one question, and not the other one? At which point do the users stop 'knowing' the answer to the question? If a question is posted, does it mean that automatically stackoverflow users 'know' about it? No, we don't known what stackoverflow users seem to know, we can only know what *some* of them consider a good question, and a good answer. That's all. and they expect exactly what I described in the earlier message. Wrong. From those links only one person expected this to happen, and he received zero votes, which can be translated to; nobody cares. If you ask your search engine about FETCH_HEAD, you would find other real-world use of it as well (one of them I found was about somebody requesting to teach TortoiseGit to offer FETCH_HEAD as a candate to be merged, IIRC). I'm sure there are people out there that would find uses for FETCH_HEAD, you can probably count them with the fingers in your hand, even if half of them were missing. This patch would do *nothing* to harm those suers, because they can still do 'git fetch .', or even 'git fetch --allow-local'. The two persons in the world that do expect 'git fetch' without arguments to update FETCH_HEAD, will have to specify another single character argument. Big whooping do. The rest of the results in Google show basically only bugs, and patches. Even if all those results were about interesting things people do with FETCH_HEAD, that has absolutely nothing to do with the issue at hand, which is 'git fetch' without arguments. This is just a red herring. Incidentally, this
About overzealous compatibility
Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Fri, May 17, 2013 at 1:30 PM, Junio C Hamano gits...@pobox.com wrote: So when the user is running git fetch on mywork branch that happens to be forked from a local master,... we still need to have FETCH_HEAD updated to point at what we would be merging if she did a git pull. No, we don't need that. That is only needed by 'git pull', and in fact, it should be possible to reimplement 'git pull' so that it skips FETCH_HEAD when the remote is local. These are mere implementation details. You seem to be incapable to understand what backward compatibility is. Really? Do you even remember the time when you changed out of nowhere all the 'git-foo' commands with 'git foo' and all hell broke loose? I remember some lonely voice of reason shouting for clear deprecation warnings: http://article.gmane.org/gmane.comp.version-control.git/94262 You seem to not understand what is the purpose of backwards compatibility, so let me explain. The most important aspect of a project is not the technical details, or the performance, but it's *usefulness* to the users; once a user has decided to use the software, he expects it to keep working for his use-cases in the foreseeable future. If something changes, and the user was relying on that, the user would get annoyed, but most likely would not stop using the software. If this happens time and again, he might. In order to keep as many users as possible, and ensure the project keeps growing with as strong user and developer base, the old behaviors should remain in place as much as possible. This keeps old users happy, and allows the project to achieve it's goal; to be useful to many people as possible. The safest way to never break old behaviors is to never make any change, but this is not ideal, no software is perfect, and the software needs to be constantly evolving, otherwise users will stop using the software as well. Moving forward while not breaking old behaviors is often times difficult, but necessary, to keep the old users, and the steady growth. Many projects make the mistake of simply disregarding old users, and breaking things constantly; and while doing so, they keep loosing users, and never grow as much as they could. *Nobody* is suggesting Git should do that. It's inevitable that there would come a point in time where there would be a conflict of interests, and in order to move forward it's necessary to break old behaviors. When such situation arises, it's important to exhaust any other options that might allow both the old and new behaviors (like a configuration option), or different implementations. But it might be, that there are no alternatives, and either the project doesn't move forward, or compatibility is retained. When analyzing such cases, it's important to understand the impact it has, and how many users it affects; if it's a small change to the user work-flow, but it affects too many users, it might be best to don't go forward with the change, or only do so after a major version release, with good communication to minimize the damage. And if it's a big change, but it affects a small user base, it still might make sense to go ahead with the change. Many times it won't be possible to know how many users would be affects, and in those cases it might make sense to release, with a hand in the breaks, so, if a lot of users complain, the change is reverted, and nobody, or very few, complain, leave it there. The Linux project does this; revert when somebody shouts. In conclusion; you shouldn't blindly dogmatically avoid changes for the sake of it; you have to understand *why* you _try_ to retain backwards compatibility, and when it best serves the project not to do so. If you try to be too overzealous in your backwards compatibility, you run the risk of not moving fast enough in order to not annoy a handful of users, or even imaginary ones who don't even exist. This is not good for the project. A few days ago somebody used a very appropriate name for it: compabeaten[1]. We want to keep our users happy, but we also want to keep our developers happy, and not being able to move forward with as many features as one would want to is very annoying. In the worst case scenario, we can revert a change that many users are complaining about, we are not GNOME. [1] http://article.gmane.org/gmane.comp.version-control.git/224510 -- Felipe Contreras -- 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] New kind of upstream branch: base branch
Junio C Hamano wrote: Kevin Bracey ke...@bracey.fi writes: And it would be ideal if the initial base and push tracking information could be set up automatically on the first git checkout -b/git branch and git push. I think checkout and branch is already covered with -t. There may even be a configuration option to implicitly add -t to them (I didn't check). branch.autosetupmerge=always (For one, I've always found it odd that there's an asymmetry - if you check out a topic branch from the server to work on or use it, you get a local copy with upstream set by default. But if you create a topic branch yourself then push it, the upstream isn't set by default - you need the -u flag. This seems odd to me, and I've seen others confused by this). Yeah, I would imagine that it would be trivial to add an option to cause git push to do that, and it would be useful if you push to and pull from the same place (I haven't thought about ramifications such an option would have on the triangular workflows, though). It would work wonderfully. Now: == remote == % git checkout -b master origin/master # work % git push == local == % git checkout -b topic master # work % git push # ERR % git push -u origin/master # work % git push # ERR That doesn't work. We could set branch.autosetupmerge=always as the default, but it still wouldn't work, because push.default=simple (or at least it will be), and the names don't match. But with my patches, and branch.autosetupmerge=always (hopefully for 2.0): == local == % git checkout -b topic master # work % git push # ERR % git push --set-downstream origin/master # work % git push Cheers. -- Felipe Contreras -- 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/2] remote-helpers: tests: use python directly
Junio C Hamano wrote: David Aguilar dav...@gmail.com writes: On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano gits...@pobox.com wrote: So it is a right thing to do in that sense. I however am having this nagging feeling that I may be missing something subtle here. Comments from others are very much welcome. Yes, this is correct. Another way to skin this cat would be to do search/replace in a Makefile to burn in the PYTHON_PATH similar to how we do for the .sh scripts and other .py files in the main Makefile. The remote helpers are in contrib/ so they do not go through the main Makefile, which is the root cause. Longer-term, it would be good to treat these uniformly, but this is no worse for now. Ahh, so my nagging feeling was that remote-helpers could in theory be updated to follow the PYTHON_PATH like the rest of the system and matching these two in that direction is the better longer-term fix? Indeed, and I already have patches for that. FTR, the Makefile does loop through the main Makefile, which is what made implementing this so easy. I somehow thought that there were valid reasons that we could not do so for some technical reason (e.g. the instalation of python used to run hg and/or bzr via these remote helpers and the installation of python we use may need to be different?). As long as the right longer-term direction is not we cannot fundamentally unify them, then I am very happy. I didn't think of that, and it might actually be a problem: I don't think Mercurial works with python 3, and Bazaar probably never will. But I don't think the current situation of always using 'python' helps either way. Cheers. -- Felipe Contreras -- 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 v5 01/15] Add new git-related helper to contrib
Junio C Hamano wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Sat, May 18, 2013 at 6:46 AM, Felipe Contreras felipe.contre...@gmail.com wrote: contrib/related/git-related | 124 1 file changed, 124 insertions(+) create mode 100755 contrib/related/git-related I tried everything and I don't think it's physically possible to make this script any simpler without severely crippling it's main goal. Hmm, I haven't read these patches yet (I just came back a few hours ago to a state in which I am well enough to read and write e-mails), but did anybody complain that it is too complex? This comment was again targetted to Ramkumar, who yes, indeed he complained about it being too complex, at least v3. -- Felipe Contreras -- 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 v8 0/3] Begin replacing OpenSSL with CommonCrypto
This is a re-roll of David Aguilar's patch series [1] which eliminates some of the OpenSSL deprecation warnings on Mac OS X. Changes since v7: - Avoid double-negation (#ifndef NO_APPLE_COMMON_CRYPTO) - Don't break imap-send.c for platforms other than Apple [1]: http://thread.gmane.org/gmane.comp.version-control.git/224744 David Aguilar (3): Makefile: add support for Apple CommonCrypto facility cache.h: eliminate SHA-1 deprecation warnings on Mac OS X imap-send: eliminate HMAC deprecation warnings on Mac OS X Makefile| 14 ++ imap-send.c | 10 ++ 2 files changed, 24 insertions(+) -- 1.8.2.3 -- 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 v8 1/3] Makefile: add support for Apple CommonCrypto facility
From: David Aguilar dav...@gmail.com As of Mac OS X 10.7, Apple deprecated all OpenSSL functions due to OpenSSL ABI instability, thus leading to build warnings. As a replacement, Apple encourages developers to migrate to its own (stable) CommonCrypto facility. Introduce boilerplate which controls whether Apple's CommonCrypto facility is employed (enabled by default). Also add a NO_APPLE_COMMON_CRYPTO build flag with which the user can opt out to use OpenSSL instead. [es: extracted CommonCrypto-related Makefile boilerplate into separate introductory patch] Signed-off-by: David Aguilar dav...@gmail.com Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- Makefile | 8 1 file changed, 8 insertions(+) diff --git a/Makefile b/Makefile index f698c1a..cd24c94 100644 --- a/Makefile +++ b/Makefile @@ -137,6 +137,10 @@ all:: # specify your own (or DarwinPort's) include directories and # library directories by defining CFLAGS and LDFLAGS appropriately. # +# Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X +# and do not want to use Apple's CommonCrypto library. This allows you +# to provide your own OpenSSL library, for example from MacPorts. +# # Define BLK_SHA1 environment variable to make use of the bundled # optimized C SHA1 routine. # @@ -1054,6 +1058,10 @@ ifeq ($(uname_S),Darwin) BASIC_LDFLAGS += -L/opt/local/lib endif endif + ifndef NO_APPLE_COMMON_CRYPTO + APPLE_COMMON_CRYPTO = YesPlease + COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO + endif NO_REGEX = YesPlease PTHREAD_LIBS = endif -- 1.8.2.3 -- 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 v8 2/3] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
From: David Aguilar dav...@gmail.com As of Mac OS X 10.7, Apple deprecated all OpenSSL functions due to OpenSSL ABI instability, thus leading to build diagnostics such as: warning: 'SHA1_Init' is deprecated (declared at /usr/include/openssl/sha.h:121) Silence the warnings by using Apple's CommonCrypto SHA-1 replacement functions for SHA1_Init(), SHA1_Update(), and SHA1_Final(). COMMON_DIGEST_FOR_OPENSSL is defined to instruct CommonCrypto/CommonDigest.h to provide compatibility macros associating OpenSSL SHA-1 functions with their CommonCrypto counterparts. [es: reworded commit message] Signed-off-by: David Aguilar dav...@gmail.com Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- Makefile | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Makefile b/Makefile index cd24c94..5e7cadf 100644 --- a/Makefile +++ b/Makefile @@ -1397,10 +1397,16 @@ ifdef PPC_SHA1 LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o LIB_H += ppc/sha1.h else +ifdef APPLE_COMMON_CRYPTO + COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL + SHA1_HEADER = CommonCrypto/CommonDigest.h +else SHA1_HEADER = openssl/sha.h EXTLIBS += $(LIB_4_CRYPTO) endif endif +endif + ifdef NO_PERL_MAKEMAKER export NO_PERL_MAKEMAKER endif -- 1.8.2.3 -- 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 v8 3/3] imap-send: eliminate HMAC deprecation warnings on Mac OS X
From: David Aguilar dav...@gmail.com As of Mac OS X 10.7, Apple deprecated all OpenSSL functions due to OpenSSL ABI instability. Silence the warnings by using Apple's CommonCrypto HMAC replacement functions. [es: reworded commit message; check APPLE_COMMON_CRYPTO instead of abusing COMMON_DIGEST_FOR_OPENSSL] Signed-off-by: David Aguilar dav...@gmail.com Signed-off-by: Eric Sunshine sunsh...@sunshineco.com --- Junio Hamano writes: Doesn't this mean people on platforms that do not care what Apple does have to define NO_APPLE_COMMON_CRYPTO? It certainly does break other platforms. Thanks for catching. How about stopping this double-negation and do it more like this? Double-negation begone. imap-send.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/imap-send.c b/imap-send.c index d9bcfb4..8ea180f 100644 --- a/imap-send.c +++ b/imap-send.c @@ -29,8 +29,18 @@ #ifdef NO_OPENSSL typedef void *SSL; #else +#ifdef APPLE_COMMON_CRYPTO +#include CommonCrypto/CommonHMAC.h +#define HMAC_CTX CCHmacContext +#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len) +#define HMAC_Update CCHmacUpdate +#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash) +#define HMAC_CTX_cleanup +#define EVP_md5() kCCHmacAlgMD5 +#else #include openssl/evp.h #include openssl/hmac.h +#endif #include openssl/x509v3.h #endif -- 1.8.2.3 -- 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 7/9] for-each-ref: prepoplulate all atoms before show_ref()
By the time show_ref() is called, atom values for all refs are
ready. This can be taken advantage of later.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/for-each-ref.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index db5c211..a9d189c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1010,15 +1010,24 @@ static void show_ref(struct strbuf *sb, struct refinfo
*info,
static void show_refs(struct refinfo **refs, int maxcount,
const char *format, int quote_style)
{
- struct strbuf sb = STRBUF_INIT;
+ struct strbuf *sb;
int i;
+ sb = xmalloc(sizeof(*sb) * maxcount);
for (i = 0; i maxcount; i++) {
- strbuf_reset(sb);
- show_ref(sb, refs[i], format, quote_style);
- fputs(sb.buf, stdout);
+ strbuf_init(sb + i, 256);
+ if (!refs[i]-value) {
+ populate_value(refs[i]);
+ fill_missing_values(refs[i]-value);
+ }
+ }
+
+ for (i = 0; i maxcount; i++) {
+ show_ref(sb + i, refs[i], format, quote_style);
+ fputs(sb[i].buf, stdout);
+ strbuf_release(sb + i);
}
- strbuf_release(sb);
+ free(sb);
}
static struct ref_sort *default_sort(void)
--
1.8.2.83.gc99314b
--
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 8/9] for-each-ref: merge show_ref into show_refs
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/for-each-ref.c | 39 +++
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index a9d189c..1390da8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -989,29 +989,12 @@ static void emit(struct strbuf *sb, const char *cp, const
char *ep)
}
}
-static void show_ref(struct strbuf *sb, struct refinfo *info,
-const char *format, int quote_style)
-{
- const char *cp, *sp, *ep;
-
- for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- ep = strchr(sp, ')');
- if (cp sp)
- emit(sb, cp, sp);
- print_value(sb, info, parse_atom(sp + 2, ep), quote_style);
- }
- if (*cp) {
- sp = cp + strlen(cp);
- emit(sb, cp, sp);
- }
- strbuf_addch(sb, '\n');
-}
-
static void show_refs(struct refinfo **refs, int maxcount,
const char *format, int quote_style)
{
struct strbuf *sb;
- int i;
+ const char *cp, *sp, *ep;
+ int i, atom;
sb = xmalloc(sizeof(*sb) * maxcount);
for (i = 0; i maxcount; i++) {
@@ -1022,8 +1005,24 @@ static void show_refs(struct refinfo **refs, int
maxcount,
}
}
+ for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
+ ep = strchr(sp, ')');
+ if (cp sp) {
+ for (i = 0; i maxcount; i++)
+ emit(sb + i, cp, sp);
+ }
+ atom = parse_atom(sp + 2, ep);
+ for (i = 0; i maxcount; i++)
+ print_value(sb + i, refs[i], atom, quote_style);
+ }
+ if (*cp) {
+ sp = cp + strlen(cp);
+ for (i = 0; i maxcount; i++)
+ emit(sb + i, cp, sp);
+ }
+
for (i = 0; i maxcount; i++) {
- show_ref(sb + i, refs[i], format, quote_style);
+ strbuf_addch(sb + i, '\n');
fputs(sb[i].buf, stdout);
strbuf_release(sb + i);
}
--
1.8.2.83.gc99314b
--
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 9/9] for-each-ref: support %(...:aligned) for left alignment
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/for-each-ref.c | 25 ++---
1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1390da8..3240ca0 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -11,6 +11,7 @@
#include remote.h
#include color.h
#include branch.h
+#include utf8.h
/* Quoting styles */
#define QUOTE_NONE 0
@@ -733,7 +734,7 @@ static void populate_value(struct refinfo *ref)
/* look for short refname format */
if (formatp) {
formatp++;
- if (!strcmp(formatp, short))
+ if (!prefixcmp(formatp, short))
refname = shorten_unambiguous_ref(refname,
warn_ambiguous_refs);
else
@@ -994,7 +995,7 @@ static void show_refs(struct refinfo **refs, int maxcount,
{
struct strbuf *sb;
const char *cp, *sp, *ep;
- int i, atom;
+ int i, atom, aligned, max_length, len;
sb = xmalloc(sizeof(*sb) * maxcount);
for (i = 0; i maxcount; i++) {
@@ -1012,8 +1013,26 @@ static void show_refs(struct refinfo **refs, int
maxcount,
emit(sb + i, cp, sp);
}
atom = parse_atom(sp + 2, ep);
- for (i = 0; i maxcount; i++)
+ aligned = !suffixcmp(used_atom[atom], :aligned);
+ for (i = 0, max_length = 0; aligned i maxcount; i++) {
+ struct atom_value *v;
+ get_value(refs[i], atom, v);
+ len = utf8_strnwidth(v-s, -1, 1);
+ if (len max_length)
+ max_length = len;
+ }
+ for (i = 0; i maxcount; i++) {
+ int old_len = sb[i].len;
print_value(sb + i, refs[i], atom, quote_style);
+ if (aligned) {
+ len = max_length -
+ utf8_strnwidth(sb[i].buf + old_len, -1,
1);
+ while (len) {
+ strbuf_addch(sb + i, ' ');
+ len--;
+ }
+ }
+ }
}
if (*cp) {
sp = cp + strlen(cp);
--
1.8.2.83.gc99314b
--
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 18/05/2013 23:37, Peter Lauri wrote: Great, I have gotten the concept now :) My workaround for my problem is to rename the file to default and then all will work out well :) Copy the file then and locally modify it, but it will be in .gitignore so not tracked :) Over in the #git IRC channel, we point users asking this question to https://gist.github.com/canton7/1423106. It contains that approach, and quite a few others. Antony -- 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/WIP 0/9] for-each-ref format improvements
Nguyễn Thái Ngọc Duy wrote: The purpose of this series is to make for-each-ref --format powerful enough to display what branch -v and branch -vv do so that we could get rid of those display code and use for-each-ref code instead. Damn, you beat me to it. I just introduced color, and was working on alignment. See $gmane/224692. Yes, I think this is the direction we should be taking. Poorly thought-out stuff like -v and -vv should be deprecated. This series introduces: - %(current), which either shows * if the ref is pointed by HEAD or a space. Junio called it %(headness). I don't like that. I don't like %(current) either but we have to start somewhere. Name suggestion? %(marker)?? How about %(HEAD)? - %(tracking[:upstream]) gives us the exact output that branch -v[v] does. %(upstream) does not include []. We can't change its semantics. There's already an atom called upstream, and upstream:short works. Why not introduce upstream:diff for [ahead x, behind y] and upstream:shortdiff for (like in the prompt)? - %(color:...) is pretty much the same as %C family in pretty code. I haven't added code for %(color:foo) == %C(foo) yet. There's a potential ambiguity here: %C(red) == %Cred or %C(red)?? I'd vote for dropping %Cname altogether and just go with %C(name). Why do we need %(color:name) at all? - %(...:aligned) to do left aligning. I'm not entirely sure about this. We might be able to share code with %, % and % from pretty.c. But we need improvements there too because in for-each-ref case, we could calculate column width but % would require the user to specify the width. Yeah, I think we should go with the % and % you introduced in pretty.c. Yes, I want to be able to specify width. Do people expect fancy layout with for-each-ref (and branch)? If so we might need to have %(align) or something instead of the simple left alignment case in %(...:aligned) Why should we deviate from the pretty case? What is different here? - We may need an equivalent of the space following % in pretty format. If the specifier produces something, then prepend a space, otherwise produce nothing. Do it like %C( tracking) vs %C(tracking)?? Yeah, sounds good. You can try this after applying the series, which should give you the about close to 'branch -v'. %(tracking) coloring does not work though. Why doesn't %(tracking) coloring work? Nguyễn Thái Ngọc Duy (9): I'll have a look at this. Also, I think it'll help to have a --pretty=format:string equivalent to --format=string so that we can introduce pretty names like oneline, short, medium, full. We can eventually deprecate --format for consistency. -- 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 4/9] for-each-ref: add %(current) for current branch marker
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 08d4eb1..498d703 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -75,6 +75,7 @@ static struct {
{ upstream },
{ symref },
{ flag },
+ { current },
};
So you've implemented it as another atom. Good.
--
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 3/9] for-each-ref: avoid printing each element directly to stdout
[PATCH 3/9] for-each-ref: avoid printing each element directly to stdout Why did you do this? Atoms are designed to be independent of each other. I'll keep reading: might find out in a later 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 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 498d703..b10d48a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -76,6 +76,8 @@ static struct {
{ symref },
{ flag },
{ current },
+ { tracking },
+ { tracking:upstream },
};
You just threw the upstream atom (and upstream:short) out the window :|
--
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-remote-hg: set stdout to binary mode on win32
On Mon, Jan 28, 2013 at 4:13 PM, Amit Bakshi ambak...@gmail.com wrote:
git clone hangs on windows (msysgit/cygwin), and
file.write would return errno 22 inside of mercurial's
windows.winstdout wrapper class. This patch sets
stdout's mode to binary, fixing both issues.
---
contrib/remote-helpers/git-remote-hg | 21 +
1 file changed, 21 insertions(+)
diff --git a/contrib/remote-helpers/git-remote-hg
b/contrib/remote-helpers/git-remote-hg
index 328c2dc..95f4c1f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -62,6 +62,24 @@ def get_config(config):
output, _ = process.communicate()
return output
+#
+# On Windows (msysgit/cygwin) have to set stdout to binary
+# mode (_O_BINARY is 32768). Otherwise clone hangs, and pushing
+# to remote fails when doing a write to mercurial's wrapper
+# windows.winstdout wrapper class.
+#
+def set_binmode(fd):
+try:
+if sys.platform == win32:
+import msvcrt
+msvcrt.setmode(fd, os.O_BINARY)
+elif sys.platform == 'cygwin':
+import ctypes
+msvcrt = ctypes.CDLL('msvcrt.dll')
+msvcrt._setmode(fd, 32768) # On Cygwin os.O_BINARY is different
+except OSError:
+pass
I tried many things, and it seems it's true that we need to set the
binary mode in Windows, but it seemed to work fine cygwin. I saw in
many places the workaround for 'win32', but I didn't find the one for
'cygwin'. Where did you find it? Did you test in cygwin? Is it needed
there?
Cheers.
--
Felipe Contreras
--
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 6/9] for-each-ref: add %(color:...)
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b10d48a..db5c211 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -78,6 +81,7 @@ static struct {
{ current },
{ tracking },
{ tracking:upstream },
+ { color },
};
No, I intentionally did not make color an atom in $gmane/224692. It
doesn't print any information about the branch, like the other atoms
do: have it augment the final printing.
/*
@@ -707,6 +711,21 @@ static void populate_value(struct refinfo *ref)
v-s = sb.buf;
continue;
}
+ else if (!prefixcmp(name, color:)) {
+ const char *color = name + 6;
+ ref-auto_color = 0;
+ if (!prefixcmp(color, red))
+ v-s = GIT_COLOR_RED;
+ else if (!prefixcmp(color, green))
+ v-s = GIT_COLOR_GREEN;
+ else if (!prefixcmp(color, blue))
+ v-s = GIT_COLOR_BLUE;
+ else if (!prefixcmp(color, reset))
+ v-s = GIT_COLOR_RESET;
+ else if (!prefixcmp(color, auto))
+ ref-auto_color = 1;
+ continue;
+ }
So I can't have %(color:yellow)? :(
Why are you parsing it here when you can use color_parse_name()?
--
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 9/9] for-each-ref: support %(...:aligned) for left alignment
I don't think [7/9] and [8/9] belong in this series. Let's see how
you've used it in :aligned.
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1390da8..3240ca0 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1012,8 +1013,26 @@ static void show_refs(struct refinfo **refs, int
maxcount,
emit(sb + i, cp, sp);
}
atom = parse_atom(sp + 2, ep);
- for (i = 0; i maxcount; i++)
+ aligned = !suffixcmp(used_atom[atom], :aligned);
+ for (i = 0, max_length = 0; aligned i maxcount; i++) {
+ struct atom_value *v;
+ get_value(refs[i], atom, v);
+ len = utf8_strnwidth(v-s, -1, 1);
+ if (len max_length)
+ max_length = len;
Why?! Why are you denying me the pleasure of using %, %|, %, %|,
%, %|, %, and %| that you invented in pretty? The code is
already there: you just have to hook it up.
--
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/WIP 0/9] for-each-ref format improvements
On Sun, May 19, 2013 at 6:11 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Yes, I think this is the direction we should be taking. Poorly thought-out stuff like -v and -vv should be deprecated. Of course not. They are useful and user-friendly. The only question is what should be the format by default. Also, I think it'll help to have a --pretty=format:string equivalent to --format=string so that we can introduce pretty names like oneline, short, medium, full. We can eventually deprecate --format for consistency. I don't see the point of --pretty. I think there should be shortcuts, something like --hash for the SHA1s only, and --names for the refs only, or something like that. -- Felipe Contreras -- 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 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info
On Sun, May 19, 2013 at 6:18 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 498d703..b10d48a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -76,6 +76,8 @@ static struct {
{ symref },
{ flag },
{ current },
+ { tracking },
+ { tracking:upstream },
};
You just threw the upstream atom (and upstream:short) out the window :|
Huh? Those don't print the tracking information, do they?
tracking:upstream prints the upstream, but other things as well I
suppose.
--
Felipe Contreras
--
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/9] quote.c: make sq_quote_print a slight wrapper of sq_quote_buf
On Sun, May 19, 2013 at 5:27 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- quote.c | 17 - 1 file changed, 4 insertions(+), 13 deletions(-) No functional changes I suppose. -- Felipe Contreras -- 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/9] for-each-ref: convert to use *_quote_buf instead of _quote_print
On Sun, May 19, 2013 at 5:27 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- builtin/for-each-ref.c | 14 ++ quote.c| 44 ++-- quote.h| 6 +++--- 3 files changed, 35 insertions(+), 29 deletions(-) No functional changes I suppose. -- Felipe Contreras -- 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 9/9] for-each-ref: support %(...:aligned) for left alignment
On Sun, May 19, 2013 at 6:32 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
I don't think [7/9] and [8/9] belong in this series. Let's see how
you've used it in :aligned.
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1390da8..3240ca0 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1012,8 +1013,26 @@ static void show_refs(struct refinfo **refs, int
maxcount,
emit(sb + i, cp, sp);
}
atom = parse_atom(sp + 2, ep);
- for (i = 0; i maxcount; i++)
+ aligned = !suffixcmp(used_atom[atom], :aligned);
+ for (i = 0, max_length = 0; aligned i maxcount; i++) {
+ struct atom_value *v;
+ get_value(refs[i], atom, v);
+ len = utf8_strnwidth(v-s, -1, 1);
+ if (len max_length)
+ max_length = len;
Why?! Why are you denying me the pleasure of using %, %|, %, %|,
%, %|, %, and %| that you invented in pretty? The code is
already there: you just have to hook it up.
Because for-each-ref only understands %(...) not %|, i.e. % followed
by a (. We need more changes in for-each-ref code to make it accept
%|, I think. Also %|(N) needs N. In case of branch -v that
should be calculated automatically, so we need a syntax to say align
to the left, use the smallest width that fits all. I guess %|(*)?
--
Duy
--
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 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info
On Sun, May 19, 2013 at 6:38 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Sun, May 19, 2013 at 6:18 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Nguyễn Thái Ngọc Duy wrote:
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 498d703..b10d48a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -76,6 +76,8 @@ static struct {
{ symref },
{ flag },
{ current },
+ { tracking },
+ { tracking:upstream },
};
You just threw the upstream atom (and upstream:short) out the window :|
Huh? Those don't print the tracking information, do they?
tracking:upstream prints the upstream, but other things as well I
suppose.
Exactly. I already explained why %(upstream) can't be used in 00/09.
tracking may not be perfect. Somebody might want
tracking:upstream:short. It does not look quite nice.
--
Duy
--
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: Random thoughts on upstream
Junio C Hamano wrote: If somebody implements the push.default = single (see the original message you are responding to), then the change might be smaller. I think this is a bad hack covering up an underlying problem: it is ugly, confusing, and inextensible in my opinion. I think of push.default as merely a default remote.name.push value for all remotes, that can be overridden on a per-remote basis. I suspect that the issue you're trying to address is: [remote ram] push = refs/heads/*:refs/heads/rr/* not dictating which refs to push when I say 'git push' (it'll push all the refs under refs/heads/*, not respecting push.default=current in my scheme). That is a valid problem, and it is a problem with our refspec: I can say HEAD (which is what push.default=current is) to mean current branch, but I can't dictate the rhs of the refspec then. In other words, I cannot have the refspec HEAD:refs/heads/rr/%1, and _this_ is the problem. What do you think? -- 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: Random thoughts on upstream
On Sun, May 19, 2013 at 6:44 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Junio C Hamano wrote: If somebody implements the push.default = single (see the original message you are responding to), then the change might be smaller. I think this is a bad hack covering up an underlying problem: it is ugly, confusing, and inextensible in my opinion. I think of push.default as merely a default remote.name.push value for all remotes, that can be overridden on a per-remote basis. You can't represent push.default = single either. -- Felipe Contreras -- 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 5/9] for-each-ref: add %(tracking[:upstream]) for tracking info
Duy Nguyen wrote: Exactly. I already explained why %(upstream) can't be used in 00/09. tracking may not be perfect. Somebody might want tracking:upstream:short. It does not look quite nice. Which is why I suggested keeping upstream, upstream:short, and introducing upstream:diff and upstream:shortdiff (or :tracking if you prefer that) in [0/9]. -- 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: add new fetch.default configuration
Felipe Contreras wrote:
% git checkout fc/remote/hg-next
% git rebase -i # rebase to master
% git checkout fc/remote/hg-notes
% git rebase -i # rebase to fc/remote/hg-next
% git checkout fc/remote/hg-gitifyhg-compat
% git rebase -i # rebase to fc/remote/hg-notes
So it is rebase, but rebase.defaultUpstream = origin won't suffice.
You want to specify a custom base. What I don't understand is why
you're abusing @{u}, and crippling remote (by hard-interpreting it as
origin) to achieve this. For the record, I didn't think your
branch.name.base was a bad idea at all (re-visiting now).
--
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] Improve rebase documenation
Recent discussions have shown that rebase isn't as well understood as as perhaps it should be for the basic user. Add a softer introductory paragraph to the man page description, and in the second patch, add a second paragraph explaining the build up of the command so that users have a historical context in which to rationalise the command structure. The second patch/paragraph will probably need the quoting checking as to which are quoting the generic command and which are the same text as a command example. Basic wording is borrowed from on-list clarifications. Philip Oakley (2): Doc rebase: Describe rebase as excluding merge commits Doc rebase: describe the priority of published work Documentation/git-rebase.txt | 10 ++ 1 file changed, 10 insertions(+) -- 1.8.1.msysgit.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
[RFC/PATCH 2/2] Doc rebase: describe the priority of published work
Give details of the implied priority in the description section. Signed-off-by: Philip Oakley philipoak...@iee.org --- wording based on: http://article.gmane.org/gmane.comp.version-control.git/222581 http://article.gmane.org/gmane.comp.version-control.git/223816 http://article.gmane.org/gmane.comp.version-control.git/217410 http://article.gmane.org/gmane.comp.version-control.git/222763 --- Documentation/git-rebase.txt | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 87a8095..24d16ef 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -20,6 +20,12 @@ DESCRIPTION by default, to the head of the branch's upstream, or onto a new base if given. +In its minimal form, having built locally on the (current) branch, +use `git rebase` to catch up with its published upstream state to +create a new linear history. Importantly, `git rebase` is a way to +replay your work on top of the work of others. It is not about +integrating other people's changes into your work. + If branch is specified, 'git rebase' will perform an automatic `git checkout branch` before doing anything else. Otherwise it remains on the current branch. -- 1.8.1.msysgit.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
[RFC/PATCH 1/2] Doc rebase: Describe rebase as excluding merge commits
Describe rebase in the description section. Include a softer paraphrased version from the crytic, well-loved, but sometimes parodied, Name description, and tell users that merge commits are excluded by default. Signed-off-by: Philip Oakley philipoak...@iee.org --- http://article.gmane.org/gmane.comp.version-control.git/217410 http://article.gmane.org/gmane.comp.version-control.git/217495 --- Documentation/git-rebase.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index aca8405..87a8095 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -16,6 +16,10 @@ SYNOPSIS DESCRIPTION --- +'git rebase' will transfer local commits, excluding merge commits +by default, to the head of the branch's upstream, or onto a new base +if given. + If branch is specified, 'git rebase' will perform an automatic `git checkout branch` before doing anything else. Otherwise it remains on the current branch. -- 1.8.1.msysgit.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
[RFC/PATCH 1/2] config doc: add dot-repository note
branch.name.remote can be set to '.' (period) as a dot repository as part of the remote name dwimmery. Tell the reader. Signed-off-by: Philip Oakley philipoak...@iee.org --- Documentation/config.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 6e53fc5..fd42509 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -734,6 +734,8 @@ branch.name.remote:: overridden by `branch.name.pushremote`. If no remote is configured, or if you are not on any branch, it defaults to `origin` for fetching and `remote.pushdefault` for pushing. + Additionally, a `.` (period) means the current local repository + (a dot-repository), see `branch.name.merge`'s final note below. branch.name.pushremote:: When on branch name, it overrides `branch.name.remote` for -- 1.8.1.msysgit.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
[RFC/PATCH 2/2] doc: command line interface (cli) dot-repository dwimmery
The Git cli will generally accept dot '.' (period) as equivalent to the current repository when appropriate. Tell the reader of this 'do what I mean' (dwim)mery action. Signed-off-by: Philip Oakley philipoak...@iee.org --- Documentation/gitcli.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt index 9ac5088..64bb386 100644 --- a/Documentation/gitcli.txt +++ b/Documentation/gitcli.txt @@ -59,6 +59,10 @@ working tree. After running `git add hello.c; rm hello.c`, you will _not_ see `hello.c` in your working tree with the former, but with the latter you will. +Just as, by convention, the filesystem '.' refers to the current directory, +using a '.' (period) as a repository name in Git (a dot-repository) refers +to your local repository. + Here are the rules regarding the flags that you should follow when you are scripting Git: -- 1.8.1.msysgit.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
[RFC/PATCH 0/2] Extend dot repository documentation
The dot repository convention is not well know to users and its documenation is hidden as a note in an ancilliary config variable's documenation. Document the dot repository 'do what I mean' convention in the config variable it is used in, and in the cli (command line interface) documenation page to give regular users a chance to find it. Philip Oakley (2): config doc: add dot-repository note doc: command line interface (cli) dot-repository dwimmery Documentation/config.txt | 2 ++ Documentation/gitcli.txt | 4 2 files changed, 6 insertions(+) -- 1.8.1.msysgit.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: [PATCH/WIP 0/9] for-each-ref format improvements
On Sun, May 19, 2013 at 6:11 PM, Ramkumar Ramachandra artag...@gmail.com wrote: Nguyễn Thái Ngọc Duy wrote: The purpose of this series is to make for-each-ref --format powerful enough to display what branch -v and branch -vv do so that we could get rid of those display code and use for-each-ref code instead. Damn, you beat me to it. I just introduced color, and was working on alignment. See $gmane/224692. Hmm.. I missed that mail (or I wouldn't have worked on this already). Do you want to take over? - %(tracking[:upstream]) gives us the exact output that branch -v[v] does. %(upstream) does not include []. We can't change its semantics. There's already an atom called upstream, and upstream:short works. Why not introduce upstream:diff for [ahead x, behind y] and upstream:shortdiff for (like in the prompt)? branch -vv shows [upstream: ahead x, behind y]. We need a syntax to cover that too. - %(color:...) is pretty much the same as %C family in pretty code. I haven't added code for %(color:foo) == %C(foo) yet. There's a potential ambiguity here: %C(red) == %Cred or %C(red)?? I'd vote for dropping %Cname altogether and just go with %C(name). Why do we need %(color:name) at all? pretty and for-each-ref format seem to be on the opposite: one is terse, one verbose. Unless you are going to introduce a lot of new specifiers (and in the worst case, bring all pretty specifiers over, unify underlying code), I think we should stick with %(xx) convention. - %(...:aligned) to do left aligning. I'm not entirely sure about this. We might be able to share code with %, % and % from pretty.c. But we need improvements there too because in for-each-ref case, we could calculate column width but % would require the user to specify the width. Yeah, I think we should go with the % and % you introduced in pretty.c. Yes, I want to be able to specify width. I still think we should follow %(...), e.g. %(left:N), %(right:N) as equivalent of % and %... Do people expect fancy layout with for-each-ref (and branch)? If so we might need to have %(align) or something instead of the simple left alignment case in %(...:aligned) Why should we deviate from the pretty case? What is different here? Laziness plays a big factor :) So again, you want to take over? ;) - We may need an equivalent of the space following % in pretty format. If the specifier produces something, then prepend a space, otherwise produce nothing. Do it like %C( tracking) vs %C(tracking)?? Yeah, sounds good. You can try this after applying the series, which should give you the about close to 'branch -v'. %(tracking) coloring does not work though. Why doesn't %(tracking) coloring work? it uses builtin/branch.c:branch_use_color. Eventually fill_tracking_info() should be moved to for-each-ref.c and pass branch_use_color in as an argument. But for now, I just leave it broken. -- Duy -- 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: Random thoughts on upstream
Felipe Contreras wrote: You can't represent push.default = single either. Right. And I propose that we extend the refspec to be able to represent it, instead of having single sticking out like a sore thumb (and possibly introducing more sore thumbs like this in the future). -- 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
