Re: [PATCH v3 3/4] The name of the hash function is SHA-1, not SHA1
On 04/15/2013 08:15 PM, Junio C Hamano wrote: Thomas Ackermann th.ac...@arcor.de writes: Use SHA-1 instead of SHA1 whenever we talk about the hash function. When used as a programming symbol, we keep SHA1. Signed-off-by: Thomas Ackermann th.ac...@arcor.de --- Thanks. Will queue as-is for now, but I wonder if we want to fix them to more official object name, if we are going to the trouble of fixing all of these. It depends on how many places already correctly spell SHA-1, I guess. I like the idea of making the Git documentation (and the source code) more algorithm-agnostic. But personally, I think object name is a bad generic term for describing object hashes. The word name suggests a moniker that was intentionally given to the object. I suppose that this is a big reason that the term SHA-1 is used so frequently rather than object name--because it is transparently obvious that an SHA-1 is a hash as opposed to, say, a filename. In my opinion, rather than expand the use of the term object name, we should pick a better official term that makes it more obvious what we are talking about, like object hash. While we are at it, if being more algorithm-agnostic is considered a worthy goal, maybe it would be helpful to establish a source code naming convention to be used in new code in favor of sha1; for example, ohash = hash of an object of unknown type chash = hash of a commit object etc. Obviously I'm not suggesting that Git should transition away from using SHA-1s, just that the choice of hashing algorithm need not be quite so explicit in source code that doesn't really need to care. On a related topic, I find it shocking how often the hard-coded constants 20, 40, and 41 appear in git source code: $ git grep -e '\20\' -- '*.c' '*.h' '*.sh' '*.perl' | wc -l 689 $ git grep -e '\4[01]\' -- '*.c' '*.h' '*.sh' '*.perl' | wc -l 339 The vast majority of these have to do with the length of a SHA-1 hash. I think it would aid source-code readability if there were named constants for the lengths of object hashes in binary and hex format. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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] gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM
Junio C Hamano wrote: In order to just pick and use the more appropriate one (or a useful combination of the two), a clean description of what each of them do without historical cruft is more readable and useful, isn't it? I would expect that most of them who are newly configuring a system would pick COMMON one and override per instance as needed, without touching the SYSTEM one (fallback default) after reading the above, and that is what we want to happen. Do you think sysadmins need a history lesson to understand why there are two different possibilities? [...] I think the new text conveys the necessary information to the intended audience with more clarity without the history lesson or the record of your past frustration. Am I mistaken? Note also that this is about *gitweb/INSTALL*, which is meant to be *short* and succint description on how to install gitweb, and not about the reference documentation: gitweb(1) or gitweb.conf(5). Description of historical behavior (and backward compatibility) has place (if any) in manpages, not gitweb/INSTALL. -- Jakub Narębski -- 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] usage: refactor die-recursion checks
Am 4/16/2013 4:50, schrieb Jeff King:
On Mon, Apr 15, 2013 at 07:34:07PM -0700, Brandon Casey wrote:
Right. My assumption was that we are primarily interested in protecting
against the die_routine. Compat functions should never be calling die.
I think the rule we have been enforcing is less strict than that. We
have only said that any compat function in a die handler path should
never call die. But maybe that's what you meant.
No, I assumed we were following the stronger rule. If you are a compat
function for a C library function, then you should never need to die.
You should be conforming to the existing interface, which will have some
mechanism for passing back an error.
This rule has been violated LNG ago, and not only in compat/mingw.c
(see xmalloc in compat/qsort.c, for example).
The primary motivation was that Hannes Sixt had to step in and point
out yet again that the high-level memory allocators should not be
called in anything that could be in a die handler code path. I was on
the thread, but I don't remember the topic (ah, Jonathan has stepped
in with the answer). I do remember that I was not the only one who
had forgotten about that rule though.
Yeah, it is subtle enough that it may be worth protecting against.
Too late.
To implement this check correctly/completely (i.e. detect recursion in
the main thread as well as in any child threads), I think you really
do need to use thread-local storage as you mentioned in 3/3 which
could look something like:
static pthread_key_t dying;
static pthread_once_t dying_once = PTHREAD_ONCE_INIT;
void setup_die_counter(void)
{
pthread_key_create(dying, NULL);
}
check_die_recursion(void)
{
pthread_once(dying_once, setup_die_counter);
if (pthread(getspecific(dying)) {
puts(BUG: recursion...);
exit(128);
}
pthread_setspecific(dying, dying);
}
Yeah, that seems sane; my biggest worry was that it would create
headaches for Windows folks, who would have to emulate pthread_key. But
it seems like we already added support in 9ba604a.
pthread_key is not a problem, but pthread_once is. It's certainly
solvable, but do we really have to?
-- Hannes
--
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: Ensimag students projects, version 2013
Matthieu Moy wrote: I tend to agree with you, but the idea has explicitly been rejected in the past. The problem with an option like this is that it would also disable the advices that may be added in the future. By letting people disable the advices one by one, people see new advices as they arrive. You may think of it like do not show this message again tickboxes in some graphical user interfaces. Too controversial area for newcommers I guess ;-). This is the kind of nonsense that I absolutely won't stand for. Am I a less important customer than a newcomer? Hell, if anything, I'm the _more_ important customer because I spend time improving git while a newcomer makes no contribution whatsoever. In my opinion, the most important customers of git are (in this order of precedence): 1. Developers who hack on git to make it better. This means that the implementation must have a pleasing consistency, and end-user expectations of UI are secondary. For some reason, Junio seems to disagree with this. 2. Advanced users hacking on projects that demand effective use of git like linux.git and git.git, as opposed to some little project on GitHub that just accepts pull requests. 3. Newcomers. I don't develop git for newcomers. I develop git for myself, and scratch my personal itches. The most important customer to me is myself, and everyone else is secondary. That said, I don't feel strongly about this particular advice.ui issue, and Jeff/ Junio have presented a reasonably cogent argument. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] Test the Git version string
From: Junio C Hamano gits...@pobox.com Sent: Monday, April 15, 2013 2:39 AM Philip Oakley philipoak...@iee.org writes: If the parsing is done for white/blacklist purposes, is there a need to straight-jacket the verison string format like this series? The purpose is to document what we felt we could guarantee, and that which was open to variation, so that those, like the Git-Gui, can code in a sensible check for the version. Two digits (X.Y) should pass the existing Git Gui check. I'll drop the length limit, and keep to an X.Y check Is the end of t-basic.sh a sensible place for the check? Sorry, but I still do not understand what you are trying to achieve. What kind of benefit are you envisioning out of this? The purpose of tests is to detect mistakes and spot regressions. A change to the 'git version X.Y.z' string would be a regression, as I spotted earlier, as it conflicts with expectations of standard programmes such as git-gui. For a future version, people would not know what incompatibilities it would introduce, so case $(git version) in git verison[2-9].*) echo unsupported version exit 1 ;; esac is a nonsense check. For all released versions, people know how they looked like and we do not have anything further to specify. Git 1.5.0 will forever identify itself as: $ git version git version 1.5.0 Worse yet, for an untagged version, you may get something like git version 1.8.2.1-515-g78d2372 However, if it passes the test [all the tests], one expects it will be reasonably (almost completely) compatibility with external expectations, such as those of git gui. The questions I'm posing is from the other direction - use of tests for quality control. and it may or may not behave the same way as 1.8.2.1 depending on what trait you are interested in. That will depend on the tests if [deliberately?] failed. I'll tidy up the patches and commit meesage and see how it looks then. 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: Ensimag students projects, version 2013
Junio C Hamano wrote: I'd hate to see any Git developers running with advice turned off for this exact reason. Improving advice is your itch, but it is certainly not my itch. I don't want to see messages like Commit your changes or stash them, or try --continue | --skip | --abort cluttering up my valuable terminal output when I know what to do. I don't care how they can be made better, because I don't want to see them in the first place. You should have nothing against me for running with all advice turned off. It's not your job to dictate how a software should be used, but rather design it so that intended usage is the most intuitive. -- 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] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Jeff King wrote: So there is some information that is per-clone (the objects, the remote tips), but there is some information that is per-submodule (where our local branches are, the index, the worktree). I can see why it is advantageous to share the per-clone information between similar clones (because it avoids disk space and network transfer). But I do not think you can escape having some form of per-submodule repo, even if it is a thin git-new-workdir-ish repo that points back to a parent repo for the clone. I want the flexibility to do the following: 1. Do a simple clone, where the clone contains the GITDIR embedded in the worktree. This is the most common case, and there is no reason to complicate it. I can optionally attach additional workdirs to this clone. I can also optionally relocate the GITDIR at a later date, if I feel the need to do so. 2. Attach a worktree to any object store without having to write a gitfile and set core.worktree by hand. The limitation is that you can't have two submodules from two different superprojects sharing the same object store (since both of them are worktrees). However, for the purpose of working on the submodule repository as an independent repository (this is a very common case for me), I can attach a new workdir to the GITDIR very easily. 3. Attach multiple submodules to the same object store. This will require maintaining a separate index, HEAD and logs/HEAD (aka. workdir) for each additional submodule (the first one doesn't need it) in .git/modules of the superproject. Is there some part of your proposal that I am missing? It seems like you would still need one/.git/modules/foo for this thin repo. You're talking about #3, while I'm still working on #2. And why do you want to use a hammer again if I don't want to share the same object store with multiple submodules? This .git/modules/name is completely optional, and is only required for the _second_ submodule onwards that I'm attaching to the same object store. And by the way, I am actually not sure that such a shared-object setup is a good idea, but only that _if_ you are going to do it with submodules, you might as well do it for all repos. In theory, it is not that hard to have a big per-user object-only repository (either for all repos, or for related ones). But we can do that already with git clone -s, and people do not generally bother, because the maintenance is very tricky (especially dealing with reachability and pruning). No, no. I'm against dumping objects from all repositories into one giant object store. That's a sledgehammer solution, while I'm looking for control and flexibility. Moreover, it has lots of downsides, as you already pointed out. I am open to the argument that solving it in a specific case (submodules) lets us make assumptions that simplify the problem from the general case, but I do not offhand see how it would be any easier in this case. So my proposal is to build a new first-class tool to make manipulations in #1, #2 and #3 easily possible. The first step is to formalize the names bare worktree (which refers to a worktree with a gitfile), worktree (which refers to a worktree with a GITDIR embedded in it), and workdir (which refers to a worktree with a thin GITDIR). The reason I want to build it for submodules first is because the non-submodule case (#2) is simply a reduced case of the submodule case (#3): - When I attempt to attach a new worktree to an existing GITDIR with a worktree attached, I will create a workdir instead. This simply involves creating a thin .git directory in the worktree in the non-submodule case. In the submodule case, it is more complicated: I have to locate the superproject's .git directory, and put it there. -- 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] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Marc Branchaud wrote: If git add is all about specifying what lives under paths in the worktree, what's wrong with letting git add go beyond specifying just files? Syntax aside for the moment, I think a command like git add git-repo-reference foo is perfectly natural: It specifies what is inside worktree path foo. I never said just files. Files, directories, symlinks and submodules are all things in the worktree, and all fine. Remote URLs, on the other hand, have nothing to do with the worktree. -- 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] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Junio C Hamano wrote: It does not relieve git add (or git submodulea add) from the responsibility of moving .git directory. It only reduces the need to do so. When the user says add and the repository has .git directory in it, add (or submodule add) is still responsible for relocating it. Since you're so stubborn about it, I suppose 'git add' could call a function in my new first-class program to attach detach worktrees/workdirs and relocate GITDIRs as a last resort (if the user somehow managed to put a GITDIR in the submodule worktree despite our well-designed tools). But last resort is not what we should be discussing now: we're discussing what the design should ideally be. And ideally, I think we both agree that it's best if init/clone did the relocation. -- 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 v3 00/13] nd/pretty-formats
I've been updating this series on and off over a long span of time, I don't think I remember exactly what changes I've made. But basically I think I have covered all comments from v2. The only semantics change is %C(auto) now turns auto coloring on for all following placeholders until another valid %C is encountered. Nguyễn Thái Ngọc Duy (13): pretty: save commit encoding from logmsg_reencode if the caller needs it pretty: get the correct encoding for --pretty:format=%e pretty-formats.txt: wrap long lines pretty: share code between format_decoration and show_decorations utf8.c: move display_mode_esc_sequence_len() for use by other functions utf8.c: add utf8_strnwidth() with the ability to skip ansi sequences utf8.c: add reencode_string_len() that can handle NULs in string pretty: two phase conversion for non utf-8 commits pretty: split color parsing into a separate function pretty: add %C(auto) for auto-coloring pretty: support padding placeholders, % % and % pretty: support truncating in %, % and % pretty: support % that steal trailing spaces Documentation/pretty-formats.txt | 35 +++- builtin/blame.c | 2 +- builtin/commit.c | 2 +- commit.h | 1 + compat/precompose_utf8.c | 2 +- log-tree.c | 48 -- log-tree.h | 1 + pretty.c | 349 --- revision.c | 2 +- t/t4205-log-pretty-formats.sh| 179 t/t4207-log-decoration-colors.sh | 8 +- t/t6006-rev-list-format.sh | 12 +- utf8.c | 104 +--- utf8.h | 23 ++- 14 files changed, 644 insertions(+), 124 deletions(-) -- 1.8.2.82.gc24b958 -- 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 v3 01/13] pretty: save commit encoding from logmsg_reencode if the caller needs it
The commit encoding is parsed by logmsg_reencode, there's no need for
the caller to re-parse it again. The reencoded message now has the new
encoding, not the original one. The caller would need to read commit
object again before parsing.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/blame.c | 2 +-
builtin/commit.c | 2 +-
commit.h | 1 +
pretty.c | 16
revision.c | 2 +-
5 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 86100e9..104a948 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1425,7 +1425,7 @@ static void get_commit_info(struct commit *commit,
commit_info_init(ret);
encoding = get_log_output_encoding();
- message = logmsg_reencode(commit, encoding);
+ message = logmsg_reencode(commit, NULL, encoding);
get_ac_line(message, \nauthor ,
ret-author, ret-author_mail,
ret-author_time, ret-author_tz);
diff --git a/builtin/commit.c b/builtin/commit.c
index 4620437..d2f30d9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -955,7 +955,7 @@ static const char *read_commit_message(const char *name)
if (!commit)
die(_(could not lookup commit %s), name);
out_enc = get_commit_output_encoding();
- return logmsg_reencode(commit, out_enc);
+ return logmsg_reencode(commit, NULL, out_enc);
}
static int parse_and_validate_options(int argc, const char *argv[],
diff --git a/commit.h b/commit.h
index 87b4b6c..ad55213 100644
--- a/commit.h
+++ b/commit.h
@@ -101,6 +101,7 @@ struct userformat_want {
extern int has_non_ascii(const char *text);
struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
extern char *logmsg_reencode(const struct commit *commit,
+char **commit_encoding,
const char *output_encoding);
extern void logmsg_free(char *msg, const struct commit *commit);
extern void get_commit_format(const char *arg, struct rev_info *);
diff --git a/pretty.c b/pretty.c
index d3a82d2..c361b9b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -594,6 +594,7 @@ static char *replace_encoding_header(char *buf, const char
*encoding)
}
char *logmsg_reencode(const struct commit *commit,
+ char **commit_encoding,
const char *output_encoding)
{
static const char *utf8 = UTF-8;
@@ -615,9 +616,15 @@ char *logmsg_reencode(const struct commit *commit,
sha1_to_hex(commit-object.sha1), typename(type));
}
- if (!output_encoding || !*output_encoding)
+ if (!output_encoding || !*output_encoding) {
+ if (commit_encoding)
+ *commit_encoding =
+ get_header(commit, msg, encoding);
return msg;
+ }
encoding = get_header(commit, msg, encoding);
+ if (commit_encoding)
+ *commit_encoding = encoding;
use_encoding = encoding ? encoding : utf8;
if (same_encoding(use_encoding, output_encoding)) {
/*
@@ -658,7 +665,8 @@ char *logmsg_reencode(const struct commit *commit,
if (out)
out = replace_encoding_header(out, output_encoding);
- free(encoding);
+ if (!commit_encoding)
+ free(encoding);
/*
* If the re-encoding failed, out might be NULL here; in that
* case we just return the commit message verbatim.
@@ -1288,7 +1296,7 @@ void format_commit_message(const struct commit *commit,
context.commit = commit;
context.pretty_ctx = pretty_ctx;
context.wrap_start = sb-len;
- context.message = logmsg_reencode(commit, output_enc);
+ context.message = logmsg_reencode(commit, NULL, output_enc);
strbuf_expand(sb, format, format_commit_item, context);
rewrap_message_tail(sb, context, 0, 0, 0);
@@ -1451,7 +1459,7 @@ void pretty_print_commit(const struct
pretty_print_context *pp,
}
encoding = get_log_output_encoding();
- msg = reencoded = logmsg_reencode(commit, encoding);
+ msg = reencoded = logmsg_reencode(commit, NULL, encoding);
if (pp-fmt == CMIT_FMT_ONELINE || pp-fmt == CMIT_FMT_EMAIL)
indent = 0;
diff --git a/revision.c b/revision.c
index 71e62d8..2e77397 100644
--- a/revision.c
+++ b/revision.c
@@ -2314,7 +2314,7 @@ static int commit_match(struct commit *commit, struct
rev_info *opt)
* in it.
*/
encoding = get_log_output_encoding();
- message = logmsg_reencode(commit, encoding);
+ message = logmsg_reencode(commit, NULL, encoding);
/* Copy the commit to temporary if we are using fake headers */
if (buf.len)
--
1.8.2.82.gc24b958
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
[PATCH v3 02/13] pretty: get the correct encoding for --pretty:format=%e
parse_commit_header() provides the commit encoding for '%e' and it
reads it from the re-encoded message, which contains the new encoding,
not the original one in the commit object. This never happens because
--pretty=format:xxx never respects i18n.logoutputencoding. But that's
a different story.
Get the commit encoding from logmsg_reencode() instead.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
pretty.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/pretty.c b/pretty.c
index c361b9b..e59688b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -776,12 +776,12 @@ struct format_commit_context {
unsigned commit_message_parsed:1;
struct signature_check signature_check;
char *message;
+ char *commit_encoding;
size_t width, indent1, indent2;
/* These offsets are relative to the start of the commit message. */
struct chunk author;
struct chunk committer;
- struct chunk encoding;
size_t message_off;
size_t subject_off;
size_t body_off;
@@ -828,9 +828,6 @@ static void parse_commit_header(struct
format_commit_context *context)
} else if (!prefixcmp(msg + i, committer )) {
context-committer.off = i + 10;
context-committer.len = eol - i - 10;
- } else if (!prefixcmp(msg + i, encoding )) {
- context-encoding.off = i + 9;
- context-encoding.len = eol - i - 9;
}
i = eol;
}
@@ -1185,7 +1182,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
msg + c-committer.off, c-committer.len,
c-pretty_ctx-date_mode);
case 'e': /* encoding */
- strbuf_add(sb, msg + c-encoding.off, c-encoding.len);
+ if (c-commit_encoding)
+ strbuf_addstr(sb, c-commit_encoding);
return 1;
case 'B': /* raw body */
/* message_off is always left at the initial newline */
@@ -1296,11 +1294,14 @@ void format_commit_message(const struct commit *commit,
context.commit = commit;
context.pretty_ctx = pretty_ctx;
context.wrap_start = sb-len;
- context.message = logmsg_reencode(commit, NULL, output_enc);
+ context.message = logmsg_reencode(commit,
+ context.commit_encoding,
+ output_enc);
strbuf_expand(sb, format, format_commit_item, context);
rewrap_message_tail(sb, context, 0, 0, 0);
+ free(context.commit_encoding);
logmsg_free(context.message, commit);
free(context.signature_check.gpg_output);
free(context.signature_check.signer);
--
1.8.2.82.gc24b958
--
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 v3 03/13] pretty-formats.txt: wrap long lines
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/pretty-formats.txt | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index afac703..6bde67e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -106,18 +106,22 @@ The placeholders are:
- '%P': parent hashes
- '%p': abbreviated parent hashes
- '%an': author name
-- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1] or
linkgit:git-blame[1])
+- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1]
+ or linkgit:git-blame[1])
- '%ae': author email
-- '%aE': author email (respecting .mailmap, see linkgit:git-shortlog[1] or
linkgit:git-blame[1])
+- '%aE': author email (respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
- '%ad': author date (format respects --date= option)
- '%aD': author date, RFC2822 style
- '%ar': author date, relative
- '%at': author date, UNIX timestamp
- '%ai': author date, ISO 8601 format
- '%cn': committer name
-- '%cN': committer name (respecting .mailmap, see linkgit:git-shortlog[1] or
linkgit:git-blame[1])
+- '%cN': committer name (respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
- '%ce': committer email
-- '%cE': committer email (respecting .mailmap, see linkgit:git-shortlog[1] or
linkgit:git-blame[1])
+- '%cE': committer email (respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
- '%cd': committer date
- '%cD': committer date, RFC2822 style
- '%cr': committer date, relative
@@ -138,9 +142,11 @@ The placeholders are:
- '%gD': reflog selector, e.g., `refs/stash@{1}`
- '%gd': shortened reflog selector, e.g., `stash@{1}`
- '%gn': reflog identity name
-- '%gN': reflog identity name (respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
+- '%gN': reflog identity name (respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
- '%ge': reflog identity email
-- '%gE': reflog identity email (respecting .mailmap, see
linkgit:git-shortlog[1] or linkgit:git-blame[1])
+- '%gE': reflog identity email (respecting .mailmap, see
+ linkgit:git-shortlog[1] or linkgit:git-blame[1])
- '%gs': reflog subject
- '%Cred': switch color to red
- '%Cgreen': switch color to green
--
1.8.2.82.gc24b958
--
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 v3 04/13] pretty: share code between format_decoration and show_decorations
This also adds color support to format_decorations()
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
log-tree.c | 48 ++--
log-tree.h | 1 +
pretty.c | 20 ++---
t/t4207-log-decoration-colors.sh | 8 +++
4 files changed, 39 insertions(+), 38 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 7cc7d59..1946e9c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -175,36 +175,52 @@ static void show_children(struct rev_info *opt, struct
commit *commit, int abbre
}
}
-void show_decorations(struct rev_info *opt, struct commit *commit)
+/*
+ * The caller makes sure there is no funny color before
+ * calling. format_decorations makes sure the same after return.
+ */
+void format_decorations(struct strbuf *sb,
+ const struct commit *commit,
+ int use_color)
{
const char *prefix;
struct name_decoration *decoration;
const char *color_commit =
- diff_get_color_opt(opt-diffopt, DIFF_COMMIT);
+ diff_get_color(use_color, DIFF_COMMIT);
const char *color_reset =
- decorate_get_color_opt(opt-diffopt, DECORATION_NONE);
+ decorate_get_color(use_color, DECORATION_NONE);
- if (opt-show_source commit-util)
- printf(\t%s, (char *) commit-util);
- if (!opt-show_decorations)
- return;
decoration = lookup_decoration(name_decoration, commit-object);
if (!decoration)
return;
prefix = (;
while (decoration) {
- printf(%s, prefix);
- fputs(decorate_get_color_opt(opt-diffopt, decoration-type),
- stdout);
+ strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, prefix);
+ strbuf_addstr(sb, decorate_get_color(use_color,
decoration-type));
if (decoration-type == DECORATION_REF_TAG)
- fputs(tag: , stdout);
- printf(%s, decoration-name);
- fputs(color_reset, stdout);
- fputs(color_commit, stdout);
+ strbuf_addstr(sb, tag: );
+ strbuf_addstr(sb, decoration-name);
+ strbuf_addstr(sb, color_reset);
prefix = , ;
decoration = decoration-next;
}
- putchar(')');
+ strbuf_addstr(sb, color_commit);
+ strbuf_addch(sb, ')');
+ strbuf_addstr(sb, color_reset);
+}
+
+void show_decorations(struct rev_info *opt, struct commit *commit)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ if (opt-show_source commit-util)
+ printf(\t%s, (char *) commit-util);
+ if (!opt-show_decorations)
+ return;
+ format_decorations(sb, commit, opt-diffopt.use_color);
+ fputs(sb.buf, stdout);
+ strbuf_release(sb);
}
static unsigned int digits_in_number(unsigned int number)
@@ -540,8 +556,8 @@ void show_log(struct rev_info *opt)
printf( (from %s),
find_unique_abbrev(parent-object.sha1,
abbrev_commit));
+ fputs(diff_get_color_opt(opt-diffopt, DIFF_RESET), stdout);
show_decorations(opt, commit);
- printf(%s, diff_get_color_opt(opt-diffopt, DIFF_RESET));
if (opt-commit_format == CMIT_FMT_ONELINE) {
putchar(' ');
} else {
diff --git a/log-tree.h b/log-tree.h
index 9140f48..d6ecd4d 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,6 +13,7 @@ int log_tree_diff_flush(struct rev_info *);
int log_tree_commit(struct rev_info *, struct commit *);
int log_tree_opt_parse(struct rev_info *, const char **, int);
void show_log(struct rev_info *opt);
+void format_decorations(struct strbuf *sb, const struct commit *commit, int
use_color);
void show_decorations(struct rev_info *opt, struct commit *commit);
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **subject_p,
diff --git a/pretty.c b/pretty.c
index e59688b..e0f93ba 100644
--- a/pretty.c
+++ b/pretty.c
@@ -908,23 +908,6 @@ static void parse_commit_message(struct
format_commit_context *c)
c-commit_message_parsed = 1;
}
-static void format_decoration(struct strbuf *sb, const struct commit *commit)
-{
- struct name_decoration *d;
- const char *prefix = (;
-
- load_ref_decorations(DECORATE_SHORT_REFS);
- d = lookup_decoration(name_decoration, commit-object);
- while (d) {
- strbuf_addstr(sb, prefix);
- prefix = , ;
- strbuf_addstr(sb, d-name);
- d = d-next;
- }
- if (prefix[0] == ',')
- strbuf_addch(sb, ')');
-}
-
static void strbuf_wrap(struct strbuf *sb,
[PATCH v3 05/13] utf8.c: move display_mode_esc_sequence_len() for use by other functions
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
utf8.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/utf8.c b/utf8.c
index 7f64857..6ed93c3 100644
--- a/utf8.c
+++ b/utf8.c
@@ -9,6 +9,20 @@ struct interval {
int last;
};
+static size_t display_mode_esc_sequence_len(const char *s)
+{
+ const char *p = s;
+ if (*p++ != '\033')
+ return 0;
+ if (*p++ != '[')
+ return 0;
+ while (isdigit(*p) || *p == ';')
+ p++;
+ if (*p++ != 'm')
+ return 0;
+ return p - s;
+}
+
/* auxiliary function for binary search in interval table */
static int bisearch(ucs_char_t ucs, const struct interval *table, int max)
{
@@ -303,20 +317,6 @@ static void strbuf_add_indented_text(struct strbuf *buf,
const char *text,
}
}
-static size_t display_mode_esc_sequence_len(const char *s)
-{
- const char *p = s;
- if (*p++ != '\033')
- return 0;
- if (*p++ != '[')
- return 0;
- while (isdigit(*p) || *p == ';')
- p++;
- if (*p++ != 'm')
- return 0;
- return p - s;
-}
-
/*
* Wrap the text, if necessary. The variable indent is the indent for the
* first line, indent2 is the indent for all other lines.
--
1.8.2.82.gc24b958
--
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 v3 06/13] utf8.c: add utf8_strnwidth() with the ability to skip ansi sequences
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
utf8.c | 20 ++--
utf8.h | 1 +
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/utf8.c b/utf8.c
index 6ed93c3..9df087f 100644
--- a/utf8.c
+++ b/utf8.c
@@ -266,18 +266,26 @@ int utf8_width(const char **start, size_t *remainder_p)
* string, assuming that the string is utf8. Returns strlen() instead
* if the string does not look like a valid utf8 string.
*/
-int utf8_strwidth(const char *string)
+int utf8_strnwidth(const char *string, int len, int skip_ansi)
{
int width = 0;
const char *orig = string;
- while (1) {
- if (!string)
- return strlen(orig);
- if (!*string)
- return width;
+ if (len == -1)
+ len = strlen(string);
+ while (string string orig + len) {
+ int skip;
+ while (skip_ansi
+ (skip = display_mode_esc_sequence_len(string)))
+ string += skip;
width += utf8_width(string, NULL);
}
+ return string ? width : len;
+}
+
+int utf8_strwidth(const char *string)
+{
+ return utf8_strnwidth(string, -1, 0);
}
int is_utf8(const char *text)
diff --git a/utf8.h b/utf8.h
index 1f8ecad..d3da96f 100644
--- a/utf8.h
+++ b/utf8.h
@@ -4,6 +4,7 @@
typedef unsigned int ucs_char_t; /* assuming 32bit int */
int utf8_width(const char **start, size_t *remainder_p);
+int utf8_strnwidth(const char *string, int len, int skip_ansi);
int utf8_strwidth(const char *string);
int is_utf8(const char *text);
int is_encoding_utf8(const char *name);
--
1.8.2.82.gc24b958
--
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 v3 07/13] utf8.c: add reencode_string_len() that can handle NULs in string
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
compat/precompose_utf8.c | 2 +-
utf8.c | 10 +++---
utf8.h | 19 ---
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 8cf5955..d9203d0 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -78,7 +78,7 @@ void precompose_argv(int argc, const char **argv)
size_t namelen;
oldarg = argv[i];
if (has_non_ascii(oldarg, (size_t)-1, namelen)) {
- newarg = reencode_string_iconv(oldarg, namelen,
ic_precompose);
+ newarg = reencode_string_iconv(oldarg, namelen,
ic_precompose, NULL);
if (newarg)
argv[i] = newarg;
}
diff --git a/utf8.c b/utf8.c
index 9df087f..ac630bc 100644
--- a/utf8.c
+++ b/utf8.c
@@ -468,7 +468,7 @@ int utf8_fprintf(FILE *stream, const char *format, ...)
#else
typedef char * iconv_ibp;
#endif
-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv)
+char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int
*outsz_p)
{
size_t outsz, outalloc;
char *out, *outpos;
@@ -502,13 +502,17 @@ char *reencode_string_iconv(const char *in, size_t insz,
iconv_t conv)
}
else {
*outpos = '\0';
+ if (outsz_p)
+ *outsz_p = outpos - out;
break;
}
}
return out;
}
-char *reencode_string(const char *in, const char *out_encoding, const char
*in_encoding)
+char *reencode_string_len(const char *in, int insz,
+ const char *out_encoding, const char *in_encoding,
+ int *outsz)
{
iconv_t conv;
char *out;
@@ -534,7 +538,7 @@ char *reencode_string(const char *in, const char
*out_encoding, const char *in_e
return NULL;
}
- out = reencode_string_iconv(in, strlen(in), conv);
+ out = reencode_string_iconv(in, insz, conv, outsz);
iconv_close(conv);
return out;
}
diff --git a/utf8.h b/utf8.h
index d3da96f..a43ef9a 100644
--- a/utf8.h
+++ b/utf8.h
@@ -17,12 +17,25 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const
char *data, int len,
int indent, int indent2, int width);
#ifndef NO_ICONV
-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv);
-char *reencode_string(const char *in, const char *out_encoding, const char
*in_encoding);
+char *reencode_string_iconv(const char *in, size_t insz,
+ iconv_t conv, int *outsz);
+char *reencode_string_len(const char *in, int insz,
+ const char *out_encoding,
+ const char *in_encoding,
+ int *outsz);
#else
-#define reencode_string(a,b,c) NULL
+#define reencode_string_len(a,b,c,d,e) NULL
#endif
+static inline char *reencode_string(const char *in,
+ const char *out_encoding,
+ const char *in_encoding)
+{
+ return reencode_string_len(in, strlen(in),
+ out_encoding, in_encoding,
+ NULL);
+}
+
int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
#endif
--
1.8.2.82.gc24b958
--
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 v3 08/13] pretty: two phase conversion for non utf-8 commits
Always assume format_commit_item() takes an utf-8 string for string
handling simplicity (we can handle utf-8 strings, but can't with other
encodings).
If commit message is in non-utf8, or output encoding is not, then the
commit is first converted to utf-8, processed, then output converted
to output encoding. This of course only works with encodings that are
compatible with Unicode.
This also fixes the iso8859-1 test in t6006. It's supposed to create
an iso8859-1 commit, but the commit content in t6006 is in UTF-8.
t6006 is now converted back in UTF-8 (the downside is we can't put
utf-8 strings there anymore).
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
pretty.c | 24 ++--
t/t6006-rev-list-format.sh | 12 ++--
2 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/pretty.c b/pretty.c
index e0f93ba..5947275 100644
--- a/pretty.c
+++ b/pretty.c
@@ -954,7 +954,8 @@ static int format_reflog_person(struct strbuf *sb,
return format_person_part(sb, part, ident, strlen(ident), dmode);
}
-static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
+static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
+ const char *placeholder,
void *context)
{
struct format_commit_context *c = context;
@@ -1193,7 +1194,8 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
return 0; /* unknown placeholder */
}
-static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
+static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
+const char *placeholder,
void *context)
{
int consumed;
@@ -1273,6 +1275,7 @@ void format_commit_message(const struct commit *commit,
{
struct format_commit_context context;
const char *output_enc = pretty_ctx-output_encoding;
+ const char *utf8 = UTF-8;
memset(context, 0, sizeof(context));
context.commit = commit;
@@ -1285,6 +1288,23 @@ void format_commit_message(const struct commit *commit,
strbuf_expand(sb, format, format_commit_item, context);
rewrap_message_tail(sb, context, 0, 0, 0);
+ if (output_enc) {
+ if (same_encoding(utf8, output_enc))
+ output_enc = NULL;
+ } else {
+ if (context.commit_encoding
+ !same_encoding(context.commit_encoding, utf8))
+ output_enc = context.commit_encoding;
+ }
+
+ if (output_enc) {
+ int outsz;
+ char *out = reencode_string_len(sb-buf, sb-len,
+ output_enc, utf8, outsz);
+ if (out)
+ strbuf_attach(sb, out, outsz, outsz + 1);
+ }
+
free(context.commit_encoding);
logmsg_free(context.message, commit);
free(context.signature_check.gpg_output);
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 3fc3b74..0393c9f 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -184,7 +184,7 @@ Test printing of complex bodies
This commit message is much longer than the others,
and it will be encoded in iso8859-1. We should therefore
-include an iso8859 character: ¡bueno!
+include an iso8859 character: �bueno!
EOF
test_expect_success 'setup complex body' '
git config i18n.commitencoding iso8859-1
@@ -192,14 +192,14 @@ git config i18n.commitencoding iso8859-1
'
test_format complex-encoding %e 'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
iso8859-1
commit 131a310eb913d107dd3c09a65d1651175898735d
commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
EOF
test_format complex-subject %s 'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
Test printing of complex bodies
commit 131a310eb913d107dd3c09a65d1651175898735d
changed foo
@@ -208,17 +208,17 @@ added foo
EOF
test_format complex-body %b 'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
This commit message is much longer than the others,
and it will be encoded in iso8859-1. We should therefore
-include an iso8859 character: ¡bueno!
+include an iso8859 character: �bueno!
commit 131a310eb913d107dd3c09a65d1651175898735d
commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
EOF
test_expect_success '%x00 shows NUL' '
- echo expect commit f58db70b055c5718631e5c61528b28b12090cdea
+ echo expect commit 1ed88da4a5b5ed8c449114ac131efc62178734c3
echo expect fooQbar
git rev-list -1 --format=foo%x00bar HEAD actual.nul
nul_to_q actual.nul actual
--
1.8.2.82.gc24b958
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a
[PATCH v3 09/13] pretty: split color parsing into a separate function
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
pretty.c | 71 +++-
1 file changed, 39 insertions(+), 32 deletions(-)
diff --git a/pretty.c b/pretty.c
index 5947275..e0413e3 100644
--- a/pretty.c
+++ b/pretty.c
@@ -954,6 +954,44 @@ static int format_reflog_person(struct strbuf *sb,
return format_person_part(sb, part, ident, strlen(ident), dmode);
}
+static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
+ const char *placeholder,
+ struct format_commit_context *c)
+{
+ if (placeholder[1] == '(') {
+ const char *begin = placeholder + 2;
+ const char *end = strchr(begin, ')');
+ char color[COLOR_MAXLEN];
+
+ if (!end)
+ return 0;
+ if (!prefixcmp(begin, auto,)) {
+ if (!want_color(c-pretty_ctx-color))
+ return end - placeholder + 1;
+ begin += 5;
+ }
+ color_parse_mem(begin,
+ end - begin,
+ --pretty format, color);
+ strbuf_addstr(sb, color);
+ return end - placeholder + 1;
+ }
+ if (!prefixcmp(placeholder + 1, red)) {
+ strbuf_addstr(sb, GIT_COLOR_RED);
+ return 4;
+ } else if (!prefixcmp(placeholder + 1, green)) {
+ strbuf_addstr(sb, GIT_COLOR_GREEN);
+ return 6;
+ } else if (!prefixcmp(placeholder + 1, blue)) {
+ strbuf_addstr(sb, GIT_COLOR_BLUE);
+ return 5;
+ } else if (!prefixcmp(placeholder + 1, reset)) {
+ strbuf_addstr(sb, GIT_COLOR_RESET);
+ return 6;
+ } else
+ return 0;
+}
+
static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -967,38 +1005,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
/* these are independent of the commit */
switch (placeholder[0]) {
case 'C':
- if (placeholder[1] == '(') {
- const char *begin = placeholder + 2;
- const char *end = strchr(begin, ')');
- char color[COLOR_MAXLEN];
-
- if (!end)
- return 0;
- if (!prefixcmp(begin, auto,)) {
- if (!want_color(c-pretty_ctx-color))
- return end - placeholder + 1;
- begin += 5;
- }
- color_parse_mem(begin,
- end - begin,
- --pretty format, color);
- strbuf_addstr(sb, color);
- return end - placeholder + 1;
- }
- if (!prefixcmp(placeholder + 1, red)) {
- strbuf_addstr(sb, GIT_COLOR_RED);
- return 4;
- } else if (!prefixcmp(placeholder + 1, green)) {
- strbuf_addstr(sb, GIT_COLOR_GREEN);
- return 6;
- } else if (!prefixcmp(placeholder + 1, blue)) {
- strbuf_addstr(sb, GIT_COLOR_BLUE);
- return 5;
- } else if (!prefixcmp(placeholder + 1, reset)) {
- strbuf_addstr(sb, GIT_COLOR_RESET);
- return 6;
- } else
- return 0;
+ return parse_color(sb, placeholder, c);
case 'n': /* newline */
strbuf_addch(sb, '\n');
return 1;
--
1.8.2.82.gc24b958
--
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 v3 10/13] pretty: add %C(auto) for auto-coloring
This is not simply convenient over %C(auto,xxx). Some placeholders
(actually only one, %d) do multi coloring and we can't emit a multiple
colors with %C(auto,xxx).
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/pretty-formats.txt | 3 ++-
pretty.c | 17 +++--
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 6bde67e..bad627a 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -156,7 +156,8 @@ The placeholders are:
adding `auto,` at the beginning will emit color only when colors are
enabled for log output (by `color.diff`, `color.ui`, or `--color`, and
respecting the `auto` settings of the former if we are going to a
- terminal)
+ terminal). `auto` alone (i.e. `%C(auto)`) will turn on auto coloring
+ on the next placeholders until the color is switched again.
- '%m': left, right or boundary mark
- '%n': newline
- '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index e0413e3..f385176 100644
--- a/pretty.c
+++ b/pretty.c
@@ -778,6 +778,7 @@ struct format_commit_context {
char *message;
char *commit_encoding;
size_t width, indent1, indent2;
+ int auto_color_next;
/* These offsets are relative to the start of the commit message. */
struct chunk author;
@@ -1005,7 +1006,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
/* these are independent of the commit */
switch (placeholder[0]) {
case 'C':
- return parse_color(sb, placeholder, c);
+ if (!prefixcmp(placeholder + 1, (auto))) {
+ c-auto_color_next = 1;
+ return 7;
+ } else {
+ int ret = parse_color(sb, placeholder, c);
+ if (ret)
+ c-auto_color_next = 0;
+ return ret;
+ }
case 'n': /* newline */
strbuf_addch(sb, '\n');
return 1;
@@ -1051,13 +1060,17 @@ static size_t format_commit_one(struct strbuf *sb, /*
in UTF-8 */
switch (placeholder[0]) {
case 'H': /* commit hash */
+ strbuf_addstr(sb, diff_get_color(c-auto_color_next,
DIFF_COMMIT));
strbuf_addstr(sb, sha1_to_hex(commit-object.sha1));
+ strbuf_addstr(sb, diff_get_color(c-auto_color_next,
DIFF_RESET));
return 1;
case 'h': /* abbreviated commit hash */
+ strbuf_addstr(sb, diff_get_color(c-auto_color_next,
DIFF_COMMIT));
if (add_again(sb, c-abbrev_commit_hash))
return 1;
strbuf_addstr(sb, find_unique_abbrev(commit-object.sha1,
c-pretty_ctx-abbrev));
+ strbuf_addstr(sb, diff_get_color(c-auto_color_next,
DIFF_RESET));
c-abbrev_commit_hash.len = sb-len - c-abbrev_commit_hash.off;
return 1;
case 'T': /* tree hash */
@@ -1095,7 +1108,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
return 1;
case 'd':
load_ref_decorations(DECORATE_SHORT_REFS);
- format_decorations(sb, commit, 0);
+ format_decorations(sb, commit, c-auto_color_next);
return 1;
case 'g': /* reflog info */
switch(placeholder[1]) {
--
1.8.2.82.gc24b958
--
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 v3 11/13] pretty: support padding placeholders, % % and %
Either %, % or % standing before a placeholder specifies how many
columns (at least as the placeholder can exceed it) it takes. Each
differs on how spaces are padded:
% pads on the right (aka left alignment)
% pads on the left (aka right alignment)
% pads both ways equally (aka centered)
The (N) follows them, e.g. `%(100)', to specify the number of
columns the next placeholder takes.
However, if '|' stands before (N), e.g. `%|(100)', then the number
of columns is calculated so that it reaches the Nth column on screen.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/pretty-formats.txt | 8 +++
pretty.c | 117 +++-
t/t4205-log-pretty-formats.sh| 126 +++
3 files changed, 250 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index bad627a..e2345d2 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -164,6 +164,14 @@ The placeholders are:
- '%x00': print a byte from a hex code
- '%w([w[,i1[,i2]]])': switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
+- '%(N)': make the next placeholder take at least N columns,
+ padding spaces on the right if necessary
+- '%|(N)': make the next placeholder take at least until Nth
+ columns, padding spaces on the right if necessary
+- '%(N)', '%|(N)': similar to '%(N)', '%|(N)'
+ respectively, but padding spaces on the left
+- '%(N)', '%|(N)': similar to '%(N)', '%|(N)'
+ respectively, but padding both sides (i.e. the text is centered)
NOTE: Some placeholders may depend on other options given to the
revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index f385176..7debfb2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -769,16 +769,25 @@ struct chunk {
size_t len;
};
+enum flush_type {
+ no_flush,
+ flush_right,
+ flush_left,
+ flush_both
+};
+
struct format_commit_context {
const struct commit *commit;
const struct pretty_print_context *pretty_ctx;
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
struct signature_check signature_check;
+ enum flush_type flush_type;
char *message;
char *commit_encoding;
size_t width, indent1, indent2;
int auto_color_next;
+ int padding;
/* These offsets are relative to the start of the commit message. */
struct chunk author;
@@ -993,6 +1002,52 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8
*/
return 0;
}
+static size_t parse_padding_placeholder(struct strbuf *sb,
+ const char *placeholder,
+ struct format_commit_context *c)
+{
+ const char *ch = placeholder;
+ enum flush_type flush_type;
+ int to_column = 0;
+
+ switch (*ch++) {
+ case '':
+ flush_type = flush_right;
+ break;
+ case '':
+ if (*ch == '') {
+ flush_type = flush_both;
+ ch++;
+ } else
+ flush_type = flush_left;
+ break;
+ default:
+ return 0;
+ }
+
+ /* the next value means wide enough to that column */
+ if (*ch == '|') {
+ to_column = 1;
+ ch++;
+ }
+
+ if (*ch == '(') {
+ const char *start = ch + 1;
+ const char *end = strchr(start, ')');
+ char *next;
+ int width;
+ if (!end || end == start)
+ return 0;
+ width = strtoul(start, next, 10);
+ if (next == start || width == 0)
+ return 0;
+ c-padding = to_column ? -width : width;
+ c-flush_type = flush_type;
+ return end - placeholder + 1;
+ }
+ return 0;
+}
+
static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
void *context)
@@ -1052,6 +1107,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
return end - placeholder + 1;
} else
return 0;
+
+ case '':
+ case '':
+ return parse_padding_placeholder(sb, placeholder, c);
}
/* these depend on the commit */
@@ -1214,6 +1273,59 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
return 0; /* unknown placeholder */
}
+static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
+ const char *placeholder,
+ struct format_commit_context *c)
+{
+
[PATCH v3 12/13] pretty: support truncating in %, % and %
%(N,trunc) truncates the right part after N columns and replace the
last two letters with ... ltrunc does the same on the left. mtrunc
cuts the middle out.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/pretty-formats.txt | 7 --
pretty.c | 51 +---
t/t4205-log-pretty-formats.sh| 39 ++
utf8.c | 46
utf8.h | 2 ++
5 files changed, 140 insertions(+), 5 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e2345d2..d3450bf 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -164,8 +164,11 @@ The placeholders are:
- '%x00': print a byte from a hex code
- '%w([w[,i1[,i2]]])': switch line wrapping, like the -w option of
linkgit:git-shortlog[1].
-- '%(N)': make the next placeholder take at least N columns,
- padding spaces on the right if necessary
+- '%(N[,trunc|ltrunc|mtrunc])': make the next placeholder take at
+ least N columns, padding spaces on the right if necessary.
+ Optionally truncate at the beginning (ltrunc), the middle (mtrunc)
+ or the end (trunc) if the output is longer than N columns.
+ Note that truncating only works correctly with N = 2.
- '%|(N)': make the next placeholder take at least until Nth
columns, padding spaces on the right if necessary
- '%(N)', '%|(N)': similar to '%(N)', '%|(N)'
diff --git a/pretty.c b/pretty.c
index 7debfb2..a6c029c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -776,6 +776,13 @@ enum flush_type {
flush_both
};
+enum trunc_type {
+ trunc_none,
+ trunc_left,
+ trunc_middle,
+ trunc_right
+};
+
struct format_commit_context {
const struct commit *commit;
const struct pretty_print_context *pretty_ctx;
@@ -783,6 +790,7 @@ struct format_commit_context {
unsigned commit_message_parsed:1;
struct signature_check signature_check;
enum flush_type flush_type;
+ enum trunc_type truncate;
char *message;
char *commit_encoding;
size_t width, indent1, indent2;
@@ -1033,7 +1041,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
if (*ch == '(') {
const char *start = ch + 1;
- const char *end = strchr(start, ')');
+ const char *end = start + strcspn(start, ,));
char *next;
int width;
if (!end || end == start)
@@ -1043,6 +1051,23 @@ static size_t parse_padding_placeholder(struct strbuf
*sb,
return 0;
c-padding = to_column ? -width : width;
c-flush_type = flush_type;
+
+ if (*end == ',') {
+ start = end + 1;
+ end = strchr(start, ')');
+ if (!end || end == start)
+ return 0;
+ if (!prefixcmp(start, trunc)))
+ c-truncate = trunc_right;
+ else if (!prefixcmp(start, ltrunc)))
+ c-truncate = trunc_left;
+ else if (!prefixcmp(start, mtrunc)))
+ c-truncate = trunc_middle;
+ else
+ return 0;
+ } else
+ c-truncate = trunc_none;
+
return end - placeholder + 1;
}
return 0;
@@ -1302,9 +1327,29 @@ static size_t format_and_pad_commit(struct strbuf *sb,
/* in UTF-8 */
total_consumed++;
}
len = utf8_strnwidth(local_sb.buf, -1, 1);
- if (len padding)
+ if (len padding) {
+ switch (c-truncate) {
+ case trunc_left:
+ strbuf_utf8_replace(local_sb,
+ 0, len - (padding - 2),
+ ..);
+ break;
+ case trunc_middle:
+ strbuf_utf8_replace(local_sb,
+ padding / 2 - 1,
+ len - (padding - 2),
+ ..);
+ break;
+ case trunc_right:
+ strbuf_utf8_replace(local_sb,
+ padding - 2, len - (padding - 2),
+ ..);
+ break;
+ case trunc_none:
+ break;
+ }
strbuf_addstr(sb, local_sb.buf);
- else {
+ } else {
int sb_len = sb-len, offset = 0;
if (c-flush_type == flush_left)
offset = padding - len;
diff --git
[PATCH v3 13/13] pretty: support % that steal trailing spaces
This is pretty useful in `%(100)%s%Cred%(20)% an' where %s does not
use up all 100 columns and %an needs more than 20 columns. By
replacing %(20) with %(20), %an can steal spaces from %s.
% understands escape sequences, so %Cred does not stop it from
stealing spaces in %(100).
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/pretty-formats.txt | 5 -
pretty.c | 34 ++
t/t4205-log-pretty-formats.sh| 14 ++
utf8.c | 2 +-
utf8.h | 1 +
5 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index d3450bf..c96ff41 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -173,7 +173,10 @@ The placeholders are:
columns, padding spaces on the right if necessary
- '%(N)', '%|(N)': similar to '%(N)', '%|(N)'
respectively, but padding spaces on the left
-- '%(N)', '%|(N)': similar to '%(N)', '%|(N)'
+- '%(N)', '%|(N)': similar to '%(N)', '%|(N)'
+ respectively, except that if the next placeholder takes more spaces
+ than given and there are spaces on its left, use those spaces
+- '%(N)', '%|(N)': similar to '% (N)', '%|(N)'
respectively, but padding both sides (i.e. the text is centered)
NOTE: Some placeholders may depend on other options given to the
diff --git a/pretty.c b/pretty.c
index a6c029c..306ba08 100644
--- a/pretty.c
+++ b/pretty.c
@@ -773,6 +773,7 @@ enum flush_type {
no_flush,
flush_right,
flush_left,
+ flush_left_and_steal,
flush_both
};
@@ -1026,6 +1027,9 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
if (*ch == '') {
flush_type = flush_both;
ch++;
+ } else if (*ch == '') {
+ flush_type = flush_left_and_steal;
+ ch++;
} else
flush_type = flush_left;
break;
@@ -1327,6 +1331,36 @@ static size_t format_and_pad_commit(struct strbuf *sb,
/* in UTF-8 */
total_consumed++;
}
len = utf8_strnwidth(local_sb.buf, -1, 1);
+
+ if (c-flush_type == flush_left_and_steal) {
+ const char *ch = sb-buf + sb-len - 1;
+ while (len padding ch sb-buf) {
+ const char *p;
+ if (*ch == ' ') {
+ ch--;
+ padding++;
+ continue;
+ }
+ /* check for trailing ansi sequences */
+ if (*ch != 'm')
+ break;
+ p = ch - 1;
+ while (ch - p 10 *p != '\033')
+ p--;
+ if (*p != '\033' ||
+ ch + 1 - p != display_mode_esc_sequence_len(p))
+ break;
+ /*
+* got a good ansi sequence, put it back to
+* local_sb as we're cutting sb
+*/
+ strbuf_insert(local_sb, 0, p, ch + 1 - p);
+ ch = p - 1;
+ }
+ strbuf_setlen(sb, ch + 1 - sb-buf);
+ c-flush_type = flush_left;
+ }
+
if (len padding) {
switch (c-truncate) {
case trunc_left:
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d1e9677..d006f76 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -264,4 +264,18 @@ EOF
test_cmp expected actual
'
+test_expect_success 'left/right alignment formatting with stealing' '
+ git commit --amend -m short --author long long long l...@me.com
+ git log --pretty=format:%(10,trunc)%s%(10,ltrunc)% an actual
+ # complete the incomplete line at the end
+ echo actual
+ cat \EOF | delete_trailing_dollar expected
+short long long long$
+message .. A U Thor$
+add bar A U Thor$
+initial A U Thor$
+EOF
+ test_cmp expected actual
+'
+
test_done
diff --git a/utf8.c b/utf8.c
index 0428e76..76e5c04 100644
--- a/utf8.c
+++ b/utf8.c
@@ -9,7 +9,7 @@ struct interval {
int last;
};
-static size_t display_mode_esc_sequence_len(const char *s)
+size_t display_mode_esc_sequence_len(const char *s)
{
const char *p = s;
if (*p++ != '\033')
diff --git a/utf8.h b/utf8.h
index edde8ee..32a7bfb 100644
--- a/utf8.h
+++ b/utf8.h
@@ -3,6 +3,7 @@
typedef unsigned int ucs_char_t; /* assuming 32bit int */
+size_t display_mode_esc_sequence_len(const char *s);
int utf8_width(const char **start, size_t *remainder_p);
int utf8_strnwidth(const char *string, int len, int skip_ansi);
Re: [PATCH v3 07/13] utf8.c: add reencode_string_len() that can handle NULs in string
Torsten,
I can't compile compat/precomposed_utf8.c on linux even though I make
some changes there. Can you check if I break something? I'm pretty
sure I don't, but just in case.
On Tue, Apr 16, 2013 at 6:24 PM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
compat/precompose_utf8.c | 2 +-
utf8.c | 10 +++---
utf8.h | 19 ---
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 8cf5955..d9203d0 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -78,7 +78,7 @@ void precompose_argv(int argc, const char **argv)
size_t namelen;
oldarg = argv[i];
if (has_non_ascii(oldarg, (size_t)-1, namelen)) {
- newarg = reencode_string_iconv(oldarg, namelen,
ic_precompose);
+ newarg = reencode_string_iconv(oldarg, namelen,
ic_precompose, NULL);
if (newarg)
argv[i] = newarg;
}
diff --git a/utf8.c b/utf8.c
index 9df087f..ac630bc 100644
--- a/utf8.c
+++ b/utf8.c
@@ -468,7 +468,7 @@ int utf8_fprintf(FILE *stream, const char *format, ...)
#else
typedef char * iconv_ibp;
#endif
-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv)
+char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int
*outsz_p)
{
size_t outsz, outalloc;
char *out, *outpos;
@@ -502,13 +502,17 @@ char *reencode_string_iconv(const char *in, size_t
insz, iconv_t conv)
}
else {
*outpos = '\0';
+ if (outsz_p)
+ *outsz_p = outpos - out;
break;
}
}
return out;
}
-char *reencode_string(const char *in, const char *out_encoding, const char
*in_encoding)
+char *reencode_string_len(const char *in, int insz,
+ const char *out_encoding, const char *in_encoding,
+ int *outsz)
{
iconv_t conv;
char *out;
@@ -534,7 +538,7 @@ char *reencode_string(const char *in, const char
*out_encoding, const char *in_e
return NULL;
}
- out = reencode_string_iconv(in, strlen(in), conv);
+ out = reencode_string_iconv(in, insz, conv, outsz);
iconv_close(conv);
return out;
}
diff --git a/utf8.h b/utf8.h
index d3da96f..a43ef9a 100644
--- a/utf8.h
+++ b/utf8.h
@@ -17,12 +17,25 @@ void strbuf_add_wrapped_bytes(struct strbuf *buf, const
char *data, int len,
int indent, int indent2, int width);
#ifndef NO_ICONV
-char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv);
-char *reencode_string(const char *in, const char *out_encoding, const char
*in_encoding);
+char *reencode_string_iconv(const char *in, size_t insz,
+ iconv_t conv, int *outsz);
+char *reencode_string_len(const char *in, int insz,
+ const char *out_encoding,
+ const char *in_encoding,
+ int *outsz);
#else
-#define reencode_string(a,b,c) NULL
+#define reencode_string_len(a,b,c,d,e) NULL
#endif
+static inline char *reencode_string(const char *in,
+ const char *out_encoding,
+ const char *in_encoding)
+{
+ return reencode_string_len(in, strlen(in),
+ out_encoding, in_encoding,
+ NULL);
+}
+
int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
#endif
--
1.8.2.82.gc24b958
--
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/RFC] l10n: de.po: translate 39 new messages
Ralf Thielow ralf.thie...@gmail.com writes:
msgid You are currently reverting commit %s.
-msgstr Sie sind gerade bei einer binären Suche in Zweig '%s'.
+msgstr Sie kehren gerade Version '%s' um.
Is revert-umkehren new? I can see it's in the glossary, but
#: sequencer.c:536
#, c-format
msgid could not revert %s... %s
msgstr Konnte %s nicht zurücksetzen... %s
#: sequencer.c:1016
msgid Can't revert as initial commit
msgstr Kann nicht zu initialer Version zurücksetzen.
which I don't really like either now that you mention it -- I would
re-translate it as 'reset'. But either way they should be consistent.
msgid Commit %s has an untrusted GPG signature, allegedly by %s.
-msgstr
+msgstr Version %s hat eine nicht vertrauenswürdige GPG-Signatur,
+vermeintlich von %s.
msgid Commit %s has a bad GPG signature allegedly by %s.
-msgstr
+msgstr Version %s hat eine ungültige GPG-Signatur, vermeintlich von %s.
You could say angeblich instead, which is more to the point.
#: git-submodule.sh:626
-#, fuzzy, sh-format
+#, sh-format
msgid Submodule '$name' ($url) unregistered for path '$sm_path'
-msgstr Unterprojekt '$name' ($url) ist für Pfad '$sm_path' registriert
+msgstr Unterprojekt '$name' ($url) ist nicht für Pfad '$sm_path'
registriert.
This is in cmd_deinit(), and the comment for the enclosing block says
# Remove the whole section so we have a clean state when
# the user later decides to init this submodule again
So it would seem to use unregister as a verb, not an adjective, and
the correct translation is
msgstr Unterprojekt '$name' ($url) für '$sm_path' deregistriert.
or some such. Deregistriert is not very nice; in the absence of
context I would suggest ausgetragen, but that problably collides with
our use of eintragen. Perhaps you can go the long way for this
isolated use and just say aus der Konfiguration entfernt (and
similarly for 'register').
In any case you should also add 'register' and 'unregister' to the
glossary once you've settled on something.
--
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: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Jonathan Nieder wrote: I quite like .git/modules/subproject name (for some reasons that I've mentioned in other threads) and don't consider it nonsense, which makes me assume I don't understand the goal of this patch, either. Please don't take that personally. There's nothing to take personally, Jonathan. We're designing software, and the rationale for choosing a design is never Jonathan personally likes this particular design, so therefore we'll go with it, but rather Ram's design is objectively superior, and therefore we'll go with it. I'll proceed with bashing .git/modules, while your job is to defend it: 1. The path to the object store of a submodule depends upon how deeply it is nested in other submodules, and hence how many /modules/ components to add to the path to the project's name. Presumably, this is to avoid conflicts: but it's an overkill for such a simple job. In the 98% case, I never have two submodules with the same name in my superproject; for the 2% case, I can live with the inconvenience of naming a directory by hand, rather than putting up with this ugliness. 2. This ugliness complicates implementation of add/ rm/ mv, because each of them will have to know about this contrived path solution. 3. The paths in the gitfiles in various submodules is horribly ugly with tons of ../ components. This is especially the case in deeply nested submodules. We can't use an absolute path, because the superproject directory can be moved anywhere in the filesystem. 4. To relocate the object store and reuse it elsewhere is almost impossible. What if I want to remove the submodule, but work on it independently from the superproject? Re-clone? My solution fixes all these problems, and we need .git/modules/name.git (no path-to-submodule nonsense) only as a last resort: #3 (ref: my email to Peff). -- 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] fast-export: add --signed-tags=warn-strip mode
On Mon, Apr 15, 2013 at 09:50:42PM -0700, Sverre Rabbelier wrote: On Mon, Apr 15, 2013 at 9:47 PM, Junio C Hamano gits...@pobox.com wrote: When you see 78 in the output and you know you have 92 tags in the repository, is that sufficient to let you go on, or do we also need an easy way to tell which ones are those 78 that were stripped and the remaining 14 were not stripped? There is no reason to worry about some signed tags are stripped but not others, so it feels that the number alone should be sufficient, I guess. If those remaining 14 weren't stripped, that is (at least at the moment) by definition because they are unsigned, annotated tags. Or because they were not exported? Perhaps 78 tags stripped, 92 exported in total. I think I prefer Jonathan's suggestion to this one if we need to change it. The reason I didn't do this initially was that I assumed that from a remote helper we would, in general, not be pushing any tags which already exist, so the number of tags to push will be small. Printing one message per tag also matches the current behaviour for --signed-tags=warn. I don't want to make the behaviour for warn and warn-strip different, so should warn also print a summary message instead of a message for each tag? -- 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 03/33] refs: document do_for_each_ref() and do_one_ref()
On 04/15/2013 07:38 PM, Junio C Hamano wrote: +/* + * Call fn for each reference in the specified submodule for which the + * refname begins with base. If trim is non-zero, then trim that many + * characters off the beginning of each refname before passing the + * refname to fn. flags can be DO_FOR_EACH_INCLUDE_BROKEN to include + * broken references in the iteration. + */ Early termination due to fn() returning non-zero needs to be documented here, no? static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn, Correct, thanks. Will be fixed in re-roll. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: Because git rebase needs multiple runs in case of conflicts. You have to store the information somewhere in the filesystem, not in a variable. What you need to store is whether you need to unstash, and where you are in the process (in Junio's version, you may be doing the actual rebase, or fixing conflicts in index state application, or fixing conflicts in tree state application, or done). Storing what you have to do and where you are in the process really sounds like a job for the instruction sheet, no? No. Ultimately, the entry point of all these invocations is git-rebase.sh. The plan is to refactor calls from git-rebase.sh to git-rebase--*.sh scripts so that those scripts return control to git-rebase.sh, which will be the final exit point. The logic is very simple: On the very first invocation of rebase (ie. no existing rebase in progress), stash. If the return statement from the specific rebase script is 1 (which means that there are conflicts to be resolved), exit as usual. If it is 0 (which means that the rebase completely successfully), pop the stash before exiting as usual. What's so complicated about that? I'm against leaking the autostash implementation detail into specific rebases, because I value a clean and pleasant implementation over everything else. -- 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 10/33] refs: extract a function ref_resolves_to_object()
On 04/15/2013 06:51 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
It is a nice unit of work and soon will be needed from multiple
locations.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index c523978..dfc8600 100644
--- a/refs.c
+++ b/refs.c
@@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)
#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
/*
+ * Return true iff the reference described by entry can be resolved to
+ * an object in the database. Emit a warning if the referred-to
+ * object does not exist.
+ */
+static int ref_resolves_to_object(struct ref_entry *entry)
+{
+if (entry-flag REF_ISBROKEN)
+return 0;
+if (!has_sha1_file(entry-u.value.sha1)) {
+error(%s does not point to a valid object!, entry-name);
+return 0;
+}
+return 1;
+}
+
+/*
* current_ref is a performance hack: when iterating over references
* using the for_each_ref*() functions, current_ref is set to the
* current reference's entry before calling the callback function. If
@@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn
fn, int trim,
if (prefixcmp(entry-name, base))
return 0;
-if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)) {
-if (entry-flag REF_ISBROKEN)
-return 0; /* ignore broken refs e.g. dangling symref */
-if (!has_sha1_file(entry-u.value.sha1)) {
-error(%s does not point to a valid object!,
entry-name);
-return 0;
-}
-}
+if (!((flags DO_FOR_EACH_INCLUDE_BROKEN) ||
+ ref_resolves_to_object(entry)))
+return 0;
+
The original says Unless flags tell us to include broken ones,
return 0 for the broken ones and the ones that point at invalid
objects.
The updated says Unless flags tell us to include broken ones or the
ref is a good one, return 0.
Are they the same? If we have a good one, and if we are not told to
include broken one, the original just passes the control down to the
remainder of the function. The updated one will return 0.
Oh, wait, that is not the case. The OR is inside !( A || B ), so
what it really wants to say is:
if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)
!ref_resolves_to_object(entry))
return 0;
that is, When we are told to exclude broken ones and the one we are
looking at is broken, return 0.
Am I the only one who was confused with this, or was the way the
patch wrote this logic unnecessarily convoluted?
I find either way of writing it hard to read quickly. I find that the
construct
if (!(situation in which the function should continue))
return
makes it easier to pick out the situation in which the function should
continue. But granted, the nesting of multiple parentheses across
lines compromises the clarity.
In projects where I can choose the coding standard, I like to use extra
whitespace to help the eye pick out the range of parentheses, like
if (!(
(flags DO_FOR_EACH_INCLUDE_BROKEN) ||
ref_resolves_to_object(entry)
))
return 0;
but I've never seen this style in the Git project so I haven't used it here.
Since you prefer the other version, I will change it.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra artag...@gmail.com writes: If it is 0 (which means that the rebase completely successfully), pop the stash before exiting as usual. And you're going to pop the stash even if no stash were triggered when starting the rebase. Really, think about it again: you're not going to guess whether you have to unstash without storing this information somewhere. You may argue about storing it outside the todolist, you can't unstash unconditionally. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 13/33] peel_ref(): fix return value for non-peelable, not-current reference
On 04/15/2013 07:38 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: The old version was inconsistent: when a reference was REF_KNOWS_PEELED but with a null peeled value, it returned non-zero for the current reference but zero for other references. Change the behavior for non-current references to match that of current_ref, which is what callers expect. Document the behavior. Current callers did not trigger the previously-buggy behavior. Is that because we were lucky by codeflow, or is it just that we didn't have a testcase to trigger the behaviour? Existing callers only called peel_ref() from within a for_each_ref-style iteration and only for the current ref. Therefore the buggy code path was impossible to reach. I will note that in the commit message. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy wrote: And you're going to pop the stash even if no stash were triggered when starting the rebase. Really, think about it again: you're not going to guess whether you have to unstash without storing this information somewhere. You may argue about storing it outside the todolist, you can't unstash unconditionally. Yes, touching a simple autostash file at stash-time, and removing it at pop-time will do. I don't see why it should be part of a (potentially user-editable) todo-list, which serves an entirely different purpose. -- 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] read_revisions_from_stdin: make copies for handle_revision_arg
read_revisions_from_stdin() has passed pointers to its read buffer down to handle_revision_arg() since its inception way back in 42cabc3 (Teach rev-list an option to read revs from the standard input., 2006-09-05). Even back then, this was a bug: through add_pending_object, the argument was recorded in the object_array's 'name' field. Fix it by making a copy whenever read_revisions_from_stdin() passes an argument down the callchain. The other caller runs handle_revision_arg() on argv[], where it would be redundant to make a copy. Signed-off-by: Thomas Rast tr...@inf.ethz.ch --- So I changed my mind. Your easy fix looks to me the right thing to do. So here's the same with a commit message and signoff. I hope I got my history right; I didn't look too long if it had any users, but it was definitely recorded. revision.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index 3a20c96..181a8db 100644 --- a/revision.c +++ b/revision.c @@ -1277,7 +1277,8 @@ static void read_revisions_from_stdin(struct rev_info *revs, } die(options not supported in --stdin mode); } - if (handle_revision_arg(sb.buf, revs, 0, REVARG_CANNOT_BE_FILENAME)) + if (handle_revision_arg(xstrdup(sb.buf), revs, 0, + REVARG_CANNOT_BE_FILENAME)) die(bad revision '%s', sb.buf); } if (seen_dashdash) -- 1.8.2.1.703.g2535f49 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras felipe.contre...@gmail.com writes:
Clearly, that's the correct behavior. Why would anybody send a change
that does something other than the correct behavior?
Along the same lines, why would anyone write broken code? Nobody does,
right?
If anyone reads that commit message in more than a few weeks, then it's
because some of the code is *broken*. So the reader is investigating a
situation where there must be a flaw somewhere, and trying to pin down
the source. Having access to the thinking behind each commit means s/he
can more easily verify whether that thinking was correct and still
applies.
And your commit messages do nothing towards that end.
A cursory look^W^Wreview of the messages in fc/remote-hg:
remote-hg: fix bad file paths
Mercurial allows absolute file paths, and Git doesn't like that.
Only describes the problem; no reasoning as to what the chosen solution
is or why it is correct. (I can at least infer the former from the
code, but not the latter.)
remote-hg: show more proper errors
When cloning or pushing fails, we don't want to show a stack-trace.
So what do we show?
It also seems that you do not actually use the import you add, or do
you?
remote-hg: force remote push
Ideally we shouldn't do this, as it's not recommended in mercurial
documentation, but there's no other way to push multiple bookmarks (on
the same branch), which would be the behavior most similar to git.
At the same time, add a configuration option for the people that don't
want to risk creating new remote heads.
This one, for a change, says what it does but doesn't say what problem
it fixes.
I'll refrain from commenting on all the one-line messages, and just
point at this one:
remote-hg: trivial test cleanups
In $DAYJOB the advice is to avoid trivial (and similarly obvious):
either it *is* trivial, in which case you don't need to point that out,
or you're just trying to handwave over the fact that it's not. Like
this:
git_clone () {
- hg -R $1 bookmark -f -r tip master
git clone -q hg::$PWD/$1 $2
}
Not knowing the code I can only conjecture, but surely there was a
reason that the hg call lived in a function called git_clone? And
surely there must be a good reason why it is no longer needed?
My personal favorite however is this one:
remote-bzr: improve tag handling
revision_history() is deprecated and doesn't do what we want (revno
instead of dotted_revno?).
I don't even know how to parse that question mark. Does it actually ask
a question? Does it mean to imply, by the intonation suggested by a
question mark, how could anyone ever have been so silly as to use a
revno instead of a dotted_revno?
By the way, it's easy to find similarly helpful messages in git.git in
the old days. One that I remember stumbling across was:
Add the --color-words option to the diff options family
With this option, the changed words are shown inline. For example,
if a file containing This is foo is changed to This is bar, the diff
will now show This is in plain text, foo in red, and bar in green.
How could it not be obvious how it achieves this to anyone who has read
the ~170 lines of code it adds?
Luckily *that* code was correct and feature-complete right from the
start, so nobody ever had to actually read it to figure out what's going
on.
But that was back in 2006. I should think that git.git has improved
since; when I wrote my first patches in 2008, I was impressed with the
readable history and extensive reviews.
--
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: [PATCH 3/3] pull: introduce --[no-]autostash and pull.autostash
Ramkumar Ramachandra artag...@gmail.com writes: Yes, touching a simple autostash file at stash-time, and removing it at pop-time will do. I don't see why it should be part of a (potentially user-editable) todo-list, which serves an entirely different purpose. You'll have to apply the index state and then the tree state. How do you know whether the next call to git rebase should apply one or the other? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy matthieu@grenoble-inp.fr writes: Ramkumar Ramachandra artag...@gmail.com writes: Yes, touching a simple autostash file at stash-time, and removing it at pop-time will do. I don't see why it should be part of a (potentially user-editable) todo-list, which serves an entirely different purpose. You'll have to apply the index state and then the tree state. How do you know whether the next call to git rebase should apply one or the other? Plus: what happens if the user ran git stash during rebase? In Junio's version, the right commits are applied. Running blindly stash pop may pop the wrong stash. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM
On Tue, Apr 16, 2013 at 3:11 AM, Jakub Narębski jna...@gmail.com wrote: Junio C Hamano wrote: In order to just pick and use the more appropriate one (or a useful combination of the two), a clean description of what each of them do without historical cruft is more readable and useful, isn't it? I would expect that most of them who are newly configuring a system would pick COMMON one and override per instance as needed, without touching the SYSTEM one (fallback default) after reading the above, and that is what we want to happen. Do you think sysadmins need a history lesson to understand why there are two different possibilities? [...] I think the new text conveys the necessary information to the intended audience with more clarity without the history lesson or the record of your past frustration. Am I mistaken? Note also that this is about *gitweb/INSTALL*, which is meant to be *short* and succint description on how to install gitweb, and not about the reference documentation: gitweb(1) or gitweb.conf(5). Description of historical behavior (and backward compatibility) has place (if any) in manpages, not gitweb/INSTALL. -- Jakub Narębski Let us then agree that it should be mentioned somewhere in gitweb.conf.txt then (as it currently is not). -- -Drew Northup -- As opposed to vegetable or mineral error? -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- 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] gitweb/INSTALL: Simplify description of GITWEB_CONFIG_SYSTEM
On Tue, Apr 16, 2013 at 12:36 AM, Junio C Hamano gits...@pobox.com wrote: Drew Northup n1xim.em...@gmail.com writes: + Note that the GITWEB_CONFIG_SYSTEM system-wide configuration file is + only used for instances that lack per-instance configuration file. + You can use GITWEB_CONFIG_COMMON common system-wide configuration + file (normally /etc/gitweb-common.conf) to keep common default + settings that apply to all instances. Settings from per-instance or system-wide configuration file override those from common system-wide configuration file. That's the point of explaining SPECIFICALLY why the then current behavior wasn't being replaced, and this other mechanism (which would otherwise have no obvious reason for existing) was being introduced. In order to just pick and use the more appropriate one (or a useful combination of the two), a clean description of what each of them do without historical cruft is more readable and useful, isn't it? I am not demanding the retention of cruft, and the rewording is definitely more pleasant to read. I would expect that most of them who are newly configuring a system would pick COMMON one and override per instance as needed, without touching the SYSTEM one (fallback default) after reading the above, and that is what we want to happen. Do you think sysadmins need a history lesson to understand why there are two different possibilities? We don't need a full history lesson. What we do need to know is that Hey, they don't do that the way everybody else does because it would break things. That's enough to get the point across, and as Jakub noted gitweb.conf.txt is the correct place for it. For example, bash reads some but not all possible configuration files. I would expect .bashrc to be read even for login shells after reading .bash_login; alas, that is not what happens. The manual does not apologize that the authors now know better and understand that it is a stupid behaviour. The order the rc files are read is just described matter-of-factly, and it gives sufficient information without unnecessary backstory. I think the new text conveys the necessary information to the intended audience with more clarity without the history lesson or the record of your past frustration. Am I mistaken? The back-story isn't needed; the Hey this is different part is. I think Jakub's suggestion of covering that (succinctly) in gitweb.conf.txt is the correct solution. -- -Drew Northup -- As opposed to vegetable or mineral error? -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- 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] gitweb/INSTALL: GITWEB_CONFIG_SYSTEM is for backward compatibility
Signed-off-by: Jakub Narebski jna...@gmail.com --- This can be either squashed with previous patch to gitweb/INSTALL, kept as separate patch or discarded. Drew: gitweb(1) or gitweb.conf(5) solution is more involved, so perhaps something like that? gitweb/INSTALL |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gitweb/INSTALL b/gitweb/INSTALL index 7ad1050..386e62f 100644 --- a/gitweb/INSTALL +++ b/gitweb/INSTALL @@ -243,7 +243,6 @@ for gitweb (in gitweb/README), and gitweb.conf(5) manpage. GITWEB_CONFIG_SYSTEM build configuration variable, and override it through the GITWEB_CONFIG_SYSTEM environment variable. - Note that the GITWEB_CONFIG_SYSTEM system-wide configuration file is only used for instances that lack per-instance configuration file. You can use GITWEB_CONFIG_COMMON common system-wide configuration @@ -252,6 +251,8 @@ for gitweb (in gitweb/README), and gitweb.conf(5) manpage. system-wide configuration file override those from common system-wide configuration file. + (Idiosyncratic GITWEB_CONFIG_SYSTEM is present for backward compatibility.) + - The gitweb config file is a fragment of perl code. You can set variables using our $variable = value; text from # character until the end of a line is ignored. See perlsyn(1) for details. -- 1.7.10.4 -- 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] gitweb/INSTALL: GITWEB_CONFIG_SYSTEM is for backward compatibility
On Tue, Apr 16, 2013 at 8:26 AM, Jakub Narębski jna...@gmail.com wrote: Drew: gitweb(1) or gitweb.conf(5) solution is more involved, so perhaps something like that? That or: (or both I supposehopefully not too mangled by Google's mail gadget) -- 8 -- Subject: [PATCH] Documentation/gitweb.conf.txt: Move note about config order precedence To go along with Jakub Narebski's cleanup of gitweb/INSTALL; making it clear that gitweb's config files work differently than a lot of other system-wide software. This is unobtrusive yet to the point. --- Documentation/gitweb.conf.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt index eb63631..05c81e7 100644 --- a/Documentation/gitweb.conf.txt +++ b/Documentation/gitweb.conf.txt @@ -55,7 +55,8 @@ following order: then fallback system-wide configuration file (defaults to '/etc/gitweb.conf'). Values obtained in later configuration files override values obtained earlier -in the above sequence. +in the above sequence. This is different from many system-wide software +installations and will stay this way for historical reasons. Locations of the common system-wide configuration file, the fallback system-wide configuration file and the per-instance configuration file -- 1.8.0 -- -Drew Northup -- As opposed to vegetable or mineral error? -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- 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] gitweb/INSTALL: GITWEB_CONFIG_SYSTEM is for backward compatibility
Forgot the S-O-B... On Tue, Apr 16, 2013 at 8:47 AM, Drew Northup n1xim.em...@gmail.com wrote: On Tue, Apr 16, 2013 at 8:26 AM, Jakub Narębski jna...@gmail.com wrote: Drew: gitweb(1) or gitweb.conf(5) solution is more involved, so perhaps something like that? That or: (or both I supposehopefully not too mangled by Google's mail gadget) Subject: [PATCH] [PATCH] Documentation/gitweb.conf.txt: Move note about config order precedence To go along with Jakub Narebski's cleanup of gitweb/INSTALL; making it clear that gitweb's config files work differently than a lot of other system-wide software. This is unobtrusive yet to the point. Signed-off-by: Drew Northup n1xim.em...@gmail.com --- Documentation/gitweb.conf.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt index eb63631..05c81e7 100644 --- a/Documentation/gitweb.conf.txt +++ b/Documentation/gitweb.conf.txt @@ -55,7 +55,8 @@ following order: then fallback system-wide configuration file (defaults to '/etc/gitweb.conf'). Values obtained in later configuration files override values obtained earlier -in the above sequence. +in the above sequence. This is different from many system-wide software +installations and will stay this way for historical reasons. Locations of the common system-wide configuration file, the fallback system-wide configuration file and the per-instance configuration file -- 1.8.0 -- -Drew Northup -- As opposed to vegetable or mineral error? -John Pescatore, SANS NewsBites Vol. 12 Num. 59 -- 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] usage: refactor die-recursion checks
On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote: Yeah, that seems sane; my biggest worry was that it would create headaches for Windows folks, who would have to emulate pthread_key. But it seems like we already added support in 9ba604a. pthread_key is not a problem, but pthread_once is. It's certainly solvable, but do we really have to? I'm not clear on what you are suggesting. That we protect only the main thread from recursion, or that we drop the check entirely? Or that we implement thread-local storage for this case without using pthread_once? -Peff -- 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 14/33] refs: extract a function peel_entry()
On 04/15/2013 07:38 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
if (read_ref_full(refname, base, 1, flag))
return -1;
-if ((flag REF_ISPACKED)) {
+/*
+ * If the reference is packed, read its ref_entry from the
+ * cache in the hope that we already know its peeled value.
+ * We only try this optimization on packed references because
+ * (a) forcing the filling of the loose reference cache could
+ * be expensive and (b) loose references anyway usually do not
+ * have REF_KNOWS_PEELED.
+ */
+if (flag REF_ISPACKED) {
struct ref_entry *r = get_packed_ref(refname);
This code makes the reader wonder what happens when a new loose ref
masks a stale packed ref, but the worry is unfounded because the
read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
such a case.
But somehow the calling sequence looks like such a mistake waiting
to happen. It would be much more clear if a function that returns a
struct ref_entry * is used instead of read_ref_full() above, and
we checked (r-flag REF_ISPACKED) in the conditional, without a
separate get_packed_ref(refname).
As I'm sure you realize, I didn't change the code that you are referring
to; I just added a comment.
But yes, I sympathize with your complaint. Additionally, the code has
the drawback that get_packed_ref() is called twice: once in
read_ref_full() and again in the if block here. Unfortunately, this
isn't so easy to fix because read_ref_full() doesn't use the loose
reference cache, so the reference that it returns might not even have a
ref_entry associated with it (specifically, unless the returned flag
value has REF_ISPACKED set). So there are a couple options:
* Always read loose references through the cache; that way there would
always be a ref_entry in which the return value could be presented.
This would not be a good idea at the moment because the loose reference
cache is populated one directory at a time, and reading a whole
directory of loose references could be expensive. So before
implementing this, it would be advisable to change the code to populate
the loose reference cache more selectively when single loose references
are needed. - This approach would be well beyond the scope of this
patch series.
* Implement a function like read_ref_full() with an additional (struct
ref_entry **entry) argument that is written to *in the case* that the
reference that was returned has a ref_entry associated with it, and NULL
otherwise. This would have to be an internal function because we don't
want to expose the ref_entry structure outside of refs.c.
read_ref_full() would be implemented on top of the new function.
Either way, I'd rather put this idea on my TODO list for another time.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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 15/33] refs: change the internal reference-iteration API
On 04/15/2013 07:38 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: [...] But more importantly, this change prevents peel_ref() from returning invalid results in the following scenario: When iterating via the external API, the iteration always includes both packed and loose references, and in particular never presents a packed ref if there is a loose ref with the same name. The internal API, on the other hand, gives the option to iterate over only the packed references. During such an iteration, there is no check whether the packed ref might be hidden by a loose ref of the same name. But until now the packed ref was recorded in current_ref during the iteration. So if peel_ref() were called with the reference name corresponding to current ref, it would return the peeled version of the packed ref even though there might be a loose ref that peels to a different value. This scenario doesn't currently occur in the code, but fix it to prevent things from breaking in a very confusing way in the future. Hopefully that means in later patches in this series ;-) I don't think that the rest of the series would have triggered this problem either. In fact, if I had written repack_without_ref()'s peeling functionality using peel_ref(), then it would have *depended* on this bug for its proper operation...otherwise it would have written the peeled version of the loose ref to the packed-ref file. Of course, it's all pretty academic because the peeled version of a packed ref should never be used when it is overridden by a loose ref, so the incorrect peeled values in the packed-ref file shouldn't have any observable effects. The real problem is that calling the old peel_ref() function on a packed reference was illegitimate because the function only knew how to peel a ref that was still active. Plus it's kindof silly tucking away the current reference in a global variable then looking it up again instead of passing the ref_entry around. Callers outside of refs.c could also not have triggered this bug because they have no way to access overridden packed refs. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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] submodule deinit: clarify work tree removal message
On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann jens.lehm...@web.de wrote:
Okay, so here is the patch for that. If someone could point out
a portable and efficient way to check if a directory is already
empty I would be happy to use that to silence the Cleaned
directory message currently printed also when deinit is run on
an already empty directory.
isemptydir() {
test -d $(find $1 -maxdepth 0 -empty)
}
Sorry for the late reply. I see this patch is already in master
(which is fine with me).
Thanks,
Phil
--
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] usage: refactor die-recursion checks
Am 4/16/2013 15:01, schrieb Jeff King: On Tue, Apr 16, 2013 at 09:18:46AM +0200, Johannes Sixt wrote: Yeah, that seems sane; my biggest worry was that it would create headaches for Windows folks, who would have to emulate pthread_key. But it seems like we already added support in 9ba604a. pthread_key is not a problem, but pthread_once is. It's certainly solvable, but do we really have to? I'm not clear on what you are suggesting. That we protect only the main thread from recursion, or that we drop the check entirely? Or that we implement thread-local storage for this case without using pthread_once? Anything(*) that does not require pthread_once. A pthread_once implementation on Windows would be tricky and voluminous and and on top of it very likely to be done differently for gcc and MSVC. I don't like to go there if we can avoid it. (*) That includes doing nothing, but does not include ripping out the recursion check, as it protects us from crashes. -- Hannes -- 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 16/33] t3210: test for spurious error messages for dangling packed refs
On 04/15/2013 07:39 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: How can I get rid of the sleeps in these tests? Would test-chmtime help? Maybe I should take a step back and ask why it isn't easier to expire things regardless of age, which is sometimes a reasonable thing to do even outside of test suites. In particular, it seem to me that the most obvious interpretation of git reflog expire --expire=now --all would be that it expires *everything*. But in fact it seems to only expire things that are at least one second old, which doesn't seem at all useful in the real world. --expire=all is accepted without complaint but doesn't do what one would hope. Something like --expire=$(($(date +%s)+3600)) works, but it is not very convenient (is it portable?). I guess I can use test-chmtime for my particular test, though I will have to pass it the explicit names of the logfile(s), like find .git/logs -type f -print0 | xargs -0 test-chmtime =-60 I guess that's what I'll do if no better solution comes up. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 v3] cherry-pick: make sure all input objects are commits
On 04/15/2013 09:12 PM, Junio C Hamano wrote: The paths given to handle_refs() may also have to be copied before saved, depending on how ref iteration is implemented, details of which may change as Michael seems to be updating the area again. I think we let the callback peek ref_entry-name[] which is stable, so I suspect we are OK. ref_entry-name is stable as long as invalidate_ref_cache() is not called, and I am not even thinking of changing that (partly because I don't have the energy to audit and adjust all of the callers). Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 master] convert: The native line-ending is \r\n on MinGW
On Mon, Apr 15, 2013 at 11:43 PM, Junio C Hamano gits...@pobox.com wrote: Erik Faye-Lund kusmab...@gmail.com writes: This is absolutely the right thing to do. However, stuff have changed a bit since the patch was written; this change now needs to go in config.mak.uname instead of config.mak. Thanks for a quick response. What's your preference? I could just ignore a patch I won't be able to test myself and have you guys carry it in your tree forever, but I do not think that is necessary for something small like this. I should probably clarify; conceptually, this is the right thing to do. Git for Windows is a Windows application, and should have CRLF as the native newline. I hadn't tested this patch myself, though. Our tree is currently way behind yours, and I tried to do a rebase, but it turned out much trickier than I was hoping for. I've given it a go on top of your tree + some essential patches I'll need to get things to run, and it seems to do what it claims to do. However, I haven't been able to run the test-suite, because I need a bunch more patches from the msysGit-tree for that. I think this is low impact enough that it can directly go to 'master' or even 'maint' if I were to apply to my tree. I agree. I don't think we need it in maint; we don't track that branch for msysGit. Thanks. -- 8 -- From: Jonathan Nieder jrnie...@gmail.com Date: Sat, 4 Sep 2010 03:25:09 -0500 Subject: [PATCH] convert: The native line-ending is \r\n on MinGW If you try this: 1. Install Git for Windows (from the msysgit project) 2. Put [core] autocrlf = false eol = native in your .gitconfig. 3. Clone a project with *.txt text in its .gitattributes. Then with current git, any text files checked out have LF line endings, instead of the expected CRLF. Cc: Johannes Schindelin johannes.schinde...@gmx.de Cc: Johannes Sixt j...@kdbg.org Signed-off-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index 9080054..d78fd3d 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -507,6 +507,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) compat/win32/dirent.o EXTLIBS += -lws2_32 PTHREAD_LIBS = + NATIVE_CRLF = YesPlease X = .exe SPARSE_FLAGS = -Wno-one-bit-signed-bitfield ifneq (,$(wildcard ../THIS_IS_MSYSGIT)) -- 1.8.2.1-542-g3613165 Looks fine to me. -- 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 v3 00/13] nd/pretty-formats
The short version: all applied, compiled and test OK. Possible minor nits from apply: applying: pretty: support padding placeholders, % % and % /Users/tb/projects/git/tb.duy/.git/rebase-apply/patch:253: indent with spaces. message two$ /Users/tb/projects/git/tb.duy/.git/rebase-apply/patch:254: indent with spaces. message one$ /Users/tb/projects/git/tb.duy/.git/rebase-apply/patch:255: indent with spaces. add bar$ /Users/tb/projects/git/tb.duy/.git/rebase-apply/patch:256: indent with spaces. initial$ /Users/tb/projects/git/tb.duy/.git/rebase-apply/patch:292: indent with spaces. message two $ warning: squelched 3 whitespace errors = And a possible micronit: what happened to that? On Sun, Mar 31, 2013 at 12:06 AM, Torsten Bögershausen tbo...@web.de wrote: On 30.03.13 10:35, Nguyễn Thái Ngọc Duy wrote: [...] The short version of a review: Would it make sense to leave reencode_string() as it is, and add a new function reencode_string_len() Hmm.. yeah. /Torsten -- 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] Extend runtime prefix computation
Hello Junio, Hello list, On Wed, Mar 06, 2013 at 09:19:42AM +0100, Michael Weiser wrote: Support determining the binaries' installation path at runtime even if called without any path components (i.e. via search path). The default for any change is not to include it. Is there any reason why we want this change? It makes a binary git installation fully relocatable. Seeing how git already has basic support for it I thought other people might be interested in this. I am still interested in getting this accepted into git. Where do I push to get it committed? Thanks, -- Michael Weiserscience + computing ag Senior Systems Engineer Geschaeftsstelle Duesseldorf Martinstrasse 47-55, Haus A phone: +49 211 302 708 32 D-40223 Duesseldorf fax: +49 211 302 708 50 www.science-computing.de -- Vorstandsvorsitzender/Chairman of the board of management: Gerd-Lothar Leonhart Vorstand/Board of Management: Dr. Bernd Finkbeiner, Michael Heinrichs, Dr. Arno Steitz, Dr. Ingrid Zech Vorsitzender des Aufsichtsrats/ Chairman of the Supervisory Board: Philippe Miltin Sitz/Registered Office: Tuebingen Registergericht/Registration Court: Stuttgart Registernummer/Commercial Register No.: HRB 382196 -- 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] Extend runtime prefix computation
On Wed, Mar 6, 2013 at 9:19 AM, Michael Weiser m.wei...@science-computing.de wrote: Hello Junio, On Tue, Mar 05, 2013 at 08:13:50AM -0800, Junio C Hamano wrote: Support determining the binaries' installation path at runtime even if called without any path components (i.e. via search path). The default for any change is not to include it. Is there any reason why we want this change? It makes a binary git installation fully relocatable. Seeing how git already has basic support for it I thought other people might be interested in this. I think the motivation for the feature in the first place is Windows, where the installation path isn't known at build-time. In the unix-world, this is generally known (/usr/bin or something like that). What's the reason you want it on other platforms? -- 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] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On 13-04-16 04:13 AM, Ramkumar Ramachandra wrote: Jeff King wrote: So there is some information that is per-clone (the objects, the remote tips), but there is some information that is per-submodule (where our local branches are, the index, the worktree). I can see why it is advantageous to share the per-clone information between similar clones (because it avoids disk space and network transfer). But I do not think you can escape having some form of per-submodule repo, even if it is a thin git-new-workdir-ish repo that points back to a parent repo for the clone. I want the flexibility to do the following: 1. Do a simple clone, where the clone contains the GITDIR embedded in the worktree. This is the most common case, and there is no reason to complicate it. I can optionally attach additional workdirs to this clone. I can also optionally relocate the GITDIR at a later date, if I feel the need to do so. 2. Attach a worktree to any object store without having to write a gitfile and set core.worktree by hand. The limitation is that you can't have two submodules from two different superprojects sharing the same object store (since both of them are worktrees). However, for the purpose of working on the submodule repository as an independent repository (this is a very common case for me), I can attach a new workdir to the GITDIR very easily. Doesn't contrib/workdir/git-new-workdir do this? M. -- 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] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On 13-04-16 04:17 AM, Ramkumar Ramachandra wrote: Marc Branchaud wrote: If git add is all about specifying what lives under paths in the worktree, what's wrong with letting git add go beyond specifying just files? Syntax aside for the moment, I think a command like git add git-repo-reference foo is perfectly natural: It specifies what is inside worktree path foo. I never said just files. Files, directories, symlinks and submodules are all things in the worktree, and all fine. Remote URLs, on the other hand, have nothing to do with the worktree. But they have everything to do with submodules. You need a URL to identify a submodule. If you want a submodule in your worktree, at some point you have to specify the submodule's URL. I really feel like I'm missing something here. You seem to be saying that it's wrong to let git add interpret a URL as a submodule. Instead you seem to want to have some other mechanism create the files, directories and symlinks that make up a submodule, so that git add can then operate with the purity you desire. That's what I don't understand. As a submodule user, I want to git add a submodule. I don't see why it's necessary to have more than one command to do that. But if you're saying that it's fine for git add to work this way, then I don't see the point of the proposed change to git clone. M. -- 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] clone: introduce clone.submoduleGitDir to relocate $GITDIR
On 13-04-16 04:21 AM, Ramkumar Ramachandra wrote: Junio C Hamano wrote: It does not relieve git add (or git submodulea add) from the responsibility of moving .git directory. It only reduces the need to do so. When the user says add and the repository has .git directory in it, add (or submodule add) is still responsible for relocating it. Since you're so stubborn about it, I suppose 'git add' could call a function in my new first-class program to attach detach worktrees/workdirs and relocate GITDIRs as a last resort (if the user somehow managed to put a GITDIR in the submodule worktree despite our well-designed tools). But last resort is not what we should be discussing now: we're discussing what the design should ideally be. And ideally, I think we both agree that it's best if init/clone did the relocation. If that's the question, then put me on the disagree side. I just don't see why that approach is best, especially if the intention is to make 'git add' DTRT wrt submodules, and deprecate 'git submodule add'. M. -- 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/3] pull: introduce --[no-]autostash and pull.autostash
On Tue, Apr 16, 2013 at 5:20 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Matthieu Moy wrote: No. Ultimately, the entry point of all these invocations is git-rebase.sh. The plan is to refactor calls from git-rebase.sh to git-rebase--*.sh scripts so that those scripts return control to git-rebase.sh, which will be the final exit point. The logic is very simple: On the very first invocation of rebase (ie. no existing rebase in progress), stash. If the return statement from the specific rebase script is 1 (which means that there are conflicts to be resolved), exit as usual. If it is 0 (which means that the rebase completely successfully), pop the stash before exiting as usual. What's so complicated about that? I'm against leaking the autostash implementation detail into specific rebases, because I value a clean and pleasant implementation over everything else. It can be more complex than you realize. $ git pull --rebase --stash It seems that there is already a .git/rebase-apply directory, and I wonder if you are in the middle of another rebase. If that is the case, please try git rebase (--continue | --abort | --skip) If that is not the case, please rm -fr .git/rebase-apply and run me again. I am stopping in case you still have something valuable there. If I follow the latter advice about 'rm -rf', who will remember that 'rebase' had something stashed, and what will they do with it when they do? What if it is weeks or months later? I would be surprised to see long-forgotten wip show up in my workspace all of a sudden. Phil -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Jeff King p...@peff.net writes:
The solution is simple: now that FAILONERROR is a
per-request setting, we move it to get_active_slot to make
sure it is reset for each request.
Signed-off-by: Jeff King p...@peff.net
---
Hmph. I have no idea how this ever passed the tests, so I can only
assume that I screwed up in running them. I even recall considering this
issue while writing the patches, but I mixed up which of get_curl_handle
and get_active_slot it needed to be in when I did so.
Thanks.
http.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/http.c b/http.c
index 58c063c..48d4ff6 100644
--- a/http.c
+++ b/http.c
@@ -282,7 +282,6 @@ static CURL *get_curl_handle(void)
#endif
if (ssl_cainfo != NULL)
curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
- curl_easy_setopt(result, CURLOPT_FAILONERROR, 1);
if (curl_low_speed_limit 0 curl_low_speed_time 0) {
curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
@@ -506,6 +505,7 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot-curl, CURLOPT_POSTFIELDS, NULL);
curl_easy_setopt(slot-curl, CURLOPT_UPLOAD, 0);
curl_easy_setopt(slot-curl, CURLOPT_HTTPGET, 1);
+ curl_easy_setopt(slot-curl, CURLOPT_FAILONERROR, 1);
if (http_auth.password)
init_curl_http_auth(slot-curl);
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: [RFC/PATCH] clone: introduce clone.submoduleGitDir to relocate $GITDIR
Ramkumar Ramachandra artag...@gmail.com writes: My solution fixes all these problems, and we need .git/modules/name.git (no path-to-submodule nonsense) only as a last resort: #3 (ref: my email to Peff). Have you noticed that there are distinction between submodule path and submodule name already in the current system, and name is derived from path if you do not give it when adding a submodule merely as a convenience? If some existing code uses .git/modules/path.git in git submodule, that is a bug that needs to be fixed. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/14] dir.c: git-status --ignored: don't drop ignored directories
Firstly, great work on the series! I've just started looking into it,
so please don't take my comments too seriously: some of them may be
queries, and others may be minor suggestions, but I can't say I
understand the area you're patching. I know Junio doesn't like me
mixing queries in reviews, but I don't fully agree with his policy.
Karsten Blees wrote:
'git-status --ignored' drops ignored directories if they contain untracked
files in an untracked sub directory.
Wait, ignored directories will always contain untracked
subdirectories, unless you add -f them, right? Why are you saying
untracked files in an _untracked_ subdirectory? We don't track
directories anyway, and I would call a directory tracked if there's
atleast one file inside it is tracked. So, my understanding of this
is:
quux/
bar
baz/
foo
In this example, if quux is ignored and untracked, git status
--ignored currently shows quux/. If quux/bar is tracked (say with add
-f), but baz/ is untracked, git status --ignored doesn't show me
anything. What exactly is the bug you're fixing? I'll try to look at
the tests to infer this, but your commit message could probably be
clearer.
Nit: please s/git-status/git status/
Fix it by getting exact (recursive) excluded status in treat_directory.
Okay, so you're patching treat_directory() in dir.c to do some
recursive exclude handling. Let's see what this is.
diff --git a/dir.c b/dir.c
index 57394e4..ec4eebf 100644
--- a/dir.c
+++ b/dir.c
@@ -1060,6 +1060,15 @@ static enum directory_treatment treat_directory(struct
dir_struct *dir,
/* This is the show_other_directories case */
+ /* might be a sub directory in an excluded directory */
+ if (!exclude) {
+ struct path_exclude_check check;
+ int dt = DT_DIR;
+ path_exclude_check_init(check, dir);
+ exclude = is_path_excluded(check, dirname, len, dt);
+ path_exclude_check_clear(check);
+ }
+
So, I'm guessing that DT_DIR refers to a value that a field in struct
dirent can take; that value could be one of DIR (directory), REG
(regular file?), LNK (symbolic link?). I don't get much of this, but
what I do get is that you're setting exclude for the rest of the code
in this function.
Sorry that I'm not able to do a more thorough review.
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 0da1214..0f1034e 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -143,4 +143,31 @@ test_expect_success 'status ignored tracked directory
and uncommitted file with
test_cmp expected actual
'
+cat expected \EOF
+?? .gitignore
+?? actual
+?? expected
+!! tracked/
+EOF
Please put these segments inside the test_expect_success block, so
it's easy to think about those blocks in isolation. I know you're
just following the existing conventions existing in this test, but
those are not necessarily good conventions.
+test_expect_success 'status ignored tracked directory with uncommitted file
in untracked subdir with --ignore' '
+ rm -rf tracked/uncommitted
+ mkdir tracked/ignored
+ : tracked/ignored/uncommitted
+ git status --porcelain --ignored actual
+ test_cmp expected actual
+'
This is very confusing. How is tracked a tracked directory? Oh,
right: some previous test git add'ed tracked/committed. How do I know
about that in this test?
Yeah, changes to tracked ignored directories are not shown, but the
commit message didn't tell me this.
+cat expected \EOF
+?? .gitignore
+?? actual
+?? expected
+!! tracked/ignored/uncommitted
+EOF
+
+test_expect_success 'status ignored tracked directory with uncommitted file
in untracked subdir with --ignore -u' '
+ git status --porcelain --ignored -u actual
+ test_cmp expected actual
+'
+
test_done
I suppose the commit message told me about this one vaguely, but I
think it could be much clearer overall.
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 3/3] pull: introduce --[no-]autostash and pull.autostash
Matthieu Moy matthieu@grenoble-inp.fr writes: Ramkumar Ramachandra artag...@gmail.com writes: If it is 0 (which means that the rebase completely successfully), pop the stash before exiting as usual. And you're going to pop the stash even if no stash were triggered when starting the rebase. Really, think about it again: you're not going to guess whether you have to unstash without storing this information somewhere. You may argue about storing it outside the todolist, you can't unstash unconditionally. Yes, it can be part of todo, but it does not have to be. Regardless of where the information is stored, implementing the last step as stash pop will add a small inconsistency the end user needs to be aware of. Imagine git rebase stops, asks you to help with conflicts, and you look at it, play with the working tree, and made a mess. If this was the last stash pop invocation, the way to go back and start again is reset --hard stash pop. For all the other steps, that is not how the user goes back to the originally conflicted state in order to retry from scratch. Makes me wonder if the world were a better place if the rebase-todo list were implemented as a dedicated stash and rebase --continue were a mere stash pop followed by a commit if pop goes smoothly. I am not suggesting to actually do so, but it is an intriguing thought from the perspective of end user experience, isn't it? In any case, I am not saying that it is a _bad_ thing to implement the last restore the WIP stage as stash pop. I am just pointing out that the messaging needs to be done carefully to make sure the user understands which one is which, as the way to deal with the situation where it stops and asks the user for help would be different from normal step in the rebase sequence that replays a commit. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 15/33] refs: change the internal reference-iteration API
Michael Haggerty mhag...@alum.mit.edu writes: ... This scenario doesn't currently occur in the code, but fix it to prevent things from breaking in a very confusing way in the future. Hopefully that means in later patches in this series ;-) I don't think that the rest of the series would have triggered this problem either. Yeah, I misread the message when I said Hopefully. I somehow thought it was saying we will fix it in the future; otherwise things can break in a confusing way, which is not the case. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/14] dir.c: git-status --ignored: don't list empty ignored directories
Karsten Blees wrote:
'git-status --ignored' lists ignored tracked directories without any
ignored files if a tracked file happens to match an exclude pattern.
Here, I have:
quux/
bar
baz/
foo
So, quux is an ignored tracked directory. bar is tracked, but matches
an ignore pattern. Currently, git status --ignored lists quux/. I'm
confused.
Always exclude tracked files.
exclude it from the 'git status --ignored' output, I presume?
There's already an _exclude_ pattern in your previous sentence, so you
can see why the reader might be confused about what you're talking
about.
diff --git a/dir.c b/dir.c
index 7c9bc9c..fd1f088 100644
--- a/dir.c
+++ b/dir.c
@@ -1109,16 +1109,13 @@ static int treat_file(struct dir_struct *dir, struct
strbuf *path, int exclude,
struct path_exclude_check check;
int exclude_file = 0;
+ /* Always exclude indexed files */
+ if (index_name_exists(the_index, path-buf, path-len, ignore_case))
+ return 1;
+
if (exclude)
exclude_file = !(dir-flags DIR_SHOW_IGNORED);
else if (dir-flags DIR_SHOW_IGNORED) {
- /* Always exclude indexed files */
- struct cache_entry *ce = index_name_exists(the_index,
- path-buf, path-len, ignore_case);
-
- if (ce)
- return 1;
-
Okay, so you just moved this segment outside the else if()
conditional. Can you explain what the old logic was doing, and what
the rationale for it was?
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 4ece129..28b7d95 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -122,10 +122,34 @@ cat expected \EOF
?? .gitignore
?? actual
?? expected
+EOF
+
+test_expect_success 'status ignored tracked directory and ignored file with
--ignore' '
+ echo committed .gitignore
+ git status --porcelain --ignored actual
+ test_cmp expected actual
+'
Um, didn't really get this one. You have three untracked files, and
git status seems to be showing them fine. What am I missing?
+cat expected \EOF
+?? .gitignore
+?? actual
+?? expected
+EOF
+
+test_expect_success 'status ignored tracked directory and ignored file with
--ignore -u' '
+ git status --porcelain --ignored -u actual
+ test_cmp expected actual
+'
I didn't understand why you're invoking -u here (doesn't it imply
all, as opposed to normal when unspecified?). There are really no
directories, so I don't know what I'm expected to see.
+cat expected \EOF
+?? .gitignore
+?? actual
+?? expected
!! tracked/
EOF
test_expect_success 'status ignored tracked directory and uncommitted file
with --ignore' '
+ echo tracked .gitignore
: tracked/uncommitted
git status --porcelain --ignored actual
test_cmp expected actual
Didn't we test this in the last patch? Okay, I'm completely confused now.
Once again, apologies for my inexperienced comments: I'm contributing
whatever little I can to the review process.
--
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 14/33] refs: extract a function peel_entry()
Michael Haggerty mhag...@alum.mit.edu writes:
On 04/15/2013 07:38 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
if (read_ref_full(refname, base, 1, flag))
return -1;
- if ((flag REF_ISPACKED)) {
+ /*
+* If the reference is packed, read its ref_entry from the
+* cache in the hope that we already know its peeled value.
+* We only try this optimization on packed references because
+* (a) forcing the filling of the loose reference cache could
+* be expensive and (b) loose references anyway usually do not
+* have REF_KNOWS_PEELED.
+*/
+ if (flag REF_ISPACKED) {
struct ref_entry *r = get_packed_ref(refname);
This code makes the reader wonder what happens when a new loose ref
masks a stale packed ref, but the worry is unfounded because the
read_ref_full() wouldn't have gave us REF_ISPACKED in the flag in
such a case.
But somehow the calling sequence looks like such a mistake waiting
to happen. It would be much more clear if a function that returns a
struct ref_entry * is used instead of read_ref_full() above, and
we checked (r-flag REF_ISPACKED) in the conditional, without a
separate get_packed_ref(refname).
As I'm sure you realize, I didn't change the code that you are referring
to; I just added a comment.
Yes.
But yes, I sympathize with your complaint. Additionally, the code has
the drawback that get_packed_ref() is called twice: once in
read_ref_full() and again in the if block here. Unfortunately, this
isn't so easy to fix because read_ref_full() doesn't use the loose
reference cache, so the reference that it returns might not even have a
ref_entry associated with it (specifically, unless the returned flag
value has REF_ISPACKED set). So there are a couple options:
* Always read loose references through the cache; that way there would
always be a ref_entry in which the return value could be presented.
This would not be a good idea at the moment because the loose reference
cache is populated one directory at a time, and reading a whole
directory of loose references could be expensive. So before
implementing this, it would be advisable to change the code to populate
the loose reference cache more selectively when single loose references
are needed. - This approach would be well beyond the scope of this
patch series.
* Implement a function like read_ref_full() with an additional (struct
ref_entry **entry) argument that is written to *in the case* that the
reference that was returned has a ref_entry associated with it, and NULL
otherwise. This would have to be an internal function because we don't
want to expose the ref_entry structure outside of refs.c.
read_ref_full() would be implemented on top of the new function.
Isn't there another?
Instead of using read_ref_full() at this callsite, can it call a
function, given a refname, returns a pointer to struct ref_entry,
using the proper a loose ref covers the packed ref with the same
name semantics?
I guess that may need the same machinery for your first option to
implement it efficiently; right now read_ref_full() goes directly to
the single file that would hold the named ref without scanning and
populating sibling refs in the in-core loose ref hierarchy, and
without doing so you cannot return a struct ref_entry. Hmm...
--
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 master] convert: The native line-ending is \r\n on MinGW
Am 16.04.2013 16:39, schrieb Erik Faye-Lund: On Mon, Apr 15, 2013 at 11:43 PM, Junio C Hamano gits...@pobox.com wrote: Erik Faye-Lund kusmab...@gmail.com writes: This is absolutely the right thing to do. However, stuff have changed a bit since the patch was written; this change now needs to go in config.mak.uname instead of config.mak. Thanks for a quick response. What's your preference? I could just ignore a patch I won't be able to test myself and have you guys carry it in your tree forever, but I do not think that is necessary for something small like this. I should probably clarify; conceptually, this is the right thing to do. Git for Windows is a Windows application, and should have CRLF as the native newline. I hadn't tested this patch myself, though. Our tree is currently way behind yours, and I tried to do a rebase, but it turned out much trickier than I was hoping for. I've given it a go on top of your tree + some essential patches I'll need to get things to run, and it seems to do what it claims to do. However, I haven't been able to run the test-suite, because I need a bunch more patches from the msysGit-tree for that. I have been using this patch or an equivalent one since at least one and a half years (until a month or two ago, as I discovered today, but that is only by accident). But I do not use any text attributes or eol configuration, so I can only say that it does not regress this use case. I think this is low impact enough that it can directly go to 'master' or even 'maint' if I were to apply to my tree. I agree. I don't think we need it in maint; we don't track that branch for msysGit. Yes, master is good enough. Thanks. Thanks. -- 8 -- From: Jonathan Nieder jrnie...@gmail.com Date: Sat, 4 Sep 2010 03:25:09 -0500 Subject: [PATCH] convert: The native line-ending is \r\n on MinGW If you try this: 1. Install Git for Windows (from the msysgit project) 2. Put [core] autocrlf = false eol = native in your .gitconfig. 3. Clone a project with *.txt text in its .gitattributes. Then with current git, any text files checked out have LF line endings, instead of the expected CRLF. Cc: Johannes Schindelin johannes.schinde...@gmx.de Cc: Johannes Sixt j...@kdbg.org Signed-off-by: Jonathan Nieder jrnie...@gmail.com Signed-off-by: Junio C Hamano gits...@pobox.com --- config.mak.uname | 1 + 1 file changed, 1 insertion(+) diff --git a/config.mak.uname b/config.mak.uname index 9080054..d78fd3d 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -507,6 +507,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) compat/win32/dirent.o EXTLIBS += -lws2_32 PTHREAD_LIBS = + NATIVE_CRLF = YesPlease X = .exe SPARSE_FLAGS = -Wno-one-bit-signed-bitfield ifneq (,$(wildcard ../THIS_IS_MSYSGIT)) -- 1.8.2.1-542-g3613165 Looks fine to me. -- 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] t3903 (stash): add failing test for ref of form ^{/message}
While a 'git stash show stash^{/quuxery}' works just fine, a 'git
stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
stash reference'. This confusing behavior arises from the differences
in logic that 'show' and 'pop' internally employ to validate the
specified ref. Document this bug by adding a failing testcase for it.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
So if you look at git-stash.sh:377, you'll notice that it's doing a
the shell substitution ${REV%@*} to figure out whether the stash
ref is a valid ref. This hacky myopic design has to be done away
with immediately, and we should really compare the SHA-1 hex of the
specified ref with those in the stash reflog.
The only reason I haven't written a fix yet is because I'm not sure
why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
the first place. Can someone enlighten me as to what is going on?
t/t3903-stash.sh | 9 +
1 file changed, 9 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..04ba983 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -583,6 +583,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n
= N' '
git stash drop
'
+test_expect_failure 'valid ref of the form stash^{/message}' '
+ git stash clear
+ echo bar file
+ git add file
+ git stash save quuxery
+ git stash show stash^{/quuxery}
+ git stash pop stash^{/quuxery}
+'
+
test_expect_success 'stash branch should not drop the stash if the branch
exists' '
git stash clear
echo foo file
--
1.8.2.1.390.g924f6c3.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()
Michael Haggerty mhag...@alum.mit.edu writes: In projects where I can choose the coding standard, I like to use extra whitespace to help the eye pick out the range of parentheses, like if (!( (flags DO_FOR_EACH_INCLUDE_BROKEN) || ref_resolves_to_object(entry) )) return 0; but I've never seen this style in the Git project so I haven't used it here. I _think_ at least to me, it is not the textual grouping style but the unless X we skip the rest logic itself that confused me. It took the same number of minutes to grok the above as the original. We skip the rest of this function unless the condition inside !() holds, and the condition is we are told to include broken ones, in which case we know we will do the remainder without checking anything else, or we do the checking and find that it is not broken. I suspect the root cause of the confusion to force the double negation is INCLUDE_BROKEN; we have to express when we are told to ignore broken one and this thing is broken, ignore it without negation, i.e. if ( !(flags INCLUDE_BROKEN) is_broken(thing) ) return; which would have been much more pleasant to read if it were if ( (flags IGNORE_BROKEN) is_broken(thing) ) return; Unfortunately, we cannot change it to IGNORE_BROKEN and flip the meaning of the bit, because almost all callers do want to ignore broken ones. Either way is fine by me, even though I find that !(A || B) needs a bit more mental exercise to grok than (!A !B). Perhaps it is just me who is not so strong at math ;-) -- 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 log - crash and core dump
Am 16.04.2013 18:55, schrieb Ivan Lyapunov:
git version 1.8.2.1 crashes on my ArchLinux x86_64 on git log command
gdb bt is attached
git log | less
does not crash in same repo
I cannot share a repo for a debug purposes since it's private repo of
my employer
but I can perform any suitable tests on repo to help this bug to be fixed
#0 0x7722b3e6 in strtoull_l_internal () from /usr/lib/libc.so.6
#1 0x004b31d4 in pp_user_info (pp=pp@entry=0x7fffd310,
what=what@entry=0x521379 Author, sb=sb@entry=0x7fffd290,
line=line@entry=0x7b3a45 Ivan Lyapunov ilyapu...@trueconf.ru-
1354083115 +0400\ncommitter Ivan Lyapunov ilyapu...@trueconf.ru
So this is the author information, correct?
Ivan Lyapunov ilyapu...@trueconf.ru- 1354083115 +0400
|author name| |---author email| ^^^ |--time--| |tz-|
How did you manage to add the - after the email address?
What does git log in version 1.8.1 or earlier show for this commit?
1354083115 +0400\n\n- small merge fixes,
encoding=encoding@entry=0x505400 UTF-8) at pretty.c:441
#2 0x004b533a in pp_header (sb=0x7fffd290,
msg_p=0x7fffd228, commit=0x7c1e10, encoding=0x505400 UTF-8,
pp=0x7fffd310) at pretty.c:1415
#3 pretty_print_commit (pp=pp@entry=0x7fffd310,
commit=commit@entry=0x7c1e10, sb=sb@entry=0x7fffd290) at
pretty.c:1545
#4 0x004a0b45 in show_log (opt=opt@entry=0x7fffd4d0) at
log-tree.c:683
#5 0x004a1616 in log_tree_commit
(opt=opt@entry=0x7fffd4d0, commit=commit@entry=0x7c1e10) at
log-tree.c:859
#6 0x00438b03 in cmd_log_walk (rev=rev@entry=0x7fffd4d0)
at builtin/log.c:310
#7 0x004395dd in cmd_log (argc=1, argv=0x7fffdd30,
prefix=0x0) at builtin/log.c:582
#8 0x0040562d in run_builtin (argv=0x7fffdd30, argc=1,
p=0x754d18 commands.21404+1080) at git.c:282
#9 handle_internal_command (argc=1, argv=0x7fffdd30) at git.c:444
#10 0x00404a6f in run_argv (argv=0x7fffdbd0,
argcp=0x7fffdbdc) at git.c:490
#11 main (argc=1, argv=0x7fffdd30) at git.c:565
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
- unsigned long time;
- int tz;
+ unsigned long time = 0;
+ int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
- time = strtoul(ident.date_begin, date, 10);
- tz = strtol(date, NULL, 10);
+ if (ident.date_begin) {
+ time = strtoul(ident.date_begin, date, 10);
+ tz = strtol(date, NULL, 10);
+ }
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
--
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
[Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
While a 'git stash show stash^{/quuxery}' works just fine, a 'git
stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
stash reference'. This confusing behavior arises from the differences
in logic that 'show' and 'pop' internally employ to validate the
specified ref. Document this bug by adding a failing testcase for it.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
So sorry about misspelling Junio's address in my previous email.
Please respond to this one instead.
So if you look at git-stash.sh:377, you'll notice that it's doing a
the shell substitution ${REV%@*} to figure out whether the stash
ref is a valid ref. This hacky myopic design has to be done away
with immediately, and we should really compare the SHA-1 hex of the
specified ref with those in the stash reflog.
The only reason I haven't written a fix yet is because I'm not sure
why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
the first place. Can someone enlighten me as to what is going on?
t/t3903-stash.sh | 9 +
1 file changed, 9 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..04ba983 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -583,6 +583,15 @@ test_expect_success 'invalid ref of the form stash@{n}, n
= N' '
git stash drop
'
+test_expect_failure 'valid ref of the form stash^{/message}' '
+ git stash clear
+ echo bar file
+ git add file
+ git stash save quuxery
+ git stash show stash^{/quuxery}
+ git stash pop stash^{/quuxery}
+'
+
test_expect_success 'stash branch should not drop the stash if the branch
exists' '
git stash clear
echo foo file
--
1.8.2.1.390.g924f6c3.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] Test the Git version string
Philip Oakley philipoak...@iee.org writes: What kind of benefit are you envisioning out of this? The purpose of tests is to detect mistakes and spot regressions. A change to the 'git version X.Y.z' string would be a regression, as I spotted earlier, as it conflicts with expectations of standard programmes such as git-gui. Sorry, but I do not follow. A released version says git version 1.8.2.1. In a month or so, I'll have another one that says git version 1.8.3. Or I may decide to bump in preparation for 2.0 and it may identify itself as git version 1.9. Neither of which no existing program such as git-gui has ever seen. In what way is that a regression? -- 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: Ensimag students projects, version 2013
Ramkumar Ramachandra artag...@gmail.com writes: Matthieu Moy wrote: I tend to agree with you, but the idea has explicitly been rejected in the past. The problem with an option like this is that it would also disable the advices that may be added in the future. By letting people disable the advices one by one, people see new advices as they arrive. You may think of it like do not show this message again tickboxes in some graphical user interfaces. Too controversial area for newcommers I guess ;-). This is the kind of nonsense that I absolutely won't stand for. Am I a less important customer than a newcomer? Who said anything about a customer? A newcomer to a community (i.e. Matthieu's student) needs not just to show technical excellence with patches, but needs to make a good argument on a larger design decision; old timers already tried to achieve a concensus on it, and did not manage to do so the last time we tried. It is a tough topic for a newcomer developer to tackle. -- 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] Extend runtime prefix computation
nobody writes: Hello Junio, Hello list, On Wed, Mar 06, 2013 at 09:19:42AM +0100, Michael Weiser wrote: Support determining the binaries' installation path at runtime even if called without any path components (i.e. via search path). The default for any change is not to include it. Is there any reason why we want this change? It makes a binary git installation fully relocatable. Seeing how git already has basic support for it I thought other people might be interested in this. I am still interested in getting this accepted into git. Where do I push to get it committed? I do not have a strong objection to what it tries to achieve, but I'd prefer to see no #ifdef platform code in a very generic part of the system like exec_cmd. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] Test the Git version string
On Tue, Apr 16, 2013 at 11:12 AM, Junio C Hamano gits...@pobox.com wrote: Philip Oakley philipoak...@iee.org writes: What kind of benefit are you envisioning out of this? The purpose of tests is to detect mistakes and spot regressions. A change to the 'git version X.Y.z' string would be a regression, as I spotted earlier, as it conflicts with expectations of standard programmes such as git-gui. Sorry, but I do not follow. A released version says git version 1.8.2.1. In a month or so, I'll have another one that says git version 1.8.3. Or I may decide to bump in preparation for 2.0 and it may identify itself as git version 1.9. Neither of which no existing program such as git-gui has ever seen. In what way is that a regression? Sorry. I was the one that first suggested that this was an issue. The regression is that there are scripts and tools in the wild that need to know the git version when deciding whether or not to use some new feature. e.g. git status --ignore-submodules=dirty did not appear until git 1.7.2. A script may want to use this flag, but it will only want to use it when available. If this string started saying The Git Version Control System v2.0 then these scripts would be broken since they would no longer recognize this as a post-1.7.2 Git. The unstated suggestion here is that it may be helpful to others if we were to declare that the git version output is plumbing. Folks are already using it that way so making it official would provide a guarantee that we won't break them in the future. -- 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: [PATCH master] convert: The native line-ending is \r\n on MinGW
Johannes Sixt j...@kdbg.org writes: I agree. I don't think we need it in maint; we don't track that branch for msysGit. Yes, master is good enough. Thanks to all those involved. Applied. -- 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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Ramkumar Ramachandra artag...@gmail.com writes:
While a 'git stash show stash^{/quuxery}' works just fine, a 'git
stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
stash reference'. This confusing behavior arises from the differences
in logic that 'show' and 'pop' internally employ to validate the
specified ref. Document this bug by adding a failing testcase for it.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
So sorry about misspelling Junio's address in my previous email.
Please respond to this one instead.
So if you look at git-stash.sh:377, you'll notice that it's doing a
the shell substitution ${REV%@*} to figure out whether the stash
ref is a valid ref. This hacky myopic design has to be done away
with immediately, and we should really compare the SHA-1 hex of the
specified ref with those in the stash reflog.
The only reason I haven't written a fix yet is because I'm not sure
why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
the first place. Can someone enlighten me as to what is going on?
I think they were an attempt to catch command line argument errors
early to be helpful to the end users.
See ef763129d105 (detached-stash: introduce parse_flags_and_revs
function, 2010-08-21). As the advertised and originally intended
use for stash was to name them with stash@{number}, chomping at
the first at-sign to make sure it names refs/stash does not sound
too bad a check.
I do not think anybody considered the approach to look at the commit
object name and making sure it appears in the reflog that implements
the stash. It sounds like a more robust check if done right.
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
On Tue, Apr 16, 2013 at 11:09 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
While a 'git stash show stash^{/quuxery}' works just fine, a 'git
stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
stash reference'.
I don't think it is appropriate to use the ^{/text} notation with stashes.
The stash is implemented using the reflog. The ^{/text} notation
searches the commit history, not the reflog. So I think it will be
able to match the first entry in your stash stack, but not any of the
other ones.
Try inserting another stash (see below) on top of the one that
contains the string quuxery and I think you'll find that your 'git
stash show stash^{/quuxery}' no longer works.
An extension to the reflog dwimery that implements @{/text} could be
interesting though.
This confusing behavior arises from the differences
in logic that 'show' and 'pop' internally employ to validate the
specified ref. Document this bug by adding a failing testcase for it.
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
So sorry about misspelling Junio's address in my previous email.
Please respond to this one instead.
So if you look at git-stash.sh:377, you'll notice that it's doing a
the shell substitution ${REV%@*} to figure out whether the stash
ref is a valid ref.
This hacky myopic design has to be done away
with immediately, and we should really compare the SHA-1 hex of the
specified ref with those in the stash reflog.
Just a bit of advice, maybe you should think about softening your tone
a bit hmm? I find this last sentence to be somewhat repelling and
tend to refrain from responding to such.
The only reason I haven't written a fix yet is because I'm not sure
why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
the first place. Can someone enlighten me as to what is going on?
t/t3903-stash.sh | 9 +
1 file changed, 9 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5dfbda7..04ba983 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -583,6 +583,15 @@ test_expect_success 'invalid ref of the form stash@{n},
n = N' '
git stash drop
'
+test_expect_failure 'valid ref of the form stash^{/message}' '
+ git stash clear
+ echo bar file
+ git add file
+ git stash save quuxery
# Save another stash here
echo bash file
git add file
git stash save something
# Now git stash show stash^{/quuxery} no longer works.
+ git stash show stash^{/quuxery}
+ git stash pop stash^{/quuxery}
+'
+
test_expect_success 'stash branch should not drop the stash if the branch
exists' '
git stash clear
echo foo file
-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 0/2] Test the Git version string
David Aguilar dav...@gmail.com writes: The regression is that there are scripts and tools in the wild that need to know the git version when deciding whether or not to use some new feature. e.g. git status --ignore-submodules=dirty did not appear until git 1.7.2. A script may want to use this flag, but it will only want to use it when available. If this string started saying The Git Version Control System v2.0 then these scripts would be broken since they would no longer recognize this as a post-1.7.2 Git. Blacklisting known-bad version and hoping all other versions including the ones you have never seen to behave in the way you expect usually works but there is a limit. A change to say The Git Version Control System %s will not happen willy-nilly, but when there is a good reason to do so, we would. I do not think a test that hardcodes the output is a good way to make sure a change is being done with a good reason. After all, a patch that updates the git version %s string can just update the expected output in the same patch. The only reason such a change will be caught is because during the review, somebody notices that the change touches the expected output of a test; for that to reliably protect the output, the test *has* to be commented to say that this expected output should be changed very carefully. A much better solution would be to leave that very carefully comment next to the in-code string to warn people about ramifiations of changing it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 4:59 AM, Thomas Rast tr...@inf.ethz.ch wrote: Felipe Contreras felipe.contre...@gmail.com writes: Clearly, that's the correct behavior. Why would anybody send a change that does something other than the correct behavior? Along the same lines, why would anyone write broken code? Nobody does, right? Yes, I should change the subject to: transport-helper: update remote helper namespace, because that's exactly the thing we DON'T want to do, the purpose of this patch is to mess up everything Suree. I'm willing and knowingly introducing a change that goes diametrically opposite to what we want. If anyone reads that commit message in more than a few weeks, then it's because some of the code is *broken*. That is irrelevant. Junio said the correct behavior was not described, when if fact it clearly is. Whether or not the patch has a bug in it is irrelevant to the fact that the correct behavior is described or not. So the reader is investigating a situation where there must be a flaw somewhere, and trying to pin down the source. Having access to the thinking behind each commit means s/he can more easily verify whether that thinking was correct and still applies. Sure, and where is the thinking not clear? The remote helper ref is not updated, so we do update it. How is that not clear? And your commit messages do nothing towards that end. Oh, it does. You just don't understand how remote-helper works. A cursory look^W^Wreview of the messages in fc/remote-hg: [skipping irrelevant comments] I'm sorry, did you actually hit an issue that required to look at the commit message to understand where the issue came from? No? Then I won't bother with hypotheticals. If you want to waste your time, by all means, rewrite all my commit messages with essays that nobody will ever read. I'm not going to do that for some hypothetical case that will never happen. I'm not going to waste my time. 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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Brandon Casey draf...@gmail.com writes:
The stash is implemented using the reflog. The ^{/text} notation
searches the commit history, not the reflog. So I think it will be
able to match the first entry in your stash stack, but not any of the
other ones.
Good point, together with...
An extension to the reflog dwimery that implements @{/text} could be
interesting though.
log -g --grep=text gives you a way to eyeball, but with
@{/text} you _might_ have a good way to name the revision.
I am not however so sure if it is useful outside the context of the
stash, because the ones you would want to recover from a normal
reflog is most likely the older version of what you already amended,
so the latest hit will likely be the post-amend version, not the one
closer to the original. You would end up eyeballing the output of
log --oneline -g -grep=text and cutting from it.
Just a bit of advice, maybe you should think about softening your tone
a bit hmm? I find this last sentence to be somewhat repelling and
tend to refrain from responding to such.
Oh, so it wasn't just me. I was about to say something similar,
along the lines of people whom you just called myopic, because you
did not understand the rationale behind their past work, are less
likely to be inclined to help you. you would have more luck if you
ask them nicely., but I've long given up on helping him be a better
community member and deleted that part.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
Felipe Contreras felipe.contre...@gmail.com writes: Sure, and where is the thinking not clear? The remote helper ref is not updated, so we do update it. How is that not clear? Sure, between leaving it untouched, keeping the stale value and updating it to match what was pushed, everybody would know you mean the latter when you say correctly update. There is no third option updating it to match a random commit that is related to but is not exactly the same as what was pushed to be correct. What I felt unclear was _why_ both of these two (remote and testgit) have to get updated. In other words, correctly update it because without doing so, these bad things X, Y and Z will happen. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] avoid bogus recursion detected in die handler message
On Tue, Apr 16, 2013 at 04:13:56PM +0200, Johannes Sixt wrote:
I'm not clear on what you are suggesting. That we protect only the main
thread from recursion, or that we drop the check entirely? Or that we
implement thread-local storage for this case without using pthread_once?
Anything(*) that does not require pthread_once. A pthread_once
implementation on Windows would be tricky and voluminous and and on top of
it very likely to be done differently for gcc and MSVC. I don't like to go
there if we can avoid it.
We don't need to use pthread_once, as we can just do the initialization
of the thread-local storage along with starting the first thread (where
we already do similar initialization). Patch series to follow:
[1/2]: usage: allow pluggable die-recursion checks
[2/2]: run-command: use thread-aware die_is_recursing routine
(*) That includes doing nothing, but does not include ripping out the
recursion check, as it protects us from crashes.
I don't think doing nothing is a good idea. The recursion-detection is
triggering erroneously, blocking real error messages and replacing them
with the scary red-herring recursion detected in die handler.
The absolute simplest thing I think we could do is basically:
diff --git a/run-command.c b/run-command.c
index 765c2ce..3b0ad44 100644
--- a/run-command.c
+++ b/run-command.c
@@ -599,11 +599,14 @@ static NORETURN void die_async(const char *err, va_list
params)
return (void *)ret;
}
+extern int dying;
+
static NORETURN void die_async(const char *err, va_list params)
{
vreportf(fatal: , err, params);
if (!pthread_equal(main_thread, pthread_self())) {
+ dying = 0; /* undo counter */
struct async *async = pthread_getspecific(async_key);
if (async-proc_in = 0)
close(async-proc_in);
diff --git a/usage.c b/usage.c
index 40b3de5..cf8a968 100644
--- a/usage.c
+++ b/usage.c
@@ -6,7 +6,7 @@
#include git-compat-util.h
#include cache.h
-static int dying;
+int dying;
void vreportf(const char *prefix, const char *err, va_list params)
{
Obviously it would help to wrap it in a clear_die_counter() function,
but it would still suffer from the problem that there is no
synchronization. In the moment between incrementing and resetting the
dying counter, another thread (including the main program) could check
it. In practice, this does not happen in the current code, because we
do not start many async threads (and we only die in the main thread once
the async thread dies). But it seems unnecessarily flaky and prone to
future problems.
It would also be possible to use mutexes to make it work reliably, but
I'd be very concerned about increasing the complexity of the die code
path. We would never want a hung thread to prevent the main program
from successfully exiting, for example.
So I think the right solution is just a per-thread counter.
-Peff
--
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 log - crash and core dump
René Scharfe rene.scha...@lsrfire.ath.cx writes:
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
- unsigned long time;
- int tz;
+ unsigned long time = 0;
+ int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
- time = strtoul(ident.date_begin, date, 10);
- tz = strtol(date, NULL, 10);
+ if (ident.date_begin) {
+ time = strtoul(ident.date_begin, date, 10);
+ tz = strtol(date, NULL, 10);
+ }
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
Looks like a sensible change. split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had where the timestamp string was required), so the
updated code leaves the time/tz unspecified.
It still is curious how a malformed line was created in the first
place. I wouldn't worry if a private tool used hash-object to
create such a commit, but if it is something that is commonly used
(e.g. git commit), others may suffer from the same and the tool
needs to be tightened a bit.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] usage: allow pluggable die-recursion checks
When any git code calls die or die_errno, we use a counter
to detect recursion into the die functions from any of the
helper functions. However, such a simple counter is not good
enough for threaded programs, which may call die from a
sub-thread, killing only the sub-thread (but incrementing
the counter for everyone).
Rather than try to deal with threads ourselves here, let's
just allow callers to plug in their own recursion-detection
function. This is similar to how we handle the die routine
(the caller plugs in a die routine which may kill only the
sub-thread).
Signed-off-by: Jeff King p...@peff.net
---
git-compat-util.h | 1 +
usage.c | 20 ++--
2 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index cde442f..e955bb5 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -331,6 +331,7 @@ extern void set_error_routine(void (*routine)(const char
*err, va_list params));
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err,
va_list params));
extern void set_error_routine(void (*routine)(const char *err, va_list
params));
+extern void set_die_is_recursing_routine(int (*routine)(void));
extern int prefixcmp(const char *str, const char *prefix);
extern int suffixcmp(const char *str, const char *suffix);
diff --git a/usage.c b/usage.c
index 40b3de5..ed14645 100644
--- a/usage.c
+++ b/usage.c
@@ -6,8 +6,6 @@
#include git-compat-util.h
#include cache.h
-static int dying;
-
void vreportf(const char *prefix, const char *err, va_list params)
{
char msg[4096];
@@ -49,12 +47,19 @@ static void (*warn_routine)(const char *err, va_list
params) = warn_builtin;
vreportf(warning: , warn, params);
}
+static int die_is_recursing_builtin(void)
+{
+ static int dying;
+ return dying++;
+}
+
/* If we are in a dlopen()ed .so write to a global variable would segfault
* (ugh), so keep things static. */
static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) =
usage_builtin;
static NORETURN_PTR void (*die_routine)(const char *err, va_list params) =
die_builtin;
static void (*error_routine)(const char *err, va_list params) = error_builtin;
static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
+static int (*die_is_recursing)(void) = die_is_recursing_builtin;
void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list
params))
{
@@ -66,6 +71,11 @@ void set_error_routine(void (*routine)(const char *err,
va_list params))
error_routine = routine;
}
+void set_die_is_recursing_routine(int (*routine)(void))
+{
+ die_is_recursing = routine;
+}
+
void NORETURN usagef(const char *err, ...)
{
va_list params;
@@ -84,11 +94,10 @@ void NORETURN die(const char *err, ...)
{
va_list params;
- if (dying) {
+ if (die_is_recursing()) {
fputs(fatal: recursion detected in die handler\n, stderr);
exit(128);
}
- dying = 1;
va_start(params, err);
die_routine(err, params);
@@ -102,12 +111,11 @@ void NORETURN die_errno(const char *fmt, ...)
char str_error[256], *err;
int i, j;
- if (dying) {
+ if (die_is_recursing()) {
fputs(fatal: recursion detected in die_errno handler\n,
stderr);
exit(128);
}
- dying = 1;
err = strerror(errno);
for (i = j = 0; err[i] j sizeof(str_error) - 1; ) {
--
1.8.2.8.g44e4c28
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
On Tue, Apr 16, 2013 at 2:19 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Sure, and where is the thinking not clear? The remote helper ref is not updated, so we do update it. How is that not clear? Sure, between leaving it untouched, keeping the stale value and updating it to match what was pushed, everybody would know you mean the latter when you say correctly update. There is no third option updating it to match a random commit that is related to but is not exactly the same as what was pushed to be correct. What I felt unclear was _why_ both of these two (remote and testgit) have to get updated. In other words, correctly update it because without doing so, these bad things X, Y and Z will happen. The bad thing that would happen is that it won't be up-to-date. If you don't know what an outdated ref causes, then you don't know what transport-helper does with it, and if you don't know that, why are you bothering trying to review this patch? Is the purpose of a patch to educate people? Here it goes. The remote helper ref is going to be used to tell fast-export which refs to negate (e.g. ^refs/testgit/origin/master), so that extra commits are not generated, which the remote helper should ignore anyway, because it should already have marks for those. So doing two consecutive pushes, would push the commits twice. It's worth noting this is the first time anybody asks what is the negative effect of this not getting fixed. 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
[PATCH v2 2/2] run-command: use thread-aware die_is_recursing routine
If we die from an async thread, we do not actually exit the
program, but just kill the thread. This confuses the static
counter in usage.c's default die_is_recursing function; it
updates the counter once for the thread death, and then when
the main program calls die() itself, it erroneously thinks
we are recursing. The end result is that we print recursion
detected in die handler instead of the real error in such a
case (the easiest way to trigger this is having a remote
connection hang up while running a sideband demultiplexer).
This patch solves it by using a per-thread counter when the
async_die function is installed; we detect recursion in each
thread (including the main one), but they do not step on
each other's toes.
Other threaded code does not need to worry about this, as
they do not install specialized die handlers; they just let
a die() from a sub-thread take down the whole program.
Since we are overriding the default recursion-check
function, there is an interesting corner case that is not a
problem, but bears some explanation. Imagine the main thread
calls die(), and then in the die_routine starts an async
call. We will switch to using thread-local storage, which
starts at 0, for the main thread's counter, even though
the original counter was actually at 1. That's OK, though,
for two reasons:
1. It would miss only the first level of recursion, and
would still find recursive failures inside the async
helper.
2. We do not currently and are not likely to start doing
anything as heavyweight as starting an async routine
from within a die routine or helper function.
Signed-off-by: Jeff King p...@peff.net
---
run-command.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/run-command.c b/run-command.c
index 765c2ce..1b32a12 100644
--- a/run-command.c
+++ b/run-command.c
@@ -588,6 +588,7 @@ static pthread_key_t async_key;
static pthread_t main_thread;
static int main_thread_set;
static pthread_key_t async_key;
+static pthread_key_t async_die_counter;
static void *run_thread(void *data)
{
@@ -614,6 +615,14 @@ static NORETURN void die_async(const char *err, va_list
params)
exit(128);
}
+
+static int async_die_is_recursing(void)
+{
+ void *ret = pthread_getspecific(async_die_counter);
+ pthread_setspecific(async_die_counter, (void *)1);
+ return ret != NULL;
+}
+
#endif
int start_async(struct async *async)
@@ -695,7 +704,9 @@ int start_async(struct async *async)
main_thread_set = 1;
main_thread = pthread_self();
pthread_key_create(async_key, NULL);
+ pthread_key_create(async_die_counter, NULL);
set_die_routine(die_async);
+ set_die_is_recursing_routine(async_die_is_recursing);
}
if (proc_in = 0)
--
1.8.2.8.g44e4c28
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
On Tue, Apr 16, 2013 at 12:11 PM, Junio C Hamano gits...@pobox.com wrote:
Brandon Casey draf...@gmail.com writes:
The stash is implemented using the reflog. The ^{/text} notation
searches the commit history, not the reflog. So I think it will be
able to match the first entry in your stash stack, but not any of the
other ones.
Good point, together with...
An extension to the reflog dwimery that implements @{/text} could be
interesting though.
log -g --grep=text gives you a way to eyeball, but with
@{/text} you _might_ have a good way to name the revision.
I am not however so sure if it is useful outside the context of the
stash, because the ones you would want to recover from a normal
reflog is most likely the older version of what you already amended,
so the latest hit will likely be the post-amend version, not the one
closer to the original. You would end up eyeballing the output of
log --oneline -g -grep=text and cutting from it.
Yeah, I think that's true. I can't think of a reason, at the moment,
where it would be useful outside of with 'git stash'. I mainly wanted
to spell out @{/text} so that the mental link could be made back
to the code in git-stash that removes the @* suffix.
-Brandon
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Junio C Hamano wrote: Brandon Casey writes: Just a bit of advice, maybe you should think about softening your tone a bit hmm? I find this last sentence to be somewhat repelling and tend to refrain from responding to such. Oh, so it wasn't just me. I was about to say something similar, along the lines of people whom you just called myopic, because you did not understand the rationale behind their past work, are less likely to be inclined to help you. you would have more luck if you ask them nicely., but I've long given up on helping him be a better community member and deleted that part. I'm truly sorry. I've only had a cursory look at git-stash.sh, and was merely saying what came to my mind first after a cursory glance: it wasn't an opinion of any sort. A lot of things I say are stupid in retrospect: I'm not ashamed to admit it; I'm an inexperienced kid, and I make lots of mistakes. And please don't interpret my comments as attacking the people who wrote the code: in a community like ours, I don't believe in associating blame to any one person; I believe that all of us are equally responsible for all parts of the code as a collective; if something doesn't match what I expect, why didn't I participate in the discussion of the patch that led up to it? I complain very loudly about little things that annoy me, and I think this is a good attribute. People who are generally happy with the current state of affairs cannot make a big difference. This does not mean that I go on a stubborn rampage breaking backward compatibility everywhere, but rather that I raise the kind of questions that other people normally don't. I do not blame people for who they are: they are just a product of their histories; a sum of absorbed influences. It is, therefore, irrational to be rude to someone. If someone is not behaving as I expect them to, I send them a polite off-list email pointing out what I think their negative attributes are, and attempt to nudge them in the desired direction. I'll try to be a better community member 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
Re: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Brandon Casey wrote:
# Save another stash here
echo bash file
git add file
git stash save something
# Now git stash show stash^{/quuxery} no longer works.
Ah, yes. My stupidity. Why was I expecting ^{/quuxery} to dig
through the reflog?
An extension to the reflog dwimery that implements @{/text} could be
interesting though.
Yeah, this sounds interesting.
My initial itch that led up to this: I wanted a way to stash something
away and recover it at a later time predictably for rebase.autostash
(there might have been other stash invocations in between).
Originally, I thought I'd need a refs/stashes/* or something of the
sort to solve this problem, but git-stash.sh hard-codes refs/stash
everywhere (and so do other things like reflog). So, I was thinking
about retrieving it based on commit message, but the solution is still
short of ideal. What are your thoughts on my original refs/stashes/*
idea?
--
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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Junio C Hamano wrote:
I do not think anybody considered the approach to look at the commit
object name and making sure it appears in the reflog that implements
the stash. It sounds like a more robust check if done right.
Actually, if you think about it, there is really only one way to
specify revisions in the stash's reflog: stash@*. Since these
commits don't have to be reachable from any refs in the general case,
all the other revision syntaxes are useless. So, I would argue that
${REV%@*} is sufficient and correct*: anything beyond it is an
unnecessary overhead.
However, the initial bug is still valid: show should not show
something that pop cannot operate on.
* although I'd have been more comfortable if we had a neater way to specify that
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] help.c: add a compatibility comment to cmd_version()
External projects have been known to parse the output of
git version. Help prevent future authors from changing
its format by adding a comment to its implementation.
Signed-off-by: David Aguilar dav...@gmail.com
---
help.c | 4
1 file changed, 4 insertions(+)
diff --git a/help.c b/help.c
index 1dfa0b0..02ba043 100644
--- a/help.c
+++ b/help.c
@@ -397,6 +397,10 @@ const char *help_unknown_cmd(const char *cmd)
int cmd_version(int argc, const char **argv, const char *prefix)
{
+ /*
+* The format of this string should be kept stable for compatibility
+* with external projects that rely on the output of git version.
+*/
printf(git version %s\n, git_version_string);
return 0;
}
--
1.8.2.1.652.ge104b5e
--
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 v3 11/13] pretty: support padding placeholders, % % and %
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
+delete_trailing_dollar() {
+ sed 's/\$$//'
+}
This is what we have qz_to_tab_space for, isn't it? With it, you
can not just avoid trailing whitespace, but also indent with
spaces, like this:
Q message thousandZ
--
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: Ensimag students projects, version 2013
Junio C Hamano wrote: Who said anything about a customer? A newcomer to a community (i.e. Matthieu's student) needs not just to show technical excellence with patches, but needs to make a good argument on a larger design decision; old timers already tried to achieve a concensus on it, and did not manage to do so the last time we tried. And sorry about the huge misunderstanding. I thought Matthieu was saying that the proposed configuration variable would be harmful to newcomers, and we should therefore not add it. I feel very silly about having written such a long response to something I misunderstood. -- 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 v3 11/13] pretty: support padding placeholders, % % and %
Junio C Hamano gits...@pobox.com writes:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
+delete_trailing_dollar() {
+sed 's/\$$//'
+}
This is what we have qz_to_tab_space for, isn't it? With it, you
can not just avoid trailing whitespace, but also indent with
spaces, like this:
Q message thousandZ
Sorry, the above needs s/Q/Z/;
--
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] submodule deinit: don't output Cleared directory when directory is empty
Currently a submodule deinit run on a non-populated submodule will still
print the Cleared directory message even though the directory is already
empty. While that is technically correct (as the directory is removed and
created again), it is rather surprising to see this message for an empty
submodule directory where nothing is to be cleared.
Fix that by using 'test ! -d $(find $sm_path -maxdepth 0 -empty)' to
test for the directory being not empty before removing and recreating it.
Thanks-to: Phil Hord phil.h...@gmail.com
Signed-off-by: Jens Lehmann jens.lehm...@web.de
---
Am 16.04.2013 15:32, schrieb Phil Hord:
On Mon, Apr 1, 2013 at 3:02 PM, Jens Lehmann jens.lehm...@web.de wrote:
Okay, so here is the patch for that. If someone could point out
a portable and efficient way to check if a directory is already
empty I would be happy to use that to silence the Cleaned
directory message currently printed also when deinit is run on
an already empty directory.
isemptydir() {
test -d $(find $1 -maxdepth 0 -empty)
}
Sorry for the late reply. I see this patch is already in master
(which is fine with me).
Thanks, I managed to miss that solution when googling for it.
git-submodule.sh | 35 +++
t/t7400-submodule-basic.sh | 8
2 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..52ecbf1 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -594,27 +594,30 @@ cmd_deinit()
die_if_unmatched $mode
name=$(module_name $sm_path) || exit
- # Remove the submodule work tree (unless the user already did
it)
- if test -d $sm_path
+ # Remove the submodule work tree (unless the user already did
it or it is empty)
+ if test ! -d $(find $sm_path -maxdepth 0 -empty)
then
- # Protect submodules containing a .git directory
- if test -d $sm_path/.git
+ if test -d $sm_path
then
- echo 2 $(eval_gettext Submodule work tree
'\$sm_path' contains a .git directory)
- die $(eval_gettext (use 'rm -rf' if you
really want to remove it including all of its history))
- fi
+ # Protect submodules containing a .git directory
+ if test -d $sm_path/.git
+ then
+ echo 2 $(eval_gettext Submodule
work tree '\$sm_path' contains a .git directory)
+ die $(eval_gettext (use 'rm -rf' if
you really want to remove it including all of its history))
+ fi
- if test -z $force
- then
- git rm -qn $sm_path ||
- die $(eval_gettext Submodule work tree
'\$sm_path' contains local modifications; use '-f' to discard them)
+ if test -z $force
+ then
+ git rm -qn $sm_path ||
+ die $(eval_gettext Submodule work
tree '\$sm_path' contains local modifications; use '-f' to discard them)
+ fi
+ rm -rf $sm_path
+ say $(eval_gettext Cleared directory
'\$sm_path') ||
+ say $(eval_gettext Could not remove submodule
work tree '\$sm_path')
fi
- rm -rf $sm_path
- say $(eval_gettext Cleared directory '\$sm_path') ||
- say $(eval_gettext Could not remove submodule work
tree '\$sm_path')
- fi
- mkdir $sm_path || say $(eval_gettext Could not create empty
submodule directory '\$sm_path')
+ mkdir $sm_path || say $(eval_gettext Could not
create empty submodule directory '\$sm_path')
+ fi
# Remove the .git/config entries (unless the user already did
it)
if test -n $(git config --get-regexp submodule.$name\.)
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ff26535..b56e4a5 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -792,7 +792,7 @@ test_expect_success 'submodule deinit deinits a submodule
when its work tree is
test -z $(git config --get-regexp submodule\.example\.)
test -z $(git config --get-regexp submodule\.example2\.)
test_i18ngrep ! Cleared directory .init actual
- test_i18ngrep Cleared directory .example2 actual
+ test_i18ngrep ! Cleared directory .example2 actual
rmdir init
'
@@ -842,15 +842,15 @@
Re: [PATCH 3/3] pull: introduce --[no-]autostash and pull.autostash
Phil Hord wrote: If I follow the latter advice about 'rm -rf', who will remember that 'rebase' had something stashed, and what will they do with it when they do? What if it is weeks or months later? I would be surprised to see long-forgotten wip show up in my workspace all of a sudden. Ultimately, I think an ideal implementation requires this entire autostash implementation to reside completely within the $state_dir. The issue of a long-forgotten WIP is then the same issue as a long-forgotten rebase, and a rm -rf $state_dir will get rid of the WIP as well. The other reason is that it shouldn't interact with the rest of git. This means no touching any refs or reflogs, and this can be problematic if we decide to use the standard git stash. I'm currently working towards seeing if it's possible to get stash to create named stashes that we can predictably retrieve later, to avoid rolling our own homegrown stash. Yes, the problem is much more complex than I initially thought. It was much simpler to implement it for git-pull.sh. -- 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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
Ramkumar Ramachandra artag...@gmail.com writes:
Junio C Hamano wrote:
I do not think anybody considered the approach to look at the commit
object name and making sure it appears in the reflog that implements
the stash. It sounds like a more robust check if done right.
Actually, if you think about it, there is really only one way to
specify revisions in the stash's reflog: stash@*.
...
* although I'd have been more comfortable if we had a neater way to specify
that
Yup. git stash pop +4, without a must-be-it token stash or
mandatory @{} frill would have been much nicer.
--
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 log - crash and core dump
First, lest I forget again: Thank you, Ivan, for the very useful
bug report!
Am 16.04.2013 21:45, schrieb Junio C Hamano:
René Scharfe rene.scha...@lsrfire.ath.cx writes:
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
-unsigned long time;
-int tz;
+unsigned long time = 0;
+int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
-time = strtoul(ident.date_begin, date, 10);
-tz = strtol(date, NULL, 10);
+if (ident.date_begin) {
+time = strtoul(ident.date_begin, date, 10);
+tz = strtol(date, NULL, 10);
+}
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
Looks like a sensible change. split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had where the timestamp string was required), so the
updated code leaves the time/tz unspecified.
We'd need update pretty.c::format_person_part() and
builtin/blame.c::get_ac_line() as well, though.
How about making split_ident_line() a bit friendlier be letting it
provide the epoch as default time stamp instead of NULL? We shouldn't
do that if we'd like to be able to tell a missing/broken time stamp
apart from a commit that was actually made back in 1970 (e.g. an
imported one). Or if we'd like to not show a time stamp in git log
output at all in that case.
-- 8 --
Subject: ident: let split_ident_line() provide a default time stamp
If a commit has a broken time stamp, split_ident_line() sets
date_begin, date_end, tz_begin and tz_end to NULL. Not all callers
are prepared to handle that case and segfault.
Instead of fixing them and having to be careful while implementing
the next caller, provide a string consisting of the number zero as
default value, representing the UNIX epoch. That's the value that
git log showed before it was converted to use split_ident_line().
Reported-by: Ivan Lyapunov dron...@gmail.com
Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
---
ident.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/ident.c b/ident.c
index 1c123e6..ee840f4 100644
--- a/ident.c
+++ b/ident.c
@@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb,
const char *src)
sb-buf[sb-len] = '\0';
}
+static const char zero_string[] = 0;
+
/*
* Reverse of fmt_ident(); given an ident line, split the fields
* to allow the caller to parse it.
@@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const
char *line, int len)
return 0;
person_only:
- split-date_begin = NULL;
- split-date_end = NULL;
- split-tz_begin = NULL;
- split-tz_end = NULL;
+ split-date_begin = zero_string;
+ split-date_end = zero_string + strlen(zero_string);
+ split-tz_begin = zero_string;
+ split-tz_end = zero_string + strlen(zero_string);
return 0;
}
--
1.8.2.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: [RFC/PATCH 0/2] Test the Git version string
From: Junio C Hamano gits...@pobox.com Sent: Tuesday, April 16, 2013 7:12 PM Philip Oakley philipoak...@iee.org writes: What kind of benefit are you envisioning out of this? The purpose of tests is to detect mistakes and spot regressions. A change to the 'git version X.Y.z' string would be a regression, as I spotted earlier, as it conflicts with expectations of standard programmes such as git-gui. Sorry, but I do not follow. A released version says git version 1.8.2.1. In a month or so, I'll have another one that says git version 1.8.3. Or I may decide to bump in preparation for 2.0 and it may identify itself as git version 1.9. Neither of which no existing program such as git-gui has ever seen. In what way is that a regression? But they both pass the test and test suite, yes? And even if you use git-gui with them, git-gui will not barf on start up, which it would if the version string fails my test. Passing the test suite should be a reasonble guarantee that co-tools will work, including those that check for version. This is a check for that version string. However if someone[1] creates My Special Git Version 1-9-3 (Index V7b), and here I'm suggesting they may have other special changes, then the version check will tell them that even when they have fixed their special changes to pass the other parts of the test suite, other co-tools could barf. Its about pushing the piece of string from the users end ;-) It also stops others trying to change 'git' to 'Git' within this message, just as I did. Philip [1] my first draft had 'you', but that is not a reasonable starting position. It's more about *others* attempting to create release versions, which announce their name, that we expect to be compatible with say git-gui (via the rest of the test suite), and need to check that announcement. -- 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 log - crash and core dump
On Tue, Apr 16, 2013 at 9:45 PM, Junio C Hamano gits...@pobox.com wrote: It still is curious how a malformed line was created in the first place. I wouldn't worry if a private tool used hash-object to create such a commit, but if it is something that is commonly used (e.g. git commit), others may suffer from the same and the tool needs to be tightened a bit. I already happened to see one like that, and it was clearly imported through remote-hg. I've not been able to reproduce though, and the parser in git-fast-import seemed already robust enough to me to not allow this kind of messed-up line. I will see if I can find some time to reproduce/investigate this deeper. -- 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: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}
On Tue, Apr 16, 2013 at 1:11 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
Brandon Casey wrote:
# Save another stash here
echo bash file
git add file
git stash save something
# Now git stash show stash^{/quuxery} no longer works.
Ah, yes. My stupidity. Why was I expecting ^{/quuxery} to dig
through the reflog?
An extension to the reflog dwimery that implements @{/text} could be
interesting though.
Yeah, this sounds interesting.
My initial itch that led up to this: I wanted a way to stash something
away and recover it at a later time predictably for rebase.autostash
(there might have been other stash invocations in between).
Originally, I thought I'd need a refs/stashes/* or something of the
sort to solve this problem, but git-stash.sh hard-codes refs/stash
everywhere (and so do other things like reflog). So, I was thinking
about retrieving it based on commit message, but the solution is still
short of ideal. What are your thoughts on my original refs/stashes/*
idea?
You can create a stash without modifying the refs/stash reflog using
'sha1=`git stash create`' and then later apply it using 'git stash
apply --index $sha1'. You'll have to reset the work directory
yourself though since 'git stash create' does not do so. The stash
created this way is just a dangling commit so it will have a lifetime
according to the gc.pruneexpire (default 2 weeks currently).
-Brandon
--
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
