Re: [PATCH v12 13/13] bisect--helper: `bisect_start` shell function partially in C
On Wed, Aug 10, 2016 at 11:57 PM, Pranit Bauva wrote:
>
> @@ -431,6 +434,244 @@ static int bisect_terms(struct bisect_terms *terms,
> const char **argv, int argc)
> return 0;
> }
>
> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
> + const char **argv, int argc)
> +{
> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
> + int flags, pathspec_pos;
> + struct string_list revs = STRING_LIST_INIT_DUP;
> + struct string_list states = STRING_LIST_INIT_DUP;
> + struct strbuf start_head = STRBUF_INIT;
> + struct strbuf bisect_names = STRBUF_INIT;
> + struct strbuf orig_args = STRBUF_INIT;
> + const char *head;
> + unsigned char sha1[20];
> + FILE *fp;
> + struct object_id oid;
> +
> + if (is_bare_repository())
> + no_checkout = 1;
> +
> + for (i = 0; i < argc; i++) {
> + char *commit_id = xstrfmt("%s^{commit}", argv[i]);
> + if (!strcmp(argv[i], "--")) {
> + has_double_dash = 1;
> + break;
> + }
In the shell code there is a loop dedicated to checking if there is a
double dash in the arguments before the real argument parsing loop.
There is a reason for that.
If you do it in the same loop, has_double_dash will not be set when
the arguments that are before the double dash will be parsed.
> + else if (!strcmp(argv[i], "--no-checkout"))
> + no_checkout = 1;
> + else if (!strcmp(argv[i], "--term-good") ||
> +!strcmp(argv[i], "--term-old")) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_good);
> + strbuf_addstr(&terms->term_good, argv[++i]);
> + }
> + else if (skip_prefix(argv[i], "--term-good=", &argv[i])) {
(Maybe you could put the "else if (...) {" on the same line as the "}" above.)
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_good);
> + strbuf_addstr(&terms->term_good, argv[i]);
> + }
> + else if (skip_prefix(argv[i], "--term-old=", &argv[i])) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_good);
> + strbuf_addstr(&terms->term_good, argv[i]);
> + }
> + else if (!strcmp(argv[i], "--term-bad") ||
> +!strcmp(argv[i], "--term-new")) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_bad);
> + strbuf_addstr(&terms->term_bad, argv[++i]);
> + }
> + else if (skip_prefix(argv[i], "--term-bad=", &argv[i])) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_bad);
> + strbuf_addstr(&terms->term_bad, argv[i]);
> + }
> + else if (skip_prefix(argv[i], "--term-new=", &argv[i])) {
> + must_write_terms = 1;
> + strbuf_reset(&terms->term_good);
> + strbuf_addstr(&terms->term_good, argv[i]);
> + }
> + else if (starts_with(argv[i], "--") &&
> +!one_of(argv[i], "--term-good", "--term-bad", NULL))
> {
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + die(_("unrecognised option: '%s'"), argv[i]);
> + }
> + else if (get_oid(argv[i], &oid) && !has_double_dash) {
And here checking "!has_double_dash" has no meaning if you check for a
double dash in the same loop, because there is a "break" after
has_double_dash is set above.
> + string_list_clear(&revs, 0);
> + string_list_clear(&states, 0);
> + free(commit_id);
> + die(_("'%s' does not appear to be a valid revision"),
> argv[i]);
> + }
> + else {
> + free(commit_id);
> + string_list_append(&revs, oid_to_hex(&oid));
> + }
> + }
> + pathspec_pos = i;
> +
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A note from the maintainer
On Fri, Aug 12, 2016 at 10:42:55PM +, Eric Wong wrote: > Junio C Hamano wrote: > > For those who prefer to read it over NNTP: > > > > nntp://news.gmane.org/gmane.comp.version-control.git > > > > is still available. An alternative > > > > nntp://news.public-inbox.org/inbox.comp.version-control.git > > > > will become usable once it catches up with old messages. > > Mostly caught up, I injected 33 more today which were > cross-posted (which tripped up some of my anti-spam rules) or > simply missed by gmane. > > There may be more in some personal archives gmane doesn't > have... Is there an easy way to get _just_ the list of message-ids you are storing (I know I can download the whole archive, but it's big)? Then I can cross-reference with my archive. I doubt I'll have anything significant that you don't. My archive of the early days was pulled from gmane, though I have been collecting steadily via mailing list delivery since 2007 or so. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Speed up sparse checkout when $GIT_DIR/info/sparse-checkout is unchanged
Ping..
On Tue, Jul 12, 2016 at 1:15 AM, Nguyễn Thái Ngọc Duy wrote:
> When a "tree unpacking" operation is needed, which is part of
> switching branches using "git checkout", the following happens in a
> sparse checkout:
>
> 1) Run all existing entries through $GIT_DIR/info/sparse-checkout,
>mark entries that are to-be-excluded or to-be-included.
>
> 2) Do n-way merge stuff to add, modify and delete entries.
>
> 3) Run all new entries added at step 2 through
>$GIT_DIR/info/sparse-checkout, mark entries that are to-be-excluded
>or to-be-included.
>
> 4) Compare the current excluded/include status with the to-be-status
>from steps 1 and 3, delete newly excluded entries from worktree and
>add newly included ones to worktree.
>
> The "all existing entries" number in step 1 does not scale well. As
> worktree gets bigger (or more sparse patterns are added), step 1 runs
> slower. Which does not help because large worktrees are the reason
> sparse-checkout is used, to keep the real worktree small again.
>
> If we know that $GIT_DIR/info/sparse-checkout has not changed, we know
> that running checking again would result in the exact same
> included/excluded as recorded in the current index because
> "sparse-checkout" is the only input to the exclude machinery. In this
> case, marking the to-be-status is simply copying the current status
> over, which is a lot faster.
>
> The time breakdown of "git checkout" (no arguments) on webkit.git
> (100k files) with a sparse checkout file of 4 negative patterns is
> like this, where "sparse checkout loop #1" takes about 10% execution
> time, which is the time saved after this patch.
>
> read-cache.c:1661 performance: 0.057816104 s: read cache .git/index
> files-backend.c:1097performance: 0.23980 s: read packed refs
> preload-index.c:104 performance: 0.039178367 s: preload index
> read-cache.c:1260 performance: 0.002700730 s: refresh index
> name-hash.c:128 performance: 0.030409968 s: initialize name hash
>
> unpack-trees.c:1173 performance: 0.100353572 s: sparse checkout loop #1
>
> cache-tree.c:431performance: 0.137213472 s: cache_tree_update
> unpack-trees.c:1305 performance: 0.648923590 s: unpack_trees
> read-cache.c:2139 performance: 0.074800165 s: write index, changed mask
> = 28
> unpack-trees.c:1305 performance: 0.137108835 s: unpack_trees
> diff-lib.c:506 performance: 0.137152238 s: diff-index
> trace.c:420 performance: 0.972682413 s: git command: 'git'
> 'checkout'
>
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> I mentioned about this some time ago and finally got curious enough
> to try out. The saving is in the signficant range in my opinion, but
> real world effect is probably lower (or much higher if you have so
> many patterns in $GIT_DIR/info/sparse-checkout)
>
> Note that both cache_tree_update and sparse checkout loop #1 are part
> of unpack_trees() so actual time spent on this function is more like
> 0.4s. It's still a lot, but then this function is very scary to
> optimize.
>
> Documentation/technical/index-format.txt | 6 +
> cache.h | 2 ++
> read-cache.c | 22 -
> unpack-trees.c | 42
> ++--
> 4 files changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/technical/index-format.txt
> b/Documentation/technical/index-format.txt
> index ade0b0c..3b0770a 100644
> --- a/Documentation/technical/index-format.txt
> +++ b/Documentation/technical/index-format.txt
> @@ -295,3 +295,9 @@ The remaining data of each directory block is grouped by
> type:
> in the previous ewah bitmap.
>
>- One NUL.
> +
> +== Sparse checkout cache
> +
> + Sparse checkout extension saves the 20 bytes SHA-1 hash of
> + $GIT_DIR/info/sparse-checkout at the time it is applied to the
> + index.
> diff --git a/cache.h b/cache.h
> index f1dc289..cc4c2b1 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -320,6 +320,7 @@ static inline unsigned int canon_mode(unsigned int mode)
> #define CACHE_TREE_CHANGED (1 << 5)
> #define SPLIT_INDEX_ORDERED(1 << 6)
> #define UNTRACKED_CHANGED (1 << 7)
> +#define SPARSE_CHECKOUT_CHANGED(1 << 8)
>
> struct split_index;
> struct untracked_cache;
> @@ -338,6 +339,7 @@ struct index_state {
> struct hashmap dir_hash;
> unsigned char sha1[20];
> struct untracked_cache *untracked;
> + unsigned char sparse_checkout[20];
> };
>
> extern struct index_state the_index;
> diff --git a/read-cache.c b/read-cache.c
> index db27766..371d2c7 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -40,11 +40,13 @@ static struct cache_entry *refresh_cache_entry(struct
> cache_entry *ce,
> #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
> #define CACHE_EXT_LINK 0x6c696e6b/* "link" */
> #define CACHE_EXT_UNTRACKED 0x554E5452 /* "UNTR"
Re: [PATCH v4] config: add conditional include
Ping..
On Thu, Jul 14, 2016 at 10:33 PM, Nguyễn Thái Ngọc Duy
wrote:
> Helped-by: Jeff King
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> The diff from v3 is mostly clarification in code and document.
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 18623ee..d971334 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -99,10 +99,9 @@ Supported keywords are:
>
> `gitdir`::
> The environment variable `GIT_DIR` must match the following
> - pattern for files to be included. The pattern can contain
> - standard globbing wildcards and two additional ones, `**/` and
> - `/**`, that can match multiple path components. Please refer
> - to linkgit:gitignore[5] for details. For convenience:
> + pattern for files to be included. The pattern shares the same
> + syntax as patterns in link:gitignore[5] with a few exceptions
> + below:
>
> * If the pattern starts with `~/`, `~` will be substituted with the
> content of the environment variable `HOME`.
> diff --git a/config.c b/config.c
> index ff44e00..690f3d5 100644
> --- a/config.c
> +++ b/config.c
> @@ -183,6 +183,11 @@ static int prepare_include_condition_pattern(struct
> strbuf *pat)
>
> strbuf_add_absolute_path(&path, home);
> strbuf_splice(pat, 0, 1, path.buf, path.len);
> + /*
> +* This part, path.buf[0..len], should be considered
> +* a literal string even if it has wildcards in it,
> +* because those wildcards are not wanted by the user.
> +*/
> prefix = path.len + 1 /*slash*/;
> strbuf_release(&path);
> } else if (pat->buf[0] == '.' && is_dir_sep(pat->buf[1])) {
> @@ -198,6 +203,11 @@ static int prepare_include_condition_pattern(struct
> strbuf *pat)
> if (!slash)
> die("BUG: how is this possible?");
> strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
> + /*
> +* This part, path.buf[0..slash], should be consider
> +* a literal string even if it has wildcards in it,
> +* because those wildcards are not wanted by the user.
> +*/
> prefix = slash - path.buf + 1 /* slash */;
> strbuf_release(&path);
> } else if (!is_absolute_path(pat->buf))
> @@ -224,8 +234,9 @@ static int include_by_gitdir(const char *cond, size_t
> cond_len, int icase)
>
> if (prefix > 0) {
> /*
> -* perform literal matching on the prefix part so that
> -* any wildcard character in it can't create side effects.
> +* perform literal matching on the expanded prefix
> +* part so that any wildcard character in it (e.g in
> +* the expansion of ~) can't create side effects.
> */
> if (text.len < prefix)
> goto done;
> Documentation/config.txt | 47 +++
> config.c | 113
> +-
> t/t1305-config-include.sh | 56 +++
> 3 files changed, 214 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index db05dec..d971334 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,42 @@ found at the location of the include directive. If the
> value of the
> relative to the configuration file in which the include directive was
> found. See below for examples.
>
> +Included files can be grouped into subsections where the subsection
> +name is the condition that must be met for the files to be included.
> +The condition starts with a keyword, followed by a colon and a
> +pattern. The interpretation of the pattern depends on the keyword.
> +Supported keywords are:
> +
> +`gitdir`::
> + The environment variable `GIT_DIR` must match the following
> + pattern for files to be included. The pattern shares the same
> + syntax as patterns in link:gitignore[5] with a few exceptions
> + below:
> +
> + * If the pattern starts with `~/`, `~` will be substituted with the
> + content of the environment variable `HOME`.
> +
> + * If the pattern starts with `./`, it is replaced with the directory
> + containing the current config file.
> +
> + * If the pattern does not start with either `~/`, `./` or `/`, `**/`
> + will be automatically prepended. For example, the pattern `foo/bar`
> + becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
> +
> + * If the pattern ends with `/`, `**` will be automatically added. For
> + example, the pattern `foo/` becomes `foo/**`. In other words, it
> + matches "foo" and everything inside, recursively.
> +
> +`gitdir/i`::
> +
Re: storing cover letter of a patch series?
On Tue, Aug 9, 2016 at 12:27 AM, Stefan Beller wrote: > On Thu, Sep 10, 2015 at 9:28 AM, Jacob Keller wrote: >> Hey, >> >> does anyone know of any tricks for storing a cover letter for a patch >> series inside of git somehow? I'd guess the only obvious way currently >> is to store it at the top of the series as an empty commit.. but this >> doesn't get emailed *as* the cover letter... >> >> Is there some other way? Would others be interested in such a feature? > > Being late to this thread, but I think > >branch..description >Branch description, can be edited with git branch >--edit-description. Branch description is automatically added in >the format-patch cover letter or request-pull summary. > > is what you want. Maybe we want to see a patch that adds the reverse > functionality as well, i.e. git-am will store the the cover letter as the > branch description and git-merge will propose the branch description for > the merge commit. I almost suggested the same, but there is a problem with this approach: if you're are on a detached head, where does git-am save it? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs
On Sat, Aug 13, 2016 at 01:25:05AM +0200, Michael Haggerty wrote: > > These two flags are mutually exclusive in the xdiff code, so we should > > probably handle that here. > > > > TBH, I do not care that much what: > > > > [diff] > > compactionHeuristic = true > > indentHeuristic = true > > > > does. But right now: > > > > git config diff.compactionHeuristic true > > git show --indent-heuristic > > > > still prefers the compaction heuristic, which I think is objectively > > wrong. > > I wasn't worrying about that yet, given that these two features are both > still experimental. I also have a strong inkling that at most one of > them needs to be made permanent. I propose that I repair the semantics > in the simplest way possible for now while we decide on the long-term > plan, which might conceivably be: > > * keep both options permanently > * keep only one option permanently > * choose one heuristic and use it always (i.e., make it part of the new > standard one-and-only diff algorithm) > * discard both heuristics (I hope not!) > > After we've decided on that, *then* let's decide on a suitable UI and > implement it before we declare either feature non-experimental. Is there a case where the compaction heuristic produces a better result than this indent heuristic? AFAICT, you have not found one, and I'd be surprised if there is one, because this _seems_ like a superset generally. I suppose there is always the possibility that the empirical knobs behave badly in some particular case that the compaction heuristic just happens to get right, but it should be quite rare. So assuming everything I just said isn't complete bollocks, I think we can move to a future where nobody uses the compaction heuristic. And there are three ways to deal with that: 1. The knob and feature stay. It might be useful for somebody who wants to experiment in the future. 2. The knob and feature go away completely. It was an experiment, but now we have something more useful. 3. The feature goes away, but the knob stays as noop, or maybe as an alias for the indent heuristic, just because we did ship a version that accepts "--compaction-heuristic", and maybe somebody somewhere put it in a script? I think I'd be in favor of (2). It doesn't seem likely enough for people to experiment with to merit a run-time knob; they can always patch and build if they want to do so. And (3) just seems like a pain for something that was only shipped in one version and was kind of experimental, and was unlikely to end up in scripts (much more likely is that people set the config, but that's easier to ignore). But it does violate our usual backwards-compatibility rules. So if we assume that indent is useful and compaction goes away, the only questions are "does indent it become the default" and "if so, does it still get a knob". I'd say "yes" to both. Making the new behavior the default was what we planned to do with compaction until we saw that it regressed some cases. But as a new feature, it's nice for users to be able to easily disable it to see if it's causing a problem (or to see what a big improvement it is!). I think we could get by with just a command-line option for that purpose, rather than a config option; that saves a lot of effort in having porcelains manually propagate the config option when they call a plumbing diff-tree. I guess the only users that leaves out are ones who really want stable backwards-compatible diff. I guess "patch --stable" is one such user (but that one we could handle internally). But let's say you had a code review system that attached comments to lines of a diff. You might want to disable the feature entirely to avoid invalidating old comments. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] correct FLEXPTR_* example in comment
This section is about "The FLEXPTR_* variants", so use FLEXPTR_ALLOC_STR in the example. Signed-off-by: Rene Scharfe --- git-compat-util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index 590bfdd..f52e00b 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -815,7 +815,7 @@ extern FILE *fopen_for_writing(const char *path); * you can do: * * struct foo *f; - * FLEX_ALLOC_STR(f, name, src); + * FLEXPTR_ALLOC_STR(f, name, src); * * and "name" will point to a block of memory after the struct, which will be * freed along with the struct (but the pointer can be repointed anywhere). -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A note from the maintainer
Jeff King wrote:
> On Fri, Aug 12, 2016 at 10:42:55PM +, Eric Wong wrote:
> > Junio C Hamano wrote:
> > > is still available. An alternative
> > >
> > > nntp://news.public-inbox.org/inbox.comp.version-control.git
> > >
> > > will become usable once it catches up with old messages.
> >
> > Mostly caught up, I injected 33 more today which were
> > cross-posted (which tripped up some of my anti-spam rules) or
> > simply missed by gmane.
> >
> > There may be more in some personal archives gmane doesn't
> > have...
>
> Is there an easy way to get _just_ the list of message-ids you are
> storing (I know I can download the whole archive, but it's big)?
XHDR (or HDR) over NNTP should do it (that's how I checked
against gmane):
8<-
use Net::NNTP;
my $nntp = Net::NNTP->new($ENV{NNTPSERVER} || 'news.public-inbox.org');
my ($num, $first, $last) = $nntp->group('inbox.comp.version-control.git');
my $batch = 1;
my $i;
for ($i = $first; $i < $last; $i += $batch) {
my $j = $i + $batch - 1;
$j = $last if $j > $last;
my $num2mid = $nntp->xhdr('Message-ID', "$i-$j");
for my $n ($i..$j) {
defined(my $mid = $num2mid->{$n}) or next;
print "$mid\n";
}
}
# and I forgot to optimize XHDR/HDR further in public-inbox-nntpd.
# Oh well, it seems to work, at least.
> Then I can cross-reference with my archive. I doubt I'll have anything
> significant that you don't. My archive of the early days was pulled from
> gmane, though I have been collecting steadily via mailing list delivery
> since 2007 or so.
What's odd is there's some messages with two Message-ID fields
from gmane from the old days, too. I'll dig a bit another time.
--
To unsubscribe from this list: send the line "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] mailinfo: recycle strbuf in check_header()
handle_message_id() duplicates the contents of the strbuf that is passed
to it. Its only caller proceeds to release the strbuf immediately after
that. Reuse it instead and make that change of object ownership more
obvious by inlining this short function.
Signed-off-by: Rene Scharfe
---
mailinfo.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/mailinfo.c b/mailinfo.c
index 9f19ca1..e19abe3 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -179,12 +179,6 @@ static void handle_content_type(struct mailinfo *mi,
struct strbuf *line)
}
}
-static void handle_message_id(struct mailinfo *mi, const struct strbuf *line)
-{
- if (mi->add_message_id)
- mi->message_id = strdup(line->buf);
-}
-
static void handle_content_transfer_encoding(struct mailinfo *mi,
const struct strbuf *line)
{
@@ -495,7 +489,8 @@ static int check_header(struct mailinfo *mi,
len = strlen("Message-Id: ");
strbuf_add(&sb, line->buf + len, line->len - len);
decode_header(mi, &sb);
- handle_message_id(mi, &sb);
+ if (mi->add_message_id)
+ mi->message_id = strbuf_detach(&sb, NULL);
ret = 1;
goto check_header_out;
}
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] correct FLEXPTR_* example in comment
On Sat, Aug 13, 2016 at 11:01:21AM +0200, René Scharfe wrote: > This section is about "The FLEXPTR_* variants", so use FLEXPTR_ALLOC_STR > in the example. Oops, yeah. Your patch is clearly an improvement. Since this is obviously an easy mistake to make (using one form rather than the other), I wondered if the compiler would catch it. I think it would catch an accidental use of FLEXPTR instead of FLEX, because it involves an attempted assignment of an array. But I don't think we would catch the reverse; we'd just write the data directly on top of the pointer. That would probably crash immediately at runtime, so if you exercise the code at all in tests, it is OK. But something to be aware of. I suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or something, but I'm not sure if it is worth worrying about. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] commit: introduce set_merge_remote_desc()
Add a helper function for allocating, populating and attaching struct
merge_remote_desc to a commit and use it consistently. It allocates the
necessary memory in a single block.
commit.c::get_merge_parent() forgot to check for memory allocation
failures of strdup(3).
merge-recursive.c::make_virtual_commit() didn't duplicate the string for
the name member, even though one of it's callers (indirectly through
get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.
Signed-off-by: Rene Scharfe
---
commit.c | 18 +++---
commit.h | 2 ++
merge-recursive.c | 5 +
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/commit.c b/commit.c
index 71a360d..372b200 100644
--- a/commit.c
+++ b/commit.c
@@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,
return result;
}
+void set_merge_remote_desc(struct commit *commit,
+ const char *name, struct object *obj)
+{
+ struct merge_remote_desc *desc;
+ FLEXPTR_ALLOC_STR(desc, name, name);
+ desc->obj = obj;
+ commit->util = desc;
+}
+
struct commit *get_merge_parent(const char *name)
{
struct object *obj;
@@ -1585,13 +1594,8 @@ struct commit *get_merge_parent(const char *name)
return NULL;
obj = parse_object(oid.hash);
commit = (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
- if (commit && !commit->util) {
- struct merge_remote_desc *desc;
- desc = xmalloc(sizeof(*desc));
- desc->obj = obj;
- desc->name = strdup(name);
- commit->util = desc;
- }
+ if (commit && !commit->util)
+ set_merge_remote_desc(commit, name, obj);
return commit;
}
diff --git a/commit.h b/commit.h
index 23ae0c1..84bb507 100644
--- a/commit.h
+++ b/commit.h
@@ -365,6 +365,8 @@ struct merge_remote_desc {
const char *name;
};
#define merge_remote_util(commit) ((struct merge_remote_desc
*)((commit)->util))
+extern void set_merge_remote_desc(struct commit *commit,
+ const char *name, struct object *obj);
/*
* Given "name" from the command line to merge, find the commit object
diff --git a/merge-recursive.c b/merge-recursive.c
index e5243c2..e349126 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -73,12 +73,9 @@ static struct tree *shift_tree_object(struct tree *one,
struct tree *two,
static struct commit *make_virtual_commit(struct tree *tree, const char
*comment)
{
struct commit *commit = alloc_commit_node();
- struct merge_remote_desc *desc = xmalloc(sizeof(*desc));
- desc->name = comment;
- desc->obj = (struct object *)commit;
+ set_merge_remote_desc(commit, comment, (struct object *)commit);
commit->tree = tree;
- commit->util = desc;
commit->object.parsed = 1;
return commit;
}
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] correct FLEXPTR_* example in comment
Am 13.08.2016 um 11:09 schrieb Jeff King: On Sat, Aug 13, 2016 at 11:01:21AM +0200, René Scharfe wrote: This section is about "The FLEXPTR_* variants", so use FLEXPTR_ALLOC_STR in the example. Oops, yeah. Your patch is clearly an improvement. Since this is obviously an easy mistake to make (using one form rather than the other), I wondered if the compiler would catch it. I think it would catch an accidental use of FLEXPTR instead of FLEX, because it involves an attempted assignment of an array. But I don't think we would catch the reverse; we'd just write the data directly on top of the pointer. That would probably crash immediately at runtime, so if you exercise the code at all in tests, it is OK. But something to be aware of. I suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or something, but I'm not sure if it is worth worrying about. A compilation error or warning would be nice. I scratched my head quite a bit before figuring out that the example was wrong. Copy&paste from a reputable source must be correct, right? :) René -- To unsubscribe from this list: send the line "unsubscribe 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: introduce set_merge_remote_desc()
On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:
> Add a helper function for allocating, populating and attaching struct
> merge_remote_desc to a commit and use it consistently. It allocates the
> necessary memory in a single block.
>
> commit.c::get_merge_parent() forgot to check for memory allocation
> failures of strdup(3).
>
> merge-recursive.c::make_virtual_commit() didn't duplicate the string for
> the name member, even though one of it's callers (indirectly through
> get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.
It seems like you've buried the interesting part here. This isn't just
for cleanup, but a bugfix that the oids in our virtual commits might get
overwritten by subsequent actions.
It seems like that should be the subject and beginning of the commit
message. And then the fix is to allocate, and by the way we can do so
easily with this nice new helper. :)
> diff --git a/commit.c b/commit.c
> index 71a360d..372b200 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t
> msg_len,
> return result;
> }
>
> +void set_merge_remote_desc(struct commit *commit,
> +const char *name, struct object *obj)
> +{
> + struct merge_remote_desc *desc;
> + FLEXPTR_ALLOC_STR(desc, name, name);
> + desc->obj = obj;
> + commit->util = desc;
> +}
I don't think there is any reason to prefer FLEXPTR_ALLOC over
FLEX_ALLOC, unless your struct interface is constrained by non-flex
users (that's why it is necessary for "struct exclude", for example,
which sometimes needs to carry its own string and sometimes not).
Using FLEX_ALLOC saves a few bytes per struct, and avoids an extra
pointer indirection when accessing the data.
Since it looks like you touch all of the allocations here, I think they
would both be happy as a regular flex array.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[ANNOUNCE] Git for Windows 2.9.3
Dear Git users, It is my pleasure to announce that Git for Windows 2.9.3 is available from: https://git-for-windows.github.io/ Changes since Git for Windows v2.9.2 (July 16th 2016) New Features ??? Comes with Git 2.9.3. ??? Updated Git Credential Manager to version 1.6.0. ??? Includes support for git status --porcelain=v2. ??? Avoids evaluating unnecessary patch IDs when determining which commits do not need to be rebased because they are already upstream. ??? Sports a new --smudge option for git cat-file that lets it pass blob contents through smudge filters configured for the specified path. Bug Fixes ??? When offering to Launch Git Bash after the installation, it now launches in the home directory, consistent with the Git Bash Start Menu entry. ??? When ~/.gitconfig sets core.hideDotFiles=false, git init respects that again. Filename | SHA-256 | --- Git-2.9.3-64-bit.exe | 1a642cf2914e18fa868b1ed2c6d1df4a46ba8ef30355fd1965850895a658e024 Git-2.9.3-32-bit.exe | d6b4a19536ad408018688f1242ab0d1f5dc5544109662bfddf915f685eba58a9 PortableGit-2.9.3-64-bit.7z.exe | 3423ea5c7e025a67903e05d1493680d6d5d429a9c400cc9e33beea5adb1b PortableGit-2.9.3-32-bit.7z.exe | cd142cbb124d7c3e3c3bf144785e57573afd2fc9c043447b7f5111ec044e6ced MinGit-2.9.3-64-bit.zip | 17e40cb149ce6a348c8e8bbe7f1c1fff00f82882f0e57f32d60ea5c26feeef98 MinGit-2.9.3-32-bit.zip | 3c6515c09ccb7d1aa6620db51245c87f9836fbde5ee9af6c849b4cbd506f60e8 Git-2.9.3-64-bit.tar.bz2 | 7b165d400c2bcc427881d502dd46cd442e512053d92cba8b3e46173c5028e427 Git-2.9.3-32-bit.tar.bz2 | 00f265ddbbfedd25a8a0c38a33a6cb12761101a019f0c7f6189f07e771a522b7 Ciao, Johannes
Re: git difftool and git mergetool aren't returning errors when the tool has issues
On Fri, Aug 12, 2016 at 07:13:41AM -, Tom Tanner (BLOOMBERG/ LONDON) wrote: > For instance, if you set your diff/mergetool to meld and you don't have it > installed: > > git difftool > > Viewing (1/1): 'blah' > Launch 'meld' [Y/n]? y > /home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found > > echo $? > 0 > > > /home/ttanner/bin/meld > /home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found > > echo $? > 127 Have you looked at the --trust-exit-code option to git-difftool? It would be nice if there was a way to differentiate between complete failure and just the diff tool exiting with a non-zero return status because the files differ, but I'm not sure whether we can do that reliably. POSIX uses 127 and 126 as errors that mean the command didn't run [1] so it may be sensible to to treat those as special values. [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: A note from the maintainer
On Sat, Aug 13, 2016 at 09:04:32AM +, Eric Wong wrote:
> > Is there an easy way to get _just_ the list of message-ids you are
> > storing (I know I can download the whole archive, but it's big)?
>
> XHDR (or HDR) over NNTP should do it (that's how I checked
> against gmane):
> 8<-
> use Net::NNTP;
> my $nntp = Net::NNTP->new($ENV{NNTPSERVER} || 'news.public-inbox.org');
> my ($num, $first, $last) = $nntp->group('inbox.comp.version-control.git');
> my $batch = 1;
> my $i;
> for ($i = $first; $i < $last; $i += $batch) {
> my $j = $i + $batch - 1;
> $j = $last if $j > $last;
> my $num2mid = $nntp->xhdr('Message-ID', "$i-$j");
> for my $n ($i..$j) {
> defined(my $mid = $num2mid->{$n}) or next;
> print "$mid\n";
> }
> }
Thanks, that's perfect.
I collected the message-ids from my archive. Interestingly, I had a
dozen or so that did not have message-ids at all. I think most of them
are from patches that put the "From " line in the body, like this one:
http://public-inbox.org/git/20070311033833.gb10...@spearce.org/
and then they got corrupted on a round-trip through one of the bad mbox
formats (probably downloading from gmane, I'd guess; the export there
uses mbox, and I use maildir myself, so it probably got split badly
years ago). Anyway, public-inbox seems to get this case right, which is
good.
I had several hundred message ids that you didn't. About half of them
were spam or other junk. I weeded them out manually (mostly by picking
through the subjects, so possibly there's some error). The end result is
279 messages that I think are legitimate that you don't have.
I'll send them to you off-list, as the mbox is about 300K, which the
list will reject.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] difftool: always honor "command not found" exit code
At the moment difftool's "trust exit code" logic always suppresses the exit status of the diff utility we invoke. This is useful because we don't want to exit just because diff returned "1" because the files differ, but it's confusing if the shell returns an error because the selected diff utility is not found. POSIX specifies 127 as the exit status for "command not found" and 126 for "command found but is not executable" [1] and at least bash and dash follow this specification, while diff utilities generally use "1" for the exit status we want to ignore. Handle 126 and 127 as special values, assuming that they always mean that the command could not be executed. [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02 Signed-off-by: John Keeping --- On Sat, Aug 13, 2016 at 11:36:39AM +0100, John Keeping wrote: > It would be nice if there was a way to differentiate between complete > failure and just the diff tool exiting with a non-zero return status > because the files differ, but I'm not sure whether we can do that > reliably. POSIX uses 127 and 126 as errors that mean the command didn't > run [1] so it may be sensible to to treat those as special values. Something like this perhaps? I think this is probably safe, but it's always possible that some diff utility does use 126 or 127 as a "normal" exit status. I'm not sure what we can do about that other than add a "really, really don't trust the exit status" option! git-difftool--helper.sh | 18 ++ t/t7800-difftool.sh | 6 ++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index 84d6cc0..68877d4 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -86,11 +86,21 @@ else do launch_merge_tool "$1" "$2" "$5" status=$? - if test "$status" != 0 && - test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true - then + case "$status" in + 0) + : OK + ;; + 126|127) + # Command not found or not executable exit $status - fi + ;; + *) + if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true + then + exit $status + fi + ;; + esac shift 7 done fi diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 2974900..70a2de4 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' ' test_cmp expect actual ' +test_expect_success PERL 'difftool honors exit status if command not found' ' + test_config difftool.nonexistent.cmd i-dont-exist && + test_config difftool.trustExitCode false && + test_must_fail git difftool -y -t nonexistent branch +' + test_expect_success PERL 'difftool honors --gui' ' difftool_test_setup && test_config merge.tool bogus-tool && -- 2.9.2.639.g855ae9f -- To unsubscribe from this list: send the line "unsubscribe 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: introduce set_merge_remote_desc()
Am 13.08.2016 um 11:23 schrieb Jeff King:
On Sat, Aug 13, 2016 at 11:14:31AM +0200, René Scharfe wrote:
Add a helper function for allocating, populating and attaching struct
merge_remote_desc to a commit and use it consistently. It allocates the
necessary memory in a single block.
commit.c::get_merge_parent() forgot to check for memory allocation
failures of strdup(3).
merge-recursive.c::make_virtual_commit() didn't duplicate the string for
the name member, even though one of it's callers (indirectly through
get_ref()) may pass the result of oid_to_hex(), i.e. a static buffer.
It seems like you've buried the interesting part here. This isn't just
for cleanup, but a bugfix that the oids in our virtual commits might get
overwritten by subsequent actions.
It seems like that should be the subject and beginning of the commit
message. And then the fix is to allocate, and by the way we can do so
easily with this nice new helper. :)
Bugs are usually hidden, so why not hide fixes? ;-)
diff --git a/commit.c b/commit.c
index 71a360d..372b200 100644
--- a/commit.c
+++ b/commit.c
@@ -1576,6 +1576,15 @@ int commit_tree_extended(const char *msg, size_t msg_len,
return result;
}
+void set_merge_remote_desc(struct commit *commit,
+ const char *name, struct object *obj)
+{
+ struct merge_remote_desc *desc;
+ FLEXPTR_ALLOC_STR(desc, name, name);
+ desc->obj = obj;
+ commit->util = desc;
+}
I don't think there is any reason to prefer FLEXPTR_ALLOC over
FLEX_ALLOC, unless your struct interface is constrained by non-flex
users (that's why it is necessary for "struct exclude", for example,
which sometimes needs to carry its own string and sometimes not).
Using FLEX_ALLOC saves a few bytes per struct, and avoids an extra
pointer indirection when accessing the data.
Since it looks like you touch all of the allocations here, I think they
would both be happy as a regular flex array.
Good idea.
So let's turn this dish into a full menu:
commit: use xstrdup() in get_merge_parent()
commit: factor out set_merge_remote_desc()
merge-recursive: fix verbose output for multiple base trees
commit: use FLEX_ARRAY in struct merge_remote_desc
commit.c | 18 +++---
commit.h | 4 +++-
merge-recursive.c | 5 +
t/t3030-merge-recursive.sh | 18 ++
4 files changed, 33 insertions(+), 12 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] commit: use xstrdup() in get_merge_parent()
Handle allocation errors for the name member just like we already do for the struct merge_remote_desc itself. Signed-off-by: Rene Scharfe --- commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commit.c b/commit.c index 71a360d..ccd232a 100644 --- a/commit.c +++ b/commit.c @@ -1589,7 +1589,7 @@ struct commit *get_merge_parent(const char *name) struct merge_remote_desc *desc; desc = xmalloc(sizeof(*desc)); desc->obj = obj; - desc->name = strdup(name); + desc->name = xstrdup(name); commit->util = desc; } return commit; -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] commit: factor out set_merge_remote_desc()
Export a helper function for allocating, populating and attaching a
merge_remote_desc to a commit.
Signed-off-by: Rene Scharfe
---
commit.c | 19 ---
commit.h | 2 ++
2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/commit.c b/commit.c
index ccd232a..8bad713 100644
--- a/commit.c
+++ b/commit.c
@@ -1576,6 +1576,16 @@ int commit_tree_extended(const char *msg, size_t msg_len,
return result;
}
+void set_merge_remote_desc(struct commit *commit,
+ const char *name, struct object *obj)
+{
+ struct merge_remote_desc *desc;
+ desc = xmalloc(sizeof(*desc));
+ desc->obj = obj;
+ desc->name = xstrdup(name);
+ commit->util = desc;
+}
+
struct commit *get_merge_parent(const char *name)
{
struct object *obj;
@@ -1585,13 +1595,8 @@ struct commit *get_merge_parent(const char *name)
return NULL;
obj = parse_object(oid.hash);
commit = (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
- if (commit && !commit->util) {
- struct merge_remote_desc *desc;
- desc = xmalloc(sizeof(*desc));
- desc->obj = obj;
- desc->name = xstrdup(name);
- commit->util = desc;
- }
+ if (commit && !commit->util)
+ set_merge_remote_desc(commit, name, obj);
return commit;
}
diff --git a/commit.h b/commit.h
index 23ae0c1..84bb507 100644
--- a/commit.h
+++ b/commit.h
@@ -365,6 +365,8 @@ struct merge_remote_desc {
const char *name;
};
#define merge_remote_util(commit) ((struct merge_remote_desc
*)((commit)->util))
+extern void set_merge_remote_desc(struct commit *commit,
+ const char *name, struct object *obj);
/*
* Given "name" from the command line to merge, find the commit object
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] merge-recursive: fix verbose output for multiple base trees
One of the indirect callers of make_virtual_commit() passes the result of
oid_to_hex() as the name, i.e. a pointer to a static buffer. Since the
function uses that string pointer directly in building a struct
merge_remote_desc, multiple entries can end up sharing the same name
inadvertently.
Fix that by calling set_merge_remote_desc(), which creates a copy of the
string, instead of building the struct by hand.
Signed-off-by: Rene Scharfe
---
merge-recursive.c | 5 +
t/t3030-merge-recursive.sh | 18 ++
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index e5243c2..e349126 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -73,12 +73,9 @@ static struct tree *shift_tree_object(struct tree *one,
struct tree *two,
static struct commit *make_virtual_commit(struct tree *tree, const char
*comment)
{
struct commit *commit = alloc_commit_node();
- struct merge_remote_desc *desc = xmalloc(sizeof(*desc));
- desc->name = comment;
- desc->obj = (struct object *)commit;
+ set_merge_remote_desc(commit, comment, (struct object *)commit);
commit->tree = tree;
- commit->util = desc;
commit->object.parsed = 1;
return commit;
}
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index f7b0e59..470f334 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -660,4 +660,22 @@ test_expect_success 'merging with triple rename across D/F
conflict' '
git merge other
'
+test_expect_success 'merge-recursive remembers the names of all base trees' '
+ git reset --hard HEAD &&
+
+ # more trees than static slots used by oid_to_hex()
+ for commit in $c0 $c2 $c4 $c5 $c6 $c7
+ do
+ git rev-parse "$commit^{tree}"
+ done >trees &&
+
+ # ignore the return code -- it only fails because the input is weird
+ test_must_fail git -c merge.verbosity=5 merge-recursive $(cat trees) --
$c1 $c3 >out &&
+
+ # merge-recursive prints in reverse order, but we do not care
+ sort expect &&
+ sed -n "s/^virtual //p" out | sort >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/4] commit: use FLEX_ARRAY in struct merge_remote_desc
Convert the name member of struct merge_remote_desc to a FLEX_ARRAY and
use FLEX_ALLOC_STR to build the struct. This halves the number of
memory allocations, saves the storage for a pointer and avoids an
indirection when reading the name.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
commit.c | 3 +--
commit.h | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/commit.c b/commit.c
index 8bad713..ba6dee3 100644
--- a/commit.c
+++ b/commit.c
@@ -1580,9 +1580,8 @@ void set_merge_remote_desc(struct commit *commit,
const char *name, struct object *obj)
{
struct merge_remote_desc *desc;
- desc = xmalloc(sizeof(*desc));
+ FLEX_ALLOC_STR(desc, name, name);
desc->obj = obj;
- desc->name = xstrdup(name);
commit->util = desc;
}
diff --git a/commit.h b/commit.h
index 84bb507..32e1a11 100644
--- a/commit.h
+++ b/commit.h
@@ -362,7 +362,7 @@ extern void for_each_mergetag(each_mergetag_fn fn, struct
commit *commit, void *
struct merge_remote_desc {
struct object *obj; /* the named object, could be a tag */
- const char *name;
+ char name[FLEX_ARRAY];
};
#define merge_remote_util(commit) ((struct merge_remote_desc
*)((commit)->util))
extern void set_merge_remote_desc(struct commit *commit,
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: introduce set_merge_remote_desc()
On Sat, Aug 13, 2016 at 02:07:42PM +0200, René Scharfe wrote: > So let's turn this dish into a full menu: > > commit: use xstrdup() in get_merge_parent() > commit: factor out set_merge_remote_desc() > merge-recursive: fix verbose output for multiple base trees > commit: use FLEX_ARRAY in struct merge_remote_desc Very nice. Four patches seems like a lot for such a small fix, but each one is so trivial and easy to understand, I think it's worth it. They all look good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 13/13] bisect--helper: `bisect_start` shell function partially in C
Hey Christian,
On Sat, Aug 13, 2016 at 1:04 PM, Christian Couder
wrote:
> On Wed, Aug 10, 2016 at 11:57 PM, Pranit Bauva wrote:
>>
>> @@ -431,6 +434,244 @@ static int bisect_terms(struct bisect_terms *terms,
>> const char **argv, int argc)
>> return 0;
>> }
>>
>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> + const char **argv, int argc)
>> +{
>> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
>> + int flags, pathspec_pos;
>> + struct string_list revs = STRING_LIST_INIT_DUP;
>> + struct string_list states = STRING_LIST_INIT_DUP;
>> + struct strbuf start_head = STRBUF_INIT;
>> + struct strbuf bisect_names = STRBUF_INIT;
>> + struct strbuf orig_args = STRBUF_INIT;
>> + const char *head;
>> + unsigned char sha1[20];
>> + FILE *fp;
>> + struct object_id oid;
>> +
>> + if (is_bare_repository())
>> + no_checkout = 1;
>> +
>> + for (i = 0; i < argc; i++) {
>> + char *commit_id = xstrfmt("%s^{commit}", argv[i]);
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>> + break;
>> + }
>
> In the shell code there is a loop dedicated to checking if there is a
> double dash in the arguments before the real argument parsing loop.
> There is a reason for that.
> If you do it in the same loop, has_double_dash will not be set when
> the arguments that are before the double dash will be parsed.
I had tried that before. But somehow I couldn't get it to run so I
reverted back. I have now successfully tried it and its working. You
can checkout the PR[1].
>> + else if (!strcmp(argv[i], "--no-checkout"))
>> + no_checkout = 1;
>> + else if (!strcmp(argv[i], "--term-good") ||
>> +!strcmp(argv[i], "--term-old")) {
>> + must_write_terms = 1;
>> + strbuf_reset(&terms->term_good);
>> + strbuf_addstr(&terms->term_good, argv[++i]);
>> + }
>> + else if (skip_prefix(argv[i], "--term-good=", &argv[i])) {
>
> (Maybe you could put the "else if (...) {" on the same line as the "}" above.)
>
>> + must_write_terms = 1;
>> + strbuf_reset(&terms->term_good);
>> + strbuf_addstr(&terms->term_good, argv[i]);
>> + }
>> + else if (skip_prefix(argv[i], "--term-old=", &argv[i])) {
>> + must_write_terms = 1;
>> + strbuf_reset(&terms->term_good);
>> + strbuf_addstr(&terms->term_good, argv[i]);
>> + }
>> + else if (!strcmp(argv[i], "--term-bad") ||
>> +!strcmp(argv[i], "--term-new")) {
>> + must_write_terms = 1;
>> + strbuf_reset(&terms->term_bad);
>> + strbuf_addstr(&terms->term_bad, argv[++i]);
>> + }
>> + else if (skip_prefix(argv[i], "--term-bad=", &argv[i])) {
>> + must_write_terms = 1;
>> + strbuf_reset(&terms->term_bad);
>> + strbuf_addstr(&terms->term_bad, argv[i]);
>> + }
>> + else if (skip_prefix(argv[i], "--term-new=", &argv[i])) {
>> + must_write_terms = 1;
>> + strbuf_reset(&terms->term_good);
>> + strbuf_addstr(&terms->term_good, argv[i]);
>> + }
>> + else if (starts_with(argv[i], "--") &&
>> +!one_of(argv[i], "--term-good", "--term-bad",
>> NULL)) {
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + die(_("unrecognised option: '%s'"), argv[i]);
>> + }
>> + else if (get_oid(argv[i], &oid) && !has_double_dash) {
>
> And here checking "!has_double_dash" has no meaning if you check for a
> double dash in the same loop, because there is a "break" after
> has_double_dash is set above.
I think this is the situation in the shell script too.
>> + string_list_clear(&revs, 0);
>> + string_list_clear(&states, 0);
>> + free(commit_id);
>> + die(_("'%s' does not appear to be a valid
>> revision"), argv[i]);
>> + }
>> + else {
>> + free(commit_id);
>> + string_list_append(&revs, oid_to_hex(&oid));
>> + }
>> + }
>> + pathspec_pos = i;
>> +
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.ker
Re: Faith For Charity
Dearly Beloved, I am very glad to write you this mail, my name is Mrs. Maimouna Farooq, and I am from Omani ancestral origin living in Abidjan for many years with my family. I am a good merchant; I have companies and good shares in various banks. I spend all my life on investment and Real Estate business. All the way I lost my husband and two kids in a fatal train crash in 2014 Since I lost my family, I have been living a self-minding life; I don't know much and care about people. But last year 2015 around July, I was sent a letter of medical checkup as my personal doctor testified that I have lungs cancer which can easily take off my life soon. I found it uneasy to survive myself because a lot of investment cannot be run and manage by me again. So I quickly call up a spiritual adviser to give me positive thinking on this solution as my adviser. He minister to me to share my properties to the poor and needy, people that need money to survive both students that need money for their education and business people for their investment and for less privilege around the globe. My condition would not allow me handle disbursement and carry out other functions actively, if you sincere and honest with heart for helping me distribute money to orphanages homes in your country and other country's you wish to. I will give more information to you as I wait your response immediately. Best Regards, Mrs. Maimouna Farooq -- To unsubscribe from this list: send the line "unsubscribe 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: make option --help open man pages only for Git commands
"Philip Oakley" writes: > But does it cope with the Guides? Should it cope if spelt that way? > > git help revisions > git revisions --help Hmph. Ralf's patch is not just "I wonder if we could do a bit better" but is also a regression. I do not particularly care if the latter stops working, but the former definitely should, as "git help -g" encourages readers to type that, but with the change "git help " seems to stop working. -- To unsubscribe from this list: send the line "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] receive-pack: use FLEX_ALLOC_MEM in queue_command()
Use the macro FLEX_ALLOC_MEM instead of open-coding it. This shortens and simplifies the code a bit. Signed-off-by: Rene Scharfe --- builtin/receive-pack.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 92e1213..011db00 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1478,11 +1478,9 @@ static struct command **queue_command(struct command **tail, refname = line + 82; reflen = linelen - 82; - cmd = xcalloc(1, st_add3(sizeof(struct command), reflen, 1)); + FLEX_ALLOC_MEM(cmd, ref_name, refname, reflen); hashcpy(cmd->old_sha1, old_sha1); hashcpy(cmd->new_sha1, new_sha1); - memcpy(cmd->ref_name, refname, reflen); - cmd->ref_name[reflen] = '\0'; *tail = cmd; return &cmd->next; } -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git for Windows 2.9.3
Johannes Schindelin writes: > New Features > > • Comes with Git 2.9.3. For future reference, what time (in UTC) of the day is convenient for you to see an upstream tarball? > • Sports a new --smudge option for git cat-file that lets it pass > blob contents through smudge filters configured for the specified > path. Perhaps we want to upstream this, together with a new "--clean" option for git hash-object? And after writing all of the above, I noticed that hash-object by default uses the clean machinery and that can be turned off by giving the "--no-filters" option. The reason why the option is not called "--no-clean" is because it is not just about the clean filter but is about using the entirety of convert_to_git() filter chain. We probably should teach "hash-objects" to take "--filters" for consistency. And then your "git cat-file" patch can be upstreamed with the option renamed to (or with an additional synonym) "--filters", which would make things consistent. 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 8/8] diff: improve positioning of add/delete blocks in diffs
Jeff King writes: > So assuming everything I just said isn't complete bollocks, I think we > can move to a future where nobody uses the compaction heuristic. And > there are three ways to deal with that: > > 1. The knob and feature stay. It might be useful for somebody who > wants to experiment in the future. > > 2. The knob and feature go away completely. It was an experiment, but > now we have something more useful. > > 3. The feature goes away, but the knob stays as noop, or maybe as an > alias for the indent heuristic, just because we did ship a version > that accepts "--compaction-heuristic", and maybe somebody somewhere > put it in a script? > > I think I'd be in favor of (2). I am all for (2) [*1*] This and the previous "take a blank line as a hint" are both heuristics. As long as the resulting code does not tax runtime performance visibly and improves the resulting output 99% of the time, there is no reason to leave end-users a knob. "Among 9 hunks in this patch that touch hello.c, 7 are made much more readable but 2 are worse" cannot even be helped with a command line option. [Footnote] *1* I am also strongly against (3), if only to teach people a lesson ;-). -- To unsubscribe from this list: send the line "unsubscribe 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 v1 1/2] t0027: Correct raciness in NNO test
Am 12.08.2016 um 18:51 schrieb tbo...@web.de:
From: Torsten Bögershausen
When a non-reversible CRLF conversion is done in "git add",
a warning is printed on stderr (or Git dies, depending on checksafe)
The function commit_chk_wrnNNO() in t0027 was written to test this,
but did the wrong thing: Instead of looking at the warning
from "git add", it looked at the warning from "git commit".
This is racy because "git commit" may not have to do CRLF conversion
at all if it can use the sha1 value from the index (which depends on
whether "add" and "commit" run in a single second).
Correct this and replace the commit for each and every file with a commit
of all files in one go.
The new test code does not only fix the race condition, but also tests
different things, or prepares test cases in a different manner. I would
have appreciated an explanation why this is necessary. Is it "on my
machine, the race condition was triggered consistently for a bunch of
tests, and so I recorded the wrong expected output in the test cases"?
The function commit_chk_wrnNNO() will to be renamed in a separate commit.
Thanks to Jeff King for analyzing t0027.
Reported-By: Johannes Schindelin
---
t/t0027-auto-crlf.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index 2860d2d..ab6e962 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {
fname=${pfx}_$f.txt &&
cp $f $fname &&
printf Z >>"$fname" &&
- git -c core.autocrlf=$crlf add $fname 2>/dev/null &&
- git -c core.autocrlf=$crlf commit -m "commit_$fname" $fname
>"${pfx}_$f.err" 2>&1
+ git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
done
test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
@@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '
# attrLFCRLF CRLFmixLF
LF_mix_CR CRLFNUL
commit_chk_wrnNNO "" "" false """""" ""
""
-commit_chk_wrnNNO "" "" trueLF_CRLF """" ""
""
+commit_chk_wrnNNO "" "" true"""""" ""
""
commit_chk_wrnNNO "" "" input """""" ""
""
-commit_chk_wrnNNO "auto" "" false "$WILC" """" ""
""
-commit_chk_wrnNNO "auto" "" trueLF_CRLF """" ""
""
+commit_chk_wrnNNO "auto" "" false """""" ""
""
+commit_chk_wrnNNO "auto" "" true"""""" ""
""
commit_chk_wrnNNO "auto" "" input """""" ""
""
for crlf in true false input
do
@@ -408,7 +407,7 @@ do
commit_chk_wrnNNO ""lf $crlf "" CRLF_LFCRLF_LF
"" CRLF_LF
commit_chk_wrnNNO ""crlf$crlf LF_CRLF ""LF_CRLF LF_CRLF
""
commit_chk_wrnNNO auto lf $crlf """""" ""
""
- commit_chk_wrnNNO auto crlf$crlf LF_CRLF """" ""
""
+ commit_chk_wrnNNO auto crlf$crlf """""" ""
""
commit_chk_wrnNNO text lf $crlf "" CRLF_LFCRLF_LF
"" CRLF_LF
commit_chk_wrnNNO text crlf$crlf LF_CRLF ""LF_CRLF LF_CRLF
""
done
@@ -417,7 +416,8 @@ commit_chk_wrnNNO "text" "" false "$WILC" "$WICL"
"$WAMIX""$WILC
commit_chk_wrnNNO "text" "" trueLF_CRLF ""LF_CRLF LF_CRLF
""
commit_chk_wrnNNO "text" "" input ""CRLF_LF CRLF_LF ""
CRLF_LF
-test_expect_success 'create files cleanup' '
+test_expect_success 'commit NNO and cleanup' '
+ git commit -m "commit files on top of NNO" &&
rm -f *.txt &&
git -c core.autocrlf=false reset --hard
'
--
To unsubscribe from this list: send the line "unsubscribe 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] correct FLEXPTR_* example in comment
Am 13.08.2016 um 11:09 schrieb Jeff King: On Sat, Aug 13, 2016 at 11:01:21AM +0200, René Scharfe wrote: This section is about "The FLEXPTR_* variants", so use FLEXPTR_ALLOC_STR in the example. Oops, yeah. Your patch is clearly an improvement. Since this is obviously an easy mistake to make (using one form rather than the other), I wondered if the compiler would catch it. I think it would catch an accidental use of FLEXPTR instead of FLEX, because it involves an attempted assignment of an array. Gcc 5.4 reports "invalid use of flexible array member". But I don't think we would catch the reverse; we'd just write the data directly on top of the pointer. That would probably crash immediately at runtime, so if you exercise the code at all in tests, it is OK. But something to be aware of. No hint at compile time, segfaults at runtime. I suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or something, but I'm not sure if it is worth worrying about. You can't use sizeof with an actual flexible array. It only works if FLEX_ARRAY is defined as 1 (for platforms without native support), and perhaps also if it's 0. offsetof(struct x, arr) == sizeof(struct x) won't work either because of padding. I have no idea what you can do with a flexible array that would throw a compile error when done with a pointer. René -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
