Re: [PATCH 0/3] Add a function skip_prefix_if_present()
On 02/05/2014 07:25 AM, Michael Haggerty wrote: > These patches apply on top of gitster/nd/more-skip-prefix. > > I noticed that Duy's new function skip_prefix_defval() was mostly > being called with its first and third arguments the same. So > introduce a function skip_prefix_if_present() that implements this > pattern. I see I should have read the whole previous thread [1] before firing off this patch series. What I learned when I read it just now: * Johannes Sixt didn't think changes like the following improve readability: > - if (starts_with(arg, "--upload-pack=")) { > - args.uploadpack = arg + 14; > + if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) { > + args.uploadpack = optarg; * René Scharfe submitted a patch to use a function parse_prefix() (originally suggested by Peff) instead of Duy's suggested approach: http://article.gmane.org/gmane.comp.version-control.git/239569 His patch appears to have been overlooked. * Duy seemed to offer to rewrite his patch series, but I don't think that it has happened yet. And then the conversation was drowned by Christmas eggnog. I don't have a strong feeling about (Duy's proposal plus my patches) vs. (René's parse_prefix() approach). But I definitely *do* like the idea of getting rid of all those awkward magic numbers everywhere. Michael [1] http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239569 -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] Add and use a function skip_prefix_if_present()
Most of the calls to skip_prefix_defval() had equal first and third arguments, with the effect of skipping the prefix if present, but otherwise returning the original string. So define a new function that does exactly that. Signed-off-by: Michael Haggerty --- builtin/checkout.c| 4 ++-- builtin/fast-export.c | 2 +- builtin/merge.c | 2 +- builtin/show-branch.c | 6 +++--- git-compat-util.h | 5 + git.c | 2 +- notes.c | 4 ++-- refs.c| 2 +- wt-status.c | 4 ++-- 9 files changed, 18 insertions(+), 13 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 6531ed4..84682f1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1151,8 +1151,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix) const char *argv0 = argv[0]; if (!argc || !strcmp(argv0, "--")) die (_("--track needs a branch name")); - argv0 = skip_prefix_defval(argv0, "refs/", argv0); - argv0 = skip_prefix_defval(argv0, "remotes/", argv0); + argv0 = skip_prefix_if_present(argv0, "refs/"); + argv0 = skip_prefix_if_present(argv0, "remotes/"); argv0 = strchr(argv0, '/'); if (!argv0 || !argv0[1]) die (_("Missing branch name; try -b")); diff --git a/builtin/fast-export.c b/builtin/fast-export.c index cd0a302..c87c7ea 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -476,7 +476,7 @@ static void handle_tag(const char *name, struct tag *tag) } } - name = skip_prefix_defval(name, "refs/tags/", name); + name = skip_prefix_if_present(name, "refs/tags/"); printf("tag %s\nfrom :%d\n%.*s%sdata %d\n%.*s\n", name, tagged_mark, (int)(tagger_end - tagger), tagger, diff --git a/builtin/merge.c b/builtin/merge.c index 603f80a..7b01dcf 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1109,7 +1109,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) */ branch = branch_to_free = resolve_refdup("HEAD", head_sha1, 0, &flag); if (branch) - branch = skip_prefix_defval(branch, "refs/heads/", branch); + branch = skip_prefix_if_present(branch, "refs/heads/"); if (!branch || is_null_sha1(head_sha1)) head_commit = NULL; else diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 6078132..f2c3b19 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -284,7 +284,7 @@ static void show_one_commit(struct commit *commit, int no_name) pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty); pretty_str = pretty.buf; } - pretty_str = skip_prefix_defval(pretty_str, "[PATCH] ", pretty_str); + pretty_str = skip_prefix_if_present(pretty_str, "[PATCH] "); if (!no_name) { if (name && name->head_name) { @@ -478,7 +478,7 @@ static int rev_is_head(const char *head, int headlen, const char *name, if ((!head[0]) || (head_sha1 && sha1 && hashcmp(head_sha1, sha1))) return 0; - head = skip_prefix_defval(head, "refs/heads/", head); + head = skip_prefix_if_present(head, "refs/heads/"); if (starts_with(name, "refs/heads/")) name += 11; else if (starts_with(name, "heads/")) @@ -810,7 +810,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) has_head++; } if (!has_head) - append_one_rev(skip_prefix_defval(head, "refs/heads/", head)); + append_one_rev(skip_prefix_if_present(head, "refs/heads/")); } if (!ref_name_cnt) { diff --git a/git-compat-util.h b/git-compat-util.h index 59265af..cff946c 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -368,6 +368,11 @@ static inline const char *skip_prefix(const char *str, const char *prefix) return skip_prefix_defval(str, prefix, NULL); } +static inline const char *skip_prefix_if_present(const char *str, const char *prefix) +{ + return skip_prefix_defval(str, prefix, str); +} + static inline int starts_with(const char *str, const char *prefix) { return skip_prefix(str, prefix) != NULL; diff --git a/git.c b/git.c index 321ae81..f3357d8 100644 --- a/git.c +++ b/git.c @@ -579,7 +579,7 @@ int main(int argc, char **av) argc--; handle_options(&argv, &argc, NULL); if (argc > 0) { - argv[0] = skip_prefix_defval(argv[0], "--", argv[0]); + argv[0] = skip_prefix_if_present(argv[0], "--"); } else { /* The user didn't specify a command; give them help */ commit_pager_choice(); diff --git a/notes.c b/notes.c index 31f513b..15c49d8 100644 --- a/notes.c +++ b/
[PATCH 0/3] Add a function skip_prefix_if_present()
These patches apply on top of gitster/nd/more-skip-prefix. I noticed that Duy's new function skip_prefix_defval() was mostly being called with its first and third arguments the same. So introduce a function skip_prefix_if_present() that implements this pattern. Michael Haggerty (3): Add and use a function skip_prefix_if_present() diff_scoreopt_parse(): use skip_prefix_if_present() rev_is_head(): use skip_prefix() builtin/checkout.c| 4 ++-- builtin/fast-export.c | 2 +- builtin/merge.c | 2 +- builtin/show-branch.c | 15 --- diff.c| 6 +++--- git-compat-util.h | 5 + git.c | 2 +- notes.c | 4 ++-- refs.c| 2 +- wt-status.c | 4 ++-- 10 files changed, 26 insertions(+), 20 deletions(-) -- 1.8.5.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 2/3] diff_scoreopt_parse(): use skip_prefix_if_present()
Whenever skip_prefix_defval() was called, opt was still equal to *opt_p. So we can use skip_prefix_if_present() instead. Signed-off-by: Michael Haggerty --- diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/diff.c b/diff.c index d629cc5..a07fe9d 100644 --- a/diff.c +++ b/diff.c @@ -3890,13 +3890,13 @@ static int diff_scoreopt_parse(const char **opt_p) *opt_p = opt; if (cmd == '-') { /* convert the long-form arguments into short-form versions */ - if ((opt = skip_prefix_defval(opt, "break-rewrites", *opt_p)) != *opt_p) { + if ((opt = skip_prefix_if_present(opt, "break-rewrites")) != *opt_p) { if (*opt == 0 || *opt++ == '=') cmd = 'B'; - } else if ((opt = skip_prefix_defval(opt, "find-copies", *opt_p)) != *opt_p) { + } else if ((opt = skip_prefix_if_present(opt, "find-copies")) != *opt_p) { if (*opt == 0 || *opt++ == '=') cmd = 'C'; - } else if ((opt = skip_prefix_defval(opt, "find-renames", *opt_p)) != *opt_p) { + } else if ((opt = skip_prefix_if_present(opt, "find-renames")) != *opt_p) { if (*opt == 0 || *opt++ == '=') cmd = 'M'; } -- 1.8.5.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 3/3] rev_is_head(): use skip_prefix()
Change the logic a little bit so that we can use skip_prefix() instead of doing pointer arithmetic with hardcoded numbers. Signed-off-by: Michael Haggerty --- builtin/show-branch.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/show-branch.c b/builtin/show-branch.c index f2c3b19..aee7fe5 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -475,14 +475,15 @@ static void snarf_refs(int head, int remotes) static int rev_is_head(const char *head, int headlen, const char *name, unsigned char *head_sha1, unsigned char *sha1) { + const char *short_name; + if ((!head[0]) || (head_sha1 && sha1 && hashcmp(head_sha1, sha1))) return 0; head = skip_prefix_if_present(head, "refs/heads/"); - if (starts_with(name, "refs/heads/")) - name += 11; - else if (starts_with(name, "heads/")) - name += 6; + if ((short_name = skip_prefix(name, "refs/heads/")) || + (short_name = skip_prefix(name, "heads/"))) + return !strcmp(head, short_name); return !strcmp(head, name); } -- 1.8.5.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 v3] l10n: de.po: translate 28 new messages
Translate 28 new messages came from git.pot update in df49095 (l10n: git.pot: v1.9 round 1 (27 new, 11 removed) and d57b24b (l10n: git.pot: v1.9 round 2 (1 new)). Signed-off-by: Ralf Thielow --- v3 adds one new message from l10n rnd 2. po/de.po | 92 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/po/de.po b/po/de.po index 7f8aa75..432a2b5 100644 --- a/po/de.po +++ b/po/de.po @@ -442,9 +442,9 @@ msgstr[0] "vor %lu Jahr" msgstr[1] "vor %lu Jahren" #: diffcore-order.c:24 -#, fuzzy, c-format +#, c-format msgid "failed to read orderfile '%s'" -msgstr "Fehler beim Lesen des Objektes '%s'." +msgstr "Fehler beim Lesen der Reihenfolgedatei '%s'." #: diff.c:113 #, c-format @@ -975,21 +975,23 @@ msgid "" "There is nothing to exclude from by :(exclude) patterns.\n" "Perhaps you forgot to add either ':/' or '.' ?" msgstr "" +"Es gibt nichts, was durch :(exclude) Muster ausgeschlossen wird.\n" +"Vielleicht haben Sie vergessen entweder ':/' oder '.' hinzuzufügen?" #: remote.c:753 -#, fuzzy, c-format +#, c-format msgid "Cannot fetch both %s and %s to %s" -msgstr "Kann keine Commit-Beschreibung für %s bekommen" +msgstr "Kann 'fetch' nicht für %s und %s nach %s ausführen." #: remote.c:757 #, c-format msgid "%s usually tracks %s, not %s" -msgstr "" +msgstr "%s folgt üblicherweise %s, nicht %s" #: remote.c:761 #, c-format msgid "%s tracks both %s and %s" -msgstr "" +msgstr "%s folgt %s und %s" #. #. * This last possibility doesn't occur because @@ -997,9 +999,8 @@ msgstr "" #. * the end of the list. #. #: remote.c:769 -#, fuzzy msgid "Internal error" -msgstr "interner Fehler" +msgstr "Interner Fehler" #: remote.c:1871 #, c-format @@ -1575,33 +1576,28 @@ msgid "both modified:" msgstr "von beiden geändert:" #: wt-status.c:275 -#, fuzzy msgid "new file" -msgstr "neue Datei: %s" +msgstr "neue Datei" #: wt-status.c:277 msgid "copied" -msgstr "" +msgstr "kopiert" #: wt-status.c:279 -#, fuzzy msgid "deleted" msgstr "gelöscht" #: wt-status.c:285 -#, fuzzy msgid "typechange" -msgstr "Typänderung: %s" +msgstr "Typänderung" #: wt-status.c:287 -#, fuzzy msgid "unknown" -msgstr "unbekannt:%s" +msgstr "unbekannt" #: wt-status.c:289 -#, fuzzy msgid "unmerged" -msgstr "zusammenführen" +msgstr "nicht zusammengeführt" #: wt-status.c:336 msgid "new commits, " @@ -1633,6 +1629,8 @@ msgid "" "Do not touch the line above.\n" "Everything below will be removed." msgstr "" +"Ändern Sie nicht die Zeile oberhalb.\n" +"Alles unterhalb wird entfernt." #: wt-status.c:899 msgid "You have unmerged paths." @@ -3991,14 +3989,14 @@ msgid "reference repository '%s' is not a local repository." msgstr "Referenziertes Repository '%s' ist kein lokales Repository." #: builtin/clone.c:256 -#, fuzzy, c-format +#, c-format msgid "reference repository '%s' is shallow" -msgstr "Referenziertes Repository '%s' ist kein lokales Repository." +msgstr "Referenziertes Repository '%s' ist unvollständig (shallow)." #: builtin/clone.c:259 -#, fuzzy, c-format +#, c-format msgid "reference repository '%s' is grafted" -msgstr "referenziert Repository" +msgstr "Referenziertes Repository '%s' ist gesondert eingehängt (graft)." #: builtin/clone.c:321 #, c-format @@ -4099,16 +4097,16 @@ msgstr "" #: builtin/clone.c:805 msgid "source repository is shallow, ignoring --local" -msgstr "" +msgstr "Quelle ist ein unvollständiges Repository (shallow), ignoriere --local" #: builtin/clone.c:810 msgid "--local is ignored" msgstr "--local wird ignoriert" #: builtin/clone.c:814 builtin/fetch.c:1119 -#, fuzzy, c-format +#, c-format msgid "depth %s is not a positive number" -msgstr "Objekt %s ist kein Blob" +msgstr "Tiefe %s ist keine positive Zahl" #: builtin/clone.c:824 #, c-format @@ -5215,9 +5213,8 @@ msgid "default mode for recursion" msgstr "Standard-Modus für Rekursion" #: builtin/fetch.c:109 -#, fuzzy msgid "accept refs that update .git/shallow" -msgstr "Konnte aktualisierte .gitmodules-Datei nicht lesen" +msgstr "akzeptiert Referenzen die .git/shallow aktualisieren" #: builtin/fetch.c:347 msgid "Couldn't find remote ref HEAD" @@ -5287,7 +5284,8 @@ msgstr "%s hat nicht alle erforderlichen Objekte gesendet\n" #: builtin/fetch.c:579 #, c-format msgid "reject %s because shallow roots are not allowed to be updated" -msgstr "" +msgstr "%s wurde zurückgewiesen, da Ursprungs-Commits von unvollständigen\n" +"Repositories (shallow) nicht aktualisiert werden dürfen." #: builtin/fetch.c:667 builtin/fetch.c:750 #, c-format @@ -7182,9 +7180,8 @@ msgid "git merge-base --is-ancestor " msgstr "git merge-base --is-ancestor " #: builtin/merge-base.c:33 -#, fuzzy msgid "git merge-base --fork-point []" -msgstr "git merge-base --is-ancestor " +msgstr "git merge-base --fork-point []" #: builtin/merge-base.c:214 msgid "output all common ancestors" @@ -7204,7 +7201,7 @@ msgstr "ist der Erste
Re: [PATCH 2/3] setup_pager: set MORE=R
On Feb 4, 2014, at 14:12, Jeff King wrote: On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: does complicate the point of my series, which was to add more intimate logic about how we handle LESS. ... return !x || strchr(x, 'R'); [...] I am not sure if it is even a good idea for us to have so intimate logic for various pagers in the first place. I'd seriously wonder if it is better to take this position: A platform packager who sets the default pager and/or the default environment for the pager at the build time, or an individual user who tells your Git what pager you want to use and/or with what setting that pager should be run under with environment variables. These people ought to know far better than Git what their specific choices do. Do not try to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. [snip] It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that "R" is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) I think we can do better without adding intimate pager knowledge. And at the same time make it a simple matter of tweaking the Makefile to deal with new pagers on specific systems. There are two parts to the proposal. Part 1 Add a new configuration item which I will call "pager.env" for now that can have multiple values of the form ENV=value (whether multiple lines or whitespace separated on a single line to be decided later). On a system where more can support color by setting MORE=-R it might look like this: [pager] env = LESS=-FRSX LV=-c MORE=-R On a system where more does not it might look like this: [pager] env = LESS=-FRSX LV=-c The default value of pager.env is to be configured in a system- specific way (i.e. Makefile knob) at build time. Then, if Git is going to output color AND start a pager (that logic remains unchanged by this proposal), then it processes the pager.env value by examining each ENV=value setting and if the environment variable ENV is NOT already set, then sets it to value before starting the pager. This is mostly a clean up and shouldn't really be controversial since before commit e54c1f2d2 the system basically behaved like this where pager.env was always set to "LESS=FRSX" and after it behaves as though pager.env is always set to "LESS=FRSX LV=-c". Part 2 Alongside the current false/never, always, true/auto settings for "color.ui" a new "carefully" setting is introduced and the color.ui default if not set is changed from auto (commit 4c7f1819) to the new "carefully" setting. Why a new setting? So that people who have explicitly set color.ui to auto (or one of the other values) will not be affected by the proposed new logic. Another new configuration item which I will call "pager.carefully" for now is introduced that again can have multiple values of the form name=ENV. It might look like this: [pager] carefully = less=LESS LV=lv more=MORE Again the default value of pager.carefully can be configured in a system-specific way (i.e. Makefile knob) at build time. If color.ui is set to "carefully", then the logic is as follows: a) Decide whether to output color the same way as for color.ui=auto b) Select the pager the same way as for color.ui=auto c) If (a) and (b) have selected a pager AND enabled color output then check to see if the selected pager appears in pager.carefully as one of the name values (perhaps a suffix match on the first whitespace- separated word of the selected pager value). d) If the selected pager does not appear in pager.carefully, disable color output. e) If the selected pager appears in pager.carefully, but the ENV associated with it does NOT appear in pager.env, disable color output. f) If the pager appears in pager.carefully, but the ENV associated with it is already set, disable color output. g) Otherwise, go ahead with color output as the change in Part 1 above will properly set the pager's options to enable it. This has the following advantages: 1. No intimate pager knowledge. 2. Color output will be selected on supported
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
On Tue, Feb 04, 2014 at 05:24:55PM -0800, Yuri wrote: > I think the 'experimental' approach is simpler and better. > When the git command requiring pager is first run, git would run the > pager with some simple one line escape sequence, and see if sequence > is preserved. See how? If less's stdout is not connected to a terminal, it simply passes through the output as-is. E.g., try: foo() { # blue foo printf '\x1b[34mfoo\x1b[m' } unset LESS foo | less which has the problematic output. Now try: foo | less | cat which passes through the ANSI codes unscathed. I have no idea how other pagers behave. So I think this is going back down the road of hard-coding lots of pager-specific behaviors. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 'git log' escape symbols shown as ESC[33 and ESC[m
On 01/16/2014 17:47, Jeff King wrote: Are you using "less" as your pager (it is the default in git unless you have set your PAGER environment variable)? If so, do you have the "R" option set to pass through ANSI codes? Git will set this automatically in your "LESS" variable if you do not already have such a variable (but it will not touch it if you already have it set, and are missing "R"). I think the 'experimental' approach is simpler and better. When the git command requiring pager is first run, git would run the pager with some simple one line escape sequence, and see if sequence is preserved. If it was preserved, git should just run with escape sequences. If the pager destroyed the sequence, git should issue a warning to the user: git: your default pager PAGER=more doesn't pass escape sequences, and they will be disabled them. You can revert this decision by changing the file ~/.git/pager.conf This way: * damaged sequences will not show up by default * colors will be displayed by default when possible * user would be informed, and will have a clear choice * this is easy to implement, and no elaborate and obscure reasoning should be employed in the implementation For me, if git would have told me that my pager doesn't support escape sequences, I could have taken it from there. Yuri -- To unsubscribe from this list: send the line "unsubscribe 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.c: rename and unlink pack file if it exists
On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote: > * Somehow this came to my private mailbox without Cc to list, so I >am forwarding it. > >I think with 1190a1ac (pack-objects: name pack files after >trailer hash, 2013-12-05), repacking the same set of objects may >have less chance of producing colliding names, especially if you >are on a box with more than one core, but it still would be a >good idea to get this part right in the upcoming release. Actually, since 1190a1ac, if you have repacked and gotten the same pack name, then you do not have to do any rename dance at all; you can throw away what you just generated because you know that it is byte-for-byte identical. You could collide with a pack created by an older version of git that used the original scheme, but that is quite unlikely (on the order of 2^-160). -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 2/2] reset: support "--mixed --intent-to-add" mode
On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote: > Junio C Hamano writes: > > > While I do not have any problem with adding an optional "keep lost > > paths as intent-to-add entries" feature, I am not sure why this has > > to be so different from the usual add-cache-entry codepath. The > > if/elseif chain you are touching inside this loop does: > > > > - If the tree you are resetting to has something at the path > >(which is different from the current index, obviously), create > >a cache entry to represent that state from the tree and stuff > >it in the index; > > > > - Otherwise, the tree you are resetting to does not have that > >path. We used to say "remove it from the index", but now we have > >an option to instead add it as an intent-to-add entry. > > > > So, why doesn't the new codepath do exactly the same thing as the > > first branch of the if/else chain and call add_cache_entry but with > > a ce marked with CE_INTENT_TO_ADD? That would parallel what happens > > in "git add -N" better, I would think, no? > > In other words, something along this line, perhaps? Yes. But you need something like this on top to actually set CE_INTENT_TO_ADD -- 8< -- diff --git a/read-cache.c b/read-cache.c index 325d193..87f1367 100644 --- a/read-cache.c +++ b/read-cache.c @@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce) if (write_sha1_file("", 0, blob_type, sha1)) die("cannot create an empty blob in the object database"); hashcpy(ce->sha1, sha1); + ce->ce_flags |= CE_INTENT_TO_ADD; } int add_to_index(struct index_state *istate, const char *path, struct stat *st, int flags) -- 8< -- -- To unsubscribe from this list: send the line "unsubscribe 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.c: rename and unlink pack file if it exists
Junio C Hamano writes: > This comment in builtin/repack.c: > ... Oops; there was supposed to be these lines before anythning else: From: Torsten Bögershausen Date: Sun Feb 2 16:09:56 2014 +0100 > First see if there are packs of the same name and if so > if we can move them out of the way (this can happen if we > repacked immediately after packing fully). > > When a repo was fully repacked, and is repacked again, we may run > into the situation that "new" packfiles have the same name as > already existing ones (traditionally packfiles have been named after > the list of names of objects in them, so repacking all the objects > in a single pack would have produced a packfile with the same name). > > The logic is to rename the existing ones into filename like > "old-XXX", create the new ones and then remove the "old-" ones. > When something went wrong in the middle, this sequence is rolled > back by renaming the "old-" files back. > > The renaming into "old-" did not work as designed, because > file_exists() was done on the wrong file name. This could give > problems for a repo on a network share under Windows, as reported by > Jochen Haag . > > Formulate the filenames objects/pack/pack-.{pack,idx} correctly > (the code originally lacked "pack-" prefix when checking if the file > exists). > > Also rename the variables to match what they are used for: > fname for the final name of the new packfile, fname_old for the name > of the existing one, and fname_tmp for the temporary name pack-objects > created the new packfile in. > > Signed-off-by: Torsten Bögershausen > Signed-off-by: Junio C Hamano > --- > > * Somehow this came to my private mailbox without Cc to list, so I >am forwarding it. > >I think with 1190a1ac (pack-objects: name pack files after >trailer hash, 2013-12-05), repacking the same set of objects may >have less chance of producing colliding names, especially if you >are on a box with more than one core, but it still would be a >good idea to get this part right in the upcoming release. > >The bug in the first hunk will cause us not to find any existing >file, even when there is a name clash. And then we try to >immediately the final rename without any provision for rolling >back. The other two hunks are pure noise renaming variables. > >I think the patch looks correct, but I'd appreciate a second set >of eyeballs. > >Thanks. > > builtin/repack.c | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index bca7710..3b01353 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char > *prefix) > for_each_string_list_item(item, &names) { > for (ext = 0; ext < 2; ext++) { > char *fname, *fname_old; > - fname = mkpathdup("%s/%s%s", packdir, > + fname = mkpathdup("%s/pack-%s%s", packdir, > item->string, exts[ext]); > if (!file_exists(fname)) { > free(fname); > @@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char > *prefix) > /* Now the ones with the same name are out of the way... */ > for_each_string_list_item(item, &names) { > for (ext = 0; ext < 2; ext++) { > - char *fname, *fname_old; > + char *fname, *fname_tmp; > struct stat statbuffer; > fname = mkpathdup("%s/pack-%s%s", > packdir, item->string, exts[ext]); > - fname_old = mkpathdup("%s-%s%s", > + fname_tmp = mkpathdup("%s-%s%s", > packtmp, item->string, exts[ext]); > - if (!stat(fname_old, &statbuffer)) { > + if (!stat(fname_tmp, &statbuffer)) { > statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | > S_IWOTH); > - chmod(fname_old, statbuffer.st_mode); > + chmod(fname_tmp, statbuffer.st_mode); > } > - if (rename(fname_old, fname)) > - die_errno(_("renaming '%s' failed"), fname_old); > + if (rename(fname_tmp, fname)) > + die_errno(_("renaming '%s' into '%s' failed"), > fname_tmp, fname); > free(fname); > - free(fname_old); > + free(fname_tmp); > } > } > > /* Remove the "old-" files */ > for_each_string_list_item(item, &names) { > for (ext = 0; ext < 2; ext++) { > - char *fname; > - fname = mkpath
[PATCH] repack.c: rename and unlink pack file if it exists
This comment in builtin/repack.c: First see if there are packs of the same name and if so if we can move them out of the way (this can happen if we repacked immediately after packing fully). When a repo was fully repacked, and is repacked again, we may run into the situation that "new" packfiles have the same name as already existing ones (traditionally packfiles have been named after the list of names of objects in them, so repacking all the objects in a single pack would have produced a packfile with the same name). The logic is to rename the existing ones into filename like "old-XXX", create the new ones and then remove the "old-" ones. When something went wrong in the middle, this sequence is rolled back by renaming the "old-" files back. The renaming into "old-" did not work as designed, because file_exists() was done on the wrong file name. This could give problems for a repo on a network share under Windows, as reported by Jochen Haag . Formulate the filenames objects/pack/pack-.{pack,idx} correctly (the code originally lacked "pack-" prefix when checking if the file exists). Also rename the variables to match what they are used for: fname for the final name of the new packfile, fname_old for the name of the existing one, and fname_tmp for the temporary name pack-objects created the new packfile in. Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- * Somehow this came to my private mailbox without Cc to list, so I am forwarding it. I think with 1190a1ac (pack-objects: name pack files after trailer hash, 2013-12-05), repacking the same set of objects may have less chance of producing colliding names, especially if you are on a box with more than one core, but it still would be a good idea to get this part right in the upcoming release. The bug in the first hunk will cause us not to find any existing file, even when there is a name clash. And then we try to immediately the final rename without any provision for rolling back. The other two hunks are pure noise renaming variables. I think the patch looks correct, but I'd appreciate a second set of eyeballs. Thanks. builtin/repack.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index bca7710..3b01353 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) for_each_string_list_item(item, &names) { for (ext = 0; ext < 2; ext++) { char *fname, *fname_old; - fname = mkpathdup("%s/%s%s", packdir, + fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]); if (!file_exists(fname)) { free(fname); @@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char *prefix) /* Now the ones with the same name are out of the way... */ for_each_string_list_item(item, &names) { for (ext = 0; ext < 2; ext++) { - char *fname, *fname_old; + char *fname, *fname_tmp; struct stat statbuffer; fname = mkpathdup("%s/pack-%s%s", packdir, item->string, exts[ext]); - fname_old = mkpathdup("%s-%s%s", + fname_tmp = mkpathdup("%s-%s%s", packtmp, item->string, exts[ext]); - if (!stat(fname_old, &statbuffer)) { + if (!stat(fname_tmp, &statbuffer)) { statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH); - chmod(fname_old, statbuffer.st_mode); + chmod(fname_tmp, statbuffer.st_mode); } - if (rename(fname_old, fname)) - die_errno(_("renaming '%s' failed"), fname_old); + if (rename(fname_tmp, fname)) + die_errno(_("renaming '%s' into '%s' failed"), fname_tmp, fname); free(fname); - free(fname_old); + free(fname_tmp); } } /* Remove the "old-" files */ for_each_string_list_item(item, &names) { for (ext = 0; ext < 2; ext++) { - char *fname; - fname = mkpath("%s/old-pack-%s%s", + char *fname_old; + fname_old = mkpath("%s/old-%s%s", packdir, item->string, exts[ext]); - i
Re: [PATCH 0/9] remerge diff proof of concept/RFC
Thomas Rast writes: > log --remerge-diff: show what the conflict resolution changed Yay ;-) > This series explores another angle, which I call "remerge diff". It > works by re-doing the merge in core, using features from patches 1-3 > and 7. Likely that will result in conflicts, which are formatted in > the usual <<< way. Then it diffs this "remerge" against the > merge's tree that is recorded in history. Yup, that is the obvious way to recreate what would have been shown to the person who recorded the merge result. I like that approach. > So I would welcome comments, and/or better ideas, on the following > proposed resolution: > > * Implement what I described last, to take care of delete/modify > conflicts. Naively, I would have thought that - If the recorded result of the merge does not have the path, then show the deletion of the contents relative to the side that modified it. - If the recorded result of the merge has the path, then show the change between the side that modified it and the recorded result. might be more useful. Then we will know what we lost in full (if the recorded result is to "delete" it), but it is very likely that we won't see anything if the recorded result blindly took what the modifying side left. Comparing with the synthetic stages 2+3 will at least show us there was something funky going on, so your approach would be reasonable in this case. > * Punt on more complex conflicts, by removing those files from the > index, and emitting a warning about those files instead. Sensible. -- To unsubscribe from this list: send the line "unsubscribe 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] setup_pager: set MORE=R
On 02/04/2014 14:48, Jeff King wrote: But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. Ok, git can test the pager on the given system before its first use and remember the result in ~/.git for later use for example. Such 'experimental' approach is maybe more reliable. Yuri -- To unsubscribe from this list: send the line "unsubscribe 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] setup_pager: set MORE=R
Jeff King writes: > The safest thing would be to turn off auto-color when LESS (or any of > the pager environment variables) is set at all (and not worry about > whether "R" is set; only know that _we_ are not setting it, so we cannot > count on it). But that would potentially inconvenience a lot of people > whose default color would suddenly go away. That is just as safe as disabling color for everybody, isn't it? Half of existing users have LESS with R, and the other half do not have LESS at all. The former will be harmed, the latter will not see any difference. Oh, and then new users who do not know R for LESS will not even notice that Git could support coloured output. Those among them who read manpages and find --color option will then see ESC[33m in their output and we are back to where we started X-<. So I think we are already at the safest place. Those who see ESC[33m will know they are missing some good stuff and can ask around, which is better than doing anything else at this point. I think that is the same conclusion as yours, "there is nothing to be done." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[Bug] branch.*.merge interpreted too strictly by tracking logic
Start from an empty repository like so: : gitster track; git init Initialized empty Git repository in /var/tmp/x/track/.git/ : gitster track/master; git commit --allow-empty -m initial [master (root-commit) abdcd1c] initial : gitster track/master; git branch foo : gitster track/master; git branch bar : gitster track/master; git commit --allow-empty -m second [master 78e28f4] second Now, 'master' has two commits, while 'foo' and 'bar' both are one commit behind, pointing at 'master^1'. Let's tell these branches that they are both supposed to be building on top of 'master'. : gitster track/master; git config branch.foo.remote . : gitster track/master; git config branch.foo.merge refs/heads/master : gitster track/master; git config branch.bar.remote . : gitster track/master; git config branch.bar.merge master The difference between the two is that 'foo' spells the @{upstream} branch in full (which is the recommended practice), while 'bar' is loose and asks for 'master'. Let's see what happens on these two branches. First 'foo': : gitster track/master; git checkout foo Switched to branch 'foo' Your branch is behind 'master' by 1 commit, and can be fast-forwarded. (use "git pull" to update your local branch) : gitster track/foo; git pull From . * branchmaster -> FETCH_HEAD Updating abdcd1c..78e28f4 Fast-forward The 'checkout' correctly reports that 'foo' is building on (local) 'master'. 'pull' works as expected, of course. Now, here is the bug part. The same thing on 'bar': : gitster track/foo; git checkout bar Switched to branch 'bar' Your branch is based on 'master', but the upstream is gone. (use "git branch --unset-upstream" to fixup) It knows about 'master', but what is "the upstream is gone"? It is our local repository and it definitely is not gone. But 'pull' of course works as expected (this behaviour is older and stable for a long time since even before @{upstream} was invented). : gitster track/bar; git pull From . * branchmaster -> FETCH_HEAD Updating abdcd1c..78e28f4 Fast-forward I suspect that the real culprit is somewhere in wt-status.c : gitster track/bar; git status On branch bar Your branch is based on 'master', but the upstream is gone. (use "git branch --unset-upstream" to fixup) nothing to commit, working directory clean Resolving @{upstream} works just fine for both. : gitster track/bar; git rev-parse --symbolic-full-name foo@{upstream} refs/heads/master : gitster track/bar; git rev-parse --symbolic-full-name bar@{upstream} refs/heads/master 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/3] setup_pager: set MORE=R
On Tue, Feb 04, 2014 at 02:45:11PM -0800, Yuri wrote: > On 02/04/2014 14:25, Jeff King wrote: > >Right. If git just disabled the color, I think that would be sane (and > >that is what my patch was shooting for). But somebody who sees: > > > > $ git log > > ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m > > (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m > > > >does not necessarily know what is going on. They do not know that it is > >a "less" problem, nor that their less settings are relevant. They only > >see that Git is broken out of the box. > > Maybe, instead of doing all the elaborate guess and assumption work, > have configure script check if the current PAGER supports colors and > build git accordingly? > configure could run the pager as one of its tests, and determine if > "ESC" appears on the output. But this is not a build-time problem, but rather a run-time problem. We do not know what the environment of the user will be at run-time. The safest thing would be to turn off auto-color when LESS (or any of the pager environment variables) is set at all (and not worry about whether "R" is set; only know that _we_ are not setting it, so we cannot count on it). But that would potentially inconvenience a lot of people whose default color would suddenly go away. -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] blame.c: prepare_lines should not call xrealloc for every line
From: "Philip Oakley" From: "David Kastrup" To: "Junio C Hamano" Cc: Sent: Tuesday, February 04, 2014 9:09 PM Subject: Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line [...] Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex. A bit of googling found http://marc.info/?l=gnulib-bug&m=130108029329021 which suggests that behaviour can't be relied upon, and that perhaps some code is 'buggy' relative to expectations (hence the patch it proposed). It suggests that one can't properly reference a zero length object. I think I've confused myself between the patch https://lkml.org/lkml/2010/11/24/429 which fixed a bug in memchr() and the discussion in http://lists.gnu.org/archive/html/bug-gnulib/2011-03/msg00273.html (alternate link to marc.info) that discusses realloc() which has " we changed gnulib's test-memchr to instead test memchr(zerosize_ptr(),a,0) rather than memchr(NULL,a,0). However, once you can handle memchr(,0) on any other zero-size object, most implementations also happen to do what you want for memchr(NULL,a,0), even though it is technically undefined behavior." ending with 'technically undefined behavior' which I misconstrued. Apologies for the noise. I do not remember if the rest of the logic actually depends on it (I think I use lineno[n+1] - lineno[n] to find the end of line, ... -- David Kastrup -- Philip -- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On 02/04/2014 14:25, Jeff King wrote: Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a "less" problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Maybe, instead of doing all the elaborate guess and assumption work, have configure script check if the current PAGER supports colors and build git accordingly? configure could run the pager as one of its tests, and determine if "ESC" appears on the output. Yuri -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
From: "David Kastrup" To: "Junio C Hamano" Cc: Sent: Tuesday, February 04, 2014 9:09 PM Subject: Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line Junio C Hamano writes: Junio C Hamano writes: David Kastrup writes: Whitespace error in line 1778. Should I be reposting? Heh, let me try to clean it up first and then repost for your review. Thanks. -- >8 -- From: David Kastrup Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) < sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup --- One logic difference from what was posted is that sb->lineno[num] is filled with the length of the entire buffer when the file ends with a complete line. Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex. A bit of googling found http://marc.info/?l=gnulib-bug&m=130108029329021 which suggests that behaviour can't be relied upon, and that perhaps some code is 'buggy' relative to expectations (hence the patch it proposed). It suggests that one can't properly reference a zero length object. I do not remember if the rest of the logic actually depends on it (I think I use lineno[n+1] - lineno[n] to find the end of line, Well, you do it about _half_ the time. The other half, you scan for the '\n' explicitly. The original code dates back to 2006 when the author of the code was not paid for doing anything for Git but was doing it as a weekend and evening hobby, so it may not be so surprising to find this kind of "what was I thinking when I wrote it" inefficiency in such a code with $0 monetary value ;-) Oh, _this_ patch is not in the "I want to make money from it" range. If that were the case, I should not have bothered at all. This is just the "this code offends my sense of aesthetics" class. It's purely optional to apply. It's conceivable that it will make a performance difference on non-glibc (or what it's called) platforms. -- David Kastrup -- Philip -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] setup_pager: set MORE=R
On Tue, Feb 04, 2014 at 02:17:57PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > But there's another set of people that I was intending to help with the > > patch, which is people that have set up LESS, and did not necessarily > > care about the "R" flag in the past (e.g., for many years before git > > came along, I set LESS=giM, and never even knew that "R" existed). Since > > git comes out of the box these days with color and the pager turned on, > > that means people with such a setup see broken output from day one. > > > > And I think it is Git's fault here, not the user or the packager. > > I am not particularly itnterested in whose fault it is ;-) If the > user sets LESS himself, he knows how to set it (and if he is setting > it automatically for all of his sessions, he knows where to do so), > and would know better than Git about "less", his pager of choice. > > If he did not know about R and did not see color, that is even > better. Now he knows and his update to his LESS settings will let > him view colors in outputs from programs that are not Git. Right. If git just disabled the color, I think that would be sane (and that is what my patch was shooting for). But somebody who sees: $ git log ESC[33mcommit 3c6b385c655a52fd9db176ce1e01469dc9954f91ESC[mESC[33m (ESC[1;36mHEADESC[mESC[33m, ESC[1;32mjk/meta-makeESC[mESC[33m)ESC[m does not necessarily know what is going on. They do not know that it is a "less" problem, nor that their less settings are relevant. They only see that Git is broken out of the box. Anyway, I will leave it at that. It's an unfortunate problem, but one not worth fixing. -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 2/2] reset: support "--mixed --intent-to-add" mode
Junio C Hamano writes: > While I do not have any problem with adding an optional "keep lost > paths as intent-to-add entries" feature, I am not sure why this has > to be so different from the usual add-cache-entry codepath. The > if/elseif chain you are touching inside this loop does: > > - If the tree you are resetting to has something at the path >(which is different from the current index, obviously), create >a cache entry to represent that state from the tree and stuff >it in the index; > > - Otherwise, the tree you are resetting to does not have that >path. We used to say "remove it from the index", but now we have >an option to instead add it as an intent-to-add entry. > > So, why doesn't the new codepath do exactly the same thing as the > first branch of the if/else chain and call add_cache_entry but with > a ce marked with CE_INTENT_TO_ADD? That would parallel what happens > in "git add -N" better, I would think, no? In other words, something along this line, perhaps? -- >8 -- From: Nguyễn Thái Ngọc Duy Date: Tue, 4 Feb 2014 09:20:09 +0700 When --mixed is used, entries could be removed from index if the target ref does not have them. When "reset" is used in preparation for commit spliting (in a dirty worktree), it could be hard to track what files to be added back. The new option --intent-to-add simplifies it by marking all removed files intent-to-add. Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git-reset.txt | 5 - builtin/reset.c | 38 ++ cache.h | 1 + read-cache.c| 4 ++-- t/t7102-reset.sh| 9 + 5 files changed, 42 insertions(+), 15 deletions(-) diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt index f445cb3..a077ba0 100644 --- a/Documentation/git-reset.txt +++ b/Documentation/git-reset.txt @@ -10,7 +10,7 @@ SYNOPSIS [verse] 'git reset' [-q] [] [--] ... 'git reset' (--patch | -p) [] [--] [...] -'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [] +'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [] DESCRIPTION --- @@ -60,6 +60,9 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode. Resets the index but not the working tree (i.e., the changed files are preserved but not marked for commit) and reports what has not been updated. This is the default action. ++ +If `-N` is specified, removed paths are marked as intent-to-add (see +linkgit:git-add[1]). --hard:: Resets the index and working tree. Any changes to tracked files in the diff --git a/builtin/reset.c b/builtin/reset.c index 6004803..d363bc5 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -116,25 +116,32 @@ static void update_index_from_diff(struct diff_queue_struct *q, struct diff_options *opt, void *data) { int i; + int intent_to_add = *(int *)data; for (i = 0; i < q->nr; i++) { struct diff_filespec *one = q->queue[i]->one; - if (one->mode && !is_null_sha1(one->sha1)) { - struct cache_entry *ce; - ce = make_cache_entry(one->mode, one->sha1, one->path, - 0, 0); - if (!ce) - die(_("make_cache_entry failed for path '%s'"), - one->path); - add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | - ADD_CACHE_OK_TO_REPLACE); - } else + int is_missing = !(one->mode && !is_null_sha1(one->sha1)); + struct cache_entry *ce; + + if (is_missing && !intent_to_add) { remove_file_from_cache(one->path); + continue; + } + + ce = make_cache_entry(one->mode, one->sha1, one->path, + 0, 0); + if (!ce) + die(_("make_cache_entry failed for path '%s'"), + one->path); + if (is_missing) + mark_intent_to_add(ce); + add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE); } } static int read_from_tree(const struct pathspec *pathspec, - unsigned char *tree_sha1) + unsigned char *tree_sha1, + int intent_to_add) { struct diff_options opt; @@ -142,6 +149,7 @@ static int read_from_tree(const struct pathspec *pathspec, copy_pathspec(&opt.pathspec, pathspec); opt.output_format = DIFF_FORMAT_CALLBACK; opt.format_callback = update_index_from_diff; + opt.format_callback_data = &intent_to_add; if (do_diff_cache(tree_sha1, &opt)) return 1; @@ -258,6 +266,7 @@ int cmd_reset(int argc, const char **ar
[RFC PATCH 9/9] log --remerge-diff: show what the conflict resolution changed
Git has --cc as a very fast inspection tool that shows a brief summary of what a conflicted merge "looks like", and -c/-m as "give me the full information" data dumps. But --cc actually loses information: if the merge lost(!) some changes from one side, that hunk would fully agree with the other side, and therefore be elided. So --cc cannot be used to investigate mismerges. Indeed it is rather hard to find a merge that has lost changes, unless one knows where to look. The new option --remerge-diff is an attempt at filling this gap, admittedly at the cost of a lot of CPU cycles. For each merge commit, it diffs the merge result against a recursive merge of the merge's parents. For files that can be auto-merged cleanly, it will typically show nothing. However, it will make it obvious when the merge introduces extra changes. For files that result in merge conflicts, we diff against the representation with conflict hunks (what the user would usually see in the worktree). So the diff will show what was changed in the conflict hunks to resolve the conflict. It still takes a bit of staring to tell an evil from a regular merge. But at least the information is there, unlike with --cc; and the output is usually much shorter than with -c. Signed-off-by: Thomas Rast --- Documentation/rev-list-options.txt | 7 ++ log-tree.c | 60 +++ merge-recursive.c | 3 +- merge-recursive.h | 1 + revision.c | 2 + revision.h | 4 +- t/t4202-log.sh | 3 + t/t4213-log-remerge-diff.sh| 145 + 8 files changed, 223 insertions(+), 2 deletions(-) create mode 100755 t/t4213-log-remerge-diff.sh diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index d023290..6611557 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -797,6 +797,13 @@ options may be given. See linkgit:git-diff-files[1] for more options. in that case, the output represents the changes the merge brought _into_ the then-current branch. +--remerge-diff:: + Diff merge commits against a recursive merge of their parents, + with conflict hunks. Intuitively speaking, this shows what + the author of the merge changed to resolve the merge. It + assumes that all (or most) merges are recursive merges; other + strategies are not supported. + -r:: Show recursive diffs. diff --git a/log-tree.c b/log-tree.c index 4ab3ffe..fa22737 100644 --- a/log-tree.c +++ b/log-tree.c @@ -11,6 +11,8 @@ #include "gpg-interface.h" #include "sequencer.h" #include "line-log.h" +#include "cache-tree.h" +#include "merge-recursive.h" struct decoration name_decoration = { "object names" }; @@ -725,6 +727,62 @@ static int do_diff_combined(struct rev_info *opt, struct commit *commit) return !opt->loginfo; } +static int do_diff_remerge(struct rev_info *opt, struct commit *commit) +{ + struct commit_list *merge_bases; + struct commit *result, *parent1, *parent2; + struct merge_options o; + char *branch1, *branch2; + + if (commit->parents->next->next) { + printf("--remerge-diff not supported for octopus merges.\n"); + return 0; + } + + parent1 = commit->parents->item; + parent2 = commit->parents->next->item; + parse_commit(parent1); + parse_commit(parent2); + branch1 = xstrdup(sha1_to_hex(parent1->object.sha1)); + branch2 = xstrdup(sha1_to_hex(parent2->object.sha1)); + + merge_bases = get_octopus_merge_bases(commit->parents); + init_merge_options(&o); + o.verbosity = -1; + o.no_worktree = 1; + o.conflicts_in_index = 1; + o.use_ondisk_index = 0; + o.branch1 = branch1; + o.branch2 = branch2; + merge_recursive(&o, parent1, parent2, merge_bases, &result); + free(branch1); + free(branch2); + + active_cache_tree = cache_tree(); + if (cache_tree_update(active_cache_tree, + (const struct cache_entry * const *)active_cache, + active_nr, WRITE_TREE_SILENT) < 0) { + printf("BUG: merge conflicts not fully folded, cannot diff.\n"); + return 0; + } + + if (opt->loginfo && !opt->no_commit_id) { + show_log(opt); + + if (opt->verbose_header && opt->diffopt.output_format) + printf("%s%c", diff_line_prefix(&opt->diffopt), + opt->diffopt.line_termination); + } + + diff_tree_sha1(active_cache_tree->sha1, commit->tree->object.sha1, + "", &opt->diffopt); + log_tree_diff_flush(opt); + + cache_tree_free(&active_cache_tree); + + return !opt->loginfo; +} + /* * Show the diff of a commit
[PATCH 3/9] merge-recursive: -Xindex-only to leave worktree unchanged
From: Thomas Rast Using the new no_worktree flag from the previous commit, we can teach merge-recursive to leave the worktree untouched. Expose this with a new strategy option so that scripts can use it. Signed-off-by: Junio C Hamano --- Documentation/merge-strategies.txt | 4 merge-recursive.c | 2 ++ t/t3030-merge-recursive.sh | 13 + 3 files changed, 19 insertions(+) diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt index fb6e593..2934e99 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -92,6 +92,10 @@ subtree[=];; is prefixed (or stripped from the beginning) to make the shape of two trees to match. +index-only;; + Write the merge result only to the index; do not touch the + worktree. + octopus:: This resolves cases with more than two heads, but refuses to do a complex merge that needs manual resolution. It is diff --git a/merge-recursive.c b/merge-recursive.c index 35be144..f59c1d3 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -2096,6 +2096,8 @@ int parse_merge_opt(struct merge_options *o, const char *s) if ((o->rename_score = parse_rename_score(&score)) == -1 || *score != 0) return -1; } + else if (!strcmp(s, "index-only")) + o->no_worktree = 1; else return -1; return 0; diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index 2f96100..2f3a16c 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -296,6 +296,19 @@ test_expect_success 'merge-recursive result' ' ' +test_expect_success 'merge-recursive --index-only' ' + + rm -fr [abcd] && + git checkout -f "$c2" && + test_expect_code 1 git merge-recursive --index-only "$c0" -- "$c2" "$c1" && + git ls-files -s >actual && + # reuses "expected" from previous test! + test_cmp expected actual && + git diff HEAD >actual-diff && + : >expected-diff && + test_cmp expected-diff actual-diff +' + test_expect_success 'fail if the index has unresolved entries' ' rm -fr [abcd] && -- 1.9.rc2.232.gdd31389 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] merge-recursive: internal flag to avoid touching the worktree
From: Thomas Rast o->call_depth has a double function: a nonzero call_depth means we want to construct virtual merge bases, but it also means we want to avoid touching the worktree. Introduce a new flag o->no_worktree to trigger only the latter. Signed-off-by: Thomas Rast Signed-off-by: Junio C Hamano --- merge-recursive.c | 37 + merge-recursive.h | 1 + 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index c36dc79..35be144 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -408,10 +408,10 @@ static void record_df_conflict_files(struct merge_options *o, int i; /* -* If we're merging merge-bases, we don't want to bother with -* any working directory changes. +* If we're working in-core only (e.g., merging merge-bases), +* we don't want to bother with any working directory changes. */ - if (o->call_depth) + if (o->call_depth || o->no_worktree) return; /* Ensure D/F conflicts are adjacent in the entries list. */ @@ -724,7 +724,7 @@ static void update_file_flags(struct merge_options *o, int update_cache, int update_wd) { - if (o->call_depth) + if (o->call_depth || o->no_worktree) update_wd = 0; if (update_wd) { @@ -931,7 +931,8 @@ static struct merge_file_info merge_file_1(struct merge_options *o, result.clean = merge_submodule(result.sha, one->path, one->sha1, a->sha1, b->sha1, - !o->call_depth); + !(o->call_depth || +o->no_worktree)); } else if (S_ISLNK(a->mode)) { hashcpy(result.sha, a->sha1); @@ -1003,7 +1004,7 @@ static void handle_change_delete(struct merge_options *o, const char *change, const char *change_past) { char *renamed = NULL; - if (dir_in_way(path, !o->call_depth)) { + if (dir_in_way(path, !(o->call_depth || o->no_worktree))) { renamed = unique_path(o, path, a_sha ? o->branch1 : o->branch2); } @@ -1128,10 +1129,10 @@ static void handle_file(struct merge_options *o, char *add_name = unique_path(o, rename->path, other_branch); update_file(o, 0, add->sha1, add->mode, add_name); - remove_file(o, 0, rename->path, 0); + remove_file(o, 0, rename->path, o->call_depth || o->no_worktree); dst_name = unique_path(o, rename->path, cur_branch); } else { - if (dir_in_way(rename->path, !o->call_depth)) { + if (dir_in_way(rename->path, !(o->call_depth || o->no_worktree))) { dst_name = unique_path(o, rename->path, cur_branch); output(o, 1, _("%s is a directory in %s adding as %s instead"), rename->path, other_branch, dst_name); @@ -1238,7 +1239,7 @@ static void conflict_rename_rename_2to1(struct merge_options *o, * merge base just undo the renames; they can be detected * again later for the non-recursive merge. */ - remove_file(o, 0, path, 0); + remove_file(o, 0, path, o->call_depth || o->no_worktree); update_file(o, 0, mfi_c1.sha, mfi_c1.mode, a->path); update_file(o, 0, mfi_c2.sha, mfi_c2.mode, b->path); } else { @@ -1246,7 +1247,7 @@ static void conflict_rename_rename_2to1(struct merge_options *o, char *new_path2 = unique_path(o, path, ci->branch2); output(o, 1, _("Renaming %s to %s and %s to %s instead"), a->path, new_path1, b->path, new_path2); - remove_file(o, 0, path, 0); + remove_file(o, 0, path, o->call_depth || o->no_worktree); update_file(o, 0, mfi_c1.sha, mfi_c1.mode, new_path1); update_file(o, 0, mfi_c2.sha, mfi_c2.mode, new_path2); free(new_path2); @@ -1405,6 +1406,7 @@ static int process_renames(struct merge_options *o, * add-source case). */ remove_file(o, 1, ren1_src, + o->call_depth || o->no_worktree || renamed_stage == 2 || !was_tracked(ren1_src)); hashcpy(src_other.sha1, ren1->src_entry->stages[other_stage].sha); @@ -1601,7 +1603,7 @@ static int merge_content(struct merge_options *o, o->branch2 == rename_conflict_info->branch1) ?
[PATCH 7/9] Fold all merge diff variants into an enum
The four ways of displaying merge diffs, * none: no diff * -m: against each parent * -c: combined * --cc: combined-condensed were encoded in three flag bits in struct rev_info. Fold them all into a single enum field that captures the variants. This makes it easier to add new merge diff variants without yet more special casing. It should also be slightly easier to read because one does not have to ensure that the flag bits are set in an expected combination. Signed-off-by: Thomas Rast --- builtin/diff-files.c| 5 +++-- builtin/diff-tree.c | 2 +- builtin/diff.c | 9 + builtin/fmt-merge-msg.c | 2 +- builtin/log.c | 9 - builtin/merge.c | 1 - combine-diff.c | 2 +- diff-lib.c | 7 --- log-tree.c | 4 ++-- revision.c | 13 +++-- revision.h | 22 +++--- submodule.c | 4 +--- 12 files changed, 44 insertions(+), 36 deletions(-) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 9200069..172b50d 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -57,9 +57,10 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) * was not asked to. "diff-files -c -p" should not densify * (the user should ask with "diff-files --cc" explicitly). */ - if (rev.max_count == -1 && !rev.combine_merges && + if (rev.max_count == -1 && + !merge_diff_mode_is_any_combined(&rev) && (rev.diffopt.output_format & DIFF_FORMAT_PATCH)) - rev.combine_merges = rev.dense_combined_merges = 1; + rev.merge_diff_mode = MERGE_DIFF_COMBINED_CONDENSED; if (read_cache_preload(&rev.diffopt.pathspec) < 0) { perror("read_cache_preload"); diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index be6417d..2950f80 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -96,7 +96,7 @@ static int diff_tree_stdin(char *line) static void diff_tree_tweak_rev(struct rev_info *rev, struct setup_revision_opt *opt) { if (!rev->diffopt.output_format) { - if (rev->dense_combined_merges) + if (rev->merge_diff_mode == MERGE_DIFF_COMBINED_CONDENSED) rev->diffopt.output_format = DIFF_FORMAT_PATCH; else rev->diffopt.output_format = DIFF_FORMAT_RAW; diff --git a/builtin/diff.c b/builtin/diff.c index 47f663b..fd4c75f 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -192,8 +192,8 @@ static int builtin_diff_combined(struct rev_info *revs, if (argc > 1) usage(builtin_diff_usage); - if (!revs->dense_combined_merges && !revs->combine_merges) - revs->dense_combined_merges = revs->combine_merges = 1; + if (!merge_diff_mode_is_any_combined(revs)) + revs->merge_diff_mode = MERGE_DIFF_COMBINED_CONDENSED; for (i = 1; i < ents; i++) sha1_array_append(&parents, ent[i].item->sha1); diff_tree_combined(ent[0].item->sha1, &parents, revs); @@ -242,9 +242,10 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv * dense one, --cc can be explicitly asked for, or just rely * on the default). */ - if (revs->max_count == -1 && !revs->combine_merges && + if (revs->max_count == -1 && + !merge_diff_mode_is_any_combined(revs) && (revs->diffopt.output_format & DIFF_FORMAT_PATCH)) - revs->combine_merges = revs->dense_combined_merges = 1; + revs->merge_diff_mode = MERGE_DIFF_COMBINED_CONDENSED; setup_work_tree(); if (read_cache_preload(&revs->diffopt.pathspec) < 0) { diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3906eda..2deeacd 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -637,7 +637,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out, head = lookup_commit_or_die(head_sha1, "HEAD"); init_revisions(&rev, NULL); rev.commit_format = CMIT_FMT_ONELINE; - rev.ignore_merges = 1; + rev.merge_diff_mode = MERGE_DIFF_IGNORE; rev.limited = 1; strbuf_complete_line(out); diff --git a/builtin/log.c b/builtin/log.c index b97373d..cebebea 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -499,13 +499,12 @@ static int show_tree_object(const unsigned char *sha1, static void show_rev_tweak_rev(struct rev_info *rev, struct setup_revision_opt *opt) { - if (rev->ignore_merges) { + if (!rev->merge_diff_mode) { /* There was no "-m" on the command line */ - rev->ignore_merges = 0; - if (!rev->first_parent_only && !rev->combine_merges) { + rev->merge_diff_mode = MERGE_DIFF_EACH; + if (!rev->first_parent_only) {
[POC PATCH 5/9] log: add a merge base inspection option
With the new --bases, print merge commits' parents' merge bases. This is mostly a proof of viability, in particular wrt. revision walk decoupling and speed. We can do "inline" get_merge_bases() (via get_octopus_merge_bases) because the walks in get_merge_bases() only use flag bits 16-19, and we reset them after use. The get_revision()/log display walk OTOH uses only flag bits 0-15 (actually only 0-10 as of this commit). Speed-wise it turns out to be better than attempting to compute merge bases in one go, mostly because the latter approach would require extensive data structures to track flags. This commit does not have to: the commit graph will be loaded anyway, and the room for flags is already there. As a big plus, this approach also works in a streaming fashion, showing the first few commits very quickly. Signed-off-by: Thomas Rast --- As indicated in the cover letter, this is cute, but I'm not married to it. It's probably just a useless instance of feature creep. Documentation/rev-list-options.txt | 7 +++ log-tree.c | 3 +++ pretty.c | 3 +++ revision.c | 2 ++ revision.h | 2 ++ t/t4202-log.sh | 31 +++ 6 files changed, 48 insertions(+) diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 03533af..d023290 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -705,6 +705,13 @@ endif::git-rev-list[] Print also the children of the commit (in the form "commit child..."). Also enables parent rewriting, see 'History Simplification' below. +ifndef::git-rev-list[] +--bases:: + For merge commits, print the merge bases of the commit's + parents. (These are the bases that were used in the creation + of the merge itself.) +endif::git-rev-list[] + ifdef::git-rev-list[] --timestamp:: Print the raw commit timestamp. diff --git a/log-tree.c b/log-tree.c index 08970bf..080f412 100644 --- a/log-tree.c +++ b/log-tree.c @@ -622,6 +622,8 @@ void show_log(struct rev_info *opt) ctx.output_encoding = get_log_output_encoding(); if (opt->from_ident.mail_begin && opt->from_ident.name_begin) ctx.from_ident = &opt->from_ident; + if (opt->show_merge_bases && commit->parents && commit->parents->next) + ctx.merge_bases = get_octopus_merge_bases(commit->parents); pretty_print_commit(&ctx, commit, &msgbuf); if (opt->add_signoff) @@ -662,6 +664,7 @@ void show_log(struct rev_info *opt) strbuf_release(&msgbuf); free(ctx.notes_message); + free(ctx.merge_bases); } int log_tree_diff_flush(struct rev_info *opt) diff --git a/pretty.c b/pretty.c index 5e44cf8..8b28664 100644 --- a/pretty.c +++ b/pretty.c @@ -552,6 +552,9 @@ static void add_merge_info(const struct pretty_print_context *pp, return; pp_commit_list(pp, sb, "Merge:", parent); + + if (pp->merge_bases) + pp_commit_list(pp, sb, "Bases:", pp->merge_bases); } static char *get_header(const struct commit *commit, const char *msg, diff --git a/revision.c b/revision.c index a0df72f..72255fb 100644 --- a/revision.c +++ b/revision.c @@ -1729,6 +1729,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg } else if (!strcmp(arg, "--parents")) { revs->rewrite_parents = 1; revs->print_parents = 1; + } else if (!strcmp(arg, "--merge-bases")) { + revs->show_merge_bases = 1; } else if (!strcmp(arg, "--dense")) { revs->dense = 1; } else if (!strcmp(arg, "--sparse")) { diff --git a/revision.h b/revision.h index 88967d6..3111228 100644 --- a/revision.h +++ b/revision.h @@ -19,6 +19,7 @@ #define PATCHSAME (1u<<9) #define BOTTOM (1u<<10) #define ALL_REV_FLAGS ((1u<<11)-1) +/* merge-base.c uses bits 16-19. --merge-bases will break if they overlap! */ #define DECORATE_SHORT_REFS1 #define DECORATE_FULL_REFS 2 @@ -137,6 +138,7 @@ struct rev_info { preserve_subject:1; unsigned intdisable_stdin:1; unsigned intleak_pending:1; + unsigned intshow_merge_bases:1; enum date_mode date_mode; diff --git a/t/t4202-log.sh b/t/t4202-log.sh index cb03d28..64f34a6 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -841,4 +841,35 @@ test_expect_success 'dotdot is a parent directory' ' test_cmp expect actual ' +shorten () { + for arg; do + git rev-parse --short "$arg" + done +} + +fill_in_merge_bases () { + while IFS= read line; do + case "$line" in + Merge:*) + printf "%s\n" "$line" + printf "%s" "Bases:" + printf " %s" $(shorten \ +
[PATCH 8/9] merge-recursive: allow storing conflict hunks in index
Add a --conflicts-in-index option to merge-recursive, which instructs it to always store the 3-way merged result in the index. (Normally it only does so in recursive invocations, but not for the final result.) This serves as a building block for the "remerge diff" feature coming up in a subsequent patch. The external option lets us easily use it from tests, where we'd otherwise need a new test-* helper to access the feature. Furthermore, it might occasionally be useful for scripts that want to look at the result of invoking git-merge without tampering with the worktree. They could already get the _conflicts_ with --index-only, but not (conveniently) the conflict-hunk formatted files that would normally be written to the worktree. Signed-off-by: Thomas Rast --- Documentation/merge-strategies.txt | 5 + merge-recursive.c | 4 merge-recursive.h | 1 + t/t3030-merge-recursive.sh | 20 4 files changed, 30 insertions(+) diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt index 2934e99..3468d99 100644 --- a/Documentation/merge-strategies.txt +++ b/Documentation/merge-strategies.txt @@ -96,6 +96,11 @@ index-only;; Write the merge result only to the index; do not touch the worktree. +conflicts-in-index;; + For conflicted files, write 3-way merged contents with + conflict hunks to the index, instead of leaving their entries + unresolved. + octopus:: This resolves cases with more than two heads, but refuses to do a complex merge that needs manual resolution. It is diff --git a/merge-recursive.c b/merge-recursive.c index f59c1d3..b682812 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -724,6 +724,8 @@ static void update_file_flags(struct merge_options *o, int update_cache, int update_wd) { + if (o->conflicts_in_index) + update_cache = 1; if (o->call_depth || o->no_worktree) update_wd = 0; @@ -2098,6 +2100,8 @@ int parse_merge_opt(struct merge_options *o, const char *s) } else if (!strcmp(s, "index-only")) o->no_worktree = 1; + else if (!strcmp(s, "conflicts-in-index")) + o->conflicts_in_index = 1; else return -1; return 0; diff --git a/merge-recursive.h b/merge-recursive.h index d8dd7a1..9b8e20b 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -16,6 +16,7 @@ struct merge_options { unsigned buffer_output : 1; unsigned renormalize : 1; unsigned no_worktree : 1; /* do not touch worktree */ + unsigned conflicts_in_index : 1; /* index will contain conflict hunks */ long xdl_opts; int verbosity; int diff_rename_limit; diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index 2f3a16c..4192fd3 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -309,6 +309,26 @@ test_expect_success 'merge-recursive --index-only' ' test_cmp expected-diff actual-diff ' +test_expect_success 'merge-recursive --index-only --conflicts-in-index' ' + # first pass: do a merge as usual to obtain "expected" + rm -fr [abcd] && + git checkout -f "$c2" && + test_expect_code 1 git merge-recursive "$c0" -- "$c2" "$c1" && + git add [abcd] && + git ls-files -s >expected && + # second pass: actual test + rm -fr [abcd] && + git checkout -f "$c2" && + test_expect_code 1 \ + git merge-recursive --index-only --conflicts-in-index \ + "$c0" -- "$c2" "$c1" && + git ls-files -s >actual && + test_cmp expected actual && + git diff HEAD >actual-diff && + : >expected-diff && + test_cmp expected-diff actual-diff +' + test_expect_success 'fail if the index has unresolved entries' ' rm -fr [abcd] && -- 1.9.rc2.232.gdd31389 -- To unsubscribe from this list: send the line "unsubscribe 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] setup_pager: set MORE=R
Jeff King writes: > But there's another set of people that I was intending to help with the > patch, which is people that have set up LESS, and did not necessarily > care about the "R" flag in the past (e.g., for many years before git > came along, I set LESS=giM, and never even knew that "R" existed). Since > git comes out of the box these days with color and the pager turned on, > that means people with such a setup see broken output from day one. > > And I think it is Git's fault here, not the user or the packager. I am not particularly itnterested in whose fault it is ;-) If the user sets LESS himself, he knows how to set it (and if he is setting it automatically for all of his sessions, he knows where to do so), and would know better than Git about "less", his pager of choice. If he did not know about R and did not see color, that is even better. Now he knows and his update to his LESS settings will let him view colors in outputs from programs that are not Git. > So I think there is nothing to be done. But I did want to mention it in > case somebody else can come up with some clever solution. :) Sure. -- To unsubscribe from this list: send the line "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 0/9] remerge diff proof of concept/RFC
Hi, This may look intimidating, but it's actually 3.5 separate things: merge-recursive: remove dead conditional in update_stages() merge-recursive: internal flag to avoid touching the worktree merge-recursive: -Xindex-only to leave worktree unchanged These are unchanged from tr/merge-recursive-index-only. I'm just resending them here because it's been a while, the rest depends on it, and it makes it more obvious how patch 8 fits in. pretty: refactor add_merge_info() into parts log: add a merge base inspection option This seemed a good idea when I wrote it, but the more I think about it, the less useful it appears to me. I left it here as a demonstration that computing the merge bases as we go is feasible. combine-diff: do not pass revs->dense_combined_merges redundantly Fold all merge diff variants into an enum Some initial refactoring. merge-recursive: allow storing conflict hunks in index log --remerge-diff: show what the conflict resolution changed The real meat. I'll only describe the last patches here. Context: last summer (yes, it's been a while) I investigated some angles of looking at merges that make it easier to investigate mismerges. The problem space is: it is exceedingly hard to find instances of git merge -s ours foo git merge -Xours foo manually resolve some hunks in favor of one side after some time has passed. The only ways I found with existing machinery involve too much manual inspection. At the time I made some scripts that look at the problem in various ways: https://github.com/trast/evilmergediff This series explores another angle, which I call "remerge diff". It works by re-doing the merge in core, using features from patches 1-3 and 7. Likely that will result in conflicts, which are formatted in the usual <<< way. Then it diffs this "remerge" against the merge's tree that is recorded in history. This tends to give nice results in practice; I pasted a particularly pretty example at the bottom. However, this is RFC because nontrivial merges can easily lead to states that I cannot handle with the current -- pretty neat and simple, I think -- implementation. For example, a delete/modify conflict does not result in a nice conflict-hunk formatted file, it just leaves 2 (out of 3) stages in the index. I can then not write a tree from that. This could be fixed by adding a pass that turns a missing stage 2 or 3 into an empty file at that stage, so that the resulting file looks like <<< === ... stage 3 content goes here ... >>> (analogously for stage 3 missing). But that still doesn't handle at least directory/file conflicts. So I would welcome comments, and/or better ideas, on the following proposed resolution: * Implement what I described last, to take care of delete/modify conflicts. * Punt on more complex conflicts, by removing those files from the index, and emitting a warning about those files instead. Note that even without delete/modify handling, this covers the vast majority of merges: among some 8090 merges I sampled from git.git, there are only 13 that trigger the "BUG: ..." message, and 34 octopuses. Example run: $ git show --remerge-diff a39b15b4f6a3f08b67b17d968935d177821e680f [...] diff --git a/builtin/ls-files.c b/builtin/ls-files.c index 0f4473e..175e6e3 100644 --- a/builtin/ls-files.c +++ b/builtin/ls-files.c @@ -490,16 +490,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) OPT_BOOLEAN('u', "unmerged", &show_unmerged, N_("show unmerged files in the output")), OPT_BOOLEAN(0, "resolve-undo", &show_resolve_undo, -<<< f12e49ae877ad0644b9b9939b7cb742da98691d2 N_("show resolve-undo information")), - { OPTION_CALLBACK, 'x', "exclude", &dir.exclude_list[EXC_CMDL], N_("pattern"), + { OPTION_CALLBACK, 'x', "exclude", &exclude_list, N_("pattern"), N_("skip files matching pattern"), -=== - "show resolve-undo information"), - { OPTION_CALLBACK, 'x', "exclude", - &exclude_list, "pattern", - "skip files matching pattern", ->>> 72aeb18772deeb386da7dd8997b969877bd29e41 0, option_parse_exclude }, { OPTION_CALLBACK, 'X', "exclude-from", &dir, N_("file"), N_("exclude patterns are read from "), diff --git a/dir.c b/dir.c index 8c58ce4..cf1e6b0 100644 --- a/dir.c +++ b/dir.c @@ -1674,14 +1674,14 @@ void free_pathspec(struct pathspec *pathspec) pathspec->items = NULL; } -<<< f12e49ae877ad0644b9b9939b7cb742da98691d2 int limit_pathspec_to_literal(void) { static int flag = -1; if (flag < 0) flag = git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT, 0); return flag; -=== +} + /* * Free
[POC PATCH 4/9] pretty: refactor add_merge_info() into parts
pp_commit_list() will be reused later. Signed-off-by: Thomas Rast --- Necessary only for the next patch, which may be of dubious value. commit.h | 1 + pretty.c | 40 ++-- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/commit.h b/commit.h index 16d9c43..b51817b 100644 --- a/commit.h +++ b/commit.h @@ -101,6 +101,7 @@ struct pretty_print_context { struct string_list *mailmap; int color; struct ident_split *from_ident; + struct commit_list *merge_bases; /* * Fields below here are manipulated internally by pp_* functions and diff --git a/pretty.c b/pretty.c index 87db08b..5e44cf8 100644 --- a/pretty.c +++ b/pretty.c @@ -517,6 +517,31 @@ static const char *skip_empty_lines(const char *msg) return msg; } +static const char *pp_sha1_to_hex(const struct pretty_print_context *pp, + const unsigned char *sha1) +{ + const char *hex = NULL; + if (pp->abbrev) + hex = find_unique_abbrev(sha1, pp->abbrev); + if (!hex) + hex = sha1_to_hex(sha1); + return hex; +} + +static void pp_commit_list(const struct pretty_print_context *pp, + struct strbuf *sb, + const char *prefix, + const struct commit_list *list) +{ + strbuf_addstr(sb, prefix); + while (list) { + struct commit *commit = list->item; + strbuf_addf(sb, " %s", pp_sha1_to_hex(pp, commit->object.sha1)); + list = list->next; + } + strbuf_addch(sb, '\n'); +} + static void add_merge_info(const struct pretty_print_context *pp, struct strbuf *sb, const struct commit *commit) { @@ -526,20 +551,7 @@ static void add_merge_info(const struct pretty_print_context *pp, !parent || !parent->next) return; - strbuf_addstr(sb, "Merge:"); - - while (parent) { - struct commit *p = parent->item; - const char *hex = NULL; - if (pp->abbrev) - hex = find_unique_abbrev(p->object.sha1, pp->abbrev); - if (!hex) - hex = sha1_to_hex(p->object.sha1); - parent = parent->next; - - strbuf_addf(sb, " %s", hex); - } - strbuf_addch(sb, '\n'); + pp_commit_list(pp, sb, "Merge:", parent); } static char *get_header(const struct commit *commit, const char *msg, -- 1.9.rc2.232.gdd31389 -- To unsubscribe from this list: send the line "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 1/9] merge-recursive: remove dead conditional in update_stages()
From: Thomas Rast 650467c (merge-recursive: Consolidate different update_stages functions, 2011-08-11) changed the former argument 'clear' to always be true. Remove the useless conditional. Signed-off-by: Thomas Rast Signed-off-by: Junio C Hamano --- merge-recursive.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 8400a8e..c36dc79 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -545,11 +545,9 @@ static int update_stages(const char *path, const struct diff_filespec *o, * would_lose_untracked). Instead, reverse the order of the calls * (executing update_file first and then update_stages). */ - int clear = 1; int options = ADD_CACHE_OK_TO_ADD | ADD_CACHE_SKIP_DFCHECK; - if (clear) - if (remove_file_from_cache(path)) - return -1; + if (remove_file_from_cache(path)) + return -1; if (o) if (add_cacheinfo(o->mode, o->sha1, path, 1, 0, options)) return -1; -- 1.9.rc2.232.gdd31389 -- To unsubscribe from this list: send the line "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 6/9] combine-diff: do not pass revs->dense_combined_merges redundantly
The existing code passed revs->dense_combined_merges along revs itself into the combine-diff functions, which is rather redundant. Remove the 'dense' argument until much further down the callchain to simplify callers. Note that while the caller in submodule.c needs to do extra work now, the next commit will simplify this to a single setting again. Signed-off-by: Thomas Rast --- builtin/diff.c | 3 +-- combine-diff.c | 13 ++--- diff-lib.c | 6 ++ diff.h | 6 +++--- log-tree.c | 2 +- submodule.c| 5 - 6 files changed, 17 insertions(+), 18 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index 0f247d2..47f663b 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -196,8 +196,7 @@ static int builtin_diff_combined(struct rev_info *revs, revs->dense_combined_merges = revs->combine_merges = 1; for (i = 1; i < ents; i++) sha1_array_append(&parents, ent[i].item->sha1); - diff_tree_combined(ent[0].item->sha1, &parents, - revs->dense_combined_merges, revs); + diff_tree_combined(ent[0].item->sha1, &parents, revs); sha1_array_clear(&parents); return 0; } diff --git a/combine-diff.c b/combine-diff.c index 3b92c448..6e80a73 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -952,7 +952,7 @@ static void show_combined_header(struct combine_diff_path *elem, } static void show_patch_diff(struct combine_diff_path *elem, int num_parent, - int dense, int working_tree_file, + int working_tree_file, struct rev_info *rev) { struct diff_options *opt = &rev->diffopt; @@ -967,6 +967,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, struct userdiff_driver *textconv = NULL; int is_binary; const char *line_prefix = diff_line_prefix(opt); + int dense = rev->dense_combined_merges; context = opt->context; userdiff = userdiff_find_by_path(elem->path); @@ -1214,7 +1215,6 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re */ void show_combined_diff(struct combine_diff_path *p, int num_parent, - int dense, struct rev_info *rev) { struct diff_options *opt = &rev->diffopt; @@ -1226,7 +1226,7 @@ void show_combined_diff(struct combine_diff_path *p, DIFF_FORMAT_NAME_STATUS)) show_raw_diff(p, num_parent, rev); else if (opt->output_format & DIFF_FORMAT_PATCH) - show_patch_diff(p, num_parent, dense, 1, rev); + show_patch_diff(p, num_parent, 1, rev); } static void free_combined_pair(struct diff_filepair *pair) @@ -1297,7 +1297,6 @@ static void handle_combined_callback(struct diff_options *opt, void diff_tree_combined(const unsigned char *sha1, const struct sha1_array *parents, - int dense, struct rev_info *rev) { struct diff_options *opt = &rev->diffopt; @@ -1365,7 +1364,7 @@ void diff_tree_combined(const unsigned char *sha1, opt->line_termination); for (p = paths; p; p = p->next) { if (p->len) - show_patch_diff(p, num_parent, dense, + show_patch_diff(p, num_parent, 0, rev); } } @@ -1381,7 +1380,7 @@ void diff_tree_combined(const unsigned char *sha1, free_pathspec(&diffopts.pathspec); } -void diff_tree_combined_merge(const struct commit *commit, int dense, +void diff_tree_combined_merge(const struct commit *commit, struct rev_info *rev) { struct commit_list *parent = get_saved_parents(rev, commit); @@ -1391,6 +1390,6 @@ void diff_tree_combined_merge(const struct commit *commit, int dense, sha1_array_append(&parents, parent->item->object.sha1); parent = parent->next; } - diff_tree_combined(commit->object.sha1, &parents, dense, rev); + diff_tree_combined(commit->object.sha1, &parents, rev); sha1_array_clear(&parents); } diff --git a/diff-lib.c b/diff-lib.c index 346cac6..8d0f572 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -174,9 +174,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) i--; if (revs->combine_merges && num_compare_stages == 2) { - show_combined_diff(dpath, 2, - revs->dense_combined_merges, - revs); + show_combined_diff(dpath, 2, revs);
Re: [PATCH 2/3] setup_pager: set MORE=R
On Tue, Jan 21, 2014 at 11:23:30AM -0800, Junio C Hamano wrote: > > does complicate the point of my series, which was to add more intimate > > logic about how we handle LESS. > > ... > > return !x || strchr(x, 'R'); > [...] > > I am not sure if it is even a good idea for us to have so intimate > logic for various pagers in the first place. I'd seriously wonder > if it is better to take this position: > > A platform packager who sets the default pager and/or the > default environment for the pager at the build time, or an > individual user who tells your Git what pager you want to > use and/or with what setting that pager should be run under > with environment variables. These people ought to know far > better than Git what their specific choices do. Do not try > to second-guess them. Sorry for letting this topic lapse; I wanted to look at some possible Makefile improvements, which I'll be posting in a moment. I did want to address this point, though. If we are just talking about packagers and defaults (e.g., setting MORE=R on FreeBSD), then I agree that the right tool for the job is empowering the packagers, and the Makefile knob we've discussed does that. But there's another set of people that I was intending to help with the patch, which is people that have set up LESS, and did not necessarily care about the "R" flag in the past (e.g., for many years before git came along, I set LESS=giM, and never even knew that "R" existed). Since git comes out of the box these days with color and the pager turned on, that means people with such a setup see broken output from day one. And I think it is Git's fault here, not the user or the packager. Our defaults assume that the user's pager can handle color, and that is not the default for many pagers, including our default "less"! The user did not turn off "R" here; they simply set other options they cared about, and our hacky workaround to auto-enable "R" did not kick in. It seems a shame to me that we cannot do better for such users. However, the level of intimacy with the pager is getting to be a bit too much for my taste, and the saving grace is that not that many people set LESS themselves (and git is widely enough known that "R" is a common recommendation when people do). So it's probably the lesser of two evils to ignore the situation, and let people who run into it find the answers on the web. So I think there is nothing to be done. But I did want to mention it in case somebody else can come up with some clever solution. :) -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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup writes: >> so something like >> >> for (p = buf; p < end; p++) { >> p = find the end of this line >> if (!p) >> break; >> num++; >> } >> >> perhaps? > > Would crash on incomplete last line. Hmph, even with "if !p"? From your other message: + for (p = buf;; num++) { + p = memchr(p, '\n', end - p); + if (p) { + p++; + continue; } + break; + } If you flip the if statement around that would be the same as: + for (p = buf;; num++) { + p = memchr(p, '\n', end - p); + if (!p) break; p++; + } And with "loop action not in the control part", that would be the same as: for (p = buf; ; p++) { p = memchr... if (!p) break; num++; } no? Would this crash on incomplete last line? Ahh, "p < end" in "for (p = buf; p < end; p++) {" is not correct; you depend on overrunning the end of the buffer to feed length 0 to memchr and returning NULL inside the loop. So to be equivalent to your version, the contination condition needs to be for (p = buf; p <= end; p++) { But that last round of the loop will be no-op, so "p < end" vs "p <= end" does not make any difference. It is not even strictly necessary because memchr() limits the scan to end, but it would still be a good belt-and-suspenders defensive coding practice, I would think. + for (p = buf;;) { + *lineno++ = p - buf; + p = memchr(p, '\n', end - p); + if (p) { + p++; + continue; } + break; } Can we do the same transformation here? for (p = buf;;) { *lineno++ = p - buf; p = memchr... if (!p) break; p++; } which is the same as for (p = buf; ; p++) { *lineno++ = p - buf; p = memchr... if (!p) break; } and the latter has the loop control p++ at where it belongs to. The post-condition of one iteration of the body of the loop being "p points at the terminating LF of this line", incrementing the pointer is how the loop control advances the pointer to the beginning of the next line. If we wanted to have a belt-and-suspenders safety, we need "p <= end" here, not "p < end", because of how p is used when it is past the last LF. As we want to make these two loops look similar, that means we should use "p <= end" in the previous loop as well. This was fun ;-) -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano writes: > David Kastrup writes: > >> Ok, I now wrote >> >> for (p = buf;; num++, p++) { >> p = memchr(p, '\n', end - p); >> if (!p) >> break; >> } > > Looks still wrong (perhaps this is a taste issue). > > num++ is not "loop control", but the real action of this > loop to count lines. It is better left inside. Ok. > p++ is "loop control", and belongs to the third part of > for(;;). No, it isn't. The "real" loop control is the p = memchr line. p++ only skips over the newline. > Isn't the normal continuation condition "p < end"? memchr returns NULL when not finding anything any more. > so something like > > for (p = buf; p < end; p++) { > p = find the end of this line > if (!p) > break; > num++; > } > > perhaps? Would crash on incomplete last line. -- David Kastrup -- To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line
Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) < sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup --- Now without including git-compat-util.h (included by cache.h already). builtin/blame.c | 43 --- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index e44a6bb..59d62dc 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1772,25 +1772,38 @@ static int prepare_lines(struct scoreboard *sb) { const char *buf = sb->final_buf; unsigned long len = sb->final_buf_size; - int num = 0, incomplete = 0, bol = 1; + const char *end = buf + len; + const char *p; + int *lineno; + int num = 0, incomplete = 0; - if (len && buf[len-1] != '\n') - incomplete++; /* incomplete line at the end */ - while (len--) { - if (bol) { - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + 1)); - sb->lineno[num] = buf - sb->final_buf; - bol = 0; + for (p = buf;; num++) { + p = memchr(p, '\n', end - p); + if (p) { + p++; + continue; } - if (*buf++ == '\n') { - num++; - bol = 1; + break; + } + + if (len && end[-1] != '\n') + incomplete++; /* incomplete line at the end */ + + sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1)); + + for (p = buf;;) { + *lineno++ = p - buf; + p = memchr(p, '\n', end - p); + if (p) { + p++; + continue; } + break; } - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + incomplete + 1)); - sb->lineno[num + incomplete] = buf - sb->final_buf; + + if (incomplete) + *lineno++ = len; + sb->num_lines = num + incomplete; return sb->num_lines; } -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup writes: > Ok, I now wrote > > for (p = buf;; num++, p++) { > p = memchr(p, '\n', end - p); > if (!p) > break; > } Looks still wrong (perhaps this is a taste issue). num++ is not "loop control", but the real action of this loop to count lines. It is better left inside. p++ is "loop control", and belongs to the third part of for(;;). Isn't the normal continuation condition "p < end"? so something like for (p = buf; p < end; p++) { p = find the end of this line if (!p) break; num++; } perhaps? -- To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line
Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) < sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup --- builtin/blame.c | 44 +--- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index e44a6bb..c422584 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -4,6 +4,7 @@ * Copyright (c) 2006, Junio C Hamano */ +#include "git-compat-util.h" #include "cache.h" #include "builtin.h" #include "blob.h" @@ -1772,25 +1773,38 @@ static int prepare_lines(struct scoreboard *sb) { const char *buf = sb->final_buf; unsigned long len = sb->final_buf_size; - int num = 0, incomplete = 0, bol = 1; + const char *end = buf + len; + const char *p; + int *lineno; + int num = 0, incomplete = 0; - if (len && buf[len-1] != '\n') - incomplete++; /* incomplete line at the end */ - while (len--) { - if (bol) { - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + 1)); - sb->lineno[num] = buf - sb->final_buf; - bol = 0; + for (p = buf;; num++) { + p = memchr(p, '\n', end - p); + if (p) { + p++; + continue; } - if (*buf++ == '\n') { - num++; - bol = 1; + break; + } + + if (len && end[-1] != '\n') + incomplete++; /* incomplete line at the end */ + + sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1)); + + for (p = buf;;) { + *lineno++ = p - buf; + p = memchr(p, '\n', end - p); + if (p) { + p++; + continue; } + break; } - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + incomplete + 1)); - sb->lineno[num + incomplete] = buf - sb->final_buf; + + if (incomplete) + *lineno++ = len; + sb->num_lines = num + incomplete; return sb->num_lines; } -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup writes: > Junio C Hamano writes: > >> David Kastrup writes: >> >>> Anybody know offhand what I should be including here? It looks like Git >>> has some fallback definitions of its own, so it's probably not just >>> I should include? >> >> In general, no *.c files outside the compatibility layer should >> include anything "#include ", as there seem to be >> finicky systems that care about order of inclusion and feature macro >> definitions, all of which are meant to be handled by including >> git-compat-util.h as the very first thing. > > Ok, and that one's not yet in blame.c. Will include, thanks. No, don't. Some well-known *.h files of ourse, most notably cache.h, are known to include compat-util as the first thing, and that is what *.c files typically include. -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano writes: > David Kastrup writes: > >> Making a single preparation run for counting the lines will avoid memory >> fragmentation. Also, fix the allocated memory size which was wrong >> when sizeof(int *) != sizeof(int), and would have been too small >> for sizeof(int *) < sizeof(int), admittedly unlikely. >> >> Signed-off-by: David Kastrup >> --- >> builtin/blame.c | 40 >> 1 file changed, 24 insertions(+), 16 deletions(-) >> >> diff --git a/builtin/blame.c b/builtin/blame.c >> index e44a6bb..522986d 100644 >> --- a/builtin/blame.c >> +++ b/builtin/blame.c >> @@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb) >> { >> const char *buf = sb->final_buf; >> unsigned long len = sb->final_buf_size; >> -int num = 0, incomplete = 0, bol = 1; >> +const char *end = buf + len; >> +const char *p; >> +int *lineno; >> + >> +int num = 0, incomplete = 0; > > Is there any significance to the blank line between these two > variable definitions? > >> + >> +for (p = buf;;) { >> +if ((p = memchr(p, '\n', end-p)) == NULL) >> +break; >> +++num, ++p; > > You have a peculiar style that is somewhat distracting. Why isn't > this more like so? > > for (p = buf; p++, num++; ) { > p = memchr(p, '\n', end - p); > if (!p) > break; > } Ok, I now wrote for (p = buf;; num++, p++) { p = memchr(p, '\n', end - p); if (!p) break; } and you know what? Its logic seems wrong. The reason is that the p++ does not really have anything to do with the iteration, but rather steps past the '\n' from the memchr. So it's more like for (p = buf;; num++) { p = memchr(p, '\n', end - p); if (p) { p++; continue; } break; } So barring protests, that's what I'm going to use instead. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano writes: > David Kastrup writes: > >> Anybody know offhand what I should be including here? It looks like Git >> has some fallback definitions of its own, so it's probably not just >> I should include? > > In general, no *.c files outside the compatibility layer should > include anything "#include ", as there seem to be > finicky systems that care about order of inclusion and feature macro > definitions, all of which are meant to be handled by including > git-compat-util.h as the very first thing. Ok, and that one's not yet in blame.c. Will include, thanks. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano writes: > Junio C Hamano writes: > >> David Kastrup writes: >> >>> Whitespace error in line 1778. Should I be reposting? >> >> Heh, let me try to clean it up first and then repost for your >> review. >> >> Thanks. > > -- >8 -- > From: David Kastrup > > Making a single preparation run for counting the lines will avoid memory > fragmentation. Also, fix the allocated memory size which was wrong > when sizeof(int *) != sizeof(int), and would have been too small > for sizeof(int *) < sizeof(int), admittedly unlikely. > > Signed-off-by: David Kastrup > --- > > One logic difference from what was posted is that sb->lineno[num] > is filled with the length of the entire buffer when the file ends > with a complete line. Where's the difference? This is exactly what will happen with my code as well. I _do_ rely on memchr(whatever, '\n', 0) to return NULL without looking at any memory for that. If there is a fear of memchr not being able to deal with a count of 0, this code needs to be somewhat more complex. > I do not remember if the rest of the logic > actually depends on it (I think I use lineno[n+1] - lineno[n] to > find the end of line, Well, you do it about _half_ the time. The other half, you scan for the '\n' explicitly. > The original code dates back to 2006 when the author of the code > was not paid for doing anything for Git but was doing it as a > weekend and evening hobby, so it may not be so surprising to find > this kind of "what was I thinking when I wrote it" inefficiency in > such a code with $0 monetary value ;-) Oh, _this_ patch is not in the "I want to make money from it" range. If that were the case, I should not have bothered at all. This is just the "this code offends my sense of aesthetics" class. It's purely optional to apply. It's conceivable that it will make a performance difference on non-glibc (or what it's called) platforms. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup writes: > Anybody know offhand what I should be including here? It looks like Git > has some fallback definitions of its own, so it's probably not just > I should include? In general, no *.c files outside the compatibility layer should include anything "#include ", as there seem to be finicky systems that care about order of inclusion and feature macro definitions, all of which are meant to be handled by including git-compat-util.h as the very first 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: [RFC/PATCH] howto/maintain-git.txt: new version numbering scheme
From: "Junio C Hamano" "Philip Oakley" writes: If we are progressing from V1.9 to V2.0 quickly (one cycle?), which I understand is the plan, then mixing the minor development items (patch series which progress to master) with the maintenance fixes over the next few months, thus only having 1.9.x releases, sounds reasonable. If there is going to be separate maintenance fixes from the patch series developments then keeping to the previous 1.9.x.y for maintenance would be better. Will the new rapid counting continue after V2.0, such that we get to V2.9 -> V3.0 rather more quickly than V1.0 -> V2.0 ? The key discriminator would be to say when V2.0 will be out for deciding the V1.9 sequence. I do not quite follow. The time distance between v1.9 and v2.0 should not affect anything. If it is a long road, there may be v1.10, v1.11, v1.12, ... I wasn't sure if you were considering going past either 1.9.9 to 1.9.10, and going from 1.9 to 1.10 was something that hadn't occurred to me (somewhat of a Doh! moment maybe). before we have v2.0. If not, v2.0 may immediately follow v1.9 as a new feature release. There may be maintenance releases based on v1.9 that does not add any new features. Right now, if you count the maintenance releases, there are potentially four kinds of version gaps: - Between v1.8.5 and v1.8.5.1, there are fixes but no new features; - Between v1.8.5 and v1.8.6, there are new features but no compatibility worries; - Between v1.8.6 and v1.9.0, there are new features, no compatibility worries, but somehow the jump is larger than the one between v1.8.5 and v1.8.6; and - Between v1.9.0 and v2.0.0, there are new features and also compatibility concerns. Switching to 2-digit scheme and calling the upcoming one v1.9 (and the next major one v2.0) was meant to make the naming more flat, OK I'd buy that flattening approach. as the third item in the above list "somehow the jump is larger" does not seem to add much value to the end users. So the logical numbering becomes more like this: - Between v1.9 and v1.9.1, there are fixes but no new features; - Between v1.9.x and v1.10, there are new features but no compatibility worries; - Between v1.9.x and v2.0, there are new features and also compatibility concerns. With a twist, though. There seem to be many places where at least three digits are assumed to exist in our version numbers, so in order to make life easier, the updated document says vX.Y (a feature release) will identify itself as vX.Y.0 Yes. I'd be happy to support that third 'digit' for the maint releases, with zero as the initial release. Git Gui has a version string checking routine but its regex only needs two parts X.Y (we looked into the version string back in $gmane/217189 Thanks for the clarifications. Philip. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano writes: > David Kastrup writes: > >> Whitespace error in line 1778. Should I be reposting? > > Heh, let me try to clean it up first and then repost for your > review. > > Thanks. -- >8 -- From: David Kastrup Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) < sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup --- One logic difference from what was posted is that sb->lineno[num] is filled with the length of the entire buffer when the file ends with a complete line. I do not remember if the rest of the logic actually depends on it (I think I use lineno[n+1] - lineno[n] to find the end of line, so this may matter for the last line), though. The original code dates back to 2006 when the author of the code was not paid for doing anything for Git but was doing it as a weekend and evening hobby, so it may not be so surprising to find this kind of "what was I thinking when I wrote it" inefficiency in such a code with $0 monetary value ;-) builtin/blame.c | 37 + 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index e44a6bb..2364661 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1772,25 +1772,30 @@ static int prepare_lines(struct scoreboard *sb) { const char *buf = sb->final_buf; unsigned long len = sb->final_buf_size; - int num = 0, incomplete = 0, bol = 1; + const char *end = buf + len; + const char *p; + int *lineno; + int num = 0, incomplete = 0; + + for (p = buf; p < end; p++) { + p = memchr(p, '\n', end - p); + if (!p) + break; + num++; + } - if (len && buf[len-1] != '\n') + if (len && end[-1] != '\n') incomplete++; /* incomplete line at the end */ - while (len--) { - if (bol) { - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + 1)); - sb->lineno[num] = buf - sb->final_buf; - bol = 0; - } - if (*buf++ == '\n') { - num++; - bol = 1; - } + + sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1)); + + for (p = buf; p < end; p++) { + *lineno++ = p - buf; + p = memchr(p, '\n', end - p); + if (!p) + break; } - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + incomplete + 1)); - sb->lineno[num + incomplete] = buf - sb->final_buf; + sb->lineno[num + incomplete] = len; sb->num_lines = num + incomplete; return sb->num_lines; } -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
Junio C Hamano writes: > David Kastrup writes: > >> Making a single preparation run for counting the lines will avoid memory >> fragmentation. Also, fix the allocated memory size which was wrong >> when sizeof(int *) != sizeof(int), and would have been too small >> for sizeof(int *) < sizeof(int), admittedly unlikely. >> >> Signed-off-by: David Kastrup >> --- >> builtin/blame.c | 40 >> 1 file changed, 24 insertions(+), 16 deletions(-) >> >> diff --git a/builtin/blame.c b/builtin/blame.c >> index e44a6bb..522986d 100644 >> --- a/builtin/blame.c >> +++ b/builtin/blame.c >> @@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb) >> { >> const char *buf = sb->final_buf; >> unsigned long len = sb->final_buf_size; >> -int num = 0, incomplete = 0, bol = 1; >> +const char *end = buf + len; >> +const char *p; >> +int *lineno; >> + >> +int num = 0, incomplete = 0; > > Is there any significance to the blank line between these two > variable definitions? Well, I needed more than the whitespace error to be motivated for redoing. Cough, cough. >> + >> +for (p = buf;;) { >> +if ((p = memchr(p, '\n', end-p)) == NULL) >> +break; >> +++num, ++p; > > You have a peculiar style that is somewhat distracting. Why isn't > this more like so? > > for (p = buf; p++, num++; ) { More likely for (p = buf;; p++, num++) > p = memchr(p, '\n', end - p); > if (!p) > break; > } > > which I think is the prevalent style in our codebase. The same for > the other loop we see in the new code below. I rearranged a few times in order to have both loops be closely analogous. The second loop would then have to be for (p = buf;; p++) { *lineno++ = p-buf; p = memchr(p, '\n', end-p) if (!p) break; } Admittedly, that works. I am not too happy about the termination condition being at the end of the loop but not in the for statement, but yes, this seems somewhat nicer than what I proposed. > - favor post-increment unless you use it as rvalue and need >pre-increment; In my youth, the very non-optimizing C compiler I used under CP/M produced less efficient code for x++ than for ++x even when not using the resulting expression. Surprisingly habit-forming. > > - SP around each binary ops e.g. 'end - p'; Ok. >> +} >> >> -if (len && buf[len-1] != '\n') >> +if (len && end[-1] != '\n') >> incomplete++; /* incomplete line at the end */ > > OK, so far we counted "num" complete lines and "incomplete" may be > one if there is an incomplete line after them. That's pretty much the gist of the original code. >> -while (len--) { >> -if (bol) { >> -sb->lineno = xrealloc(sb->lineno, >> - sizeof(int *) * (num + 1)); >> -sb->lineno[num] = buf - sb->final_buf; >> -bol = 0; >> -} >> -if (*buf++ == '\n') { >> -num++; >> -bol = 1; >> -} >> + >> +sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1)); > > OK, this function is called only once, so we know sb->lineno is NULL > originally and there is no reason to start from xrealloc(). [...] > These really *were* unnecessary reallocations. Well, if a realloc will increase the allocation size by a constant factor each time, the amortization cost is O(n) for n entries. So with a suitable realloc, the effect will not really be noticeable. It still offends my sense of aesthetics. > Thanks for catching them, but this patch needs heavy style fixes. Well, does not look all that heavy, but I'll repost. There is another oversight: I am using memchr here, but there is no obvious header file definiting it (the respective header will likely be pulled in indirectly via something unrelated). Anybody know offhand what I should be including here? It looks like Git has some fallback definitions of its own, so it's probably not just I should include? -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup writes: > Whitespace error in line 1778. Should I be reposting? Heh, let me try to clean it up first and then repost for your review. 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] blame.c: prepare_lines should not call xrealloc for every line
David Kastrup writes: > Making a single preparation run for counting the lines will avoid memory > fragmentation. Also, fix the allocated memory size which was wrong > when sizeof(int *) != sizeof(int), and would have been too small > for sizeof(int *) < sizeof(int), admittedly unlikely. > > Signed-off-by: David Kastrup > --- > builtin/blame.c | 40 > 1 file changed, 24 insertions(+), 16 deletions(-) > > diff --git a/builtin/blame.c b/builtin/blame.c > index e44a6bb..522986d 100644 > --- a/builtin/blame.c > +++ b/builtin/blame.c > @@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb) > { > const char *buf = sb->final_buf; > unsigned long len = sb->final_buf_size; > - int num = 0, incomplete = 0, bol = 1; > + const char *end = buf + len; > + const char *p; > + int *lineno; > + > + int num = 0, incomplete = 0; Is there any significance to the blank line between these two variable definitions? > + > + for (p = buf;;) { > + if ((p = memchr(p, '\n', end-p)) == NULL) > + break; > + ++num, ++p; You have a peculiar style that is somewhat distracting. Why isn't this more like so? for (p = buf; p++, num++; ) { p = memchr(p, '\n', end - p); if (!p) break; } which I think is the prevalent style in our codebase. The same for the other loop we see in the new code below. - favor post-increment unless you use it as rvalue and need pre-increment; - SP around each binary ops e.g. 'end - p'; - avoid assignments in conditionals when you do not have to. > + } > > - if (len && buf[len-1] != '\n') > + if (len && end[-1] != '\n') > incomplete++; /* incomplete line at the end */ OK, so far we counted "num" complete lines and "incomplete" may be one if there is an incomplete line after them. > - while (len--) { > - if (bol) { > - sb->lineno = xrealloc(sb->lineno, > - sizeof(int *) * (num + 1)); > - sb->lineno[num] = buf - sb->final_buf; > - bol = 0; > - } > - if (*buf++ == '\n') { > - num++; > - bol = 1; > - } > + > + sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1)); OK, this function is called only once, so we know sb->lineno is NULL originally and there is no reason to start from xrealloc(). > + for (p = buf;;) { > + *lineno++ = p-buf; > + if ((p = memchr(p, '\n', end-p)) == NULL) > + break; > + ++p; > } > - sb->lineno = xrealloc(sb->lineno, > - sizeof(int *) * (num + incomplete + 1)); These really *were* unnecessary reallocations. Thanks for catching them, but this patch needs heavy style fixes. > - sb->lineno[num + incomplete] = buf - sb->final_buf; > + > + if (incomplete) > + *lineno++ = len; > + > sb->num_lines = num + incomplete; > return sb->num_lines; > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Miklos Vajna writes: > Hi, > > I was trying to understand the history of a piece of code in LibreOffice > and I'm facing a behaviour of git-log which is not something I can > explain. I'm not sure if this is a git bug or a user error. ;) > > Here is the situation: > > git clone git://anongit.freedesktop.org/libreoffice/core > cd core > git log --full-history -p -S'mnTitleBarHeight =' > sd/source/ui/dlg/PaneDockingWindow.cxx Lack of -m is what I would first suspect when somebody misunderstands "merge simplification". I am not saying that will be the issue, but merely pointing out that that is the first thing that jumps at me when I view the above command line. > > Here the first output I get from git-log is > b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the > commit *added* that string. So it should be there on master, I would > assume. > > But then I run: > > git grep 'mnTitleBarHeight =' sd > > and it's not there. Am I missing something, as in e.g. even with > --full-history git-log does some simplification? > > Thanks, > > Miklos -- To unsubscribe from this list: send the line "unsubscribe 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 4/6] t0060: Add tests for prefix_path when path begins with work tree
Torsten Bögershausen writes: > On 2014-02-04 15.25, Martin Erik Werner wrote: > >> t/t0060-path-utils.sh | 10 ++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh >> index b8e92e1..c0a14f6 100755 >> --- a/t/t0060-path-utils.sh >> +++ b/t/t0060-path-utils.sh >> @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only >> absolute path to work tree' ' >> test_cmp expected actual >> ' >> >> +test_expect_success 'prefix_path rejects absolute path to dir with same >> beginning as work tree' ' >> +test_must_fail test-path-utils prefix_path prefix "$(pwd)a" >> +' >> + >> +test_expect_success 'prefix_path works with absolute path to a symlink to >> work tree having same beginning as work tree' ' >> +git init repo && >> +ln -s repo repolink && >> +test "a" = "$(cd repo && test-path-utils prefix_path prefix >> "$(pwd)/../repolink/a")" >> +' > I think we need SYMLINKS here. Yes. -- To unsubscribe from this list: send the line "unsubscribe 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] blame.c: prepare_lines should not call xrealloc for every line
Whitespace error in line 1778. Should I be reposting? -- David Kastrup -- To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line
Making a single preparation run for counting the lines will avoid memory fragmentation. Also, fix the allocated memory size which was wrong when sizeof(int *) != sizeof(int), and would have been too small for sizeof(int *) < sizeof(int), admittedly unlikely. Signed-off-by: David Kastrup --- builtin/blame.c | 40 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index e44a6bb..522986d 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -1772,25 +1772,33 @@ static int prepare_lines(struct scoreboard *sb) { const char *buf = sb->final_buf; unsigned long len = sb->final_buf_size; - int num = 0, incomplete = 0, bol = 1; + const char *end = buf + len; + const char *p; + int *lineno; + + int num = 0, incomplete = 0; + + for (p = buf;;) { + if ((p = memchr(p, '\n', end-p)) == NULL) + break; + ++num, ++p; + } - if (len && buf[len-1] != '\n') + if (len && end[-1] != '\n') incomplete++; /* incomplete line at the end */ - while (len--) { - if (bol) { - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + 1)); - sb->lineno[num] = buf - sb->final_buf; - bol = 0; - } - if (*buf++ == '\n') { - num++; - bol = 1; - } + + sb->lineno = lineno = xmalloc(sizeof(int) * (num + incomplete + 1)); + + for (p = buf;;) { + *lineno++ = p-buf; + if ((p = memchr(p, '\n', end-p)) == NULL) + break; + ++p; } - sb->lineno = xrealloc(sb->lineno, - sizeof(int *) * (num + incomplete + 1)); - sb->lineno[num + incomplete] = buf - sb->final_buf; + + if (incomplete) + *lineno++ = len; + sb->num_lines = num + incomplete; return sb->num_lines; } -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Hi Jonathan, On Tue, Feb 04, 2014 at 11:48:42AM -0800, Jonathan Nieder wrote: > Luckily '-m -p' without --first-parent worked and the first commit it > showed was the right one. It produces more hits than I'd like, too, > though. Ah, excellent! :-) '-m' does what I need. Thanks a lot, Miklos signature.asc Description: Digital signature
Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
On 2014-02-04 15.25, Martin Erik Werner wrote: > t/t0060-path-utils.sh | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh > index b8e92e1..c0a14f6 100755 > --- a/t/t0060-path-utils.sh > +++ b/t/t0060-path-utils.sh > @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only > absolute path to work tree' ' > test_cmp expected actual > ' > > +test_expect_success 'prefix_path rejects absolute path to dir with same > beginning as work tree' ' > + test_must_fail test-path-utils prefix_path prefix "$(pwd)a" > +' > + > +test_expect_success 'prefix_path works with absolute path to a symlink to > work tree having same beginning as work tree' ' > + git init repo && > + ln -s repo repolink && > + test "a" = "$(cd repo && test-path-utils prefix_path prefix > "$(pwd)/../repolink/a")" > +' I think we need SYMLINKS here. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
Hi, Miklos Vajna wrote: > git clone git://anongit.freedesktop.org/libreoffice/core > cd core > git log --full-history -p -S'mnTitleBarHeight =' > sd/source/ui/dlg/PaneDockingWindow.cxx > > Here the first output I get from git-log is > b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the > commit *added* that string. So it should be there on master, I would > assume. df76bfb0695d19d201936df80192108e7ce51b8c (a merge) removed it. Plain 'git log' doesn't notice because in the default mode it skips merges. Since the culprit commit is not in the first-parent history of HEAD, my usual approach doesn't help, either: $ git log -m --first-parent -S'mnTitleBarHeight =' \ -- sd/source/ui/dlg/PaneDockingWindow.cxx $ Using -c or --cc produces too many hits. Luckily '-m -p' without --first-parent worked and the first commit it showed was the right one. It produces more hits than I'd like, too, though. The -L option doesn't interact well enough with --reverse to handle this case: $ git grep -p -e'mnTitleBarHeight =' b390fae1 -- sd/source/ui/dlg/PaneDockingWindow.cxx b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx=void PaneDockingWindow::Layout (void) b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx:mnTitleBarHeight = GetSettings().GetStyleSettings().GetTitleHeight(); b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx: mnTitleBarHeight = aToolBoxSize.Height(); b390fae1:sd/source/ui/dlg/PaneDockingWindow.cxx: mnTitleBarHeight = aToolBoxSize.Height(); $ git log --reverse b390fae1..HEAD \ -L:Layout:sd/source/ui/dlg/PaneDockingWindow.cxx fatal: -L parameter 'Layout' starting at line 1: no match Thanks for a useful example. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function
Martin Erik Werner writes: > + const char* work_tree = get_git_work_tree(); Style: asterisk sticks to the variable, not type. No need to resend---all patches looked reasonable. 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 2/2] reset: support "--mixed --intent-to-add" mode
Nguyễn Thái Ngọc Duy writes: > @@ -128,13 +129,20 @@ static void update_index_from_diff(struct > diff_queue_struct *q, > one->path); > add_cache_entry(ce, ADD_CACHE_OK_TO_ADD | > ADD_CACHE_OK_TO_REPLACE); > + } else if (*intent_to_add) { > + int pos = cache_name_pos(one->path, strlen(one->path)); > + if (pos < 0) > + die(_("%s does not exist in index"), > + one->path); > + set_intent_to_add(&the_index, active_cache[pos]); While I do not have any problem with adding an optional "keep lost paths as intent-to-add entries" feature, I am not sure why this has to be so different from the usual add-cache-entry codepath. The if/elseif chain you are touching inside this loop does: - If the tree you are resetting to has something at the path (which is different from the current index, obviously), create a cache entry to represent that state from the tree and stuff it in the index; - Otherwise, the tree you are resetting to does not have that path. We used to say "remove it from the index", but now we have an option to instead add it as an intent-to-add entry. So, why doesn't the new codepath do exactly the same thing as the first branch of the if/else chain and call add_cache_entry but with a ce marked with CE_INTENT_TO_ADD? That would parallel what happens in "git add -N" better, I would think, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log history simplification problem
On Tue, Feb 04, 2014 at 06:37:13PM +0100, Miklos Vajna wrote: > But then I run: > > git grep 'mnTitleBarHeight =' sd > > and it's not there. Am I missing something, as in e.g. even with > --full-history git-log does some simplification? I tried to reproduce this with a repo from scratch, and it seems my problem is the following: 1) "A" creates a feature branch 2) "A" works on it, and in the meantime master progresses as well 3) "A" merges master to the feature branch 4) "A" does some additional changes, and -- in an evil way -- uses "git commit -a --amend" to squeeze these into the merge commit 5) "B" (that's me) comes and try to find out where a string got deleted, but can't. Here is a reproducer script: rm -rf scratch mkdir scratch cd scratch git init echo -e "a\na\na\na\na\na\na\na\n" > a git add a git commit -m init git branch feature echo "b" >> a git add a git commit -m "more master changes" git checkout feature sed -i '1iXXX' a # insert first row git add a git commit -m "feature" git merge -m merge master sed -i '1d' a # delete first row git add a git commit --amend -m "merge" I now know that the XXX got removed by the merge commit, but how can I see it that I'm right? If I run 'git log --all -p' in the result, I see that XXX got inserted by one commit, now I don't have it, but I don't see any deletion, which confuses me. Any ideas? :-) Thanks, Miklos signature.asc Description: Digital signature
Re: [PATCH] git-tag.txt: for --contains is optional
Nguyễn Thái Ngọc Duy writes: > This goes far back to e84fb2f (branch --contains: default to HEAD - > 2008-07-08) where the same parsing code is shared with > builtin/tag.c. git-branch.txt correctly states that for > --contains is optional while git-tag.txt does not. Correct it. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > Documentation/git-tag.txt | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt > index c418c44..404257d 100644 > --- a/Documentation/git-tag.txt > +++ b/Documentation/git-tag.txt > @@ -103,8 +103,9 @@ OPTIONS > + > This option is only applicable when listing tags without annotation lines. > > ---contains :: > - Only list tags which contain the specified commit. > +--contains []:: > + Only list tags which contain the specified commit (HEAD if not > + specified). Thanks. > > --points-at :: > Only list tags of the given object. -- To unsubscribe from this list: send the line "unsubscribe 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 7/8] combine-diff: Fast changed-to-all-parents paths scanning
Kirill Smelkov writes: >> if we did not want to reinvent the whole tree walking thing, which >> would add risks for new bugs and burden to maintain this and the >> usual two-tree diff tree walking in sync. > > Junio, I understand it is not good to duplicate code and increase > maintenance risks > Instead I propose we switch to the new tree walker completely. I am not fundamentally opposed to the idea. We'll see what happens. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function
On Tue, 2014-02-04 at 10:09 -0800, Junio C Hamano wrote: > Martin Erik Werner writes: (...) > > I was trying to convey that if path is simply "/dir/repo", then the while > > loop method of replacing a '/' and checking from the beginning won't > > work for the last level, since it has no terminating '/' to replace, so > > hence it's a special case, mentioning the "part inside the work tree" > > is arguably confusing in that case, since there isn't really one, maybe > > it should be left out completely, since the "check each level" > > explanation covers it already? > > I dunno about the explanation, but it still looks strange to have > the special case to deal with "/dir/repo" before you enter the while > loop, and then also have code immediately after the loop that seems > to handle the same case. Isn't the latter one redundant? The check before the loop doesn't use 'real_path', the one after does: "/dir/repo" vs "/dir/repolink" -- Martin Erik Werner -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] receive-pack: hint that the user can stop "git push" at auto gc time
Junio C Hamano writes: > Nguyễn Thái Ngọc Duy writes: > >> Housekeeping jobs like auto gc generally should not get in the way. >> Users who are pushing may not want to wait until auto gc is done on >> the server. Give a hint for those users that it's safe now to break >> "git push" and stop waiting. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> This bandage patch may be a good compromise between running auto gc >> and not annoying users much. >> >> If I'm not mistaken, when ^C on "git push" this way, gc will still be >> running until it needs to print something out (which it should not >> normally because of --quiet). The user won't see gc errors, but the >> user generally can't do much anyway. > > If you are over local transport, I would think you would kill the > both ends. Also, wouldn't killing "git push" before it is done > talking with the receive-pack stop it before it has a chance to > update the remote tracking refs to pretend as if it fetched from > there immediately after a push? > > So, no. I do not think we should ever encourage "if this bothers > you, you can ^C it". Making it not to bother is fine, though. Instead of adding a boolean --break-ok that is hidden, why not adding an exposed boolean --daemonize, and let auto-gc run in the background? With the recent "do not let more than one gc run at the same time", that should give a lot more pleasant end user experience, 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: [PATCH/RFC 2/2] receive-pack: hint that the user can stop "git push" at auto gc time
Nguyễn Thái Ngọc Duy writes: > Housekeeping jobs like auto gc generally should not get in the way. > Users who are pushing may not want to wait until auto gc is done on > the server. Give a hint for those users that it's safe now to break > "git push" and stop waiting. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > This bandage patch may be a good compromise between running auto gc > and not annoying users much. > > If I'm not mistaken, when ^C on "git push" this way, gc will still be > running until it needs to print something out (which it should not > normally because of --quiet). The user won't see gc errors, but the > user generally can't do much anyway. If you are over local transport, I would think you would kill the both ends. Also, wouldn't killing "git push" before it is done talking with the receive-pack stop it before it has a chance to update the remote tracking refs to pretend as if it fetched from there immediately after a push? So, no. I do not think we should ever encourage "if this bothers you, you can ^C it". Making it not to bother is fine, 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 v5 4/5] setup: Add 'abspath_part_inside_repo' function
Martin Erik Werner writes: > Will you add that test or should I place it in the series with you as > author? Either is fine. Thanks. > On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote: >> Martin Erik Werner writes: >> >> > The path being exactly equal to the work tree is handled separately, >> > since then there is no directory separator between the work tree and >> > in-repo part. >> >> What is an "in-repo part"? Whatever it is, I am not sure if I >> follow that logic. After the while (*path) loop checks each level, >> you check the whole path---wouldn't that code handle that special >> case already? > > Given "/dir1/repo/dir2/file" I've used 'in-repo' to refer to > "dir2/file", sometimes interchangably with "part inside the work tree" > which might be better terminology, and should replace it? Yes, "inside the working tree" is much clearer than "inside repo", because the word "repository" often is used to mean only the stuff inside $GIT_DIR, i.e. what controls the working tree files. > I was trying to convey that if path is simply "/dir/repo", then the while > loop method of replacing a '/' and checking from the beginning won't > work for the last level, since it has no terminating '/' to replace, so > hence it's a special case, mentioning the "part inside the work tree" > is arguably confusing in that case, since there isn't really one, maybe > it should be left out completely, since the "check each level" > explanation covers it already? I dunno about the explanation, but it still looks strange to have the special case to deal with "/dir/repo" before you enter the while loop, and then also have code immediately after the loop that seems to handle the same case. Isn't the latter one redundant? >> Because it is probably the normal case not to have any symbolic link >> in the leading part (hence not having to dereference them), I think >> checking "is work_tree[] a prefix of path[]" early is justified from >> performance point of view, though. >> >> > /* >> > + * No checking if the path is in fact an absolute path is done, and it >> > must >> > + * already be normalized. >> >> This is not quite parsable to me. > Hmm, what about > The input parameter must contain an absolute path, and it must > already be normalized. OK. >> > + const char* work_tree = get_git_work_tree(); >> > + if (!work_tree) >> > + return -1; >> > + wtlen = strlen(work_tree); >> > + len = strlen(path); >> >> I expect that this is called from a callsite that _knows_ how long >> path[] is. Shouldn't len be a parameter to this function instead? > > Currently, it actually doesn't, since 'normalize_path_copy_len' is > called on it prior, which can mess with the string length. OK, strlen() here is perfectly fine, then. >> Hmph How do our callers treat an empty path? In other words, >> should these three be equivalent? >> >> cd /a && git ls-files /a >> cd /a && git ls-files "" >> cd /a && git ls-files . > > Here I have only gone by the assumption that previous code did the right > thing,... Good to know. And the answer to the above question I think is yes, these should be equivalent. 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
git log history simplification problem
Hi, I was trying to understand the history of a piece of code in LibreOffice and I'm facing a behaviour of git-log which is not something I can explain. I'm not sure if this is a git bug or a user error. ;) Here is the situation: git clone git://anongit.freedesktop.org/libreoffice/core cd core git log --full-history -p -S'mnTitleBarHeight =' sd/source/ui/dlg/PaneDockingWindow.cxx Here the first output I get from git-log is b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the commit *added* that string. So it should be there on master, I would assume. But then I run: git grep 'mnTitleBarHeight =' sd and it's not there. Am I missing something, as in e.g. even with --full-history git-log does some simplification? Thanks, Miklos signature.asc Description: Digital signature
Re: [PATCH 7/8] combine-diff: Fast changed-to-all-parents paths scanning
On Mon, Feb 03, 2014 at 03:39:02PM -0800, Junio C Hamano wrote: > Junio C Hamano writes: > > > Kirill Smelkov writes: > > > >> As was recently shown (c839f1bd "combine-diff: optimize > >> combine_diff_path sets intersection"), combine-diff runs very slowly. In > >> that commit we optimized paths sets intersection, but that accounted > >> only for ~ 25% of the slowness, and as my tracing showed, for linux.git > >> v3.10..v3.11, for merges a lot of time is spent computing > >> diff(commit,commit^2) just to only then intersect that huge diff to > >> almost small set of files from diff(commit,commit^1). > >> > >> That's because at present, to compute combine-diff, for first finding > >> paths, that "every parent touches", we use the following combine-diff > >> property/definition: > >> > >> D(A,P1...Pn) = D(A,P1) ^ ... ^ D(A,Pn) (w.r.t. paths) > >> > >> where > >> > >> D(A,P1...Pn) is combined diff between commit A, and parents Pi > >> > >> and > >> > >> D(A,Pi) is usual two-tree diff Pi..A > > > > and A ^ B means what??? > > Silly me; obviously it is the "set intersection" operator. > > We probably could instead use the "current" set of paths as literal > pathspec to compute subsequent paths, i.e. > > D(A,Pi,PS) is two tree diff P1..A limited to paths PS > > D(A,P1...Pn) = D(A,P1,[]) ^ > D(A,P2,D(A,P1,[])) ^ >... > D(A,Pn,D(A,P1...Pn-1)) > > if we did not want to reinvent the whole tree walking thing, which > would add risks for new bugs and burden to maintain this and the > usual two-tree diff tree walking in sync. Junio, I understand it is not good to duplicate code and increase maintenance risks. On the other hand, I don't quite like the approach with keeping current paths - it could work and be faster compared to what we had, but to me it is still suboptimal, because if the first diff D(A,P1) is huge then oops. Also, to implement it rationally, without delving into unneeded recursion, we would need to do the diffing without recursion, intersect results, and then recurse into overlapping subtrees, which means tree-walker rework anyway. Instead I propose we switch to the new tree walker completely. With the attached patch which draftly shows it (diff_tree is gone, the work is done through diff_tree_combined_paths, and then the result is "exported" to diff_filepairs) all the tests pass. Also, timings for git log --raw --no-abbrev --no-renames ( without -c - it would be the same - we are not touching that code, it would only add, irrelevant-to-the-change constant time ) are linux.git v3.10..v3.11became 0.1% _faster_ navy.gitbecame 1.4% slower That means, that the new tree-walker is correct and should be ok performance-wise (I had not optimized it at all, yet). What would you say? Thanks, Kirill P.S. Thanks for commenting on other patches. I'll try to correct them tomorrow, as I have no more time today and need to run. 8< From: Kirill Smelkov Date: Tue, 4 Feb 2014 20:11:16 +0400 Subject: [PATCH] X Show new tree-walker could be used instead of the old one All tests pass. Timings for git log --raw --no-abrev --no-renames linux.git v3.10..v3.11became 0.1% _faster_ navy.gitbecame 1.4% slower --- diff.h | 2 ++ line-log.c | 12 +++-- revision.c | 16 ++- tree-diff.c | 88 +++-- 4 files changed, 90 insertions(+), 28 deletions(-) diff --git a/diff.h b/diff.h index 5496560..0a9845a 100644 --- a/diff.h +++ b/diff.h @@ -189,8 +189,10 @@ const char *diff_line_prefix(struct diff_options *); extern const char mime_boundary_leader[]; +#if 0 extern int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, struct diff_options *opt); +#endif extern int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt); extern int diff_root_tree_sha1(const unsigned char *new, const char *base, diff --git a/line-log.c b/line-log.c index 717638b..5739bbf 100644 --- a/line-log.c +++ b/line-log.c @@ -766,6 +766,7 @@ void line_log_init(struct rev_info *rev, const char *prefix, struct string_list } } +#if 0 static void load_tree_desc(struct tree_desc *desc, void **tree, const unsigned char *sha1) { @@ -775,6 +776,7 @@ static void load_tree_desc(struct tree_desc *desc, void **tree, die("Unable to read tree (%s)", sha1_to_hex(sha1)); init_tree_desc(desc, *tree, size); } +#endif static int count_parents(struct commit *commit) { @@ -843,17 +845,23 @@ static void queue_diffs(struct line_log_data *range, struct commit *commit, struct commit *parent) { void *tree1 = NULL, *tree2 = NULL; - struct tree_desc desc1, desc2; +// st
Re: [PATCH 1/2] t7101, t7014: rename test files to indicate what that file is for
Nguyễn Thái Ngọc Duy wrote: > Signed-off-by: Nguyễn Thái Ngọc Duy Makes sense. [...] > t/t7101-reset-empty-subdirs.sh (new +x) | 63 > + > t/t7101-reset.sh (gone) | 63 > - > t/t7104-reset-hard.sh (new +x) | 46 > t/t7104-reset.sh (gone) | 46 Hm, summary incorporated in the diffstat. Neat. :) format-patch -M tells me that this indeed just renamed the files, so fwiw Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/6] t0060: Add test for prefix_path on symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This applies to most high-level commands but prefix_path is the common denominator for all of them. Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen --- t/t0060-path-utils.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..0bba988 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test "$sym" = "$(test-path-utils real_path "$dir2/syml")" ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink && + test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink" +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree
The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen --- t/t0060-path-utils.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba988..b8e92e1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink" ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo "" >expected && + test-path-utils prefix_path prefix "$(pwd)" >actual && + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
One edge-case that isn't currently checked in the tests is the beginning of the path matching the work tree, despite the target not actually being the work tree, for example: path = /dir/repoa work_tree = /dir/repo should fail since the path is outside the repo. However, if /dir/repoa is in fact a symlink that points to /dir/repo, it should instead succeed. Add two tests covering these cases, since they might be potential regression points. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen --- t/t0060-path-utils.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1..c0a14f6 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' + test_must_fail test-path-utils prefix_path prefix "$(pwd)a" +' + +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' + git init repo && + ln -s repo repolink && + test "a" = "$(cd repo && test-path-utils prefix_path prefix "$(pwd)/../repolink/a")" +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/6] t3004: Add test for ls-files on symlinks via absolute paths
From: Junio C Hamano When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This causes most high-level functions to misbehave when acting on symlinks given via absolute paths. For example $ git add /dir/repo/symlink attempts to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks inside the work tree into consideration). Add a known-breakage test using the ls-files function, checking both if the symlink leads to a target in the same directory, and a target in the above directory. Signed-off-by: Martin Erik Werner Tested-by: Martin Erik Werner --- t/t3004-ls-files-basic.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh index 8d9bc3c..e20c077 100755 --- a/t/t3004-ls-files-basic.sh +++ b/t/t3004-ls-files-basic.sh @@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' ' test_i18ngrep "[Uu]sage: git ls-files " broken/usage ' +test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' ' + mkdir subs && + ln -s nosuch link && + ln -s ../nosuch subs/link && + git add link subs/link && + git ls-files -s link subs/link >expect && + git ls-files -s "$(pwd)/link" "$(pwd)/subs/link" >actual && + test_cmp expect actual && + + ( + cd subs && + git ls-files -s link >../expect && + git ls-files -s "$(pwd)/link" >../actual + ) && + test_cmp expect actual +' + test_done -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths
The 'prefix_path_gently' function currently applies real_path to everything if given an absolute path, dereferencing symlinks both outside and inside the work tree. This causes most high-level functions to misbehave when acting on symlinks given via absolute paths. For example $ git add /dir/repo/symlink attempts to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. In order to manipulate symlinks in the work tree using absolute paths, symlinks should only be dereferenced outside the work tree. Modify the 'prefix_path_gently' to first normalize the path in order to make sure path levels are separated by '/', then pass the result to 'abspath_part_inside_repo' to find the part inside the work tree (without dereferencing any symlinks inside the work tree). For absolute paths, 'prefix_path_gently' did not, nor does now do, any actual prefixing, hence the result from 'abspath_part_in_repo' is returned as-is. Fixes t0060-82 and t3004-5. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen --- setup.c | 30 ++ t/t0060-path-utils.sh | 2 +- t/t3004-ls-files-basic.sh | 2 +- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/setup.c b/setup.c index 906c8e2..ba08885 100644 --- a/setup.c +++ b/setup.c @@ -86,11 +86,17 @@ char *prefix_path_gently(const char *prefix, int len, const char *orig = path; char *sanitized; if (is_absolute_path(orig)) { - const char *temp = real_path(path); - sanitized = xmalloc(len + strlen(temp) + 1); - strcpy(sanitized, temp); + sanitized = xmalloc(strlen(path) + 1); if (remaining_prefix) *remaining_prefix = 0; + if (normalize_path_copy_len(sanitized, path, remaining_prefix)) { + free(sanitized); + return NULL; + } + if (abspath_part_inside_repo(sanitized)) { + free(sanitized); + return NULL; + } } else { sanitized = xmalloc(len + strlen(path) + 1); if (len) @@ -98,26 +104,10 @@ char *prefix_path_gently(const char *prefix, int len, strcpy(sanitized + len, path); if (remaining_prefix) *remaining_prefix = len; - } - if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) - goto error_out; - if (is_absolute_path(orig)) { - size_t root_len, len, total; - const char *work_tree = get_git_work_tree(); - if (!work_tree) - goto error_out; - len = strlen(work_tree); - root_len = offset_1st_component(work_tree); - total = strlen(sanitized) + 1; - if (strncmp(sanitized, work_tree, len) || - (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) { - error_out: + if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) { free(sanitized); return NULL; } - if (sanitized[len] == '/') - len++; - memmove(sanitized, sanitized + len, total - len); } return sanitized; } diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index c0a14f6..f04b82d 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test "$sym" = "$(test-path-utils real_path "$dir2/syml")" ' -test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' +test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' ln -s target symlink && test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink" ' diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh index e20c077..9c7adbd 100755 --- a/t/t3004-ls-files-basic.sh +++ b/t/t3004-ls-files-basic.sh @@ -36,7 +36,7 @@ test_expect_success 'ls-files -h in corrupt repository' ' test_i18ngrep "[Uu]sage: git ls-files " broken/usage ' -test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' ' +test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' ' mkdir subs && ln -s nosuch link && ln -s ../nosuch subs/link && -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 0/6] Handling of in-tree symlinks for absolute paths
The amount of tweaks seemed deserving of a reroll, so here it is: * Added your test Junio, with what I figured was an appropriate commit message describing the user-visible effect (to git-add since I think it's the simplest to explain), the commit message for the second commit has been reduced somewhat, to not duplicate the message completely. * Duplicated quite a bit of the explanation from this first commit into the last commit, in order to be more obvious about the issue it fixes, I'm not sure if it's a bit too much? * Reworded the last commit to not mention the full-path special case, and replaced all occurences of in-repo with "inside work tree" or similar. * Content-wise compared to 'pu' I've tweaked a few comments, un-inlined and added the wtlen <= len check (and the ls-files test is new of course): diff --git a/setup.c b/setup.c index 32a6f6b..ba08885 100644 --- a/setup.c +++ b/setup.c @@ -6,8 +6,8 @@ static int inside_git_dir = -1; static int inside_work_tree = -1; /* - * No checking if the path is in fact an absolute path is done, and it must - * already be normalized. + * The input parameter must contain an absolute path, and it must already be + * normalized. * * Find the part of an absolute path that lies inside the work tree by * dereferencing symlinks outside the work tree, for example: @@ -17,7 +17,7 @@ static int inside_work_tree = -1; * /dir/repolink/file (repolink points to /dir/repo) -> file * /dir/repo (exactly equal to work tree) -> (empty string) */ -static inline int abspath_part_inside_repo(char *path) +static int abspath_part_inside_repo(char *path) { size_t len; size_t wtlen; @@ -32,7 +32,7 @@ static inline int abspath_part_inside_repo(char *path) off = 0; /* check if work tree is already the prefix */ - if (strncmp(path, work_tree, wtlen) == 0) { + if (wtlen <= len && !strncmp(path, work_tree, wtlen)) { if (path[wtlen] == '/') { memmove(path, path + wtlen + 1, len - wtlen); return 0; @@ -47,7 +47,7 @@ static inline int abspath_part_inside_repo(char *path) path0 = path; path += offset_1st_component(path) + off; - /* check each level */ + /* check each '/'-terminated level */ while (*path) { path++; if (*path == '/') { Junio C Hamano (1): t3004: Add test for ls-files on symlinks via absolute paths Martin Erik Werner (5): t0060: Add test for prefix_path on symlinks via absolute paths t0060: Add test for prefix_path when path == work tree t0060: Add tests for prefix_path when path begins with work tree setup: Add 'abspath_part_inside_repo' function setup: Don't dereference in-tree symlinks for absolute paths setup.c | 94 +-- t/t0060-path-utils.sh | 21 +++ t/t3004-ls-files-basic.sh | 17 + 3 files changed, 112 insertions(+), 20 deletions(-) -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the repo, it is not possible to directly use real_path, since that would dereference symlinks both outside and inside the work tree. Add an 'abspath_part_inside_repo' function which first checks if the work tree is already the prefix, then incrementally checks each path level by temporarily NUL-terminating at each '/' and comparing against the work tree path. If a match is found, it overwrites the input path with the remainder past the work tree (which will be the part inside the work tree). This function is currently only intended for use in 'prefix_path_gently'. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen --- setup.c | 64 1 file changed, 64 insertions(+) diff --git a/setup.c b/setup.c index 5432a31..906c8e2 100644 --- a/setup.c +++ b/setup.c @@ -6,6 +6,70 @@ static int inside_git_dir = -1; static int inside_work_tree = -1; /* + * The input parameter must contain an absolute path, and it must already be + * normalized. + * + * Find the part of an absolute path that lies inside the work tree by + * dereferencing symlinks outside the work tree, for example: + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file + * /dir/file (work tree is /) -> dir/file + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2 + * /dir/repolink/file (repolink points to /dir/repo) -> file + * /dir/repo (exactly equal to work tree) -> (empty string) + */ +static int abspath_part_inside_repo(char *path) +{ + size_t len; + size_t wtlen; + char *path0; + int off; + + const char* work_tree = get_git_work_tree(); + if (!work_tree) + return -1; + wtlen = strlen(work_tree); + len = strlen(path); + off = 0; + + /* check if work tree is already the prefix */ + if (wtlen <= len && !strncmp(path, work_tree, wtlen)) { + if (path[wtlen] == '/') { + memmove(path, path + wtlen + 1, len - wtlen); + return 0; + } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') { + /* work tree is the root, or the whole path */ + memmove(path, path + wtlen, len - wtlen + 1); + return 0; + } + /* work tree might match beginning of a symlink to work tree */ + off = wtlen; + } + path0 = path; + path += offset_1st_component(path) + off; + + /* check each '/'-terminated level */ + while (*path) { + path++; + if (*path == '/') { + *path = '\0'; + if (strcmp(real_path(path0), work_tree) == 0) { + memmove(path0, path + 1, len - (path - path0)); + return 0; + } + *path = '/'; + } + } + + /* check whole path */ + if (strcmp(real_path(path0), work_tree) == 0) { + *path0 = '\0'; + return 0; + } + + return -1; +} + +/* * Normalize "path", prepending the "prefix" for relative paths. If * remaining_prefix is not NULL, return the actual prefix still * remains in the path. For example, prefix = sub1/sub2/ and path is -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe 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? git push triggers auto pack when gc.auto = 0
chris writes: > Ok, given your full response, I understand how this is being > conceptualized now, thanks. However, if you look at it purely from a > user's perspective who is manually invoking these commands for the > command's primary purpose, the current behavior is annoying. > > If we assume Git is right in implementing that no server async actions > are executed on behalf of a client action, then this falls under the > category of an ill-behaved server in my opinion. Anything a server > does that is not directly related to fulfilling the requested client > action is now considered bad behavior as it blocks the client from > continuing whatever it needs to get on with. I see such > implementation in Git as favoring server's needs over clients. There are no "server's needs" at all. Git only reacts to client requests. It is in the clients' own interest when garbage collection is periodically done since it improves response time. It's arguable that it would be nicer to use an incremental compaction process that hides the periodic costs by distributing them over the request totality. That replaces the periodic "why does it have to garbage collect when _I_ am using it" annoyance with "why is this generally slow". There is no net benefit to that approach safe for a) avoiding complaints of "smart" people who have discovered that they can speed up git by disabling garbage collection, but eventually find that git is becoming slow for them but not for others. b) avoiding these mailing list discussions. The second benefit could likely be achieved by displaying "Server unreachable... retrying..." instead of reporting about git gc. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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? git push triggers auto pack when gc.auto = 0
David Kastrup gnu.org> writes: > chris hotmail.com> writes: > > That said I would naively assume that a server side house keeping > > operation that does not get invoked with every client request be a > > nice candidate for asynchronous handling without any need to tell the > > client about it. > > Except that there are _no_ asynchronously handled repository actions > executed on behalf of a client action. If the repository owner decided > to disable demand-based garbage collection in favor of a cron job, > that's his call to make. It makes some sense when there are frequent > and multiple accesses to the repository since it avoids getting denied > access because of somebody _else_ triggering garbage collection > predominantly when times are busiest. > > Usually you are not denied access by your _own_ garbage collection since > the client waits until completion. > > It would be quite bad for scripting git if you constantly had to check > after every action whether any associated garbage collection might or > might not have completed. I can't comment for every use case, but I find it strange that a client script should need to care whether the server is currently garbage collecting or not. If such a detail must be exposed to a client, then I'd put forth that there is a deeper issue here. But any details there are moving well beyond the scope I'm able to comment on. That said, I think I understand you that it currently does matter in the sense that a client can't perform other actions while garbage collection is running. > Note also that when pushing without a separate server process (like when > pushing into a local repository), there is no other job which could be > responsible for packing the repository rather than the one doing the > push. Ok, given your full response, I understand how this is being conceptualized now, thanks. However, if you look at it purely from a user's perspective who is manually invoking these commands for the command's primary purpose, the current behavior is annoying. If we assume Git is right in implementing that no server async actions are executed on behalf of a client action, then this falls under the category of an ill-behaved server in my opinion. Anything a server does that is not directly related to fulfilling the requested client action is now considered bad behavior as it blocks the client from continuing whatever it needs to get on with. I see such implementation in Git as favoring server's needs over clients. Regards, Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug: relative core.worktree is resolved from symlink and not its target
Hi, when using a submodule "sm", there is a relative worktree in its config: .git/modules/sm/config: [core] worktree = ../../../smworktree git-new-worktree (from contrib) symlinks this config the new worktree. From inside the new worktree, git reads the config, but resolves the relative worktree setting based on the symlink's location. A fix would be to resolve any relative worktree setting based on the symlink target's location (the actual config file), and not from the symlink. This is with git version 1.8.5.3. Please consider fixing this. (I know about various workarounds, e.g. copying and adjusting "config" or manually setting $GIT_WORK_TREE; more relevant discussion would be at http://comments.gmane.org/gmane.comp.version-control.git/196019) Thanks, Daniel. -- http://daniel.hahler.de/ signature.asc Description: OpenPGP digital signature
Re: bug? git push triggers auto pack when gc.auto = 0
chris writes: > David Kastrup gnu.org> writes: >> chris hotmail.com> writes: >> > Duy Nguyen gmail.com> writes: >> >> On Tue, Feb 4, 2014 at 9:20 AM, chris hotmail.com> wrote: >> >> > $ git push origin next >> >> > Counting objects: 56, done. >> >> > Delta compression using up to 4 threads. >> >> > Compressing objects: 100% (9/9), done. >> >> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done. >> >> > Total 9 (delta 8), reused 0 (delta 0) >> >> > Auto packing the repository for optimum performance. >> >> > However, I question why I should even care about this message? I'm going >> > to >> > assume that simply it is a lengthy synchronous operation that someone felt >> > deserved some verbosity to why the client push action is taking longer than >> > it should. Yet that makes me question why I'm being penalized for this >> > server side operation. My client time should not be consumed for server >> > side house keeping. >> >> Your "client time" is not consumed: this is not a busy wait. Git server >> processes are synchronous: they are initiated and completed under the >> control of a client. That means that if you run a batch script >> executing a number of commands in a client, it will not saturate the >> server with half-finished processes and/or will refuse to honor requests >> because the repository is locked. > > I'm slightly confused by your response. You say "client time" is not > consumed, but then go on to say that git server processes are > synchronous to avoid build up from batched client requests. I expect > you took "client time" to have some specific technical meaning, while > I simply meant that the client command did not return until the server > completed its own house keeping. Until the server completed the house keeping initiated under the control of the client and on behalf of its command. > But I do think we are on the same page otherwise in that the client > command is blocked until the server process completes. Sure. > That said I would naively assume that a server side house keeping > operation that does not get invoked with every client request be a > nice candidate for asynchronous handling without any need to tell the > client about it. Except that there are _no_ asynchronously handled repository actions executed on behalf of a client action. If the repository owner decided to disable demand-based garbage collection in favor of a cron job, that's his call to make. It makes some sense when there are frequent and multiple accesses to the repository since it avoids getting denied access because of somebody _else_ triggering garbage collection predominantly when times are busiest. Usually you are not denied access by your _own_ garbage collection since the client waits until completion. It would be quite bad for scripting git if you constantly had to check after every action whether any associated garbage collection might or might not have completed. Note also that when pushing without a separate server process (like when pushing into a local repository), there is no other job which could be responsible for packing the repository rather than the one doing the push. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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? git push triggers auto pack when gc.auto = 0
David Kastrup gnu.org> writes: > chris hotmail.com> writes: > > Duy Nguyen gmail.com> writes: > >> On Tue, Feb 4, 2014 at 9:20 AM, chris hotmail.com> wrote: > >> > $ git push origin next > >> > Counting objects: 56, done. > >> > Delta compression using up to 4 threads. > >> > Compressing objects: 100% (9/9), done. > >> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done. > >> > Total 9 (delta 8), reused 0 (delta 0) > >> > Auto packing the repository for optimum performance. > > > However, I question why I should even care about this message? I'm going to > > assume that simply it is a lengthy synchronous operation that someone felt > > deserved some verbosity to why the client push action is taking longer than > > it should. Yet that makes me question why I'm being penalized for this > > server side operation. My client time should not be consumed for server > > side house keeping. > > Your "client time" is not consumed: this is not a busy wait. Git server > processes are synchronous: they are initiated and completed under the > control of a client. That means that if you run a batch script > executing a number of commands in a client, it will not saturate the > server with half-finished processes and/or will refuse to honor requests > because the repository is locked. I'm slightly confused by your response. You say "client time" is not consumed, but then go on to say that git server processes are synchronous to avoid build up from batched client requests. I expect you took "client time" to have some specific technical meaning, while I simply meant that the client command did not return until the server completed its own house keeping. But I do think we are on the same page otherwise in that the client command is blocked until the server process completes. That said I would naively assume that a server side house keeping operation that does not get invoked with every client request be a nice candidate for asynchronous handling without any need to tell the client about it. Regards, Chris -- To unsubscribe from this list: send the line "unsubscribe 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? git push triggers auto pack when gc.auto = 0
chris writes: > Duy Nguyen gmail.com> writes: >> On Tue, Feb 4, 2014 at 9:20 AM, chris hotmail.com> wrote: >> > $ git push origin next >> > Counting objects: 56, done. >> > Delta compression using up to 4 threads. >> > Compressing objects: 100% (9/9), done. >> > Writing objects: 100% (9/9), 895 bytes | 0 bytes/s, done. >> > Total 9 (delta 8), reused 0 (delta 0) >> > Auto packing the repository for optimum performance. > However, I question why I should even care about this message? I'm going to > assume that simply it is a lengthy synchronous operation that someone felt > deserved some verbosity to why the client push action is taking longer than > it should. Yet that makes me question why I'm being penalized for this > server side operation. My client time should not be consumed for server > side house keeping. Your "client time" is not consumed: this is not a busy wait. Git server processes are synchronous: they are initiated and completed under the control of a client. That means that if you run a batch script executing a number of commands in a client, it will not saturate the server with half-finished processes and/or will refuse to honor requests because the repository is locked. > An obvious fix is to disable gc on the server and implement a cron job > for the house keeping task. However, as often the case one does not > have control over the server, so it is unfortunate that git has this > server side house keeping as a blocking operation to a client action. _Any_ git operation is "blocking" the respective initiating client. >> > So my question is, should gc.auto = 0 disable auto-packing from occurring >> > on >> > git push and other non-gc commands? >> >> Yes it should. > > Thanks for the confirmation. And indeed, there is no autopacking occuring on your site when doing git push. The server administrator will be rather glad that the clients' configuration variables don't affect his server's operation. -- David Kastrup -- To unsubscribe from this list: send the line "unsubscribe 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? git push triggers auto pack when gc.auto = 0
Duy Nguyen gmail.com> writes: > On Tue, Feb 4, 2014 at 12:13 PM, chris hotmail.com> wrote: > > However, I question why I should even care about this message? I'm going to > > assume that simply it is a lengthy synchronous operation that someone felt > > deserved some verbosity to why the client push action is taking longer than > > it should. Yet that makes me question why I'm being penalized for this > > server side operation. My client time should not be consumed for server > > side house keeping. > > > > An obvious fix is to disable gc on the server and implement a cron job for > > the house keeping task. However, as often the case one does not have > > control over the server, so it is unfortunate that git has this server side > > house keeping as a blocking operation to a client action. > > I agree it should not block the client. I think you can Ctrl-C "git > push" at this point without losing anything (data has already been > pushed at this point) but that's not a good advice to general cases. > Maybe we can do something at the server side to not block the client.. I'd like to avoid a Ctrl-C approach, but if an indication existed that assured me the push part of the operation had completed successfully, then that would be sufficient for when I'm impatient. > Another thing we could do is put "remote: " in front of these strings, > even in ssh case. They seem to confuse you (and me too) that things > happened locally. Yes, I would like to see more explicit clarity in what messages are coming from the server. That has always been a source of uncertainty for me with any remote git command output. Thanks for the patches and attention to this issue, I appreciate it. Chris -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html