summaryrefslogtreecommitdiff
Commit message (Collapse)AuthorAgeFilesLines
* xdiff: rename "struct group" to "struct xdlgroup"mh/diff-indent-heuristicJeff King2016-09-271-7/+7
| | | | | | | | | | | | | | | | | | | | | Commit e8adf23 (xdl_change_compact(): introduce the concept of a change group, 2016-08-22) added a "struct group" type to xdiff/xdiffi.c. But the POSIX system header "grp.h" already defines "struct group" (it is part of the getgrnam interface). This happens to work because the new type is local to xdiffi.c, and the xdiff code includes a relatively small set of system headers. But it will break compilation if xdiff ever switches to using git-compat-util.h. It can also probably cause confusion with tools that look at the whole code base, like coccinelle or ctags. Let's resolve by giving the xdiff variant a scoped name, which is closer to other xdiff types anyway (e.g., xdlfile_t, though note that xdiff is fond if typedefs when Git usually is not). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* blame: honor the diff heuristic options and configMichael Haggerty2016-09-198-19/+70
| | | | | | | | | | | | | | | | | Teach "git blame" and "git annotate" the --compaction-heuristic and --indent-heuristic options that are now supported by "git diff". Also teach them to honor the `diff.compactionHeuristic` and `diff.indentHeuristic` configuration options. It would be conceivable to introduce separate configuration options for "blame" and "annotate"; for example `blame.compactionHeuristic` and `blame.indentHeuristic`. But it would be confusing to users if blame output is inconsistent with diff output, so it makes more sense for them to respect the same configuration. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* parse-options: add parse_opt_unknown_cb()Michael Haggerty2016-09-192-0/+13
| | | | | | | | | | Add a new callback function, parse_opt_unknown_cb(), which returns -2 to indicate that the corresponding option is unknown. This can be used to add "-h" documentation for an option that will be handled externally to parse_options(). Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* diff: improve positioning of add/delete blocks in diffsMichael Haggerty2016-09-197-11/+546
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Some groups of added/deleted lines in diffs can be slid up or down, because lines at the edges of the group are not unique. Picking good shifts for such groups is not a matter of correctness but definitely has a big effect on aesthetics. For example, consider the following two diffs. The first is what standard Git emits: --- a/9c572b21dd090a1e5c5bb397053bf8043ffe7fb4:git-send-email.perl +++ b/6dcfa306f2b67b733a7eb2d7ded1bc9987809edb:git-send-email.perl @@ -231,6 +231,9 @@ if (!defined $initial_reply_to && $prompting) { } if (!$smtp_server) { + $smtp_server = $repo->config('sendemail.smtpserver'); +} +if (!$smtp_server) { foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) { if (-x $_) { $smtp_server = $_; The following diff is equivalent, but is obviously preferable from an aesthetic point of view: --- a/9c572b21dd090a1e5c5bb397053bf8043ffe7fb4:git-send-email.perl +++ b/6dcfa306f2b67b733a7eb2d7ded1bc9987809edb:git-send-email.perl @@ -230,6 +230,9 @@ if (!defined $initial_reply_to && $prompting) { $initial_reply_to =~ s/(^\s+|\s+$)//g; } +if (!$smtp_server) { + $smtp_server = $repo->config('sendemail.smtpserver'); +} if (!$smtp_server) { foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) { if (-x $_) { This patch teaches Git to pick better positions for such "diff sliders" using heuristics that take the positions of nearby blank lines and the indentation of nearby lines into account. The existing Git code basically always shifts such "sliders" as far down in the file as possible. The only exception is when the slider can be aligned with a group of changed lines in the other file, in which case Git favors depicting the change as one add+delete block rather than one add and a slightly offset delete block. This naive algorithm often yields ugly diffs. Commit d634d61ed6 improved the situation somewhat by preferring to position add/delete groups to make their last line a blank line, when that is possible. This heuristic does more good than harm, but (1) it can only help if there are blank lines in the right places, and (2) always picks the last blank line, even if there are others that might be better. The end result is that it makes perhaps 1/3 as many errors as the default Git algorithm, but that still leaves a lot of ugly diffs. This commit implements a new and much better heuristic for picking optimal "slider" positions using the following approach: First observe that each hypothetical positioning of a diff slider introduces two splits: one between the context lines preceding the group and the first added/deleted line, and the other between the last added/deleted line and the first line of context following it. It tries to find the positioning that creates the least bad splits. Splits are evaluated based only on the presence and locations of nearby blank lines, and the indentation of lines near the split. Basically, it prefers to introduce splits adjacent to blank lines, between lines that are indented less, and between lines with the same level of indentation. In more detail: 1. It measures the following characteristics of a proposed splitting position in a `struct split_measurement`: * the number of blank lines above the proposed split * whether the line directly after the split is blank * the number of blank lines following that line * the indentation of the nearest non-blank line above the split * the indentation of the line directly below the split * the indentation of the nearest non-blank line after that line 2. It combines the measured attributes using a bunch of empirically-optimized weighting factors to derive a `struct split_score` that measures the "badness" of splitting the text at that position. 3. It combines the `split_score` for the top and the bottom of the slider at each of its possible positions, and selects the position that has the best `split_score`. I determined the initial set of weighting factors by collecting a corpus of Git histories from 29 open-source software projects in various programming languages. I generated many diffs from this corpus, and determined the best positioning "by eye" for about 6600 diff sliders. I used about half of the repositories in the corpus (corresponding to about 2/3 of the sliders) as a training set, and optimized the weights against this corpus using a crude automated search of the parameter space to get the best agreement with the manually-determined values. Then I tested the resulting heuristic against the full corpus. The results are summarized in the following table, in column `indent-1`: | repository | count | Git 2.9.0 | compaction | compaction-fixed | indent-1 | indent-2 | | --------------------- | ----- | -------------- | -------------- | ---------------- | -------------- | -------------- | | afnetworking | 109 | 89 (81.7%) | 37 (33.9%) | 37 (33.9%) | 2 (1.8%) | 2 (1.8%) | | alamofire | 30 | 18 (60.0%) | 14 (46.7%) | 15 (50.0%) | 0 (0.0%) | 0 (0.0%) | | angular | 184 | 127 (69.0%) | 39 (21.2%) | 23 (12.5%) | 5 (2.7%) | 5 (2.7%) | | animate | 313 | 2 (0.6%) | 2 (0.6%) | 2 (0.6%) | 2 (0.6%) | 2 (0.6%) | | ant | 380 | 356 (93.7%) | 152 (40.0%) | 148 (38.9%) | 15 (3.9%) | 15 (3.9%) | * | bugzilla | 306 | 263 (85.9%) | 109 (35.6%) | 99 (32.4%) | 14 (4.6%) | 15 (4.9%) | * | corefx | 126 | 91 (72.2%) | 22 (17.5%) | 21 (16.7%) | 6 (4.8%) | 6 (4.8%) | | couchdb | 78 | 44 (56.4%) | 26 (33.3%) | 28 (35.9%) | 6 (7.7%) | 6 (7.7%) | * | cpython | 937 | 158 (16.9%) | 50 (5.3%) | 49 (5.2%) | 5 (0.5%) | 5 (0.5%) | * | discourse | 160 | 95 (59.4%) | 42 (26.2%) | 36 (22.5%) | 18 (11.2%) | 13 (8.1%) | | docker | 307 | 194 (63.2%) | 198 (64.5%) | 253 (82.4%) | 8 (2.6%) | 8 (2.6%) | * | electron | 163 | 132 (81.0%) | 38 (23.3%) | 39 (23.9%) | 6 (3.7%) | 6 (3.7%) | | git | 536 | 470 (87.7%) | 73 (13.6%) | 78 (14.6%) | 16 (3.0%) | 16 (3.0%) | * | gitflow | 127 | 0 (0.0%) | 0 (0.0%) | 0 (0.0%) | 0 (0.0%) | 0 (0.0%) | | ionic | 133 | 89 (66.9%) | 29 (21.8%) | 38 (28.6%) | 1 (0.8%) | 1 (0.8%) | | ipython | 482 | 362 (75.1%) | 167 (34.6%) | 169 (35.1%) | 11 (2.3%) | 11 (2.3%) | * | junit | 161 | 147 (91.3%) | 67 (41.6%) | 66 (41.0%) | 1 (0.6%) | 1 (0.6%) | * | lighttable | 15 | 5 (33.3%) | 0 (0.0%) | 2 (13.3%) | 0 (0.0%) | 0 (0.0%) | | magit | 88 | 75 (85.2%) | 11 (12.5%) | 9 (10.2%) | 1 (1.1%) | 0 (0.0%) | | neural-style | 28 | 0 (0.0%) | 0 (0.0%) | 0 (0.0%) | 0 (0.0%) | 0 (0.0%) | | nodejs | 781 | 649 (83.1%) | 118 (15.1%) | 111 (14.2%) | 4 (0.5%) | 5 (0.6%) | * | phpmyadmin | 491 | 481 (98.0%) | 75 (15.3%) | 48 (9.8%) | 2 (0.4%) | 2 (0.4%) | * | react-native | 168 | 130 (77.4%) | 79 (47.0%) | 81 (48.2%) | 0 (0.0%) | 0 (0.0%) | | rust | 171 | 128 (74.9%) | 30 (17.5%) | 27 (15.8%) | 16 (9.4%) | 14 (8.2%) | | spark | 186 | 149 (80.1%) | 52 (28.0%) | 52 (28.0%) | 2 (1.1%) | 2 (1.1%) | | tensorflow | 115 | 66 (57.4%) | 48 (41.7%) | 48 (41.7%) | 5 (4.3%) | 5 (4.3%) | | test-more | 19 | 15 (78.9%) | 2 (10.5%) | 2 (10.5%) | 1 (5.3%) | 1 (5.3%) | * | test-unit | 51 | 34 (66.7%) | 14 (27.5%) | 8 (15.7%) | 2 (3.9%) | 2 (3.9%) | * | xmonad | 23 | 22 (95.7%) | 2 (8.7%) | 2 (8.7%) | 1 (4.3%) | 1 (4.3%) | * | --------------------- | ----- | -------------- | -------------- | ---------------- | -------------- | -------------- | | totals | 6668 | 4391 (65.9%) | 1496 (22.4%) | 1491 (22.4%) | 150 (2.2%) | 144 (2.2%) | | totals (training set) | 4552 | 3195 (70.2%) | 1053 (23.1%) | 1061 (23.3%) | 86 (1.9%) | 88 (1.9%) | | totals (test set) | 2116 | 1196 (56.5%) | 443 (20.9%) | 430 (20.3%) | 64 (3.0%) | 56 (2.6%) | In this table, the numbers are the count and percentage of human-rated sliders that the corresponding algorithm got *wrong*. The columns are * "repository" - the name of the repository used. I used the diffs between successive non-merge commits on the HEAD branch of the corresponding repository. * "count" - the number of sliders that were human-rated. I chose most, but not all, sliders to rate from those among which the various algorithms gave different answers. * "Git 2.9.0" - the default algorithm used by `git diff` in Git 2.9.0. * "compaction" - the heuristic used by `git diff --compaction-heuristic` in Git 2.9.0. * "compaction-fixed" - the heuristic used by `git diff --compaction-heuristic` after the fixes from earlier in this patch series. Note that the results are not dramatically different than those for "compaction". Both produce non-ideal diffs only about 1/3 as often as the default `git diff`. * "indent-1" - the new `--indent-heuristic` algorithm, using the first set of weighting factors, determined as described above. * "indent-2" - the new `--indent-heuristic` algorithm, using the final set of weighting factors, determined as described below. * `*` - indicates that repo was part of training set used to determine the first set of weighting factors. The fact that the heuristic performed nearly as well on the test set as on the training set in column "indent-1" is a good indication that the heuristic was not over-trained. Given that fact, I ran a second round of optimization, using the entire corpus as the training set. The resulting set of weights gave the results in column "indent-2". These are the weights included in this patch. The final result gives consistently and significantly better results across the whole corpus than either `git diff` or `git diff --compaction-heuristic`. It makes only about 1/30 as many errors as the former and about 1/10 as many errors as the latter. (And a good fraction of the remaining errors are for diffs that involve weirdly-formatted code, sometimes apparently machine-generated.) The tools that were used to do this optimization and analysis, along with the human-generated data values, are recorded in a separate project [1]. This patch adds a new command-line option `--indent-heuristic`, and a new configuration setting `diff.indentHeuristic`, that activate this heuristic. This interface is only meant for testing purposes, and should be finalized before including this change in any release. [1] https://github.com/mhagger/diff-slider-tools Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* xdl_change_compact(): introduce the concept of a change groupMichael Haggerty2016-08-231-90/+203
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The idea of xdl_change_compact() is fairly simple: * Proceed through groups of changed lines in the file to be compacted, keeping track of the corresponding location in the "other" file. * If possible, slide the group up and down to try to give the most aesthetically pleasing diff. Whenever it is slid, the current location in the other file needs to be adjusted. But these simple concepts are obfuscated by a lot of index handling that is written in terse, subtle, and varied patterns. I found it very hard to convince myself that the function was correct. So introduce a "struct group" that represents a group of changed lines in a file. Add some functions that perform elementary operations on groups: * Initialize a group to the first group in a file * Move to the next or previous group in a file * Slide a group up or down Even though the resulting code is longer, I think it is easier to understand and review. Its performance is not changed appreciably (though it would be if `group_next()` and `group_previous()` were not inlined). ...and in fact, the rewriting helped me discover another bug in the --compaction-heuristic code: The update of blank_lines was never done for the highest possible position of the group. This means that it could fail to slide the group to its highest possible position, even if that position had a blank line as its last line. So for example, it yielded the following diff: $ git diff --no-index --compaction-heuristic a.txt b.txt diff --git a/a.txt b/b.txt index e53969f..0d60c5fe 100644 --- a/a.txt +++ b/b.txt @@ -1,3 +1,7 @@ 1 A + +B + +A 2 when in fact the following diff is better (according to the rules of --compaction-heuristic): $ git diff --no-index --compaction-heuristic a.txt b.txt diff --git a/a.txt b/b.txt index e53969f..0d60c5fe 100644 --- a/a.txt +++ b/b.txt @@ -1,3 +1,7 @@ 1 +A + +B + A 2 The new code gives the bottom answer. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* recs_match(): take two xrecord_t pointers as argumentsMichael Haggerty2016-08-231-7/+7
| | | | | | | | There is no reason for it to take an array and two indexes as argument, as it only accesses two elements of the array. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* is_blank_line(): take a single xrecord_t as argumentMichael Haggerty2016-08-231-4/+4
| | | | | | | | There is no reason for it to take an array and index as argument, as it only accesses a single element of the array. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* xdl_change_compact(): only use heuristic if group can't be matchedMichael Haggerty2016-08-231-19/+19
| | | | | | | | | | | | | If the changed group of lines can be matched to a group in the other file, then that positioning should take precedence over the compaction heuristic. The old code tried the heuristic unconditionally, which cost redundant effort and also was broken if the matching code had already shifted the group higher than the blank line. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* xdl_change_compact(): fix compaction heuristic to adjust ixoMichael Haggerty2016-08-231-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | The code branch used for the compaction heuristic forgot to keep ixo in sync while the group was shifted. This is certainly wrong, as it causes the two counters to get out of sync. I *think* that this bug could also have caused the function to read past the end of the rchgo array, though I haven't done the work to prove it for sure. Here is my reasoning: If ixo is not decremented correctly during one iteration of the outer while loop, then it will loose sync with the ix counter. In particular, ixo will be too large. Suppose that the next iterations of the outer while loop (i.e., processing the next block of add/delete lines) don't have any sliders. Then the ixo counter would be incremented by the number of non-changed lines in xdf, which is the same as the number of non-changed lines in xdfo that *should have* followed the group that experienced the malfunction. But since ixo was too large at the end of that iteration, it will be incremented past the end of the xdfo->rchg array, and will try to read that memory illegally. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* Merge branch 'jk/add-i-diff-compact-heuristics'Junio C Hamano2016-07-061-0/+4
|\ | | | | | | | | | | | | | | | | "git add -i/-p" learned to honor diff.compactionHeuristic experimental knob, so that the user can work on the same hunk split as "git diff" output. * jk/add-i-diff-compact-heuristics: add--interactive: respect diff.compactionHeuristic
| * add--interactive: respect diff.compactionHeuristicjk/add-i-diff-compact-heuristicsJeff King2016-06-161-0/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | We use plumbing to generate the diff, so it doesn't automatically pick up UI config like compactionHeuristic. Let's forward it on, since interactive adding is porcelain. Note that we only need to handle the "true" case. There's no point in passing --no-compaction-heuristic when the variable is false, since nothing else could have turned it on. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | Merge branch 'km/fetch-do-not-free-remote-name'Junio C Hamano2016-07-061-4/+2
|\ \ | | | | | | | | | | | | | | | | | | | | | The ownership rule for the piece of memory that hold references to be fetched in "git fetch" was screwy, which has been cleaned up. * km/fetch-do-not-free-remote-name: builtin/fetch.c: don't free remote->name after fetch
| * | builtin/fetch.c: don't free remote->name after fetchkm/fetch-do-not-free-remote-nameKeith McGuigan2016-06-141-4/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | Make fetch's string_list of remote names own all of its string items (strdup'ing when necessary) so that it can deallocate them safely when clearing. Signed-off-by: Keith McGuigan <kmcguigan@twopensource.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | Merge branch 'nd/test-lib-httpd-show-error-log-in-verbose'Junio C Hamano2016-07-061-0/+1
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | HTTPd tests learned to show the server error log to help diagnosing a failing tests. * nd/test-lib-httpd-show-error-log-in-verbose: lib-httpd.sh: print error.log on error
| * | | lib-httpd.sh: print error.log on errornd/test-lib-httpd-show-error-log-in-verboseNguyễn Thái Ngọc Duy2016-06-131-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Failure to bring up httpd for testing is not considered an error, so the trash directory, which contains this error.log file, is removed and we don't know what made httpd fail to start. Improve the situation a bit, print error.log but only in verbose mode. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | Merge branch 'jk/string-list-static-init'Junio C Hamano2016-07-0611-21/+24
|\ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Instead of taking advantage of a struct string_list that is allocated with all NULs happens to be STRING_LIST_INIT_NODUP kind, initialize them explicitly as such, to document their behaviour better. * jk/string-list-static-init: use string_list initializer consistently blame,shortlog: don't make local option variables static interpret-trailers: don't duplicate option strings parse_opt_string_list: stop allocating new strings
| * | | | use string_list initializer consistentlyjk/string-list-static-initJeff King2016-06-138-13/+13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | There are two types of string_lists: those that own the string memory, and those that don't. You can tell the difference by the strdup_strings flag, and one should use either STRING_LIST_INIT_DUP, or STRING_LIST_INIT_NODUP as an initializer. Historically, the normal all-zeros initialization has corresponded to the NODUP case. Many sites use no initializer at all, and that works as a shorthand for that case. But for a reader of the code, it can be hard to remember which is which. Let's be more explicit and actually have each site declare which type it means to use. This is a fairly mechanical conversion; I assumed each site was correct as-is, and just switched them all to NODUP. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | | | Merge branch 'jk/parseopt-string-list' into jk/string-list-static-initJunio C Hamano2016-06-134-11/+11
| |\ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * jk/parseopt-string-list: blame,shortlog: don't make local option variables static interpret-trailers: don't duplicate option strings parse_opt_string_list: stop allocating new strings
| | * | | | blame,shortlog: don't make local option variables staticJeff King2016-06-132-9/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | There's no need for these option variables to be static, except that they are referenced by the options array itself, which is static. But having all of this static is simply unnecessary and confusing (and inconsistent with most other commands, which either use a static global option list or a true function-local one). Note that in some cases we may need to actually initialize the variables (since we cannot rely on BSS to do so). This is a net improvement to readability, though, as we can use the more verbose initializers for our string_lists. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| | * | | | interpret-trailers: don't duplicate option stringsJeff King2016-06-131-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | There's no need to do so; the argv strings will last until the end of the program. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| | * | | | parse_opt_string_list: stop allocating new stringsJeff King2016-06-131-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The parse_opt_string_list callback is basically a thin wrapper to string_list_append() any string options we get. However, it calls: string_list_append(v, xstrdup(arg)); which duplicates the option value. This is wrong for two reasons: 1. If the string list has strdup_strings set, then we are making an extra copy, which is simply leaked. 2. If the string list does not have strdup_strings set, then we pass memory ownership to the string list, but it does not realize this. If we later call string_list_clear(), which can happen if "--no-foo" is passed, then we will leak all of the existing entries. Instead, we should just pass the argument straight to string_list_append, and it can decide whether to copy or not based on its strdup_strings flag. It's possible that some (buggy) caller could be relying on this extra copy (e.g., because it parses some options from an allocated argv array and then frees the array), but it's not likely. For one, we generally only use parse_options on the argv given to us in main(). And two, such a caller is broken anyway, because other option types like OPT_STRING() do not make such a copy. This patch brings us in line with them. Noticed-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | Merge branch 'jk/send-pack-stdio'Junio C Hamano2016-07-063-29/+16
|\ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Code clean-up. * jk/send-pack-stdio: write_or_die: remove the unused write_or_whine() function send-pack: use buffered I/O to talk to pack-objects
| * | | | | | write_or_die: remove the unused write_or_whine() functionjk/send-pack-stdioRamsay Jones2016-06-102-12/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Now the last caller of this function is gone, and new ones are unlikely to appear, because this function is doing very little that a regular if() does not besides obfuscating the error message (and if we ever did want something like it, we would probably prefer the function to come back with more "normal" return value semantics). Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | | | | | send-pack: use buffered I/O to talk to pack-objectsJeff King2016-06-081-17/+16
| | |_|_|/ / | |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We start a pack-objects process and then write all of the positive and negative sha1s to it over a pipe. We do so by formatting each item into a fixed-size buffer and then writing each individually. This has two drawbacks: 1. There's some manual computation of the buffer size, which is not immediately obvious is correct (though it is). 2. We write() once per sha1, which means a lot more system calls than are necessary. We can solve both by wrapping the pipe descriptor in a stdio handle; this is the same technique used by upload-pack when serving fetches. Note that we can also simplify and improve the error handling here. The original detected a single write error and broke out of the loop (presumably to avoid writing the error message over and over), but never actually acted on seeing an error; we just fed truncated input and took whatever pack-objects returned. In practice, this probably didn't matter, as the likely errors would be caused by pack-objects dying (and we'd probably just die with SIGPIPE anyway). But we can easily make this simpler and more robust; the stdio handle keeps an error flag, which we can check at the end. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | Merge branch 'pb/commit-editmsg-path'Junio C Hamano2016-07-061-7/+8
|\ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Code clean-up. * pb/commit-editmsg-path: builtin/commit.c: memoize git-path for COMMIT_EDITMSG
| * | | | | | builtin/commit.c: memoize git-path for COMMIT_EDITMSGpb/commit-editmsg-pathPranit Bauva2016-06-091-7/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This is a follow up commit for f932729c (memoize common git-path "constant" files, 10-Aug-2015). The many function calls to git_path() are replaced by git_path_commit_editmsg() and which thus eliminates the need to repeatedly compute the location of "COMMIT_EDITMSG". Mentored-by: Lars Schneider <larsxschneider@gmail.com> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | Merge branch 'ep/http-curl-trace'Junio C Hamano2016-07-064-2/+133
|\ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | HTTP transport gained an option to produce more detailed debugging trace. * ep/http-curl-trace: imap-send.c: introduce the GIT_TRACE_CURL enviroment variable http.c: implement the GIT_TRACE_CURL environment variable
| * | | | | | | imap-send.c: introduce the GIT_TRACE_CURL enviroment variableep/http-curl-traceElia Pinto2016-05-241-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Permit the use of the GIT_TRACE_CURL environment variable calling the setup_curl_trace http.c helper routine. Helped-by: Torsten Bögershausen <tboegi@web.de> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | | | | | | http.c: implement the GIT_TRACE_CURL environment variableElia Pinto2016-05-243-2/+132
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Implement the GIT_TRACE_CURL environment variable to allow a greater degree of detail of GIT_CURL_VERBOSE, in particular the complete transport header and all the data payload exchanged. It might be useful if a particular situation could require a more thorough debugging analysis. Document the new GIT_TRACE_CURL environment variable. Helped-by: Torsten Bögershausen <tboegi@web.de> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | Second batch of topics for 2.10Junio C Hamano2016-06-271-0/+65
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | Sync with maintJunio C Hamano2016-06-271-0/+28
|\ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * maint: Start preparing for 2.9.1
| * | | | | | | | Start preparing for 2.9.1Junio C Hamano2016-06-272-1/+29
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | | | | | | | Merge branch 'rs/xdiff-hunk-with-func-line' into maintJunio C Hamano2016-06-2710-93/+365
| |\ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git show -W" (extend hunks to cover the entire function, delimited by lines that match the "funcname" pattern) used to show the entire file when a change added an entire function at the end of the file, which has been fixed. * rs/xdiff-hunk-with-func-line: xdiff: fix merging of appended hunk with -W grep: -W: don't extend context to trailing empty lines t7810: add test for grep -W and trailing empty context lines xdiff: don't trim common tail with -W xdiff: -W: don't include common trailing empty lines in context xdiff: ignore empty lines before added functions with -W xdiff: handle appended chunks better with -W xdiff: factor out match_func_rec() t4051: rewrite, add more tests
| * \ \ \ \ \ \ \ \ Merge branch 'jk/rev-list-count-with-bitmap' into maintJunio C Hamano2016-06-272-1/+11
| |\ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git rev-list --count" whose walk-length is limited with "-n" option did not work well with the counting optimized to look at the bitmap index. * jk/rev-list-count-with-bitmap: rev-list: disable bitmaps when "-n" is used with listing objects rev-list: "adjust" results of "--count --use-bitmap-index -n"
| * \ \ \ \ \ \ \ \ \ Merge branch 'et/pretty-format-c-auto' into maintJunio C Hamano2016-06-272-8/+20
| |\ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The commands in `git log` family take %C(auto) in a custom format string. This unconditionally turned the color on, ignoring --no-color or with --color=auto when the output is not connected to a tty; this was corrected to make the format truly behave as "auto". * et/pretty-format-c-auto: format_commit_message: honor `color=auto` for `%C(auto)`
| * \ \ \ \ \ \ \ \ \ \ Merge branch 'ew/daemon-socket-keepalive' into maintJunio C Hamano2016-06-271-0/+14
| |\ \ \ \ \ \ \ \ \ \ \ | | |_|_|_|_|_|_|_|_|/ / | |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When "git daemon" is run without --[init-]timeout specified, a connection from a client that silently goes offline can hang around for a long time, wasting resources. The socket-level KEEPALIVE has been enabled to allow the OS to notice such failed connections. * ew/daemon-socket-keepalive: daemon: enable SO_KEEPALIVE for all sockets
* | | | | | | | | | | | Merge branch 'tb/complete-status'Junio C Hamano2016-06-271-1/+97
|\ \ \ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The completion script (in contrib/) learned to complete "git status" options. * tb/complete-status: completion: add git status completion: add __git_get_option_value helper completion: factor out untracked file modes into a variable
| * | | | | | | | | | | | completion: add git statustb/complete-statusThomas Braun2016-06-101-0/+50
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | | | | | | | | | | | completion: add __git_get_option_value helperThomas Braun2016-06-101-0/+44
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This function allows to search the commmand line and config files for an option, long and short, with mandatory value. The function would return e.g. for the command line "git status -uno --untracked-files=all" the result "all" regardless of the config option. Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
| * | | | | | | | | | | | completion: factor out untracked file modes into a variableThomas Braun2016-06-101-1/+3
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Signed-off-by: Thomas Braun <thomas.braun@virtuell-zuhause.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | | | | | | Merge branch 'mg/cherry-pick-multi-on-unborn'Junio C Hamano2016-06-271-5/+6
|\ \ \ \ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git cherry-pick A" worked on an unborn branch, but "git cherry-pick A..B" didn't. * mg/cherry-pick-multi-on-unborn: cherry-pick: allow to pick to unborn branches
| * | | | | | | | | | | | | cherry-pick: allow to pick to unborn branchesmg/cherry-pick-multi-on-unbornMichael J Gruber2016-06-061-5/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | cherry-pick allows to pick single commits to an empty HEAD, but not multiple commits. Allow the multiple commit case, too. Reported-by: Fabrizio Cucci <fabrizio.cucci@gmail.com> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | | | | | | | Merge branch 'lf/receive-pack-auto-gc-to-client'Junio C Hamano2016-06-271-2/+13
|\ \ \ \ \ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Allow messages that are generated by auto gc during "git push" on the receiving end to be explicitly passed back to the sending end over sideband, so that they are shown with "remote: " prefix to avoid confusing the users. * lf/receive-pack-auto-gc-to-client: receive-pack: send auto-gc output over sideband 2
| * | | | | | | | | | | | | | receive-pack: send auto-gc output over sideband 2lf/receive-pack-auto-gc-to-clientLukas Fleischer2016-06-061-2/+13
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Redirect auto-gc output to the sideband such that it is visible to all clients. As a side effect, all auto-gc error messages are now prefixed with "remote: " before being printed to stderr on the client-side which makes it easier to understand that those error messages originate from the server. Signed-off-by: Lukas Fleischer <lfleischer@lfos.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | | | | | | | | Merge branch 'em/newer-freebsd-shells-are-fine-with-returns'Junio C Hamano2016-06-273-6/+6
|\ \ \ \ \ \ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Comments about misbehaving FreeBSD shells have been clarified with the version number (9.x and before are broken, newer ones are OK). * em/newer-freebsd-shells-are-fine-with-returns: rebase: update comment about FreeBSD /bin/sh
| * | | | | | | | | | | | | | | rebase: update comment about FreeBSD /bin/shem/newer-freebsd-shells-are-fine-with-returnsEd Maste2016-06-173-6/+6
| | |/ / / / / / / / / / / / / | |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Commit 9f50d32 introduced a fix for FreeBSD /bin/sh misbehaviour when dot-sourcing a file containing "return" statements outside of any function, from a function in another shell script. That issue affects FreeBSD 9.x, and is not present in the /bin/sh in FreeBSD 10.3 and later. Update the comment to clarify this. The example from 9f50d32's commit message produces the expected output on FreeBSD 10.3 and -CURRENT (the upcoming 11.0): % sh script1.sh only this line should show % Signed-off-by: Ed Maste <emaste@freebsd.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | | | | | | | | Merge branch 'lv/status-say-working-tree-not-directory'Junio C Hamano2016-06-271-1/+1
|\ \ \ \ \ \ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | "git status" used to say "working directory" when it meant "working tree". * lv/status-say-working-tree-not-directory: Use "working tree" instead of "working directory" for git status
| * | | | | | | | | | | | | | | Use "working tree" instead of "working directory" for git statuslv/status-say-working-tree-not-directoryLars Vogel2016-06-091-1/+1
| | |_|_|_|_|_|_|_|_|/ / / / / | |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Working directory can be easily confused with the current directory. In one of my patches I already updated the usage of working directory with working tree for the man page but I noticed that git status also uses this incorrect term. Signed-off-by: Lars Vogel <Lars.Vogel@vogella.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* | | | | | | | | | | | | | | Merge branch 'nb/gnome-keyring-build'Junio C Hamano2016-06-271-2/+3
|\ \ \ \ \ \ \ \ \ \ \ \ \ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Build improvements for gnome-keyring (in contrib/) * nb/gnome-keyring-build: gnome-keyring: Don't hard-code pkg-config executable
| * | | | | | | | | | | | | | | gnome-keyring: Don't hard-code pkg-config executablenb/gnome-keyring-buildHeiko Becker2016-06-141-2/+3
| | |/ / / / / / / / / / / / / | |/| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Helpful if your pkg-config executable has a prefix based on the architecture, for example. Signed-off-by: Heiko Becker <heirecka@exherbo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>