Re: [PATCH] builtin/pack-objects.c:write_pack_file() replace tmpname with pack_tmp_name in warning
Sun He sunheeh...@gmail.com writes: Signed-off-by: Sun He sunheeh...@gmail.com --- The tmpname is uninitialized and it should a bug Please ignore the former patches about this with wrong format. I am sorry to cause a jam in your inbox. ^_^ In the end, I wanna thank Michael Haggerty who give me help. If you look at git log output, you would notice that people write something like Helped-by: Michael Haggerty mhag...@alum.mit.edu before your S-o-b: line for a case like this. We can see that you are replacing tmpname with pack_tmp_name in warning without you writing on the subject line. The commit log is where you explain *why* that change is the right thing to do. And that is totally lacking in this message. builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ static void write_pack_file(void) utb.modtime = --last_mtime; if (utime(pack_tmp_name, utb) 0) warning(failed utime() on %s: %s, - tmpname, strerror(errno)); + pack_tmp_name, strerror(errno)); } /* Enough space for -sha-1.pack? */ -- To unsubscribe from this list: send the line 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 1/2] Add docstrings for lookup_replace_object() and do_lookup_replace_object()
Michael Haggerty mhag...@alum.mit.edu writes:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
Reviewed-by: Junio C Hamano gits...@pobox.com
Thanks.
cache.h | 13 +
replace_object.c | 7 +++
2 files changed, 20 insertions(+)
diff --git a/cache.h b/cache.h
index b039abc..9407560 100644
--- a/cache.h
+++ b/cache.h
@@ -798,13 +798,26 @@ static inline void *read_sha1_file(const unsigned char
*sha1, enum object_type *
{
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
}
+
+/*
+ * This internal function is only declared here for the benefit of
+ * lookup_replace_object(). Please do not call it directly.
+ */
extern const unsigned char *do_lookup_replace_object(const unsigned char
*sha1);
+
+/*
+ * If object sha1 should be replaced, return the replacement object's
+ * name (replaced recursively, if necessary). The return value is
+ * either sha1 or a pointer to a permanently-allocated value. When
+ * object replacement is suppressed, always return sha1.
+ */
static inline const unsigned char *lookup_replace_object(const unsigned char
*sha1)
{
if (!check_replace_refs)
return sha1;
return do_lookup_replace_object(sha1);
}
+
static inline const unsigned char *lookup_replace_object_extended(const
unsigned char *sha1, unsigned flag)
{
if (!(flag LOOKUP_REPLACE_OBJECT))
diff --git a/replace_object.c b/replace_object.c
index c5cf9f4..31fabde 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -92,6 +92,13 @@ static void prepare_replace_object(void)
/* We allow recursive replacement. Only within reason, though */
#define MAXREPLACEDEPTH 5
+/*
+ * If a replacement for object sha1 has been set up, return the
+ * replacement object's name (replaced recursively, if necessary).
+ * The return value is either sha1 or a pointer to a
+ * permanently-allocated value. This function always respects replace
+ * references, regardless of the value of check_replace_refs.
+ */
const unsigned char *do_lookup_replace_object(const unsigned char *sha1)
{
int pos, depth = MAXREPLACEDEPTH;
--
To unsubscribe from this list: send the line 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] help.c: rename function pretty_print_string_list
Ralf Thielow ralf.thie...@gmail.com writes:
The part string_list of the name of function
pretty_print_string_list is just an implementation
detail. The function pretty-prints command names so
rename it to pretty_print_cmdnames.
Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
Just noticed this while digging through Git codebase.
Thanks. This is a leftover from 7dbc2c04 (git wrapper: basic
fixes., 2005-11-15); it used to be that this function was to pretty
print the contents of a string_list but it was updated to use its
own structure type.
help.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/help.c b/help.c
index df7d16d..b266b09 100644
--- a/help.c
+++ b/help.c
@@ -78,8 +78,7 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames
*excludes)
cmds-cnt = cj;
}
-static void pretty_print_string_list(struct cmdnames *cmds,
- unsigned int colopts)
+static void pretty_print_cmdnames(struct cmdnames *cmds, unsigned int
colopts)
{
struct string_list list = STRING_LIST_INIT_NODUP;
struct column_options copts;
@@ -209,14 +208,14 @@ void list_commands(unsigned int colopts,
const char *exec_path = git_exec_path();
printf_ln(_(available git commands in '%s'), exec_path);
putchar('\n');
- pretty_print_string_list(main_cmds, colopts);
+ pretty_print_cmdnames(main_cmds, colopts);
putchar('\n');
}
if (other_cmds-cnt) {
printf_ln(_(git commands available from elsewhere on your
$PATH));
putchar('\n');
- pretty_print_string_list(other_cmds, colopts);
+ pretty_print_cmdnames(other_cmds, colopts);
putchar('\n');
}
}
--
To unsubscribe from this list: send the line 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 update: document the '--checkout' option
Jens Lehmann jens.lehm...@web.de writes: Good point. What about this? Documentation/git-submodule.txt | 13 +++-- git-submodule.sh| 2 +- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index bfef8a0..9054217 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -15,8 +15,8 @@ SYNOPSIS 'git submodule' [--quiet] init [--] [path...] 'git submodule' [--quiet] deinit [-f|--force] [--] path... 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] - [-f|--force] [--rebase] [--reference repository] [--depth depth] - [--merge] [--recursive] [--] [path...] + [-f|--force] [--checkout|--merge|--rebase] [--reference repository] + [--depth depth] [--recursive] [--] [path...] This has already been done by 23d25e48 (submodule: explicit local branch creation in module_clone, 2014-01-26). That commit also adds some text to the description of 'update' subcommand, but not a separate entry for '--checkout' mode. Does the result of applying this patch except for this particular hunk still make sense as a whole? It appears to me that it does, but just to double check... Thanks. 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) n] [commit] [--] [path...] 'git submodule' [--quiet] foreach [--recursive] command @@ -287,6 +287,15 @@ SHA-1. If you don't want to fetch, you should use `submodule update This option is only valid for the update command. Don't fetch new objects from the remote site. +--checkout:: + This option is only valid for the update command. + Checkout the commit recorded in the superproject on a detached HEAD + in the submodule. This is the default behavior, the main use of + this option is to override `submodule.$name.update` when set to + `merge`, `rebase` or `none`. + If the key `submodule.$name.update` is either not explicitly set or + set to `checkout`, this option is implicit. + --merge:: This option is only valid for the update command. Merge the commit recorded in the superproject into the current branch diff --git a/git-submodule.sh b/git-submodule.sh index 4a30087..65cf963 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -9,7 +9,7 @@ USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference re or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...] or: $dashless [--quiet] init [--] [path...] or: $dashless [--quiet] deinit [-f|--force] [--] path... - or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] [--] [path...] + or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference repository] [--recursive] [--] [path...] or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n] [commit] [--] [path...] or: $dashless [--quiet] foreach [--recursive] command or: $dashless [--quiet] sync [--recursive] [--] [path...] -- To unsubscribe from this list: send the line 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: Branch Name Case Sensitivity
Duy Nguyen pclo...@gmail.com writes: On Sat, Mar 1, 2014 at 1:58 AM, Junio C Hamano gits...@pobox.com wrote: Karsten Blees karsten.bl...@gmail.com writes: If you are on a case-insensitive filesystem, or work on a cross-platform project, ensure that you avoid ambiguous refs. Problem solved. So its OK to lose data if you accidentally use an ambiguous ref? I cannot believe you actually meant that. I think he meant what he said: you avoid ambiguous refs. He did not say it is not Git's business to help you doing so. I think it is prudent to warn in the end-user facing layer (read: do not touch refs.c to implement something like that) when the user creates refs/heads/Next when there already is refs/heads/next, and I further think it would make sense to do so even on case sensitive platforms. That does not help when the user creates next and pulls Next from elsewhere, does it? That depends on what the project policy would be. At that point, that user needs to talk with the elsewhere person and resolve the issue (if there is one) according to the policy of their project, and it is not Git's business to _solve_ it for them. Warning I suggested was a way to help avoiding without getting in a way of projects whose policy is to allow these. -- To unsubscribe from this list: send the line 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] i18n: proposed command missing leading dash
Jonathan Nieder jrnie...@gmail.com writes:
To make life saner for translators, this should be either
untranslatable or a single multi-line string, I suspect:
diff --git i/builtin/branch.c w/builtin/branch.c
index b4d7716..972040c 100644
--- i/builtin/branch.c
+++ w/builtin/branch.c
@@ -1022,11 +1022,13 @@ int cmd_branch(int argc, const char **argv, const
char *prefix)
*/
if (argc == 1 track == BRANCH_TRACK_OVERRIDE
!branch_existed remote_tracking) {
- fprintf(stderr, _(\nIf you wanted to make '%s' track
'%s', do this:\n\n), head, branch-name);
- fprintf(stderr, _(git branch -d %s\n),
branch-name);
- fprintf(stderr, _(git branch --set-upstream-to
%s\n), branch-name);
+ fprintf(stderr, \n);
+ fprintf(stderr, _(If you wanted to make '%s' track
'%s', do this:\n\n
+ git branch -d %s\n
+ git branch --set-upstream-to
%s),
+ head, branch-name, branch-name,
branch-name);
+ fprintf(stderr, \n);
}
-
} else
usage_with_options(builtin_branch_usage, options);
What do you think?
Yes, I think it is sensible to make sure that the command examples
are not corrupted by the _() 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 v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer
Duy Nguyen pclo...@gmail.com writes:
On Thu, Feb 20, 2014 at 6:26 AM, Junio C Hamano gits...@pobox.com wrote:
Is there a reason not to do just an equivalent of
#define git_pathdup mkpathdup
and be done with it? Am I missing something?
They have a subtle difference: mkpathdup() calls cleanup_path() while
git_pathdup() does not. Without auditing all call sites, we can't be
sure this difference is insignificant. So I keep both functions
separate for now.
Thanks; that is a very good explanation why #define'ing two to be
the same would not be a workable solution to the ownership issue I
raised.
char *git_pathdup(const char *fmt, ...)
{
- char path[PATH_MAX], *ret;
+ struct strbuf *path = get_pathname();
va_list args;
va_start(args, fmt);
- ret = vsnpath(path, sizeof(path), fmt, args);
+ vsnpath(path, fmt, args);
va_end(args);
- return xstrdup(ret);
+ return strbuf_detach(path, NULL);
}
This feels somewhat wrong.
This function is for callers who are willing to take ownership of
the path buffer and promise to free the returned buffer when they
are done, because you are returning strbuf_detach()'ed piece of
memory, giving the ownership away.
The whole point of using get_pathname() is to allow callers not to
care about allocation issues on the paths they scribble on during
their short-and-simple codepaths that do not have too many uses of
similar temporary path buffers. Why borrow from that round-robin
pool (which may now cause some codepaths to overflow the number of
such active temporary path buffers---have they been all audited)?
--
To unsubscribe from this list: send the line 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: Branch Name Case Sensitivity
Lee Hopkins leer...@gmail.com writes:
I went ahead and took a stab at a solution. My solution is more
aggressive than a warning, I actually prevent the creation of
ambiguous refs. My changes are also in refs.c, which may not be
appropriate, but it seemed like the natural place.
I have never contributed to Git (in fact this is my first dive into
the source) and my C is a bit rusty, so bear with me, this is just a
suggestion:
---
refs.c | 31 ---
1 files changed, 24 insertions(+), 7 deletions(-)
Starting something like this from forbidding is likely to turn out
to be a very bad idea that can break existing repositories.
A new configuration
refs.caseInsensitive = {warn|error|allow}
that defaults to warn and the user can choose to set to error to
forbid, would be more palatable, I would say.
If the variable is not in 'core.' namespace, you should implement
this check at the Porcelain level, allowing lower-level tools like
update-ref as an escape hatch that let users bypass the restriction
to be used to correct breakages; it would mean an unconditional if
!stricmp(), it is an error in refs.c will not work well.
I think it might be OK to have
core.allowCaseInsentitiveRefs = {yes|no|warn}
which defaults to 'warn' (and 'yes' corresponds to 'allow', 'no'
corresponds to 'error', in the previous suggestion), instead. If we
wanted to prevent even lower-level tools like update-ref from
bypassing the check, that is.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 07/25] reflog: avoid constructing .lock path with git_path
Duy Nguyen pclo...@gmail.com writes: On Wed, Feb 26, 2014 at 5:44 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: git_path() soon understands the path given to it. Some paths abc may become def while other ghi may become ijk. We don't want git_path() to interfere with .lock path construction. Concatenate .lock after the path has been resolved by git_path() so if abc becomes def, we'll have def.lock, not ijk. Hmph. I am not sure if the above is readable (or if I am reading it correctly). Definitely not now that I have had my break from the series and reread it. If abc becomes def, it would take deliberate work to make abc.lock into ijk, and it would be natural to expect that abc.lock would become def.lock without any fancy trick, no? A better explanation may be, while many paths are not converted by git_path() (abc - abc), some of them will be based on the given path (def - ghi). Giving path def.lock to git_path() may confuse it and make it believe def.lock should not be converted because the signature is def.lock not def. But we want the lock file to have the same base name with the locked file (e.g. ghi.lock, not def.lock). So it's best to append .lock after git_path() has done its conversion. The alternative is teach git_path about .lock, but I don't really want to put more logic down there. I think your attempt to sound generic by using abc, def, etc. backfired and made the description only obscure. It would have been immediately understandable if it were more like this: Among pathnames in $GIT_DIR, e.g. index or packed-refs, we want to automatically and silently map some of them to the $GIT_DIR of the repository we are borrowing from via $GIT_COMMON_DIR mechanism. When we formulate the pathname for its lockfile, we want it to be in the same location as its final destination. index is not shared and needs to remain in the borrowing repository, while packed-refs is shared and needs to go to the borrowed repository. git_path() could be taught about the .lock suffix and map index.lock and packed-refs.lock the same way their basenames are mapped, but instead the caller can help by asking where the basename (e.g. index) is mapped to git_path() and then appending .lock after the mapping is done. Thanks for an explanation. With that understanding, the patch makes sense. -- To unsubscribe from this list: send the line 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] repack: add `repack.honorpackkeep` config var
Jeff King p...@peff.net writes: On Fri, Feb 28, 2014 at 10:45:39AM -0800, Junio C Hamano wrote: Exactly. The two features (bitmaps and .keep) are not compatible with each other, so you have to prioritize one. If you are using static .keep files, you might want them to continue being respected at the expense of using bitmaps for that repo. So I think you want a separate option from --write-bitmap-index to allow the appropriate flexibility. What is the appropriate flexibility, though? If the user wants to use bitmap, we would need to drop .keep, no? Or the flip side: if the user wants to use .keep, we should drop bitmaps. My point is that we do not know which way the user wants to go, so we should not tie the options together. Hmph. I think the short of your later explanation is global config may tell us to use bitmap, in which case we would need a way to defeat that and have existing .keep honored, and it makes it easier to do so if these two are kept separate, because you do not want to run around and selectively disable bitmaps in these repositories. We can instead do so with repack.packKeptObjects in the global configuration. and I tend to agree with the reasoning. 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 v2 00/11] Use ALLOC_GROW() instead of inline code
Michael Haggerty mhag...@alum.mit.edu writes: On 02/28/2014 10:29 AM, Dmitry S. Dolzhenko wrote: Thank you for your remarks. In this patch I tried to take them into account. Dmitry S. Dolzhenko (11): builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW() bundle.c: change add_to_ref_list() to use ALLOC_GROW() cache-tree.c: change find_subtree() to use ALLOC_GROW() commit.c: change register_commit_graft() to use ALLOC_GROW() diff.c: use ALLOC_GROW() instead of inline code diffcore-rename.c: use ALLOC_GROW() instead of inline code patch-ids.c: change add_commit() to use ALLOC_GROW() replace_object.c: change register_replace_object() to use ALLOC_GROW() reflog-walk.c: use ALLOC_GROW() instead of inline code dir.c: change create_simplify() to use ALLOC_GROW() attr.c: change handle_attr_line() to use ALLOC_GROW() attr.c | 7 +-- builtin/pack-objects.c | 7 +-- bundle.c | 6 +- cache-tree.c | 6 +- commit.c | 8 ++-- diff.c | 12 ++-- diffcore-rename.c | 12 ++-- dir.c | 5 + patch-ids.c| 5 + reflog-walk.c | 13 +++-- replace_object.c | 8 ++-- 11 files changed, 17 insertions(+), 72 deletions(-) Everything looks fine to me. Assuming the test suite ran 100%, Acked-by: Michael Haggerty mhag...@alum.mit.edu Looked good (modulo titles, which I think we already discussed), and queued on 'pu'. 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 v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
Jeff King p...@peff.net writes: I realize that I just bikeshedded on subject lines for half a page, and part of me wants to go kill myself in shame. But I feel like I see the technique misapplied often enough that maybe some guidance is merited. Thanks. What I queued read like these: $ git shortlog ..dd/use-alloc-grow Dmitry S. Dolzhenko (11): builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path() bundle.c: use ALLOC_GROW() in add_to_ref_list() cache-tree.c: use ALLOC_GROW() in find_subtree() commit.c: use ALLOC_GROW() in register_commit_graft() diff.c: use ALLOC_GROW() instead of inline code diffcore-rename.c: use ALLOC_GROW() instead of inline code patch-ids.c: use ALLOC_GROW() in add_commit() replace_object.c: use ALLOC_GROW() in register_replace_object() reflog-walk.c: use ALLOC_GROW() instead of inline code dir.c: use ALLOC_GROW() in create_simplify() attr.c: use ALLOC_GROW() in handle_attr_line() but I tend to agree with you that we can just stop at use ALLOC_GROW after the filename. -- To unsubscribe from this list: send the line 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] implemented strbuf_write_or_die()
Faiz Kothari faiz.of...@gmail.com writes: Signed-off-by: Faiz Kothari faiz.of...@gmail.com --- Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places to substitute write_or_die(). I spotted other places where strbuf can be used in place of buf[MAX_PATH] but that would require a change in prototype of a lot of functions and functions calling them and so on I'll look for more places where strbuf can be used safely. Thanks. builtin/cat-file.c |2 +- builtin/notes.c|4 ++-- builtin/receive-pack.c |2 +- builtin/send-pack.c|2 +- builtin/stripspace.c |2 +- builtin/tag.c |2 +- bundle.c |2 +- cache.h|1 + credential-store.c |2 +- fetch-pack.c |2 +- http-backend.c |2 +- remote-curl.c |8 +--- write_or_die.c |9 + 13 files changed, 26 insertions(+), 14 deletions(-) It does not reduce the code, it does not make the resulting code read any easier. What is the benefit of this change? -- To unsubscribe from this list: send the line 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] implemented strbuf_write_or_die()
Eric Sunshine sunsh...@sunshineco.com writes: On Sat, Mar 1, 2014 at 7:51 AM, He Sun sunheeh...@gmail.com wrote: 2014-03-01 19:21 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: diff --git a/remote-curl.c b/remote-curl.c index 10cb011..dee8716 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads) if (start_command(client)) exit(1); if (preamble) - write_or_die(client.in, preamble-buf, preamble-len); + strbuf_write_or_die(client.in, preamble); if (heads) write_or_die(client.in, heads-buf, heads-len); This should be changed. May be you can use Ctrl-F to search write_or_die(). Or if you are using vim, use / and n to find all. It's not obvious from the patch fragment, but 'heads' is not a strbuf, so Faiz correctly left this invocation alone. That is a very good sign why this change is merely a code-churn and not an improvement, isn't it? We know (and any strbuf user should know) that -buf and -len are the ways to learn the pointer and the length the strbuf holds. Why anybody thinks it is benefitial to introduce another function that is _only_ for writing out strbuf and cannot be used to write out a plain buffer is simply beyond 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: An idea for git bisect and a GSoC enquiry
Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: Here my proposal differs in that I have no way of knowing which label is good and which label is bad: I blindly accept two distinct labels and bisect with those. I gave an example of this behaviour above. I think you fundamentally cannot use two labels that are merely distinct and bisect correctly. You need to know which ones (i.e. good) are to be excluded and the other (i.e. bad) are to be included when computing the remaining to be tested set of commits. -- To unsubscribe from this list: send the line 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] commit.c:record_author_date() use skip_prefix() instead of starts_with()
Michael Haggerty mhag...@alum.mit.edu writes:
-if (!starts_with(buf, author )) {
+if (!skip_prefix(buf, author )) {
If this is the only change, there is not much point, is there? How does
this help? Perhaps there is some way to take advantage of the
difference between starts_with() and skip_prefix() to simplify the rest
of the function?
I admit I lost track, but wasn't there a discussion to use
starts_with/ends_with when appropriate (namely, the caller is
absolutely not interested in what the remainder of the string is
after skipping the prefix), moving away from skip_prefix()? Isn't
this change going in the wrong direction?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] write_pack_file: use correct variable in diagnostic
Sun He sunheeh...@gmail.com writes: 'pack_tmp_name' is the subject of the utime() check, so report it in the warning, not the uninitialized 'tmpname' Signed-off-by: Sun He sunheeh...@gmail.com --- Changing the subject and adding valid information as tutored by Eric Sunshine. Thanks to him. builtin/pack-objects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index c733379..4922ce5 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -823,7 +823,7 @@ static void write_pack_file(void) utb.modtime = --last_mtime; if (utime(pack_tmp_name, utb) 0) warning(failed utime() on %s: %s, - tmpname, strerror(errno)); + pack_tmp_name, strerror(errno)); } /* Enough space for -sha-1.pack? */ Very nicely done. Thanks. And big Thanks to Eric guiding this patch through. -- To unsubscribe from this list: send the line 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] branch.c: change install_branch_config() to use skip_prefix()
Eric Sunshine sunsh...@sunshineco.com writes:
diff --git a/branch.c b/branch.c
index 723a36b..ca4e824 100644
--- a/branch.c
+++ b/branch.c
@@ -50,7 +50,7 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
const char *shortname = remote + 11;
- int remote_is_branch = starts_with(remote, refs/heads/);
+ int remote_is_branch = (NULL != skip_prefix(remote ,refs/heads/));
This actually makes the code more difficult to read and understand.
There's a more fundamental reason to use skip_prefix() here. See if
you can figure it out. (Hint: shortname)
I've already queued 0630aa49 (branch: use skip_prefix() in
install_branch_config(), 2014-02-28) on this topic, by the way.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-config: document interactive.singlekey requires Term::Readkey
Simon Ruderich si...@ruderich.org writes:
Most distributions don't require Term::Readkey as dependency,
leaving the user to wonder why the setting doesn't work.
Signed-off-by: Simon Ruderich si...@ruderich.org
Thanks, but is it true that interactive.singlekey requries
Term::ReadKey?
The relevant part of git-add--interactive reads like so:
if ($repo-config_bool(interactive.singlekey)) {
eval {
require Term::ReadKey;
Term::ReadKey-import;
$use_readkey = 1;
};
eval {
require Term::Cap;
my $termcap = Term::Cap-Tgetent;
foreach (values %$termcap) {
$term_escapes{$_} = 1 if /^\e/;
}
$use_termcap = 1;
};
}
The implementation of prompt_single_character sub wants to use
ReadKey, but can still let the user interact with the program by
falling back to a cooked input when it is not available, so perhaps
a better fix might be something like this:
if (!$use_readkey) {
print STDERR missing Term::ReadKey, disabling
interactive.singlekey\n;
}
inside the above if() that prepares $use_readkey?
You also misspelled the package name it seems ;-)
--
To unsubscribe from this list: send the line 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] skip_prefix: rewrite so that prefix is scanned once
Siddharth Goel siddharth98...@gmail.com writes:
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Siddharth Goel siddharth98...@gmail.com
---
Added a space after colon in the subject as compared to previous
patch [PATCH v2].
[PATCH v2]: http://thread.gmane.org/gmane.comp.version-control.git/243150
Whenever you see Change, Rewrite, etc. in the subject of a patch
that touches existing code, think twice. The subject line is a
scarce real estate to be wasted on a noiseword that carries no real
information, and we already know a patch that touches existing code
changes or rewrites things.
Subject: [PATCH v3] skip_prefix: scan prefix only once
perhaps?
git-compat-util.h | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 614a5e9..550dce3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
*suffix);
static inline const char *skip_prefix(const char *str, const char *prefix)
{
- size_t len = strlen(prefix);
- return strncmp(str, prefix, len) ? NULL : str + len;
+ while (*prefix != '\0' *str == *prefix) {
+ str++;
+ prefix++;
+ }
+ return (*prefix == '\0' ? str : NULL);
Unlike another patch I saw the other day on the same topic, this
checks *prefix twice for the last round, even though I think this
one is probably slightly easier to read. I dunno.
--
To unsubscribe from this list: send the line 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 : Add --no-separate-git-dir option to add and update command.
[CC'ing the submodule area experts.] Henri GEIST geist.he...@laposte.net writes: This new option prevent git submodule add|update to clone the missing submodules with the --separate-git-dir option. Then the submodule will be regular repository and their gitdir will not be placed in the superproject gitdir/modules directory. Signed-off-by: Henri GEIST geist.he...@laposte.net --- Thanks. The above describes what the new option does, but does not explain why the new option is a good idea in the first place. Given that we used to directly clone into the superproject's working tree like this patch does, realized that it was a very bad idea and are trying to move to the direction of keeping it in modules/ subdirectory of the superproject's .git directory, there needs to be a very good explanation to justify why this going backwards is sometimes a desirable thing. -- To unsubscribe from this list: send the line 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] commit.c: Use skip_prefix() instead of starts_with()
Tanay Abhra tanay...@gmail.com writes:
In record_author_date() parse_gpg_output() ,using skip_prefix() instead of
starts_with() is more elegant and abstracts away the details.
Avoid subjective judgement like more elegant when justifying your
change; you are not your own judge.
The caller of starts_with() actually can use the string that follows
the expected prefix and that is the reason why using skip_prefix()
in these places is a good idea. There is no need to be subjective
to justify that change.
I do not think there is any more abstracting away of the details in
this change. The updated uses a different and more suitable
abstraction than the original.
diff --git a/commit.c b/commit.c
index 6bf4fe0..668c703 100644
--- a/commit.c
+++ b/commit.c
@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
static void record_author_date(struct author_date_slab *author_date,
struct commit *commit)
{
- const char *buf, *line_end;
+ const char *buf, *line_end, *skip;
char *buffer = NULL;
struct ident_split ident;
char *date_end;
@@ -566,14 +566,15 @@ static void record_author_date(struct author_date_slab
*author_date,
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ if (!(skip = skip_prefix(buf, author ))) {
We tend to avoid assignments in conditionals.
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
}
+ buf = skip;
if (split_ident_line(ident,
- buf + strlen(author ),
- line_end - (buf + strlen(author ))) ||
+ buf,
+ line_end - buf) ||
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
If you give a sensible name to what 'buf + strlen(author )' is,
then the result becomes a lot more readable compared to the
original, and I think that is what this change is about.
And skip is not a good name for that. 'but + strlen(author )'
is what split_ident_line() expects its input to be split; let's
tentatively call it ident_line and see what the call looks like:
split_ident_line(ident, ident_line, line_end - ident_line))
And that is what we want to see here. It is a bit more clear than
the original that we are splitting the ident information on the line,
ident_line (you could call it ident_begin) points at the beginning
and line_end points at the end of that ident information.
Use of skip_prefix(), which I am sure you took the name of the new
variable skip from, is merely an implementation detail of finding
where the ident begins. A good rule of thumb to remember is to name
things after what they are, not how you obtain them, how they are
used or what they are used for/as.
@@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
+ if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1))
{
/* At the very beginning of the buffer */
- found = buf + strlen(sigcheck_gpg_status[i].check + 1);
+ ;
} else {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
This hunk looks good. It can be a separate patch but they are both
minor changes so it is OK to have it in a single patch.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack: add `repack.honorpackkeep` config var
Jeff King p...@peff.net writes:
On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:
Or the flip side: if the user wants to use .keep, we should drop
bitmaps. My point is that we do not know which way the user wants to
go, so we should not tie the options together.
Hmph. I think the short of your later explanation is global config
may tell us to use bitmap, in which case we would need a way to
defeat that and have existing .keep honored, and it makes it easier
to do so if these two are kept separate, because you do not want to
run around and selectively disable bitmaps in these repositories.
We can instead do so with repack.packKeptObjects in the global
configuration. and I tend to agree with the reasoning.
Yes. Do you need a re-roll from me? I think the last version I sent +
the squash to tie the default to bitmap-writing makes the most sense.
I have 9e20b390 (repack: add `repack.packKeptObjects` config var,
2014-02-26); I do not recall I've squashed anything into it, though.
Do you mean this one?
Here's the interdiff for doing the fallback:
---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3a3d84f..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::
repack.packKeptObjects::
If set to true, makes `git repack` act as if
`--pack-kept-objects` was passed. See linkgit:git-repack[1] for
- details. Defaults to false.
+ details. Defaults to `false` normally, but `true` if a bitmap
+ index is being written (either via `--write-bitmap-index` or
+ `pack.writeBitmaps`).
rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
diff --git a/builtin/repack.c b/builtin/repack.c
index 49947b2..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,7 +9,7 @@
#include argv-array.h
static int delta_base_offset = 1;
-static int pack_kept_objects;
+static int pack_kept_objects = -1;
static char *packdir, *packtmp;
static const char *const git_repack_usage[] = {
@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, builtin_repack_options,
git_repack_usage, 0);
+ if (pack_kept_objects 0)
+ pack_kept_objects = write_bitmap;
+
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f8431a8..b1eed5c 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not
repacked' '
objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |
sed -e s/^\([0-9a-f]\{40\}\).*/\1/)
mv pack-* .git/objects/pack/
- git repack -A -d -l
+ git repack --no-pack-kept-objects -A -d -l
git prune-packed
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
@@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not
repacked' '
test -z $found_duplicate_object
'
-test_expect_success '--pack-kept-objects duplicates objects' '
+test_expect_success 'writing bitmaps duplicates .keep objects' '
# build on $objsha1, $packsha1, and .keep state from previous
- git repack -Adl --pack-kept-objects
+ git repack -Adl
test_when_finished found_duplicate_object=
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] branch: use skip_prefix
Jeff King p...@peff.net writes: On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote: From: modocache modoca...@gmail.com Both your emailed patches have this, which is due to your author name not matching your sending identity. You probably want to set user.name, or if you already have (which it looks like you might have from your Signed-off-by), use git commit --amend --reset-author to update the author information. The install_branch_config function reimplemented the skip_prefix function inline. Use skip_prefix function instead for brevity. Signed-off-by: Brian Gesiak modoca...@gmail.com Reported-by: Michael Haggerty mhag...@alum.mit.edu It's a minor thing, but usually these footer lines try to follow a chronological order. So the report would come before the signoff (and a further signoff from the maintainer would go after yours). diff --git a/branch.c b/branch.c index 723a36b..e163f3c 100644 --- a/branch.c +++ b/branch.c [...] The patch itself looks OK to me. -Peff Thanks. Queued and pushed out on 'pu' with fixups already. -- To unsubscribe from this list: send the line 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 GSoC idea: new git config features
Jeff King p...@peff.net writes: Most callbacks would convert to a query system in a pretty straightforward way, but some that have side effects might be tricky. Converting them all may be too large for a GSoC project, but I think you could do it gradually: 1. Convert the parser to read into an in-memory representation, but leave git_config() as a wrapper which iterates over it. 2. Add query functions like config_string_get() above. 3. Convert callbacks to query functions one by one. 4. Eventually drop git_config(). A GSoC project could take us partway through (3). I actually discarded the read from these config files to preparsed structure to memory, later to be consumed by repeated calls to the git_config() callback functions, making the only difference from the current scheme that the preparsed structure will be reset when there is the new 'reset to the original' definition as obvious and uninteresting. This is one of these times that I find myself blessed with capable others that can go beyond, building on top of such an idea that I may have discarded without thinking it through, around me ;-) Yes, the new abstraction like config_type_get() that can live alongside the existing git_config() feeds callback chain everything and gradually replace the latter, would be a good way forward. Given that we read configuration multiple times anyway for different purposes, even without the new abstraction, the end result might perform better if we read the files once and reused in later calls to git_config(). 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] implemented strbuf_write_or_die()
Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Sat, Mar 1, 2014 at 7:51 AM, He Sun sunheeh...@gmail.com wrote: 2014-03-01 19:21 GMT+08:00 Faiz Kothari faiz.of...@gmail.com: diff --git a/remote-curl.c b/remote-curl.c index 10cb011..dee8716 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads) if (start_command(client)) exit(1); if (preamble) - write_or_die(client.in, preamble-buf, preamble-len); + strbuf_write_or_die(client.in, preamble); if (heads) write_or_die(client.in, heads-buf, heads-len); This should be changed. May be you can use Ctrl-F to search write_or_die(). Or if you are using vim, use / and n to find all. It's not obvious from the patch fragment, but 'heads' is not a strbuf, so Faiz correctly left this invocation alone. That is a very good sign why this change is merely a code-churn and not an improvement, isn't it? We know (and any strbuf user should know) that -buf and -len are the ways to learn the pointer and the length the strbuf holds. Why anybody thinks it is benefitial to introduce another function that is _only_ for writing out strbuf and cannot be used to write out a plain buffer is simply beyond me. As a potential GSoC student and newcomer to the project, Faiz would not have known that this would be considered unwanted churn when he chose the task from the GSoC microproject page [1]. Perhaps it would be a good idea to retire this item from the list? I don't think I saw this on the microproject suggestion page when I last looked at it, and assumed that this was on the student's own initiative. On the other hand, it did expose Faiz to the iterative code review process on this project and gave him a taste of what would be expected of him as a GSoC student, so the microproject achieved that important goal, and thus wasn't an utter failure. [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md Surely. I would have to say that this is not a good sample exercise to suggest to new students and I'd encourage dropping it from the list. You could argue that it is an effective way to cull people with bad design taste to mix suggestions to make the codebase worse and see who picks them, but I do not think it is very fair ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] rebase: accept -number as another way of saying HEAD~number
Michael Haggerty mhag...@alum.mit.edu writes: This might be a reason that -NUM is a bad idea. Or perhaps -NUM should fail with an error message if any of the last NUM commits are merges. In that restricted scenario (which probably accounts for 99% of rebases), -NUM is equivalent to HEAD~NUM. That sounds like one possible way out; the opposite would be to enable mode that preserges merges when we see any. If rebase had a --first-parent mode that simply replays only the commits on the first parent chain, merging the same second and later parents without looking at their history when dealing with merge commits, and always used that mode unless --flatten-history was given, the world might have been a better place. We cannot go there in a single step, but we could plan such a backward incompatible migration if we wanted to. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] rebase: accept -number as another way of saying HEAD~number
Matthieu Moy matthieu@grenoble-inp.fr writes:
Michael Haggerty mhag...@alum.mit.edu writes:
Or perhaps -NUM should fail with an error message if any of the last
NUM commits are merges. In that restricted scenario (which probably
accounts for 99% of rebases), -NUM is equivalent to HEAD~NUM.
Makes sense to me. So, -NUM would actually mean rebase the last NUM
commits (as well as being an alias for HEAD~NUM), but would fail when
it does not make sense (with an error message explaining the situation
and pointing the user to HEAD~N if this is what he wanted).
This would actually be a feature for me: I often want to rebase recent
enough history, and when my @{upstream} isn't well positionned,...
Could you elaborate on this a bit? What does isn't well
positioned mean? Do you mean the upstream has advanced but there
is no reason for my topic to build on that---I'd rather want to make
sure I can view 'diff @{1} HEAD' and understand what my changes
before the rebase was? That is, what you really want is
git rebase -i --onto $(git merge-base @{upstream} HEAD) @{upstream}
but that is too long to type?
If it is very common (and I suspect it is), we may want to support
such a short-hand---the above does not make any sense without '-i',
but I would say with '-i' you do not want to reBASE on an updated
base most of the time. git rebase -i @{upstream}...HEAD or
something?
--
To unsubscribe from this list: send the line 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: My advice for GSoC applicants
Michael Haggerty mhag...@alum.mit.edu writes: Based on my experience so far as a first-time Google Summer of Code mentor, I just wrote a blog article containing some hopefully useful advice for students applying to the program. Please note that this is my personal opinion only and doesn't necessarily reflect the views of the Git/libgit2 projects as a whole. My secret tip for GSoC success http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html Thanks for writing this. Also thanks for the MicroProject approach to introduce potential students and the community. Multiple students seem to be hitting the same microprojects (aka we are running out of micros), which might be a bit unfortunate. I think the original plan might have been that for a student candidate to pass, his-or-her patch must hit my tree and queued somewhere, but with these duplicates I do not think it is fair to disqualify those who interacted with reviewers well but solved an already solved micro. Even with the duplicates I think we are learning how well each student respond to reviews (better ones even seem to pick up lessons from reviews on others' threads that tackle micros different from their own) and what his-or-her general cognitive ability is. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] skip_prefix: rewrite so that prefix is scanned once
Junio C Hamano gits...@pobox.com writes:
Siddharth Goel siddharth98...@gmail.com writes:
Helped-by: Eric Sunshine sunsh...@sunshineco.com
Signed-off-by: Siddharth Goel siddharth98...@gmail.com
---
Added a space after colon in the subject as compared to previous
patch [PATCH v2].
[PATCH v2]: http://thread.gmane.org/gmane.comp.version-control.git/243150
Whenever you see Change, Rewrite, etc. in the subject of a patch
that touches existing code, think twice. The subject line is a
scarce real estate to be wasted on a noiseword that carries no real
information, and we already know a patch that touches existing code
changes or rewrites things.
Subject: [PATCH v3] skip_prefix: scan prefix only once
perhaps?
git-compat-util.h | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index 614a5e9..550dce3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
*suffix);
static inline const char *skip_prefix(const char *str, const char *prefix)
{
-size_t len = strlen(prefix);
-return strncmp(str, prefix, len) ? NULL : str + len;
+while (*prefix != '\0' *str == *prefix) {
+str++;
+prefix++;
+}
+return (*prefix == '\0' ? str : NULL);
Unlike another patch I saw the other day on the same topic, this
checks *prefix twice for the last round, even though I think this
one is probably slightly easier to read. I dunno.
That is, something like this instead. After looking at it again, I
do not think it is less readable than the above.
git-compat-util.h | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..68ffaef 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char *suffix);
static inline const char *skip_prefix(const char *str, const char *prefix)
{
- size_t len = strlen(prefix);
- return strncmp(str, prefix, len) ? NULL : str + len;
+ while (1) {
+ if (!*prefix)
+ return str;
+ if (*str != *prefix)
+ return NULL;
+ prefix++;
+ str++;
+ }
}
#if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 05/14] diff.c: use ALLOC_GROW()
Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru writes:
Use ALLOC_GROW() instead inline code in
diffstat_add() and diff_q()
...instead of open coding it in... may read better.
Signed-off-by: Dmitry S. Dolzhenko dmitrys.dolzhe...@yandex.ru
---
diff.c | 12 ++--
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index e800666..aebdfda 100644
--- a/diff.c
+++ b/diff.c
@@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct
diffstat_t *diffstat,
{
struct diffstat_file *x;
x = xcalloc(sizeof (*x), 1);
- if (diffstat-nr == diffstat-alloc) {
- diffstat-alloc = alloc_nr(diffstat-alloc);
- diffstat-files = xrealloc(diffstat-files,
- diffstat-alloc * sizeof(x));
- }
+ ALLOC_GROW(diffstat-files, diffstat-nr + 1, diffstat-alloc);
diffstat-files[diffstat-nr++] = x;
if (name_b) {
x-from_name = xstrdup(name_a);
@@ -3965,11 +3961,7 @@ struct diff_queue_struct diff_queued_diff;
void diff_q(struct diff_queue_struct *queue, struct diff_filepair *dp)
{
- if (queue-alloc = queue-nr) {
- queue-alloc = alloc_nr(queue-alloc);
- queue-queue = xrealloc(queue-queue,
- sizeof(dp) * queue-alloc);
- }
+ ALLOC_GROW(queue-queue, queue-nr + 1, queue-alloc);
queue-queue[queue-nr++] = dp;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 00/14] Use ALLOC_GROW() instead of inline code
Dmitry S. Dolzhenko (14): builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path() bundle.c: use ALLOC_GROW() in add_to_ref_list() cache-tree.c: use ALLOC_GROW() in find_subtree() commit.c: use ALLOC_GROW() in register_commit_graft() diff.c: use ALLOC_GROW() diffcore-rename.c: use ALLOC_GROW() patch-ids.c: use ALLOC_GROW() in add_commit() replace_object.c: use ALLOC_GROW() in register_replace_object() reflog-walk.c: use ALLOC_GROW() dir.c: use ALLOC_GROW() in create_simplify() attr.c: use ALLOC_GROW() in handle_attr_line() builtin/mktree.c: use ALLOC_GROW() in append_to_tree() read-cache.c: use ALLOC_GROW() in add_index_entry() sha1_file.c: use ALLOC_GROW() in pretend_sha1_file() All looked cleanly done. The resulting code of 1, 3, 4, 6 and 8 share this pattern: ALLOC_GROW(table, number + 1, alloc); number++; which may be easier to understand if done the other way around: number++; ALLOC_GROW(table, number, alloc); That is, we know we want one more, so make sure they fit in the table. But that is just a minor issue; I suspect many existing callsites to ALLOC_GROW() already follow the former pattern, and if we decide to to switch the former to the latter, we shouldn't be doing so within this series (we should do that as a separate series on top of this). Thanks; will queue. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so
Eric Sunshine sunsh...@sunshineco.com writes: On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr ilya.bo...@gmail.com wrote: We used to show (missing ) next to tests skipped because they are specified in GIT_SKIP_TESTS. Use (matched by GIT_SKIP_TESTS) instead. Bikeshedding: That's pretty verbose. Perhaps just say (excluded)? Sounds good, or at least better than matched GIT_SKIP_TESTS, to me ;-). 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 2/2] test-lib: GIT_TEST_ONLY to run only specific tests
Eric Sunshine sunsh...@sunshineco.com writes:
On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr ilya.bo...@gmail.com wrote:
This is a counterpart to GIT_SKIP_TESTS. Mostly useful when debugging.
To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?
I actually do not like the interface to use two variables very much.
Can't we just allow negative entries on to be skipped list?
That is
GIT_SKIP_TESTS='t9??? !t91??'
would skip nine-thousand series, but would run 91xx series, and all
the others are not excluded.
Simple rules to consider:
- If the list consists of _only_ negated patterns, pretend that
there is unless otherwise specified with negatives, skip all
tests, i.e. treat GIT_SKIP_TESTS='!t91??' just the same way you
would treat GIT_SKIP_TESTS='* !t91??'.
- The orders should not matter for simplicity of the semantics;
before running each test, check if it matches any negative (and
run it if it matches, without looking at any positives), and
otherwise check if it matches any positive (and skip it if it
does not).
Hmm?
---
t/README | 15 +++
t/test-lib.sh |8
2 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/t/README b/t/README
index caeeb9d..f939987 100644
--- a/t/README
+++ b/t/README
@@ -187,6 +187,21 @@ and either can match the t[0-9]{4} part to skip the
whole
test, or t[0-9]{4} followed by .$number to say which
particular test to skip.
+Sometimes the opposite is desired - ability to execute only one or
+several tests. Mostly while debugging tests. For that you can say
+
+$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
+
+or, similrary to GIT_SKIP_TESTS
+
+$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
+
+In additiona to matching against test suite number.test number
s/additiona/addition/
Plus the other typos already mentioned by Philip...
+GIT_TEST_ONLY is matched against just the test numbes. This comes
+handy when you are running only one test:
+
+$ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
+
Note that some tests in the existing test suite rely on previous
test item, so you cannot arbitrarily disable one and expect the
remainder of test to check what the test originally was intended
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 89a405b..12bf436 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -464,6 +464,14 @@ test_skip () {
fi
skipped_reason=missing $missing_prereq${of_prereq}
fi
+ if test -z $to_skip test -n $GIT_TEST_ONLY
+ ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY
+ ! match_pattern_list $test_count $GIT_TEST_ONLY
+ then
+ to_skip=t
+ skipped_reason=not in GIT_TEST_ONLY
+ fi
+
case $to_skip in
t)
say_color skip 3 skipping test: $@
--
1.7.9
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line 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] skip_prefix: rewrite so that prefix is scanned once
David Kastrup d...@gnu.org writes:
How about a function body of
do {
if (!*prefix)
return str;
} while (*str++ == *prefix++);
return NULL;
I'm not too fond of while (1) and tend to use for (;;) instead, but that
may again partly be due to some incredibly non-optimizing compiler back
in the days of my youth. At any rate, the do-while loop seems a bit
brisker.
I do not have strong preference between while (1) and for (;;),
but I tend to agree
for (;; prefix++, str++) {
if (!*prefix)
return str;
if (*str != *prefix)
return NULL;
}
may be easier to read than what I suggested. Your do-while loop is
concise and very readable, so let's take that one (I'll forge your
Sign-off ;-)).
I haven't looked at the generated assembly of any of these, though.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so
Eric Sunshine sunsh...@sunshineco.com writes: That new message in patch #2 says not in GIT_TEST_ONLY, but isn't (excluded) also applicable to that case? Is it important to be able to distinguish between the two excluded reasons? An obvious solution is not to *have* two reasons in the first place ;-) -- To unsubscribe from this list: send the line 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] commit.c: Use skip_prefix() instead of starts_with()
Max Horn m...@quendi.de writes:
On 03.03.2014, at 20:43, Junio C Hamano gits...@pobox.com wrote:
Tanay Abhra tanay...@gmail.com writes:
@@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
+ if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1))
{
/* At the very beginning of the buffer */
- found = buf + strlen(sigcheck_gpg_status[i].check + 1);
+ ;
} else {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
This hunk looks good. It can be a separate patch but they are both
minor changes so it is OK to have it in a single patch.
Hm, but that hunk also introduces an assignment in a conditional,
and introduces an empty block. Maybe like this?
Much better.
If we anticipate that we may add _more_ ways to find the thing
later, then I would say this code structure is better:
/* Is it at the beginning of the buffer? */
found = skip_prefix(...);
if (!found) {
/* Oh, maybe it is on the second or later line? */
found = ... find it on a later line...
}
if (!found)
continue;
but I do not think that is the case for this particular one. We
either try to find it at the very beginning (i.e. no leading
newline), or we have some other lines before it (i.e. require
leading newline), and there will be no future extension, so what you
suggested below looks sensible.
diff --git a/commit.c b/commit.c
index 6bf4fe0..0ee0725 100644
--- a/commit.c
+++ b/commit.c
@@ -1193,10 +1193,8 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
- /* At the very beginning of the buffer */
- found = buf + strlen(sigcheck_gpg_status[i].check +
1);
- } else {
+ found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+ if (!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
continue;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests
Ilya Bobyr ilya.bo...@gmail.com writes: It might be that we are looking at different use cases, as you are talking about whole test suits. I do not think so. I do not see anything prevents you from saying GIT_SKIP_TESTS='t !t.1 !t.4' to specify test-pieces in individual tests so that you can run the setup step (step .1) and the specific test (step .4) without running two tests in between. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-bisect.sh: fix a few style issues
Jacopo Notarstefano jacopo.notarstef...@gmail.com writes: Redirection operators should have a space before them, but not after them. Signed-off-by: Jacopo Notarstefano jacopo.notarstef...@gmail.com --- Looks obviously harmless ;-) 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 2/2] test-lib: GIT_TEST_ONLY to run only specific tests
Ilya Bobyr ilya.bo...@gmail.com writes: While it could be done, it looks less obvious than this: GIT_TEST_ONLY='1 4' ./t0001-init.sh If you are thinking about affecting only one test, then you shouldn't be mucking with environment variables in the first place, primarily because running: $ GIT_TEST_ONLY='1 4' make test to run test .1 and .4 of all the test scripts would not make any sense. I think your simplicity argument is a total red-herring. Of course if you do not have to say the test script name, your specification would be shorter, but that is only because your specification is not specific enough to be useful. Giving that as a command line argument to the specific script, e.g. $ sh ./t0001-init.sh --only='1 4' might make some sense, but the above GIT_TEST_ONLY does not make any sense from the UI point of view. There are many reasons that makes me unenthused about this line of change in the first place: * Both at the philosophical level and at the practical level, I've found that it always makes sense to run most of the tests, i.e. skipping ought to be an exception not the norm. Over the course of this project, I often saw an alleged fix to one part of the system introduces breakages that are caught by tests that checks parts of the system that does not have any superficial link to it (e.g. update the refs code and find a rebase test break). * Even though GIT_SKIP_TESTS mechanism still allows you to skip individual test pieces, it has never been a serious feature in the first place. Many of the tests unfortunately do rely on state previous sequences of tests left behind, so it is not realistic to expect that you can skip test pieces randomly and exercise later test pieces reliably. * The numbering of individual test pieces can easily change by new tests inserted in the middle; again, many tests do take advantge of the states earlier tests leave behind, so do not add new tests in the middle is not a realistic rule to enforce, unless you are willing to clean up existing test scripts so that each test piece is independent from all the previous ones. The latter two makes the ability to skip individual test pieces a theoretically it could be made useful but practically not so much misfeature. I am very hesitant to see the test framework code churned only to enhance its usefulness when there isn't any in the first place, without first making changes that fundamentally improves its usefulness (e.g. to solve test numbering is not stable problem, you could identify the tests with test names instead of numbers to make it more stable, but that is not what your patch is even attempting to do). I view such a change as merely a code churn, damaging the codebase that is already less nice than ideal and turning it more difficult to fix properly to make it truly nicer later. My suggestion to cram everything into GIT_SKIP_TESTS is primarily because it is one way I can easily see how it allows us to limit the damage to the codebase--the suggested change could be contained within a single shell function match_pattern_list and no other code has to change to support it. I am not saying it is the only way, but glancing at your patch, adding an extra environment variable would need to also modify its callers, so the extent of the damage to the codebase seemed somewhat larger. So, I dunno. I am not yet enthused. -- To unsubscribe from this list: send the line 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] upload-pack: allow shallow fetching from a read-only repository
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
pack-objects - 2013-08-16) upload-pack does not write to the source
repository. cdab485 starts to write $GIT_DIR/shallow_XX if it's a
shallow fetch, so the source repo must be writable.
git:// servers do not need write access to repos and usually don't,
which mean cdab485 breaks shallow clone over git://
Fall back to $TMPDIR if $GIT_DIR/shallow_XX cannot be created in
this case. Note that in other cases that write $GIT_DIR/shallow_XX
and eventually rename it to $GIT_DIR/shallow, there is no fallback to
$TMPDIR.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Rebased on top of jk/shallow-update-fix
Hmph.
I notice that the original code, with or without this change, allows
upload-pack spawned by daemon to attempt to write into GIT_DIR.
As upload-pack is supposed to be a read-only operation, this is
quite bad.
Perhaps we should give server operators an option to run their
daemon - upload-pack chain to always write to a throw-away
directory of their choice, without ever attempting to write to
GIT_DIR it serves?
How well is the access to the temporary shallow file controlled in
your code (sorry, but I do not recall carefully reading your patch
that added the mechanism with security issues in mind, so now I am
asking)? When it is redirected to TMPDIR (let's forget GIT_DIR for
now---if an attacker can write into there, the repository is already
lost), can an attacker race with us to cause us to overwrite we do
not expect to?
Even if it turns out that this patch is secure enough as-is, we
definitely need to make sure that server operators, who want to keep
their upload-pack truly a read-only operation, know that it is
necessary to (1) keep the system user they run git-daemon under
incapable of writing into GIT_DIR, and (2) make sure TMPDIR points
at somewhere only git-daemon user and nobody else can write into,
somewhere in the documentation.
diff --git a/fetch-pack.c b/fetch-pack.c
index ae8550e..b71d186 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -853,7 +853,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
*args,
setup_alternate_shallow(shallow_lock, alternate_shallow_file,
NULL);
else if (si-nr_ours || si-nr_theirs)
- alternate_shallow_file = setup_temporary_shallow(si-shallow);
+ alternate_shallow_file = setup_temporary_shallow(si-shallow,
0);
else
alternate_shallow_file = NULL;
if (get_pack(args, fd, pack_lockfile))
diff --git a/shallow.c b/shallow.c
index c7602ce..ad28af6 100644
--- a/shallow.c
+++ b/shallow.c
@@ -224,7 +224,8 @@ static void remove_temporary_shallow_on_signal(int signo)
raise(signo);
}
-const char *setup_temporary_shallow(const struct sha1_array *extra)
+const char *setup_temporary_shallow(const struct sha1_array *extra,
+ int read_only)
{
static int installed_handler;
struct strbuf sb = STRBUF_INIT;
@@ -235,7 +236,15 @@ const char *setup_temporary_shallow(const struct
sha1_array *extra)
if (write_shallow_commits(sb, 0, extra)) {
strbuf_addstr(temporary_shallow, git_path(shallow_XX));
- fd = xmkstemp(temporary_shallow.buf);
+ fd = mkstemp(temporary_shallow.buf);
+ if (read_only fd 0) {
+ strbuf_grow(temporary_shallow, PATH_MAX);
+ fd = git_mkstemp(temporary_shallow.buf, PATH_MAX,
+ shallow_XX);
+ }
+ if (fd 0)
+ die_errno(Unable to create temporary file '%s',
+ temporary_shallow.buf);
if (!installed_handler) {
atexit(remove_temporary_shallow);
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index b0fa738..171db88 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,6 +173,19 @@ EOF
)
'
+test_expect_success POSIXPERM 'shallow fetch from a read-only repo' '
s/POSIXPERM/,SANITY/, perhaps?
Thinking of it again, perhaps POSIXPERM should imply SANITY is required?
+ cp -R .git read-only.git
+ find read-only.git -print | xargs chmod -w
+ test_when_finished find read-only.git -type d -print | xargs chmod +w
+ git clone --no-local --depth=2 read-only.git from-read-only
+ git --git-dir=from-read-only/.git log --format=%s actual
+ cat expect EOF
+add-1-back
+4
+EOF
+ test_cmp expect actual
+'
+
if test -n $NO_CURL -o -z $GIT_TEST_HTTPD; then
say 'skipping remaining tests, git built without http support'
test_done
diff --git a/upload-pack.c b/upload-pack.c
index a3c52f6..b538f32 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -84,7
[PATCH] gitk: replace SHA1 entry field on keyboard paste
From: Ilya Bobyr ilya.bo...@gmail.com
Date: Thu, 27 Feb 2014 22:51:37 -0800
We already replace old SHA with the clipboard content for the mouse
paste event. It seems reasonable to do the same when pasting from
keyboard.
Signed-off-by: Ilya Bobyr ilya.bo...@gmail.com
---
* Paul? I do not use Paste on my keyboard, so I am not in the
position to say that this patch is correct (or not). I am just
forwarding it in case you think gitk users will find it useful.
The original patch was done against my tree, so I hand tweaked it
to apply to your tree.
Thanks.
gitk |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/gitk b/gitk
index 90764e8..2f58bcf 100755
--- a/gitk
+++ b/gitk
@@ -2585,6 +2585,7 @@ proc makewindow {} {
bind $fstring Key-Return {dofind 1 1}
bind $sha1entry Key-Return {gotocommit; break}
bind $sha1entry PasteSelection clearsha1
+bind $sha1entry Paste clearsha1
bind $cflist 1 {sel_flist %W %x %y; break}
bind $cflist B1-Motion {sel_flist %W %x %y; break}
bind $cflist ButtonRelease-1 {treeclick %W %x %y}
--
To unsubscribe from this list: send the line 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] rev-parse: support OPT_NUMBER_CALLBACK in --parseopt
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
If the option spec is
-NUM Help string
then rev-parse will accept and parse -([0-9]+) and return -NUM $1
Even though the hardcoded NUM token initially gave me a knee-jerk
Yuck reaction, that literal option name is very unlikely to be
desired by scripts/commands for their real option names, and being
in all uppercase it is very clear that it is magic convention
between the parsing mechanism and the script it uses.
It however felt funny to me without a matching (possibly hidden)
mechanism to allow parse-options machinery to consume such an output
as its input. In a script that uses this mechanism to parse out the
numeric option -NUM 3 out of git script -3 and uses that three
to drive an underlying command (e.g. git grep -3), wouldn't it be
more natural if that underlying command can be told to accept the
same notation (i.e. git grep -NUM 3)? For that to be consistent
with the rest of the system, -NUM would not be a good token; being
it multi-character, it must be --NUM or something with double-dash
prefix.
I kind of like the basic idea, the capability it tries to give
scripted Porcelain implementations. But my impression is that
rebase -i -4, which this mechanism was invented for, is not
progressing, so perhaps we should wait until the real user of this
feature appears.
Thanks.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/rev-parse.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 45901df..b37676f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -331,6 +331,8 @@ static int parseopt_dump(const struct option *o, const
char *arg, int unset)
struct strbuf *parsed = o-value;
if (unset)
strbuf_addf(parsed, --no-%s, o-long_name);
+ else if (o-type == OPTION_NUMBER)
+ strbuf_addf(parsed, -NUM);
else if (o-short_name (o-long_name == NULL || !stuck_long))
strbuf_addf(parsed, -%c, o-short_name);
else
@@ -338,7 +340,7 @@ static int parseopt_dump(const struct option *o, const
char *arg, int unset)
if (arg) {
if (!stuck_long)
strbuf_addch(parsed, ' ');
- else if (o-long_name)
+ else if (o-long_name || o-type == OPTION_NUMBER)
strbuf_addch(parsed, '=');
sq_quote_buf(parsed, arg);
}
@@ -439,7 +441,10 @@ static int cmd_parseopt(int argc, const char **argv,
const char *prefix)
if (s - sb.buf == 1) /* short option only */
o-short_name = *sb.buf;
- else if (sb.buf[1] != ',') /* long option only */
+ else if (s - sb.buf == 4 !strncmp(sb.buf, -NUM, 4)) {
+ o-type = OPTION_NUMBER;
+ o-flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG;
+ } else if (sb.buf[1] != ',') /* long option only */
o-long_name = xmemdupz(sb.buf, s - sb.buf);
else {
o-short_name = *sb.buf;
--
To unsubscribe from this list: send the line 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] rebase: new convenient option to edit a single commit
Michael Haggerty mhag...@alum.mit.edu writes: ... All of the following seem to make sense: git rebase --edit COMMIT A long-form for the -e option we have been talking about. It is unfortunately that this spelling sounds like the --edit option on git commit --edit and git merge --edit, so people might use it when they really mean git rebase --reword COMMIT. I agree, so the --edit does *not* make sense as it invites confusion. git rebase --reword COMMIT Yes, that would make sense. git rebase --fixup COMMIT git rebase --squash COMMIT I am not sure about these. What does it even mean to --fixup (or --squash for that matter) a single commit without specifying what it is squashed into? Or are you assuming that all of these is only to affect pre-populated rebase-i insn sheet that is to be further edited before the actual rebasing starts? I somehow had an impression that the reason to have these new options is to skip the editing of the insn sheet in the editor altogether. git rebase --kill COMMIT This _does_ make sense under my assumption: remove this commit from the insn-sheet and go ahead with it, without bothering me to edit the insn-sheet further. I'm quite confident that I would use all of these commands. If --kill takes only one, I would probably do rebase --onto without bothering with -i at all, but if it lets me drop multiple non-consecutive commits, by accepting more than one --kill, I see how I would be using it myself. I can see how --reword/--amend would be useful even when dealing with only a single commit. I do not know about --fixup/--squash though. -- To unsubscribe from this list: send the line 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 1/2] i18n: proposed command missing leading dash
From: Sandy Carter sandy.car...@savoirfairelinux.com Date: Mon, 3 Mar 2014 09:55:53 -0500 Add missing leading dash to proposed commands in french output when using the command: git branch --set-upstream remotename/branchname and when upstream is gone Signed-off-by: Sandy Carter sandy.car...@savoirfairelinux.com Signed-off-by: Junio C Hamano gits...@pobox.com --- * Forwarding to the i18n coordinator. I don't do French, but this seems trivially correct. po/fr.po | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/po/fr.po b/po/fr.po index e10263f..0b9d59e 100644 --- a/po/fr.po +++ b/po/fr.po @@ -1075,7 +1075,7 @@ msgstr Votre branche est basée sur '%s', mais la branche amont a disparu.\n #: remote.c:1875 msgid (use \git branch --unset-upstream\ to fixup)\n -msgstr (utilisez \git branch -unset-upstream\ pour corriger)\n +msgstr (utilisez \git branch --unset-upstream\ pour corriger)\n #: remote.c:1878 #, c-format @@ -3266,7 +3266,7 @@ msgstr git branch -d %s\n #: builtin/branch.c:1027 #, c-format msgid git branch --set-upstream-to %s\n -msgstr git branch -set-upstream-to %s\n +msgstr git branch --set-upstream-to %s\n #: builtin/bundle.c:47 #, c-format -- 1.9.0 -- To unsubscribe from this list: send the line 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] implemented strbuf_write_or_die()
Michael Haggerty mhag...@alum.mit.edu writes: On 03/03/2014 07:31 PM, Junio C Hamano wrote: That is a very good sign why this change is merely a code-churn and not an improvement, isn't it? We know (and any strbuf user should know) that -buf and -len are the ways to learn the pointer and the length the strbuf holds. ... ... Writing strbufs comes up frequently and will hopefully increase in usage and I think it is a positive thing to encourage the use of strbufs by making them increasingly first-class citizens. Yeah, I understand that. I suspect that the conclusion would have been very different if we were a C++ project; most likely it would be an excellent idea to add an often-used write_or_die() method to the strbuf class. But we are writing C. Faiz, this is the way things go on the Git mailing list. It would be boring if everybody agreed all the time :-) Surely, and 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 2/2] test-lib: GIT_TEST_ONLY to run only specific tests
Ilya Bobyr ilya.bo...@gmail.com writes: On 3/4/2014 12:29 AM, Junio C Hamano wrote: ... then you shouldn't be mucking with environment variables in the first place, primarily because running: $ GIT_TEST_ONLY='1 4' make test to run test .1 and .4 of all the test scripts would not make any sense. No it does not. It only makes sense for one test suite. I think your simplicity argument is a total red-herring. Of course if you do not have to say the test script name, your specification would be shorter, but that is only because your specification is not specific enough to be useful. In my case it is very useful :) It invites a nonsense usage (i.e. running make test under that environment variable setting); that is not a good trade-off. * Even though GIT_SKIP_TESTS mechanism still allows you to skip individual test pieces, it has never been a serious feature in the first place. Many of the tests unfortunately do rely on state previous sequences of tests left behind, so it is not realistic to expect that you can skip test pieces randomly and exercise later test pieces reliably. * The numbering of individual test pieces can easily change by new tests inserted in the middle; again, many tests do take advantge of the states earlier tests leave behind, so do not add new tests in the middle is not a realistic rule to enforce, unless you are willing to clean up existing test scripts so that each test piece is independent from all the previous ones. Both are true, but do not apply to the TDD case. The existing tests are designed to be black-box tests, not function level unit tests, and touching lower level code carelessly affects other parts of the system you did not know the interactions about. What does TDD case change anything in that equation? -- To unsubscribe from this list: send the line 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] rev-parse --parseopt: option argument name hints
Ilya Bobyr ilya.bo...@gmail.com writes:
Built-in commands can specify names for option arguments, that are shown
when usage text is generated for the command. sh based commands should
be able to do the same.
Option argument name hint is any text that comes after [*=?!] after the
argument name up to the first whitespace. Underscores are replaced with
whitespace. It is unlikely that an underscore would be useful in the
hint text.
Signed-off-by: Ilya Bobyr ilya.bo...@gmail.com
---
Documentation/git-rev-parse.txt | 11 +--
builtin/rev-parse.c | 17 -
t/t1502-rev-parse-parseopt.sh | 20
3 files changed, 45 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..4cb6e02 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -284,13 +284,13 @@ Input Format
'git rev-parse --parseopt' input format is fully text based. It has two
parts,
separated by a line that contains only `--`. The lines before the separator
-(should be more than one) are used for the usage.
+(could be more than one) are used for the usage.
Good spotting. I think the original author meant to say there
should be at least one line to serve as the usage string, so
updating it to should be one or more may be more accurate, but
could be more than one would also work.
The lines after the separator describe the options.
Each line of options has this format:
-opt_specflags* SP+ help LF
+opt_specflags*argh? SP+ help LF
`opt_spec`::
@@ -313,6 +313,12 @@ Each line of options has this format:
* Use `!` to not make the corresponding negated long option available.
+`argh`::
+ `argh`, if specified, is used as a name of the argument, if the
+ option takes an argument. `argh` is terminated by the first
+ whitespace. Angle braces are added automatically. Underscore symbols
+ are replaced with spaces.
I had a hard time understanding this Angle brackets are added
automatically one (obviously nobody wants extra angle brackets
added around option arguments given by the user), until I looked at
the addition of the test to realize that this description is only
about how it appears in the help output. The description needs to
be clarified to avoid confusion.
@@ -333,6 +339,7 @@ h,helpshow the help
foo some nifty option --foo
bar= some cool option --bar with an argument
+baz=arg another cool option --baz with an argument named arg
It probably is better not to have named arg at the end here, as
that gives an apparent-but-false contradiction with the Angle
brackets are added *automatically* and confuse readers. At least,
it confused _this_ reader.
After the eval in the existing example to parse the $@ argument
list in this part of the documentation, it may be a good idea to say
something like:
The above command, when $@ is --help, produces the
following help output:
... sample output here ...
to show the actual output. That way, we can illustrate how input
baz?arg description of baz is turned into --baz[=arg] output
clearly (yes, I am suggesting to use '?' in the new example, not '='
whose usage is already shown in the existing example).
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..83a769e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv,
const char *prefix)
usage[unb++] = strbuf_detach(sb, NULL);
}
- /* parse: (short|short,long|long)[=?]? SP+ help */
+ /* parse: (short|short,long|long)[*=?!]*arghint? SP+ help */
while (strbuf_getline(sb, stdin, '\n') != EOF) {
const char *s;
+ const char *argh;
Let's spell that variable name out, e.g. arg_hint or something.
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 83b1300..bf0db05 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -18,6 +18,17 @@ An option group Header
-C[...] option C with an optional argument
-d, --data[=...] short and long option with an optional argument
+Argument hints
+-b arg short option required argument
+--bar2 arg long option required argument
+-e, --fuz with spaces
+ short and long option required argument
+-s[some]short option optional argument
+--long[=data] long option optional argument
+-g, --fluf[=path] short and long option optional argument
+--longest a very long argument hint
+ a very long argument hint
+
Extras
--extra1 line above used to cause a segfault but no longer
does
@@ -39,6 +50,15 @@ b,baz a short and
Re: [PATCH v3] commit.c: use skip_prefix() instead of starts_with()
Tanay Abhra tanay...@gmail.com writes:
In record_author_date() parse_gpg_output() ,using skip_prefix() instead of
starts_with() is a more suitable abstraction.
Thanks. Will queue with a reworded message to clarify what exactly
A more suitable means.
Here is what I tentatively came up with.
-- 8 --
From: Tanay Abhra tanay...@gmail.com
Date: Tue, 4 Mar 2014 00:42:20 -0800
Subject: [PATCH] commit.c: use skip_prefix() instead of starts_with()
In record_author_date() parse_gpg_output(), the callers of
starts_with() not just want to know if the string starts with the
prefix, but also can benefit from knowing the string that follows
the prefix.
By using skip_prefix(), we can do both at the same time.
Helped-by: Max Horn m...@quendi.de
Helped-by: Junio C Hamano gits...@pobox.com
Helped-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Tanay Abhra tanay...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
commit.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/commit.c b/commit.c
index 6bf4fe0..6c92acb 100644
--- a/commit.c
+++ b/commit.c
@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
static void record_author_date(struct author_date_slab *author_date,
struct commit *commit)
{
- const char *buf, *line_end;
+ const char *buf, *line_end, *ident_line;
char *buffer = NULL;
struct ident_split ident;
char *date_end;
@@ -566,14 +566,16 @@ static void record_author_date(struct author_date_slab
*author_date,
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ ident_line = skip_prefix(buf, author );
+ if (!ident_line) {
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
}
+ buf = ident_line;
if (split_ident_line(ident,
-buf + strlen(author ),
-line_end - (buf + strlen(author ))) ||
+buf,
+line_end - buf) ||
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
@@ -1193,10 +1195,9 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
- /* At the very beginning of the buffer */
- found = buf + strlen(sigcheck_gpg_status[i].check + 1);
- } else {
+ found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+ /* At the very beginning of the buffer */
+ if(!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
continue;
--
1.9.0-186-gd464cb7
--
To unsubscribe from this list: send the line 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] make --set-upstream work for local branches not in refs/heads
Krzesimir Nowak krzesi...@endocode.com writes:
It might be possible (in Gerrited setups) to have local branches
outside refs/heads/, like for example in following fetch config:
[remote origin]
url = ssh://u...@example.com/my-project
fetch = +refs/heads/*:refs/remotes/origin/*
fetch = +refs/wip/*:refs/remotes/origin-wip/*
Let's say that 'test' branch already exist in origin/refs/wip/. If I
call:
git checkout test
then it will create a new branch and add an entry to .git/config:
[branch test]
remote = origin
merge = refs/wip/test
But if I create a branch 'test2' and call:
git push --set-upstream origin test2:refs/wip/test2
then branch is pushed, but no entry in .git config is created.
By definition anything otuside refs/heads/ is not a branch, so do
not call things in refs/wip branches. Retitle it to work for
local refs outside refs/heads or something.
Having said that, I do not see a major problem if we allowed
[branch test2]
remote = origin
merge = refs/wip/test2
to be created when push --setupstream is requested, making
test2@{upstream} to point at refs/remotes/origin-wip/test2.
I do not know what the correct implementation of such a feature
should be, though.
--
To unsubscribe from this list: send the line 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] commit.c: use skip_prefix() instead of starts_with()
Max Horn m...@quendi.de writes:
+buf = ident_line;
if (split_ident_line(ident,
- buf + strlen(author ),
- line_end - (buf + strlen(author ))) ||
+ buf,
+ line_end - buf) ||
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
Why not get rid of that assignment to buf, and use ident_line
instead of buf below? That seems like it would be more readable,
wouldn't it?
Yes, and also now the argument list is much shorter, you could
probably do it on two lines instead of three:
if (split_ident_line(ident,
ident_line, line_end - ident_line) ||
...
@@ -1193,10 +1195,9 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
-if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
-/* At the very beginning of the buffer */
-found = buf + strlen(sigcheck_gpg_status[i].check + 1);
-} else {
+found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+/* At the very beginning of the buffer */
Do we really need that comment, and in that spot? The code seemed
clear enough to me without it. But if you think keeping is better,
perhaps move it to *before* the skip_prefix, and add a trailing
?
Both good suggestions (I tend to prefer the removal).
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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: When we are creating a pack to send to a remote, we should make sure that we are not respecting grafts or replace refs. Otherwise, we may end up sending a broken pack to the other side that does not contain all objects (either omitting them entirely, or using objects that the other side does not have as delta bases). We already make an attempt to do the right thing in several places by turning off read_replace_refs. However, we missed at least one case (during bundle creation), and we do nothing anywhere to handle grafts. Doing nothing for grafts has been pretty much a deliberate omission. Because we have no way to transfer how histories are grafted together, people cloning from a repository that grafts away a commit that records a mistakenly committed sekrit will end up with a disjoint history, instead of exposing the sekrit to them, and are expected to join the history by recreating grafts (perhaps a README of such a project instructs them to do so). That was deemed far better than exposing the hidden history, I think. And replace tries to do the right thing was an attempt to rectify that misfeature of grafts in that we now do have a way to transfer how the history is grafted together, so that project README does not have to instruct the fetcher of doing anything special. It _might_ be a misfeature, however, for the object connectivity layer to expose a part of the history replaced away to the party that fetches from such a repository. Ideally, the right thing ought to be to include history that would be omitted if we did not have the replacement (i.e. adding parents the underlying commit does not record), while not following the history that replacement wants to hide (i.e. excluding the commits replacement commits overlay). -- To unsubscribe from this list: send the line 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] cache_tree_find(): remove redundant checks
Michael Haggerty mhag...@alum.mit.edu writes:
while (*path) {
- const char *slash;
struct cache_tree_sub *sub;
+ const char *slash = strchr(path, '/');
- slash = strchr(path, '/');
if (!slash)
slash = path + strlen(path);
Isn't the above a strchrnul()?
Combining a freestanding decl with intializer assignment to lose one
line is sort of cheating on the line count, but replacing the three
lines with a single strchrnul() would be a real code reduction ;-)
- /* between path and slash is the name of the
- * subtree to look for.
+ /*
+ * Between path and slash is the name of the subtree
+ * to look for.
*/
sub = find_subtree(it, path, slash - path, 0);
if (!sub)
return NULL;
it = sub-cache_tree;
- if (slash)
- while (*slash *slash == '/')
- slash++;
- if (!slash || !*slash)
- return it; /* prefix ended with slashes */
path = slash;
+ while (*path == '/')
+ path++;
}
return 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
[microproject idea]
Find places where we scan a string twice unnecessarily, once with strchr() and then with strlen(), e.g. const char *colon = strchr(name, ':'); int namelen = colon ? colon - name : strlen(name); and rewrite such a pattern using strchrnul() as appropriate. The above example can become const char *colon = strchrnul(name, ':'); int namelen = colon - name; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()
Tanay Abhra tanay...@gmail.com writes:
In record_author_date() parse_gpg_output(), the callers of
starts_with() not just want to know if the string starts with the
prefix, but also can benefit from knowing the string that follows
the prefix.
By using skip_prefix(), we can do both at the same time.
Helped-by: Max Horn m...@quendi.de
Helped-by: Junio C Hamano gits...@pobox.com
Helped-by: Michael Haggerty mhag...@alum.mit.edu
Signed-off-by: Tanay Abhra tanay...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
Do not add the last when sending; I never signed-off this particular
version of this patch.
diff --git a/commit.c b/commit.c
index 6bf4fe0..01526f7 100644
--- a/commit.c
+++ b/commit.c
@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
static void record_author_date(struct author_date_slab *author_date,
struct commit *commit)
{
- const char *buf, *line_end;
+ const char *buf, *line_end, *ident_line;
char *buffer = NULL;
struct ident_split ident;
char *date_end;
@@ -566,14 +566,14 @@ static void record_author_date(struct author_date_slab
*author_date,
buf;
buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, author )) {
+ ident_line = skip_prefix(buf, author );
+ if (!ident_line) {
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
}
if (split_ident_line(ident,
- buf + strlen(author ),
- line_end - (buf + strlen(author ))) ||
+ ident_line, line_end -
ident_line) ||
Funny indentation with some SP followed by HT followed by SP.
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed author line */
break;
@@ -1193,10 +1193,8 @@ static void parse_gpg_output(struct signature_check
*sigc)
for (i = 0; i ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
- /* At the very beginning of the buffer */
- found = buf + strlen(sigcheck_gpg_status[i].check + 1);
- } else {
+ found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+ if(!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
continue;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()
Junio C Hamano gits...@pobox.com writes:
+found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+if(!found) {
Missing SP between the control keyword and parenthesized expression
the keyword uses.
I've fixed this (and the broken indentation) locally and queued the
result to 'pu', so no need to resend to correct this one.
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 v2] cache_tree_find(): remove redundant checks
Michael Haggerty mhag...@alum.mit.edu writes:
Isn't the above a strchrnul()?
Oh, cool, I never realized that this GNU extension was blessed for use
in Git. Will change.
We do have our own fallbacks for non-glibc platforms, so it should
be safe.
Combining a freestanding decl with intializer assignment to lose one
line is sort of cheating on the line count, but replacing the three
lines with a single strchrnul() would be a real code reduction ;-)
I suppose you are just teasing me, but for the record I consider line
count only a secondary metric. The reason for combining initialization
with declaration is to reduce by one the number of times that the reader
has to think about that variable when analyzing the code.
...
I really wish we could mix declarations with statements because I think
it is a big help to readability.
Unfortunately, I think we are in violent disagreement.
A variable declaration block with initializations on only some but
not all variables is extremely annoying. If none of the variable
declaration has initialization (or initialization to trivial values
that do not depend on the logic flow), and the first statement is
separated from the decl block, then I do not have to read the decl
part when reading the code/logic *at all* (the compiler will find
missing variables, variables declared as a wrong type, etc.).
In other words, a trivial initialization at the beginning of the
block, if the logic flow only sometimes makes assignment to the
variable, is perfectly fine, e.g.
const char *string = NULL;
if (...) {
string = ...
}
But I would wish people stop doing this:
const char *string = strchrnul(name, ':');
... the real logic of the block that uses string follows ...
and instead say
const char *string;
string = strchrnul(name, ':');
... the real logic of the block that uses string follows ...
--
To unsubscribe from this list: send the line 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 1/2] t3200-branch: test setting branch as own upstream
Brian Gesiak modoca...@gmail.com writes: No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. Signed-off-by: Brian Gesiak modoca...@gmail.com --- t/t3200-branch.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fcdb867..6164126 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,6 +507,14 @@ EOF test_cmp expected actual ' +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' + git branch --set-upstream-to refs/heads/my13 my13 2actual + cat expected EOF +warning: Not setting branch my13 as its own upstream. +EOF + test_i18ncmp expected actual +' + Checking the error message is fine, but we are also interested in seeing that we do not leave such a nonsense configuration, if not more. Shouldn't we check the resulting config as well here? # Keep this test last, as it changes the current branch cat expect EOF $_z40 $HEAD $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL 1117150200 + branch: Created from master -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Mar 2014, #01; Tue, 4)
What's cooking in git.git (Mar 2014, #01; Tue, 4) -- Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. A handful of GSoC warm-up microprojects have been queued on 'pu'. Thanks for reviewing them. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to master] * al/docs (2014-02-11) 4 commits (merged to 'next' on 2014-02-25 at 0c1a734) + docs/git-blame: explain more clearly the example pickaxe use + docs/git-clone: clarify use of --no-hardlinks option + docs/git-remote: capitalize first word of initial blurb + docs/merge-strategies: remove hyphen from mis-merges Originally merged to 'next' on 2014-02-13 A handful of documentation updates, all trivially harmless. * bc/gpg-sign-everywhere (2014-02-11) 9 commits (merged to 'next' on 2014-02-25 at 7db014c) + pull: add the --gpg-sign option. + rebase: add the --gpg-sign option + rebase: parse options in stuck-long mode + rebase: don't try to match -M option + rebase: remove useless arguments check + am: add the --gpg-sign option + am: parse options in stuck-long mode + git-sh-setup.sh: add variable to use the stuck-long mode + cherry-pick, revert: add the --gpg-sign option Originally merged to 'next' on 2014-02-13 Teach --gpg-sign option to many commands that create commits. * bk/refresh-missing-ok-in-merge-recursive (2014-02-24) 4 commits (merged to 'next' on 2014-02-25 at 2651cb0) + merge-recursive.c: tolerate missing files while refreshing index + read-cache.c: extend make_cache_entry refresh flag with options + read-cache.c: refactor --ignore-missing implementation + t3030-merge-recursive: test known breakage with empty work tree Originally merged to 'next' on 2014-01-29 Allow merge-recursive to work in an empty (temporary) working tree again when there are renames involved, correcting an old regression in 1.7.7 era. * bs/stdio-undef-before-redef (2014-01-31) 1 commit (merged to 'next' on 2014-02-25 at 77c4b5f) + git-compat-util.h: #undef (v)snprintf before #define them Originally merged to 'next' on 2014-01-31 When we replace broken macros from stdio.h in git-compat-util.h, #undef them to avoid re-definition warnings from the C preprocessor. * da/pull-ff-configuration (2014-01-15) 2 commits (merged to 'next' on 2014-02-25 at b9e4f61) + pull: add --ff-only to the help text + pull: add pull.ff configuration Originally merged to 'next' on 2014-01-22 git pull learned to pay attention to pull.ff configuration variable. * dk/blame-janitorial (2014-02-25) 5 commits (merged to 'next' on 2014-02-25 at d5faeb2) + builtin/blame.c::find_copy_in_blob: no need to scan for region end + blame.c: prepare_lines should not call xrealloc for every line + builtin/blame.c::prepare_lines: fix allocation size of sb-lineno + builtin/blame.c: eliminate same_suspect() + builtin/blame.c: struct blame_entry does not need a prev link Originally merged to 'next' on 2014-02-13 Code clean-up. * ds/rev-parse-required-args (2014-01-28) 1 commit (merged to 'next' on 2014-02-25 at bba6e79) + rev-parse: check i before using argv[i] against argc Originally merged to 'next' on 2014-01-31 git rev-parse --default without the required option argument did not diagnose it as an error. * ep/varscope (2014-01-31) 7 commits (merged to 'next' on 2014-02-25 at e967c7e) + builtin/gc.c: reduce scope of variables + builtin/fetch.c: reduce scope of variable + builtin/commit.c: reduce scope of variables + builtin/clean.c: reduce scope of variable + builtin/blame.c: reduce scope of variables + builtin/apply.c: reduce scope of variables + bisect.c: reduce scope of variable Originally merged to 'next' on 2014-01-31 Shrink lifetime of variables by moving their definitions to an inner scope where appropriate. * jk/config-path-include-fix (2014-01-28) 2 commits (merged to 'next' on 2014-02-25 at 3604f75) + handle_path_include: don't look at NULL value + expand_user_path: do not look at NULL path Originally merged to 'next' on 2014-01-31 include.path variable (or any variable that expects a path that can use ~username expansion) in the configuration file is not a boolean, but the code failed to check it. * jk/pack-bitmap (2014-02-12) 26 commits (merged to 'next' on 2014-02-25 at 5f65d26) + ewah: unconditionally ntohll ewah data + ewah: support platforms that require aligned reads + read-cache: use get_be32 instead of hand-rolled ntoh_l + block-sha1: factor out get_be and put_be wrappers + do not discard revindex when re-preparing packfiles + pack-bitmap: implement optional name_hash cache + t/perf: add tests for pack bitmaps + t: add basic bitmap functionality tests +
Re: [PATCH v2] cache_tree_find(): remove redundant checks
Junio C Hamano gits...@pobox.com writes: Michael Haggerty mhag...@alum.mit.edu writes: I really wish we could mix declarations with statements because I think it is a big help to readability. ... Unfortunately, I think we are in violent disagreement. After re-reading the above, I realize that my statement may have sounded a lot stronger than I intended it to. If our codebase allowed decl-after-stmt, that would change the equation and a different style might help readability somewhat. If decl-after-stmt were allowed, the group of lines that declare variables at the beginning before the real logic begins do not even have to be there, and if some variables have initialization that involve program logic that need to be read carefully, the declaration at the beginning no longer can be coasted over as boilerplate complaint disappears. The entire block can become the logic, declaring variables as necessary at the point they are required, without having to have a separate decl at the beginning. Note that I am not advocating to allow decl-after-stmt; I do not think the imagined readability benefit is worth 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: [RFC 2/3] merge: Add hints to tell users about git merge --abort
Andrew Wong andrew.k...@gmail.com writes: On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder jrnie...@gmail.com wrote: Andrew Wong wrote: --- a/builtin/merge.c +++ b/builtin/merge.c @@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing) fclose(fp); rerere(allow_rerere_auto); printf(_(Automatic merge failed; - fix conflicts and then commit the result.\n)); + fix conflicts and then commit the result.\n + To abort the merge, use \git merge --abort\.\n)); Seems reasonable, but I worry about the command growing too noisy. Could this be guarded by an advice.something setting? (See advice.* in git-config(1) for what I mean.) I was planning to use advice.resolveConflict, but as I went through merge.c, I noticed there could be a few other situations where we could print out the same message: 1. when prepare_to_commit() fails, due to hook error, editor error, or empty commit message 2. git commit --no-commit This means contexts are no longer only about resolving conflict, so I was thinking of renaming advice.resolveConflict to something like advice.mergeHints. Any thoughts? I have no strong opinion on the naming, other than that I doubt this particular new how to abort message is worth the headache associated with the rename which involves transition planning of deprecating the old, supporting both for a while and then removing the old. The existing message above in suggest-conflicts is about hinting the user to first resolve the conflict before attempting to continue, and that is perfectly in line with the existing use of advice.resolveConfict in die_conflict() in git-pull that tells the user there is an unresolved conflict. On the other hand, the additional how to abort message does not have to be limited to you have conflicted paths in the index case. If the user said git merge while another git merge is still outstanding, we would want to say You have not concluded your previous merge and die, and you presumably want to add the same how to abort message there. Such a codepath is unlikely to be covered by existing advice.resolveConflict, and it sounds more natural, at least to me, to use a separate variable to squelch only the new how to abort 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: [PATCH] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote: I do not recall any past discussion on this topic, and searching the archive only shows people echoing what I said above. Is this something we've promised to work in the past? The history lesson is coming solely from my recollection of a discussion I and Linus had on the list back when we were doing the original graft and thinking about the interaction between it and the object/history transfer; especially the only add new ones, but hide the ones that the user wants to be hidden part is something suggested by Linus but we couldn't implement back then, IIRC. Perhaps the right response is grafts are broken, use git-replace instead. But then should we think about deprecating grafts? I am sort of surprised to hear that question, actually ;-) I didn't say that in the message you are responding to because I somehow thought that everybody has been in agreement with these two lines for a long while. Ever since I suggested the replace as an alternative grafts done right and outlined how it should work to Christian while sitting next to him in one of the early GitTogether, the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. -- To unsubscribe from this list: send the line 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: New directory lost by git am
Jeff King p...@peff.net writes: But I have not thought hard about it, so maybe there is a good reason not to (it is a little weird just because the resulting index is a partial application of the patch). Originally .rej was a deliberate attempt to be not very Git but more like 'patch', so I wouldn't be surprised if the combination between --reject and --index did not work, in the sense that we did not add such a partial change to the index. I do not offhand think of a reason to forbid the combination, though, as long as we make sure that git apply --index --reject still exits with failure to prevent a partial application to be auto-committed. -- To unsubscribe from this list: send the line 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote: ... the plan, at least in my mind, has always been exactly that: grafts were a nice little attempt but is broken---if you really wanted to muck with the history without rewriting (which is still discouraged, by the way), do not use graft, but use replace. I certainly had in the back of my mind that grafts were a lesser form of replace, and that eventually we could get rid of the former. Perhaps my question should have been: why haven't we deprecated grafts yet?. Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 18/27] setup.c: support multi-checkout repo setup
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: core.worktree:: Set the path to the root of the working tree. + If GIT_COMMON_DIR environment variable is set, core.worktree + is ignored and not used for determining the root of working tree. Just thinking aloud to see if I got the full implication of the above right... If we find ourselves in the multi-checkout mode because we saw .git/commondir on the filesystem, it is clear that the root of the working tree is the parent directory of that .git directory. If the reason we think we are in the multi-checkout mode is not because of .git/commondir but because $GIT_COMMON_DIR is set, should we assume the same relationship between the root of the working tree and the GIT_DIR (however we find it) when the environment variable $GIT_WORK_TREE is not set? Or should that configuration be an error? With $GIT_DIR set without $GIT_WORK_TREE set, the user is telling us that the $cwd is the root of the working tree, so perhaps we should do the same? -- To unsubscribe from this list: send the line 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] upload-pack: allow shallow fetching from a read-only repository
Duy Nguyen pclo...@gmail.com writes: If only there is a way to pass this info without a temporary file. Multiplexing it to pack-objects' stdin should work. It may be ugly, but it's probably the safest way. Wait it does not look that ugly. We can feed --shallow SHA1 lines before sending want/have/edge objects. Something like this seems to work (just ran a few shallow-related tests, not the whole test suite) Sounds like it is a much better approach to the issue. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 24/27] prune: strategies for linked checkouts
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
+ if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/locked, sb_repo.buf);
+ write_file(sb.buf, 1, located on a different file system\n);
+ keep_locked = 1;
+ } else {
+ strbuf_reset(sb);
+ strbuf_addf(sb, %s/link, sb_repo.buf);
+ (void)link(sb_git.buf, sb.buf);
+ }
Just in case you did not realize, casting the return away with
(void) will not squelch this out of the compiler:
builtin/checkout.c: In function 'prepare_linked_checkout':
builtin/checkout.c:947:3: error: ignoring return value of 'link', declared
with attribute warn_unused_result [-Werror=unused-result]
It still feels fishy to see we attempt to link but we do not care
if it works or not to me, with or without the unused result
issue.
--
To unsubscribe from this list: send the line 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] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote: Given that we discourage grafts strongly and replace less so (but still discourage it), telling the users that biting the bullet and rewriting the history is _the_ permanent solution, I think it is understandable why nobody has bothered to. Perhaps the patch below would help discourage grafts more? The notable place in the documentation where grafts are still used is git-filter-branch.txt. But since the example there is about cementing rewritten history, it might be OK to leave. Thanks; I agree with the reasoning. -- To unsubscribe from this list: send the line 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] diff: simplify cpp funcname regex
Jeff King p...@peff.net writes: types, we simply look for an identifier at the start of the line that contains a (, meaning it is either a function definition or a function call, and then not containing ; which would indicate it is a call or declaration. It is not worth worrying about: foo(arg, another); that is not indented, so I think that simplicity is good. For example, for top-level changes outside functions, we might find: N_(some text that is long that is part of: const char *foo = N_(some text that is long and spans multiple lines); Unfortunate, but cannot be avoided. Before this change, we would skip past it (using the cpp regex, that is; the default one tends to find the same line) and either report nothing, or whatever random function was before us. So it's a behavior change, but the existing behavior is really no better. True. -- To unsubscribe from this list: send the line 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 0/3] push: detect local refspec errors early
Jeff King p...@peff.net writes: We can't fully process the refspecs until we have talked to the other side, because they may involve matching refs from the remote; I don't think git even really looks at them until after we've connected. But I think there are some obvious cases, like a bogus left-hand side (i.e., what you have here) that cannot ever succeed, no matter what the other side has. We could sanity check the refspecs before doing anything else. The user's wallclock time is more important than machine cycles, checking things we could check before having the user do things is a good principle to follow. I wish that the solution did not have to involve doing the same computation twice, but I do not think there is a clean way around that in this codepath. 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 2/3] merge: Add hints to tell users about git merge --abort
Andrew Wong andrew.k...@gmail.com writes: ... merge hints in the future as well. I actually wish we did not have to add any hints in the first place. Having one advice config/variable for every single situation seems a bit overkill, and we would end up with too many variables. That goes without saying. -- To unsubscribe from this list: send the line 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: fnmatch vs regex
Vincenzo di Cicco enzodici...@gmail.com writes: But: why the decision to support the Blob Pattern instead of the Regular Expressions? s/Blob/glob/; Matching pathnames using fnmatch/glob is a fine UNIX tradition; because we generally consider refnames also as pathname-like things, we use fnmatch for them, too (what we actually use is wildmatch, which can be thought of as a natural evolution of 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: [PATCH v3] Replace memcpy with hashcpy when dealing hash copy globally
Sun He sunheeh...@gmail.com writes: Replacing memcpy with hashcpy is more directly and elegant. Can we justify the change without being subjective? Leave ppc/sha1.c alone, as it is an isolated component. Pull cache.h(actually ../cache.h) in just for one memcpy there is not proper. That is not the reason why that memcpy of 20-byte must stay as it is. If for whatever reason we later choose to switch to using another hash function, say MD5, to come up with the object names, the majority of memcpy(..., 20) must change to copy 16 bytes, and it makes sense to contain that implementation-specific knowledge of twenty behind the hashcpy() abstraction. The 20-byte memcpy() call in ppc/sha1.c, however, is an implementation of *THE* SHA-1 algorithm, whose output is and will always be 20-byte. It will not change when we decide to replace what hash function is used to name our objects (which would result in updating the implementation of hashcpy()). That is the reason why you shouldn't touch that one. It has to be explicitly 20 byte, without ever getting affected by what length our hashcpy() may choose to copy. Perhaps... We invented hashcpy() to keep the abstraction of object name behind it. Use it instead of calling memcpy() with hard-coded 20-byte length when moving object names between pieces of memory. Leave ppc/sha1.c as-is, because the function *is* about *the* SHA-1 hash algorithm whose output is and will always be 20-byte. or something. Find the potential places with memcpy by the bash command: $ find . | xargs grep memcpy.*\(.*20.*\) If you are planning to hack on git, learn how to use it first ;-) $ git grep 'memcpy.*, 20)' -- To unsubscribe from this list: send the line 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 1/2] i18n: proposed command missing leading dash
Jiang Xin worldhello@gmail.com writes: 2014-03-05 2:40 GMT+08:00 Junio C Hamano gits...@pobox.com: From: Sandy Carter sandy.car...@savoirfairelinux.com Date: Mon, 3 Mar 2014 09:55:53 -0500 Add missing leading dash to proposed commands in french output when using the command: git branch --set-upstream remotename/branchname and when upstream is gone Signed-off-by: Sandy Carter sandy.car...@savoirfairelinux.com Signed-off-by: Junio C Hamano gits...@pobox.com --- * Forwarding to the i18n coordinator. I don't do French, but this seems trivially correct. Applied to maint branch of git://github.com/git-l10n/git-po, and can be merged to master directly. 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 v4] commit.c: use skip_prefix() instead of starts_with()
tanay abhra tanay...@gmail.com writes:
On Wed, Mar 5, 2014 at 3:41 AM, Junio C Hamano gits...@pobox.com wrote:
Junio C Hamano gits...@pobox.com writes:
+found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+if(!found) {
Missing SP between the control keyword and parenthesized expression
the keyword uses.
I've fixed this (and the broken indentation) locally and queued the
result to 'pu', so no need to resend to correct this one.
Thanks.
Sorry about the indentation, it was due to not setting the tab to eight
spaces. I found your mail late, so I had already
sent a revised patch [1]. Please ignore that mail.
Also , what should be my next step ,should I present a rough draft of a
proposal , or tackle other bugs on the mailing list?
As far as I, as the maintainer of the project, am concerned [*1*],
we are done and I expect/require nothing more from you on this
change.
The MicroProject page says:
... If you've already done a microproject and are itching to do
more, then get involved in other ways, like finding and fixing
other problems in the code, or improving the documentation or
code comments, or helping to review other people's patches on
the mailing list, or answering questions on the mailing list or
in IRC, or writing new tests, etc., etc. In short, start doing
things that other Git developers do!
FYI, [GSoC 2014 timeline] (Google for it) tells us that would-be
students are supposed to be discussing project ideas with their
mentoring organizations now, in preparation for the actual student
application that begins on Mar 10 and ends on Mar 21.
[Footnote]
*1* I should mention that I am not involved in GSoC administration
and student selection for the Git project.
--
To unsubscribe from this list: send the line 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 v6 02/11] trailer: process trailers from stdin and arguments
Christian Couder chrisc...@tuxfamily.org writes:
Implement the logic to process trailers from stdin and arguments.
At the beginning trailers from stdin are in their own in_tok
doubly linked list, and trailers from arguments are in their own
arg_tok doubly linked list.
The lists are traversed and when an arg_tok should be applied,
it is removed from its list and inserted into the in_tok list.
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
Thanks.
This round is marked as the sixth, but I still see quite a many
style issues, which I expect not to see from long timers without
being told. Somewhat disappointing...
trailer.c | 197
++
1 file changed, 197 insertions(+)
diff --git a/trailer.c b/trailer.c
index db93a63..a0124f2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -47,3 +47,200 @@ static size_t alnum_len(const char *buf, size_t len)
...
+ if ((where == WHERE_AFTER ? in_tok-next :
in_tok-previous) == arg_tok)
Overlong line that does not have to be.
if ((where == WHERE_AFTER
? in_tok-next : in_tok-previous) == arg_tok)
or something?
+static void update_last(struct trailer_item **last)
+{
+ if (*last)
+ while((*last)-next != NULL)
Style. SP between control keyword and the expression.
+ *last = (*last)-next;
+}
+
+static void update_first(struct trailer_item **first)
+{
+ if (*first)
+ while((*first)-previous != NULL)
Ditto.
+static void process_trailers_lists(struct trailer_item **in_tok_first,
...
+ /* Process input from end to start */
+ for (in_tok = *in_tok_last; in_tok; in_tok = in_tok-previous) {
+ process_input_token(in_tok, arg_tok_first, WHERE_AFTER);
+ }
Needless brace pair.
+ /* Process input from start to end */
+ for (in_tok = *in_tok_first; in_tok; in_tok = in_tok-next) {
+ process_input_token(in_tok, arg_tok_first, WHERE_BEFORE);
+ }
Ditto.
--
To unsubscribe from this list: send the line 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 v6 04/11] trailer: process command line trailer arguments
Christian Couder chrisc...@tuxfamily.org writes:
diff --git a/trailer.c b/trailer.c
index 5b8e28b..5d69c00 100644
--- a/trailer.c
+++ b/trailer.c
@@ -378,3 +378,96 @@ static int git_trailer_config(const char *conf_key,
const char *value, void *cb)
...
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+ char* tok, char* val)
Asterisk sticks to the variable, not the type.
+static struct trailer_item *create_trailer_item(const char *string)
+{
...
+ return new_trailer_item(NULL, strbuf_detach(tok, NULL),
strbuf_detach(val, NULL));;
Overlong line. Perhaps that helped you to miss the double-semicolon
at the end.
--
To unsubscribe from this list: send the line 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 v6 05/11] trailer: parse trailers from stdin
Christian Couder chrisc...@tuxfamily.org writes:
diff --git a/trailer.c b/trailer.c
index 5d69c00..e0e066f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -50,6 +50,13 @@ static size_t alnum_len(const char *buf, size_t len)
return len;
}
+static inline int contains_only_spaces(const char *str)
+{
+ const char *s;
+ for (s = str; *s isspace(*s); s++);
Have an empty statement on a separate line for readability. I.e.
for (...)
; /* keep skipping */
@@ -471,3 +478,72 @@ static struct trailer_item
*process_command_line_args(int argc, const char **arg
...
+static void process_stdin(struct trailer_item **in_tok_first,
+ struct trailer_item **in_tok_last)
+{
...
+ /* Print non trailer lines as is */
+ for (i = 0; lines[i] i start; i++) {
+ printf(%s, lines[i]-buf);
+ }
Needless brace pair.
--
To unsubscribe from this list: send the line 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 v6 09/11] trailer: execute command from 'trailer.name.command'
Christian Couder chrisc...@tuxfamily.org writes:
diff --git a/trailer.c b/trailer.c
index ab93c16..67e7baf 100644
--- a/trailer.c
+++ b/trailer.c
@@ -490,12 +544,22 @@ static struct trailer_item
*process_command_line_args(int argc, const char **arg
...
+ /* Add conf commands that don't use $ARG */
+ for (item = first_conf_item; item; item = item-next) {
+ if (item-conf.command !item-conf.command_uses_arg)
+ {
Opening brace of a block sits on the same line as the closing
condition of the control. I.e.
if (...) {
--
To unsubscribe from this list: send the line 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 v6 08/11] trailer: add tests for git interpret-trailers
Christian Couder chrisc...@tuxfamily.org writes: +# We want one trailing space at the end of each line. +# Let's use sed to make sure that these spaces are not removed +# by any automatic tool. +test_expect_success 'setup 3' ' + sed -e s/ Z\$/ / complex_message_trailers -\EOF +Fixes: Z +Acked-by: Z +Reviewed-by: Z +Signed-off-by: Z +EOF +' It is a bit disappointing to see that these are flushed to the left, when the here-doc redirection uses -\EOF (not \EOF) to allow you to indent, i.e. sed ... -\EOF foo bar EOF or even more readable: sed ... -\EOF foo bar EOF no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] Halt during fetch on MacOS
Conley Owens c...@android.com writes: On Fri, Feb 28, 2014 at 3:26 PM, Conley Owens c...@android.com wrote: $ git --version # This is just the git from MacPorts git version 1.8.5.5 $ sw_vers ProductName:Mac OS X ProductVersion: 10.8.5 BuildVersion: 12F45 OK, I've tried using my own build from master, and I still get the same results. I've done a little more investigation and discovered it always hangs at: `atexit(notify_parent);` in `run-command.c:start_command` when running: trace: run_command: 'git-remote-https' 'aosp' 'https://android.googlesource.com/platform/external/tinyxml2' Could this have to do with the atexit implementation? (eg. limit on the number of functions that can be registered, etc) Thanks. An interesting theory indeed. I read that an implementation is supposed to take at least ATEXIT_MAX (32) calls to atexit(3); while I do think we register functions with atexit(3) from multiple places in our code, I doubt we would be making that many. $ cc -v Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn) Target: x86_64-apple-darwin12.5.0 Thread model: posix -- To unsubscribe from this list: send the line 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] upload-pack: send shallow info over stdin to pack-objects
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
From: Duy Nguyen pclo...@gmail.com
Before cdab485 (upload-pack: delegate rev walking in shallow fetch to
pack-objects - 2013-08-16) upload-pack does not write to the source
repository. cdab485 starts to write $GIT_DIR/shallow_XX if it's a
shallow fetch, so the source repo must be writable.
git:// servers do not need write access to repos and usually don't
have it, which means cdab485 breaks shallow clone over git://
Instead of using a temporary file as the media for shallow points, we
can send them over stdin to pack-objects as well. Prepend shallow
SHA-1 with --shallow so pack-objects knows what is
what.
read_object_list_from_stdin() does not accept --shallow lines because
upload-pack never uses that code. When upload-pack does, then it can
learn about --shallow lines.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
OK new approach, stop creating shallow_XX in upload-pack.
Thanks.
I like what I see in this patch, but I wonder if we can essentially
revert that temporary shallow file patch and replace it with the
same (or a similar) mechanism uniformly?
On the receive-pack side, the comment at the bottom of
preprare_shallow_update() makes it clear that, if we wanted to use
hooks, we cannot avoid having the proposed new shallow-file in a
temporary file, which is unfortunate. Do we have a similar issue on
hooks on the upload-pack side?
builtin/pack-objects.c | 7 +++
shallow.c| 2 ++
t/t5537-fetch-shallow.sh | 13 +
upload-pack.c| 21 -
4 files changed, 34 insertions(+), 9 deletions(-)
Nothing for Documentation/ anywhere?
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..79e848e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2467,6 +2467,13 @@ static void get_object_list(int ac, const char **av)
write_bitmap_index = 0;
continue;
}
+ if (starts_with(line, --shallow )) {
+ unsigned char sha1[20];
+ if (get_sha1_hex(line + 10, sha1))
+ die(not an SHA-1 '%s', line + 10);
+ register_shallow(sha1);
+ continue;
+ }
die(not a rev '%s', line);
}
if (handle_revision_arg(line, revs, flags,
REVARG_CANNOT_BE_FILENAME))
diff --git a/shallow.c b/shallow.c
index bbc98b5..41ff4a0 100644
--- a/shallow.c
+++ b/shallow.c
@@ -33,6 +33,8 @@ int register_shallow(const unsigned char *sha1)
graft-nr_parent = -1;
if (commit commit-object.parsed)
commit-parents = NULL;
+ if (is_shallow == -1)
+ is_shallow = 1;
return register_commit_graft(graft, 0);
}
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 3ae9092..a980574 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -173,4 +173,17 @@ EOF
)
'
+test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
+ cp -R .git read-only.git
+ find read-only.git -print | xargs chmod -w
+ test_when_finished find read-only.git -type d -print | xargs chmod +w
+ git clone --no-local --depth=2 read-only.git from-read-only
+ git --git-dir=from-read-only/.git log --format=%s actual
+ cat expect EOF
+add-1-back
+4
+EOF
+ test_cmp expect actual
+'
+
test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data,
ssize_t sz)
return sz;
}
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+ FILE *fp = cb_data;
+ if (graft-nr_parent == -1)
+ fprintf(fp, --shallow %s\n, sha1_to_hex(graft-sha1));
+ return 0;
+}
+
static void create_pack_file(void)
{
struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
const char *argv[12];
int i, arg = 0;
FILE *pipe_fd;
- char *shallow_file = NULL;
if (shallow_nr) {
- shallow_file = setup_temporary_shallow(NULL);
argv[arg++] = --shallow-file;
- argv[arg++] = shallow_file;
+ argv[arg++] = ;
}
argv[arg++] = pack-objects;
argv[arg++] = --revs;
@@ -114,6 +120,9 @@ static void create_pack_file(void)
pipe_fd = xfdopen(pack_objects.in, w);
+ if (shallow_nr)
+ for_each_commit_graft(write_one_shallow, pipe_fd);
+
for (i = 0; i want_obj.nr; i++)
fprintf(pipe_fd, %s\n,
Re: [PATCH] disable grafts during fetch/push/bundle
Jeff King p...@peff.net writes: I also noticed that the diff engine does not play well with replacements of blobs. When we are diffing the trees, we see that the sha1 for path foo is the same on either side, and do not look further, even though feeding those sha1s to builtin_diff would fetch the replacements. Sorry, I do not quite understand. In git diff A B -- path, if the object name recorded for A:path and B:path are the same, but the replacement mechanism maps the object name for that blob object to some other blob object, wouldn't the result be empty because both sides replace to the same thing anyway? -- To unsubscribe from this list: send the line 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 GSoC idea: git configuration caching (needs co-mentor!)
Michael Haggerty mhag...@alum.mit.edu writes:
I just wrote up the idea that fell out of the discussion [1] about the
other configuration features that I proposed. As far as I am concerned,
it can be merged as soon as somebody volunteers as a co-mentor. The
idea is embodied in a pull request against the git.github.io repository
[2]; the text is also appended below for your convenience.
Michael
[1] http://article.gmane.org/gmane.comp.version-control.git/242952
[2] https://github.com/git/git.github.io/pull/7
### git configuration API improvements
There are many places in Git that need to read a configuration value.
Currently, each such site calls `git_config()`, which reads and parses
the configuration files every time that it is called. This is
wasteful, because it results in the configuration files being
processed multiple times during a single `git` invocation. It also
prevents the implementation of potential new features, like adding
syntax to allow a configuration file to unset a previously-set value.
This goal of this project is to make configuration work as follows:
* Read the configuration from files once and cache the results in an
appropriate data structure in memory.
* Change `git_config()` to iterate through the pre-read values in
memory rather than re-reading the configuration files.
* Add new API calls that allow the cache to be inquired easily and
efficiently. Rewrite other functions like `git_config_int()` to be
cache-aware.
Are you sure about the second sentence of this item is what you
want?
git_config_type(name, value) are all about parsing value (string
or NULL) as type, return the parsed value or complain against a
bad value for name. They do not care where these name and
value come from right now, and there is no reason for them to
start caring about caching. They will still be the underlying
helper functions the git_config() callbacks will depend on even
after the second item in your list happens.
A set of new API calls would look more like this, I would think:
extern int git_get_config_string_multi(const char *, int *, const char
***);
const char **values;
int num_values;
if (git_get_config_string_multi(sample.str, num_values, values))
return -1;
printf([sample]\n);
for (i = 0; i num_values; i++)
printf( str = %s\n, value[i]);
printf(\n);
free(values);
with a singleton wrapper that may be in essense:
const char *git_get_config_string(const char *name)
{
const char **values, *result;
int num_values;
if (git_get_config_string_multi(sample.str, num_values,
values))
return NULL;
result = num_values ? values[num_values - 1] : NULL;
free(values);
return result;
}
that implements the last one wins semantics. The real thing would
need to avoid allocation and free overhead.
* Rewrite callers to use the new API wherever possible.
You will need to consider how to handle other config API entry points
like `git_config_early()` and `git_config_from_file()`, as well as how
to invalidate the cache correctly in the case that the configuration
is changed while `git` is executing.
See
[this mailing list
thread](http://article.gmane.org/gmane.comp.version-control.git/242952)
for some discussion about this and related ideas.
- Language: C
- Difficulty: medium
- Possible mentors: Michael Haggerty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] Replace memcpy with hashcpy when dealing hash copy globally
Sun He sunheeh...@gmail.com writes: hashcpy() can keep the abstraction of object name behind it. Do we really want to change the phrasing you took the above from to say *can* keep? Providing the object name abstraction is the whole point of the function, so of course it can keep it, but that goes without saying---it was the sole reason why it was invented in the first place. Use it instead of memcpy() with hard-coded 20-byte length when moving object names between pieces of memory. We can benefit from it, when we switch to another hash algorithm, eg. MD5, by just fixing the hashcpy(). fix can be used in two scenarios, I think. Something is broken and you fix it, or something keeps changing and you force it not to change. I do not think either applies to hashcpy(). Perhaps updating, if we really wanted to say it, but because this change is not about preparing us to any planned switch of hash function, I'd suggest dropping those two lines starting from We can benefit from Leave ppc/sha1.c as it is, because the function is about the SHA-1 hash algorithm whose output is and will always be 20-byte. Correct. -- To unsubscribe from this list: send the line 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] commit.c: Replace starts_with() with skip_prefix()
We already have 147972b1 (commit.c: use skip_prefix() instead of starts_with(), 2014-03-04) that covers the record_author_date() and parse_gpg_output(), don't we? -- To unsubscribe from this list: send the line 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] commit.c: Replace starts_with() with skip_prefix()
Karthik Nayak karthik@gmail.com writes: @@ -1098,6 +1099,7 @@ int parse_signed_commit(const unsigned char *sha1, char *buffer = read_sha1_file(sha1, type, size); int in_signature, saw_signature = -1; char *line, *tail; + const char *gpg_sig; if (!buffer || type != OBJ_COMMIT) goto cleanup; @@ -1113,9 +1115,9 @@ int parse_signed_commit(const unsigned char *sha1, next = next ? next + 1 : tail; if (in_signature line[0] == ' ') sig = line + 1; - else if (starts_with(line, gpg_sig_header) - line[gpg_sig_header_len] == ' ') - sig = line + gpg_sig_header_len + 1; + else if ((gpg_sig = skip_prefix(line, gpg_sig_header)) +gpg_sig[0] == ' ') + sig = gpg_sig + 1; I am not sure if this hunk is a great improvement, as we know the length of what we are skipping in the gpg_sig_header_len constant that is used throughout this file. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC GSoc Idea: blame: do not overly favor earlier parents
When looking at a merge, git blame inspects the blob object names of all parents and if one of them exactly match the merge result, pass the entire blame down to that parent. This is very much in line with the history simplification done with git log when traversing a history with merges. On the other hand, when the blob object in the merge result and none of the parents match exactly, we let each parent to take as much blame as they can, starting from the earlier parent, and later parents get a chance to take blame on the leftover bits. Combination of the above can lead to an unexpected results. Let's say that M is a two-parent merge, M^1 == A and M^2 == B, and that M:path == B:path != A:path (i.e. the merge result matches its second parent exactly). The entire contents of the path is blamed to the history leading to B; the history leading to A but not involved in B will not get any blame. Now, imagine if you amend M to create N, to add a single line at the end of path. M:path != N:path but there is very small difference between the two. That means B:path != N:path but the difference between this merged result and the second parent is very small. Because we give the chance to get blamed for the whole thing to the first parent, however, A will grab blame for all the lines that are common between A:path and B:path. For the lines that are the same between M:path and N:path, ideally, we should get identical results, but it results in a very inconsistent behaviour. Update blame.c::pass_blame() and give an option to arrange the list of scapegoats in the order that are similar to the end result, in order to address this issue. That way, when blaming N:path, we will inspect B:path first and let it grab as much blame, as it would happen when we started the blame for M:path. -- To unsubscribe from this list: send the line 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 GSoc Idea: blame: do not overly favor earlier parents
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: When looking at a merge, git blame inspects the blob object names of all parents and if one of them exactly match the merge result, pass the entire blame down to that parent. This is very much in line with the history simplification done with git log when traversing a history with merges. [...] Now, imagine if you amend M to create N, to add a single line at the end of path. M:path != N:path but there is very small difference between the two. That means B:path != N:path but the difference between this merged result and the second parent is very small. That sounds very much like commit d5df1593f27bfceab807242a538cb3fa01256efd Merge: 7144168 0b4e246 Author: Junio C Hamano gits...@pobox.com Date: Fri Feb 28 13:51:19 2014 -0800 Merge branch 'bl/blame-full-history' into pu That one was an attempt to solve the right issue in a wrong way, made things worse by breaking the consistency with the history simplification of git log. The Idea is to solve it in the way that is still consistent with the usual history simplification. -- To unsubscribe from this list: send the line 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] use strchrnul() in place of strchr() and strlen()
Rohit Mani rohit.m...@outlook.com writes:
Avoid scanning strings twice, once with strchr() and then with
strlen(), by using strchrnul(). Update the conditional expressions
involving the return value of strchrnul() with a check for '\0'.
Signed-off-by: Rohit Mani rohit.m...@outlook.com
---
Nicely done. I am not sure if you need to say the update the
conditional..., which is a logical consequence of such a conversion
and goes without saying, though.
cache-tree.c | 16 +++-
This part may overlap with other topics in flight, but I expect the
conflict resolution would be trivial.
diff --git a/cache-tree.c b/cache-tree.c
index 0bbec43..21a13cf 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -121,11 +121,11 @@ void cache_tree_invalidate_path(struct cache_tree *it,
const char *path)
if (!it)
return;
- slash = strchr(path, '/');
+ slash = strchrnul(path, '/');
it-entry_count = -1;
- if (!slash) {
+ if (*slash == '\0') {
Let's just say
if (!*slash)
instead; it is more idiomatic (I won't repeat this for other hunks).
int pos;
- namelen = strlen(path);
+ namelen = slash - path;
After this if (!*slash), we compute namelen = slash-path.
Perhaps we can lose this assignment and the other one by hoisting it
up before if (!*slash)?
@@ -564,10 +562,10 @@ static struct cache_tree *cache_tree_find(struct
cache_tree *it, const char *pat
+ if (*slash == '\0' || !*slash)
Huh? The byte pointed at by 'slash' is NUL, or it is NUL???
Other than that, looks good to me.
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 0/6] fix hunk editing with 'commit -p -m'
Benoit Pierre benoit.pie...@gmail.com writes: This patch fixes the fact that hunk editing with 'commit -p -m' does not work: GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched, which result in the 'hunk edit' option not launching the editor (and selecting the whole hunk). The fix consists in deferring the GIT_EDITOR override to the hook subprocess, like it's already done for GIT_INDEX_FILE: - modify 'run_hook' so the first parameter is the environment to set - add a 'run_hook_v' variant that take a va_list - add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE) I sense that this is in line with one of the leftover bits items I keep in http://git-blame.blogspot.com/p/leftover-bits.html, especially http://thread.gmane.org/gmane.comp.version-control.git/192669/focus=192806 ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] commit: fix patch hunk editing with commit -p -m
Benoit Pierre benoit.pie...@gmail.com writes:
+int run_commit_hook(int editor_is_used, const char *index_file, const char
*name, ...)
+{
+ const char *hook_env[3] = { NULL };
+ char index[PATH_MAX];
+ va_list args;
+ int ret;
+
+ snprintf(index, sizeof(index), GIT_INDEX_FILE=%s, index_file);
+ hook_env[0] = index;
+
+ /*
+ * Let the hook know that no editor will be launched.
+ */
+ if (!editor_is_used)
+ hook_env[1] = GIT_EDITOR=:;
+
+ va_start(args, name);
+ ret = run_hook_v(hook_env, name, args);
diff --git a/run-command.c b/run-command.c
index 3914d9c..4e9be12 100644
--- a/run-command.c
+++ b/run-command.c
@@ -760,13 +760,11 @@ char *find_hook(const char *name)
return path;
}
-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_v(const char *const *env, const char *name, va_list args)
{
I think you named it as foo_v() for this takes va_list in a way
similar to the v in execv(), but this also takes environment, so
perhaps we want to say ve instead?
Other than that, I like it---I admit that I am biased that I
essentially did the same earlier but with a _le variant ;-)
+int run_hook(const char *const *env, const char *name, ...)
+{
I'd rather not to see this changed in the same commit, so that any
other topic in-flight that adds a new call to run_hook() that expects
to pass the index file as its first parameter will not be broken.
Name it run_hook_le() (name modelled after execle()), and call it in
your change where you add new calls to this function, and add a thin
wrapper run_hook() that preserves the traditional We can pass only
the index-file for new callers we do not even know about on the
topics in flight.
Later we can eradicate callers of run_hook() that treats the index-file
specially, which was a grave mistake in a public API.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/6] merge hook tests: fix missing '' in test
Benoit Pierre benoit.pie...@gmail.com writes: Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- Please have these preparatory fix-ups (i.e. the changes that would be immediately useful even if it turns out that the main body of the series were not ready for inclusion) early in the series, not late as 5th patch of a 6 patch series. Thanks. t/t7505-prepare-commit-msg-hook.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index ae7b2db..604c06e 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -189,7 +189,7 @@ test_expect_success 'with failing hook (merge)' ' git add file rm -f $HOOK git commit -m other - write_script $HOOK -EOF + write_script $HOOK -EOF exit 1 EOF git checkout - -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'
Benoit Pierre benoit.pie...@gmail.com writes: Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- t/t7505-prepare-commit-msg-hook.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index 604c06e..1be6cec 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -167,7 +167,7 @@ test_expect_success 'with failing hook' ' head=`git rev-parse HEAD` echo more file git add file - ! GIT_EDITOR=\\$FAKE_EDITOR\ git commit -c $head + test_must_fail env GIT_EDITOR=\\$FAKE_EDITOR\ git commit -c $head Thanks. It is good that you avoided the common pitfall of attempting GIT_EDITOR=... test_must_fail git commit -c $head;# WRONG but do we assume everybody has env? To be portable, we can do this instead. ( GIT_EDITOR=... export GIT_EDITOR test_must_fail git commit -c $head ) -- To unsubscribe from this list: send the line 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/6] test patch hunk editing with commit -p -m
Eric Sunshine sunsh...@sunshineco.com writes: +test_expect_failure 'edit hunk commit -p -m message' ' + echo e | env GIT_EDITOR=sed s/+line3\$/+line2/ -i git commit -p -m commit2 file + git diff HEAD^ HEAD diff + test_cmp expected diff +' If you ever add more tests, is it likely that they will be using the same 'expected' file used by this test? If not, perhaps it makes sense to move creation of 'expected' into the test itself. All good points. Also, I think we try to use expect (not expected) vs actual (not diff) in most of the tests. -- To unsubscribe from this list: send the line 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] *.sh: drop useless use of env
In a bourne shell script, VAR=VAL command is sufficient to run 'command' with environment variable VAR set to value VAL without affecting the environment of the shell itself; there is no reason to say env VAR=VAL command. Signed-off-by: Junio C Hamano gits...@pobox.com --- * Just something I noticed while reading existing tests... t/t1020-subdirectory.sh | 2 +- t/t9001-send-email.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh index 1e2945e..6902320 100755 --- a/t/t1020-subdirectory.sh +++ b/t/t1020-subdirectory.sh @@ -148,7 +148,7 @@ test_expect_success 'GIT_PREFIX for built-ins' ' ( cd dir printf change two - env GIT_EXTERNAL_DIFF=./diff git diff ../actual + GIT_EXTERNAL_DIFF=./diff git diff ../actual git checkout -- two ) test_cmp expect actual diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 3119c8c..1ecdacb 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -409,7 +409,7 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' ' (echo From Example f...@example.com echo To Example t...@example.com echo - ) | env GIT_SEND_EMAIL_NOTTY=1 git send-email \ + ) | GIT_SEND_EMAIL_NOTTY=1 git send-email \ --smtp-server=$(pwd)/fake.sendmail \ $patches 2errors ! grep ^In-Reply-To: * msgtxt1 -- To unsubscribe from this list: send the line 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: gc/repack has no option to lose grafted parents
Martin Langhoff martin.langh...@gmail.com writes: Back in http://git.661346.n2.nabble.com/PATCH-0-2-Make-git-gc-more-robust-with-regard-to-grafts-td3310281.html we got gc/repack to be safer for users who might be shooting themselves in the foot. Would a patch be welcome to add --discard-grafted-objects ? or --keep-real-parents=idontthinkso ? cheers, Given that we in general frown upon long-term use of grafts (or replace for that matter), I am not sure if we want to go in that direction. Just a knee-jerk reaction, though. -- To unsubscribe from this list: send the line 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] disable grafts during fetch/push/bundle
Michael Haggerty mhag...@alum.mit.edu writes: I didn't mean to insult all Windows users in general. I was only referring to the fact that since the default Windows command line is not a POSIX shell, even an experienced Windows user might have trouble figuring out how to execute a shell loop. Putting this functionality in a git command or script, by contrast, would make it work universally, no fuss, no muss. ;-) Be it graft or replace, I do not think we want to invite people to use these mechansims too lightly to locally rewrite their history willy-nilly without fixing their mistakes at the object layer with commit --amend, rebase, bfg, etc. in the longer term. So in that sense, adding a command to make it easy is not something I am enthusiastic about. On the other hand, if the user does need to use graft or replace (perhaps to prepare for casting the fixed history in stone with filter-branch), it would be good to help them avoid making mistakes while doing so and tool support may be a way to do so. So, ... I am of two minds. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/6] merge hook tests: use 'test_must_fail' instead of '!'
Junio C Hamano gits...@pobox.com writes: Benoit Pierre benoit.pie...@gmail.com writes: Signed-off-by: Benoit Pierre benoit.pie...@gmail.com --- t/t7505-prepare-commit-msg-hook.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index 604c06e..1be6cec 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -167,7 +167,7 @@ test_expect_success 'with failing hook' ' head=`git rev-parse HEAD` echo more file git add file -! GIT_EDITOR=\\$FAKE_EDITOR\ git commit -c $head +test_must_fail env GIT_EDITOR=\\$FAKE_EDITOR\ git commit -c $head Thanks. It is good that you avoided the common pitfall of attempting GIT_EDITOR=... test_must_fail git commit -c $head;# WRONG but do we assume everybody has env? It turns out that the answer to this question seems to be yes, we already do.; so the patch is probably OK as-is. Thanks. To be portable, we can do this instead. ( GIT_EDITOR=... export GIT_EDITOR test_must_fail git commit -c $head ) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[micro] Use 'env' on test_must_fail as appropriate
Because VAR=VAL command is sufficient to run 'command' with environment variable VAR set to value VAL without affecting the environment of the shell itself, but we cannot do the same with a shell function (most notably, test_must_fail), we have subshell invocations with multiple lines like this: ... ( VAR=VAL export VAR test_must_fail git command ) ... which could be expressed as ... test_must_fail env VAR=VAL git comand ... Find and shorten such constructs in existing test scripts. Note that I am not 100% convinced myself that it is a good idea to do this, so please do not add this to the list without seeing it discussed. 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
