[PATCH v2 0/1] [Outreachy] config: move documentation to config.h
Move the documentation from Documentation/technical/api-config.txt into config.h Signed-off-by: Heba Waly heba.w...@gmail.com [heba.w...@gmail.com] Thanks for taking the time to contribute to Git! Please be advised that the Git community does not use github.com for their contributions. Instead, we use a mailing list (git@vger.kernel.org) for code submissions, code reviews, and bug reports. Nevertheless, you can use GitGitGadget ( https://gitgitgadget.github.io/) to conveniently send your Pull Requests commits to our mailing list. Please read the "guidelines for contributing" linked above! Heba Waly (1): config: move documentation to config.h Documentation/technical/api-config.txt | 319 --- config.h | 336 + 2 files changed, 336 insertions(+), 319 deletions(-) delete mode 100644 Documentation/technical/api-config.txt base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-405%2FHebaWaly%2Fconfig_documentation-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-405/HebaWaly/config_documentation-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/405 Range-diff vs v1: 1: 2e42eafb5d ! 1: 1a9aa33b46 config: add documentation to config.h @@ -1,12 +1,336 @@ Author: Heba Waly -config: add documentation to config.h - -This commit is copying and summarizing the documentation from -documentation/technical/api-config.txt to comments in config.h +config: move documentation to config.h +Move the documentation from Documentation/technical/api-config.txt into +config.h Signed-off-by: Heba Waly + diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt + deleted file mode 100644 + --- a/Documentation/technical/api-config.txt + +++ /dev/null +@@ +-config API +-== +- +-The config API gives callers a way to access Git configuration files +-(and files which have the same syntax). See linkgit:git-config[1] for a +-discussion of the config file syntax. +- +-General Usage +-- +- +-Config files are parsed linearly, and each variable found is passed to a +-caller-provided callback function. The callback function is responsible +-for any actions to be taken on the config option, and is free to ignore +-some options. It is not uncommon for the configuration to be parsed +-several times during the run of a Git program, with different callbacks +-picking out different variables useful to themselves. +- +-A config callback function takes three parameters: +- +-- the name of the parsed variable. This is in canonical "flat" form: the +- section, subsection, and variable segments will be separated by dots, +- and the section and variable segments will be all lowercase. E.g., +- `core.ignorecase`, `diff.SomeType.textconv`. +- +-- the value of the found variable, as a string. If the variable had no +- value specified, the value will be NULL (typically this means it +- should be interpreted as boolean true). +- +-- a void pointer passed in by the caller of the config API; this can +- contain callback-specific data +- +-A config callback should return 0 for success, or -1 if the variable +-could not be parsed properly. +- +-Basic Config Querying +-- +- +-Most programs will simply want to look up variables in all config files +-that Git knows about, using the normal precedence rules. To do this, +-call `git_config` with a callback function and void data pointer. +- +-`git_config` will read all config sources in order of increasing +-priority. Thus a callback should typically overwrite previously-seen +-entries with new ones (e.g., if both the user-wide `~/.gitconfig` and +-repo-specific `.git/config` contain `color.ui`, the config machinery +-will first feed the user-wide one to the callback, and then the +-repo-specific one; by overwriting, the higher-priority repo-specific +-value is left at the end). +- +-The `config_with_options` function lets the caller examine config +-while adjusting some of the default behavior of `git_config`. It should +-almost never be used by "regular" Git code that is looking up +-configuration variables. It is intended for advanced callers like +-`git-config`, which are intentionally tweaking the normal config-lookup +-process. It takes two extra parameters: +- +-`config_source`:: +-If this parameter is non-NULL, it specifies the source to parse for +-configuration, rather than looking in the usual files. See `struct +-git_config_source`
[PATCH v2 1/1] config: move documentation to config.h
From: Heba Waly Move the documentation from Documentation/technical/api-config.txt into config.h Signed-off-by: Heba Waly --- Documentation/technical/api-config.txt | 319 --- config.h | 336 + 2 files changed, 336 insertions(+), 319 deletions(-) delete mode 100644 Documentation/technical/api-config.txt diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt deleted file mode 100644 index 7d20716c32..00 --- a/Documentation/technical/api-config.txt +++ /dev/null @@ -1,319 +0,0 @@ -config API -== - -The config API gives callers a way to access Git configuration files -(and files which have the same syntax). See linkgit:git-config[1] for a -discussion of the config file syntax. - -General Usage -- - -Config files are parsed linearly, and each variable found is passed to a -caller-provided callback function. The callback function is responsible -for any actions to be taken on the config option, and is free to ignore -some options. It is not uncommon for the configuration to be parsed -several times during the run of a Git program, with different callbacks -picking out different variables useful to themselves. - -A config callback function takes three parameters: - -- the name of the parsed variable. This is in canonical "flat" form: the - section, subsection, and variable segments will be separated by dots, - and the section and variable segments will be all lowercase. E.g., - `core.ignorecase`, `diff.SomeType.textconv`. - -- the value of the found variable, as a string. If the variable had no - value specified, the value will be NULL (typically this means it - should be interpreted as boolean true). - -- a void pointer passed in by the caller of the config API; this can - contain callback-specific data - -A config callback should return 0 for success, or -1 if the variable -could not be parsed properly. - -Basic Config Querying -- - -Most programs will simply want to look up variables in all config files -that Git knows about, using the normal precedence rules. To do this, -call `git_config` with a callback function and void data pointer. - -`git_config` will read all config sources in order of increasing -priority. Thus a callback should typically overwrite previously-seen -entries with new ones (e.g., if both the user-wide `~/.gitconfig` and -repo-specific `.git/config` contain `color.ui`, the config machinery -will first feed the user-wide one to the callback, and then the -repo-specific one; by overwriting, the higher-priority repo-specific -value is left at the end). - -The `config_with_options` function lets the caller examine config -while adjusting some of the default behavior of `git_config`. It should -almost never be used by "regular" Git code that is looking up -configuration variables. It is intended for advanced callers like -`git-config`, which are intentionally tweaking the normal config-lookup -process. It takes two extra parameters: - -`config_source`:: -If this parameter is non-NULL, it specifies the source to parse for -configuration, rather than looking in the usual files. See `struct -git_config_source` in `config.h` for details. Regular `git_config` defaults -to `NULL`. - -`opts`:: -Specify options to adjust the behavior of parsing config files. See `struct -config_options` in `config.h` for details. As an example: regular `git_config` -sets `opts.respect_includes` to `1` by default. - -Reading Specific Files --- - -To read a specific file in git-config format, use -`git_config_from_file`. This takes the same callback and data parameters -as `git_config`. - -Querying For Specific Variables - -For programs wanting to query for specific variables in a non-callback -manner, the config API provides two functions `git_config_get_value` -and `git_config_get_value_multi`. They both read values from an internal -cache generated previously from reading the config files. - -`int git_config_get_value(const char *key, const char **value)`:: - - Finds the highest-priority value for the configuration variable `key`, - stores the pointer to it in `value` and returns 0. When the - configuration variable `key` is not found, returns 1 without touching - `value`. The caller should not free or modify `value`, as it is owned - by the cache. - -`const struct string_list *git_config_get_value_multi(const char *key)`:: - - Finds and returns the value list, sorted in order of increasing priority - for the configuration variable `key`. When the configuration variable - `key` is not found, returns NULL. The caller should not free or modify - the returned pointer, as it is owned by the cache. - -`void git_config_clear(void)`:: - - Resets and invalidates the config cache. - -The config API also provides type specific API functions whic
Re: [PATCH v3 4/4] git-gui: allow undoing last revert
On Mon, Oct 21, 2019 at 9:35 PM Johannes Sixt wrote: > > Am 21.10.19 um 11:16 schrieb Bert Wesarg: > > Dear Pratyush, > > > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > > the stage-list. But I think it should be disabled, like the 'Revert > > Hunk' and 'Revert Line' menu entry. > > > > Can you confirm this? > > Technically, it need not be disabled because the hunk being reverted > does not depend on the contents of any of diffs that can be shown. > > The entry should be disabled if reverting the stored hunk fails. But to > know that, it would have to be tried: the file could have been edited > since the hunk was generated so that the reversal of the hunk fails. But the "Undo" changes the worktree not the stage, sure it indirectly also changes the view of the staged content, but that is only a secondary effect. As I only can revert in the worktree list, I think we should be consistent and also only allow to undo the revert in the worktree list. And I think it is independent of 'does the undo apply at all' question. Bert > > Not sure what to do, though. > > -- Hannes
Re: [GIT PULL] arm64: Fixes for -rc4
Hello, I added the git list to Cc:. For the new readers: The context of this thread can be found at https://lwn.net/ml/linux-kernel/20191017234348.wcbbo2njexn7ixpk@willie-the-truck/ On Mon, Oct 21, 2019 at 08:46:58AM +0200, Ingo Molnar wrote: > Anyway, a small Git feature request: it would be super useful if "git > request-pull" output was a bit more dependable and at least warned about > this and didn't include what is, from the viewpoint of the person doing > the merge, a bogus diffstat. (Generating the correct diffstat is probably > beyond request-pull's abilities: it would require changing the working > tree to actually perform the merge - while request-pull is a read-only > operation right now. But detecting the condition and warning about it > should be possible?) I think Will's case is still an easy one compared with what could actually happen. The related history looks as follows: ,-. ,-. ,-.,-.,-. v5.4-rc1 --| |-...-| |-- v5.4-rc2 --| |-..-| |-..-| |-- v5.4-rc3 \ `-' `-' \ `-'/-'`-' \ ,-. ,-. \ ,-/,-. ,-. `--| |-...-| ||*|| |-...-|H| `-' `-'\ `-'`-' /-' \ ,-. ,-. / `--| |-...-| |-' `-' `-' Will asked Linus to merge the Commit marked 'H', the two merge bases are v5.4-rc2 and '*'. (FTR: * = 3e7c93bd04edfb0cae7dad1215544c9350254b8f H = 777d062e5bee0e3c0751cdcbce116a76ee2310ec , they can be found in git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git) The formally correct way to create the diffstat is to merge v5.4-rc2 and '*' (in general: all merge bases) and calculate the diff between this merge and the to-be-merged-commit. Compared to what Will did (i.e. merge Linus' HEAD and this branch and then diff @~ with @) doing it the way I described has the advantage(?) that commits that conflict with this merge request in Linus' tree since the merge bases are not in the way. In this case this can be done automatically: $ git read-tree --index-output=tralala v5.4-rc2 3e7c93bd04edfb0cae7dad1215544c9350254b8f $ GIT_INDEX=tralala git write-tree 6a2acfd1870d9da3c330ea9b648a7e858b5ee39f $ git diff --stat 6a2acfd1870d9da3c330ea9b648a7e858b5ee39f 777d062e5bee0e3c0751cdcbce116a76ee2310ec Documentation/arm64/silicon-errata.rst | 2 ++ arch/arm64/Kconfig | 17 ++ arch/arm64/include/asm/asm-uaccess.h | 7 +++--- arch/arm64/include/asm/cpucaps.h | 4 +++- arch/arm64/include/asm/memory.h| 10 ++-- arch/arm64/include/asm/pgtable.h | 3 --- arch/arm64/include/asm/sysreg.h| 2 +- arch/arm64/kernel/cpu_errata.c | 38 +++ arch/arm64/kernel/cpufeature.c | 15 arch/arm64/kernel/entry.S | 8 --- arch/arm64/kernel/hibernate.c | 9 +++- arch/arm64/kernel/process.c| 18 +++ arch/arm64/kvm/hyp/switch.c| 69 ++-- arch/arm64/mm/fault.c | 6 - include/linux/sched.h | 1 + 15 files changed, 186 insertions(+), 23 deletions(-) Would be great if git-request-pull learned to do that. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH v3 4/4] git-gui: allow undoing last revert
On Mon, Oct 21, 2019 at 9:04 PM Pratyush Yadav wrote: > > On 21/10/19 11:16AM, Bert Wesarg wrote: > > Dear Pratyush, > > > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > > the stage-list. But I think it should be disabled, like the 'Revert > > Hunk' and 'Revert Line' menu entry. > > I'm not sure what you mean. There is no "Revert Last Hunk" menu entry (I > assume you are talking about the context menu in the diff view that we > open by right clicking). > > My guess is that you mean the "Undo Last Revert" option. And you want it > disabled if the diff shown is of a staged file, correct? > > I'm not sure if disabling it would be a good idea. > > Say I revert a hunk or line while the file is not staged, and stage the > rest of the file. This would mean that file is no longer in the > "Unstaged Changes" widget. Now if I choose the file from the "Staged > Changes" widget, I get the option to undo the last revert. If I hit > that, it will put whatever I reverted in the "Unstaged Changes" widget. > So, now part of the file that was reverted is in "Unstaged Changes", and > the rest in "Unstaged Changes". > Sorry for this confusion. > IIUC, this is what you think should not happen, correct? What's wrong > with allowing the user to undo reverts from anywhere? The 'Undo' changes the worktree not the staged content,. > > The way I see it, it doesn't really matter what file is selected or > whether it is staged or not, the action of the undo remains the same, so > it makes sense to me to allow it from anywhere. It would make sense to undo the revert on the staged content, if there are no more changes to this file in the worktree. I.e., you wont be able to apply the 'undo' anymore to the worktree file if it is not listed anymore. Though even that case should be able to implement. Though the undo is currently not bound to a specific path. This may be the cause of our different view. I though the undo is bound to the path it was recorded, thus also would only be available when showing a diff on this path again. Therefore I think, having the 'Undo Last Revert' in the context menu may not be the best place to begin with, or at least indicate that this 'undo' operation does not necceseritly operate on the file currently shown. Bertt > > > Can you confirm this? > > > > On Wed, Aug 28, 2019 at 11:57 PM Pratyush Yadav > > wrote: > > > > > > Accidental clicks on the revert hunk/lines buttons can cause loss of > > > work, and can be frustrating. So, allow undoing the last revert. > > > > > > Right now, a stack or deque are not being used for the sake of > > > simplicity, so only one undo is possible. Any reverts before the > > > previous one are lost. > > > > > > Signed-off-by: Pratyush Yadav > > -- > Regards, > Pratyush Yadav
Re: Outreachy Winter 2019
Hi Karina, Please see my answer below. On Mon, Oct 21, 2019 at 6:59 PM Karina Saucedo wrote: > > Hello, my name is Karina and I'm and Outreachy applicant. > I´m interested in applying to the project 'Add did you mean hints´ and > I was wondering how can I start contributing since there seem to be no > issues on the github page. Thank you! Thank you for your introduction email and for your interest in Git! Emily posted some interesting information on the Git mailing list that you can see in the archives there: https://public-inbox.org/git/20191007203654.ga20...@google.com/ We require Outreachy (and GSoC) applicants to work on a microproject before they can be selected. There are microproject suggestions in Emily's email and the following discussions. Unfortunately we only have a web page that we prepared for the last Google Summer of Code: https://git.github.io/SoC-2019-Microprojects/ So it might not be up to date but you can still find interesting information on top of what Emily posted. Don't hesitate to ask if you have any further questions. Best, Christian.
[PATCH] t7419: change test_must_fail to ! for grep
According to t/README, test_must_fail() should only be used to test for failure in Git commands. Replace the invocations of `test_must_fail grep` with `! grep`. Signed-off-by: Denton Liu --- *sigh* Here's another cleanup patch for 'dl/submodule-set-branch'. It's inspired by Eric Sunshine's comments on the t5520 patchset from earlier. It's definitely not urgent, though, and can wait for v2.25.0. t/t7419-submodule-set-branch.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh index c4b370ea85..fd25f786a3 100755 --- a/t/t7419-submodule-set-branch.sh +++ b/t/t7419-submodule-set-branch.sh @@ -34,7 +34,7 @@ test_expect_success 'submodule config cache setup' ' test_expect_success 'ensure submodule branch is unset' ' (cd super && - test_must_fail grep branch .gitmodules + ! grep branch .gitmodules ) ' @@ -54,7 +54,7 @@ test_expect_success 'test submodule set-branch --branch' ' test_expect_success 'test submodule set-branch --default' ' (cd super && git submodule set-branch --default submodule && - test_must_fail grep branch .gitmodules && + ! grep branch .gitmodules && git submodule update --remote && cat <<-\EOF >expect && a @@ -80,7 +80,7 @@ test_expect_success 'test submodule set-branch -b' ' test_expect_success 'test submodule set-branch -d' ' (cd super && git submodule set-branch -d submodule && - test_must_fail grep branch .gitmodules && + ! grep branch .gitmodules && git submodule update --remote && cat <<-\EOF >expect && a -- 2.24.0.rc0.197.g0926ab8072
[BUG] "--show-current-patch" return a mail instead of a patch
Hello all, I try to use "git am" to apply a patch sent using "git send-email". This patch does not apply properly. I try to use "git am --show-current-patch" to understand the problem. However, since original mail is encoded in quoted- printable, data returned by --show-current-patch is not a valid patch. I expected that --show-current-patch would return decoded version of original mail or something that looks like the output of "git format-patch" or at least a valid patch. Thus, it could be processed with "git apply" or "patch". Currently I run "git mailinfo" manually to get the patch, but it is not very handy. (I use git version 2.20.1 from Debian buster) Thank you, -- Jérôme Pouiller
[PATCH v3 03/14] t5520: use sq for test case names
The usual convention is for test case names to be written between single-quotes. Change all double-quoted test case names to single-quotes except for two test case names that use variables within. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 51d6ce8aec..a3de2e19b6 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -408,7 +408,7 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' test new = "$(git show HEAD:file2)" ' -test_expect_success "pull --rebase warns on --verify-signatures" ' +test_expect_success 'pull --rebase warns on --verify-signatures' ' git reset --hard before-rebase && git pull --rebase --verify-signatures . copy 2>err && test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && @@ -416,7 +416,7 @@ test_expect_success "pull --rebase warns on --verify-signatures" ' test_i18ngrep "ignoring --verify-signatures for rebase" err ' -test_expect_success "pull --rebase does not warn on --no-verify-signatures" ' +test_expect_success 'pull --rebase does not warn on --no-verify-signatures' ' git reset --hard before-rebase && git pull --rebase --no-verify-signatures . copy 2>err && test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 04/14] t5520: let sed open its own input
We were using a redirection operator to feed input into sed. However,
since sed is capable of opening its own files, make sed open its own
files instead of redirecting input into it.
Signed-off-by: Denton Liu
---
t/t5520-pull.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index a3de2e19b6..55560ce3cd 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -5,7 +5,7 @@ test_description='pulling into void'
. ./test-lib.sh
modify () {
- sed -e "$1" <"$2" >"$2.x" &&
+ sed -e "$1" "$2" >"$2.x" &&
mv "$2.x" "$2"
}
--
2.24.0.rc0.197.g0926ab8072
[PATCH v3 00/14] t5520: various test cleanup
Like earlier patchsets, I want to implement a feature that involves
modifications to the test suite. Since that feature will probably take a
while to polish up, however, let's clean up the test suite in a separate
patchset first so it's not blocked by the feature work.
1/15 is a general improvement to test_rev_cmp() that will be used later
in the series.
Changes since v2:
* Drop 't7408: replace `test_must_fail test_path_is_file`' since it's
not a rabbit hole we want to go into right now
* Fix the output of `test_cmp_rev !` when revs are actually equal
* Rebase against the latest master since this topic hasn't been picked
up yet
Changes since v1:
* Incorporate Eric's feedback
Denton Liu (14):
t: teach test_cmp_rev to accept ! for not-equals
t5520: improve test style
t5520: use sq for test case names
t5520: let sed open its own input
t5520: replace test -f with test-lib functions
t5520: remove spaces after redirect operator
t5520: use test_line_count where possible
t5520: replace test -{n,z} with test-lib functions
t5520: use test_cmp_rev where possible
t5520: test single-line files by git with test_cmp
t5520: don't put git in upstream of pipe
t5520: replace subshell cat comparison with test_cmp
t5520: remove redundant lines in test cases
t5520: replace `! git` with `test_must_fail git`
t/t2400-worktree-add.sh | 4 +-
t/t3400-rebase.sh | 2 +-
t/t3421-rebase-topology-linear.sh | 6 +-
t/t3430-rebase-merges.sh| 2 +-
t/t3432-rebase-fast-forward.sh | 2 +-
t/t3501-revert-cherry-pick.sh | 2 +-
t/t3508-cherry-pick-many-commits.sh | 2 +-
t/t5520-pull.sh | 343 +---
t/test-lib-functions.sh | 22 +-
9 files changed, 234 insertions(+), 151 deletions(-)
Range-diff against v2:
1: 987fee4652 < -: -- t7408: replace `test_must_fail
test_path_is_file`
2: 417e808466 ! 1: 9a96f113e7 t: teach test_cmp_rev to accept ! for
not-equals
@@ t/test-lib-functions.sh: test_cmp_rev () {
- if test "$r1" != "$r2"
+ if test "$r1" "$inverted_op" "$r2"
then
++ local comp_out
++ if "x$inverted_op" = 'x='
++ then
++ comp_out='the same'
++ else
++ comp_out='different'
++ fi
cat >&4 <<-EOF
- error: two revisions point to different objects:
+- error: two revisions point to different objects:
++ error: two revisions point to $comp_out objects:
+ '$1': $r1
+ '$2': $r2
+ EOF
3: 0a56980857 = 2: dfc86a8d9b t5520: improve test style
4: dfa89ba1cb = 3: a1071038f5 t5520: use sq for test case names
5: 9fac3dff83 = 4: 0af3f5027b t5520: let sed open its own input
6: c6ca45eb17 = 5: b696ff0a67 t5520: replace test -f with test-lib functions
7: 830a8212ae = 6: d2e49fd990 t5520: remove spaces after redirect operator
8: 3d982230be = 7: fcfc3226f8 t5520: use test_line_count where possible
9: 2bca4f046d = 8: 86dafc7b54 t5520: replace test -{n,z} with test-lib
functions
10: 1a54db1d5c = 9: bf9b5023a3 t5520: use test_cmp_rev where possible
11: 52cf4f0d0f = 10: bfabf8ceff t5520: test single-line files by git with
test_cmp
12: 0cfabb201c = 11: 56bcbf3047 t5520: don't put git in upstream of pipe
13: b2d0ce21c8 = 12: e9d50b8bb0 t5520: replace subshell cat comparison with
test_cmp
14: 5aac40a029 = 13: 9db0fc2156 t5520: remove redundant lines in test cases
15: 2c0d3ac416 = 14: a721d5f119 t5520: replace `! git` with `test_must_fail
git`
--
2.24.0.rc0.197.g0926ab8072
[PATCH v3 01/14] t: teach test_cmp_rev to accept ! for not-equals
Currently, in the case where we are using test_cmp_rev() to report
not-equals, we write `! test_cmp_rev`. However, since test_cmp_rev()
contains
r1=$(git rev-parse --verify "$1") &&
r2=$(git rev-parse --verify "$2") &&
In the case where `git rev-parse` segfaults and dies unexpectedly, the
failure will be ignored.
Rewrite test_cmp_rev() to optionally accept "!" as the first argument to
do a not-equals comparison. Rewrite `! test_cmp_rev` to `test_cmp_rev !`
in all tests to take advantage of this new functionality.
Signed-off-by: Denton Liu
---
t/t2400-worktree-add.sh | 4 ++--
t/t3400-rebase.sh | 2 +-
t/t3421-rebase-topology-linear.sh | 6 +++---
t/t3430-rebase-merges.sh| 2 +-
t/t3432-rebase-fast-forward.sh | 2 +-
t/t3501-revert-cherry-pick.sh | 2 +-
t/t3508-cherry-pick-many-commits.sh | 2 +-
t/test-lib-functions.sh | 22 +++---
8 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index e819ba741e..52d476979b 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -438,7 +438,7 @@ test_expect_success 'git worktree add does not match
remote' '
cd foo &&
test_must_fail git config "branch.foo.remote" &&
test_must_fail git config "branch.foo.merge" &&
- ! test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
+ test_cmp_rev ! refs/remotes/repo_a/foo refs/heads/foo
)
'
@@ -483,7 +483,7 @@ test_expect_success 'git worktree --no-guess-remote option
overrides config' '
cd foo &&
test_must_fail git config "branch.foo.remote" &&
test_must_fail git config "branch.foo.merge" &&
- ! test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
+ test_cmp_rev ! refs/remotes/repo_a/foo refs/heads/foo
)
'
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ab18ac5f28..f267f6cd54 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -64,7 +64,7 @@ test_expect_success 'rebase sets ORIG_HEAD to pre-rebase
state' '
pre="$(git rev-parse --verify HEAD)" &&
git rebase master &&
test_cmp_rev "$pre" ORIG_HEAD &&
- ! test_cmp_rev "$pre" HEAD
+ test_cmp_rev ! "$pre" HEAD
'
test_expect_success 'rebase, with and specified as
:/quuxery' '
diff --git a/t/t3421-rebase-topology-linear.sh
b/t/t3421-rebase-topology-linear.sh
index b847064f91..325072b0a3 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -61,7 +61,7 @@ test_run_rebase () {
test_expect_$result "rebase $* -f rewrites even if upstream is an
ancestor" "
reset_rebase &&
git rebase $* -f b e &&
- ! test_cmp_rev e HEAD &&
+ test_cmp_rev ! e HEAD &&
test_cmp_rev b HEAD~2 &&
test_linear_range 'd e' b..
"
@@ -78,7 +78,7 @@ test_run_rebase () {
test_expect_$result "rebase $* -f rewrites even if remote upstream is
an ancestor" "
reset_rebase &&
git rebase $* -f branch-b branch-e &&
- ! test_cmp_rev branch-e origin/branch-e &&
+ test_cmp_rev ! branch-e origin/branch-e &&
test_cmp_rev branch-b HEAD~2 &&
test_linear_range 'd e' branch-b..
"
@@ -368,7 +368,7 @@ test_run_rebase () {
test_expect_$result "rebase $* -f --root on linear history causes
re-write" "
reset_rebase &&
git rebase $* -f --root c &&
- ! test_cmp_rev a HEAD~2 &&
+ test_cmp_rev ! a HEAD~2 &&
test_linear_range 'a b c' HEAD
"
}
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 9efcf4808a..abbdc26b1b 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -346,7 +346,7 @@ test_expect_success 'A root commit can be a cousin, treat
it that way' '
git merge --allow-unrelated-histories khnum &&
test_tick &&
git rebase -f -r HEAD^ &&
- ! test_cmp_rev HEAD^2 khnum &&
+ test_cmp_rev ! HEAD^2 khnum &&
test_cmp_graph HEAD^.. <<-\EOF &&
* Merge branch '\''khnum'\'' into asherah
|\
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 034ffc7e76..92f95b57da 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -64,7 +64,7 @@ test_rebase_same_head_ () {
test_cmp_rev \$oldhead \$newhead
elif test $cmp = diff
then
- ! test_cmp_rev \$oldhead \$newhead
+ test_cmp_rev ! \$oldhead \$newhead
fi
"
}
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index d1c68af8c5..1c51a9131d 100755
--- a/t/t3501-
[PATCH v3 05/14] t5520: replace test -f with test-lib functions
Although `test -f` has the same functionality as test_path_is_file(), in the case where test_path_is_file() fails, we get much better debugging information. Replace `test -f` with test_path_is_file() so that future developers will have a better experience debugging these test cases. Also, in the case of `! test -f`, not only should that path not be a file, it shouldn't exist at all so replace it with test_path_is_missing(). Signed-off-by: Denton Liu --- t/t5520-pull.sh | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 55560ce3cd..004d5884cd 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -39,8 +39,8 @@ test_expect_success 'pulling into void' ' cd cloned && git pull .. ) && - test -f file && - test -f cloned/file && + test_path_is_file file && + test_path_is_file cloned/file && test_cmp file cloned/file ' @@ -50,8 +50,8 @@ test_expect_success 'pulling into void using master:master' ' cd cloned-uho && git pull .. master:master ) && - test -f file && - test -f cloned-uho/file && + test_path_is_file file && + test_path_is_file cloned-uho/file && test_cmp file cloned-uho/file ' @@ -99,7 +99,7 @@ test_expect_success 'pulling into void must not create an octopus' ' ( cd cloned-octopus && test_must_fail git pull .. master master && - ! test -f file + test_path_is_missing file ) ' -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 02/14] t5520: improve test style
Improve the test style by removing leading and trailing empty lines within test cases. Also, reformat multi-line subshells to conform to the existing style. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 88 + 1 file changed, 45 insertions(+), 43 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index cf4cc32fd0..51d6ce8aec 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -538,7 +538,6 @@ test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m ' test_expect_success '--rebase with rebased upstream' ' - git remote add -f me . && git checkout copy && git tag copy-orig && @@ -552,7 +551,6 @@ test_expect_success '--rebase with rebased upstream' ' git pull --rebase me copy && test "conflicting modification" = "$(cat file)" && test file = "$(cat file2)" - ' test_expect_success '--rebase -f with rebased upstream' ' @@ -564,14 +562,12 @@ test_expect_success '--rebase -f with rebased upstream' ' ' test_expect_success '--rebase with rebased default upstream' ' - git update-ref refs/remotes/me/copy copy-orig && git checkout --track -b to-rebase2 me/copy && git reset --hard to-rebase-orig && git pull --rebase && test "conflicting modification" = "$(cat file)" && test file = "$(cat file2)" - ' test_expect_success 'rebased upstream + fetch + pull --rebase' ' @@ -588,7 +584,6 @@ test_expect_success 'rebased upstream + fetch + pull --rebase' ' ' test_expect_success 'pull --rebase dies early with dirty working directory' ' - git checkout to-rebase && git update-ref refs/remotes/me/copy copy^ && COPY="$(git rev-parse --verify me/copy)" && @@ -603,16 +598,16 @@ test_expect_success 'pull --rebase dies early with dirty working directory' ' git checkout HEAD -- file && git pull && test "$COPY" != "$(git rev-parse --verify me/copy)" - ' test_expect_success 'pull --rebase works on branch yet to be born' ' git rev-parse master >expect && mkdir empty_repo && - (cd empty_repo && -git init && -git pull --rebase .. master && -git rev-parse HEAD >../actual + ( + cd empty_repo && + git init && + git pull --rebase .. master && + git rev-parse HEAD >../actual ) && test_cmp expect actual ' @@ -646,58 +641,65 @@ test_expect_success 'pull --rebase fails on corrupt HEAD' ' test_expect_success 'setup for detecting upstreamed changes' ' mkdir src && - (cd src && -git init && -printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" > stuff && -git add stuff && -git commit -m "Initial revision" + ( + cd src && + git init && + printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" > stuff && + git add stuff && + git commit -m "Initial revision" ) && git clone src dst && - (cd src && -modify s/5/43/ stuff && -git commit -a -m "5->43" && -modify s/6/42/ stuff && -git commit -a -m "Make it bigger" + ( + cd src && + modify s/5/43/ stuff && + git commit -a -m "5->43" && + modify s/6/42/ stuff && + git commit -a -m "Make it bigger" ) && - (cd dst && -modify s/5/43/ stuff && -git commit -a -m "Independent discovery of 5->43" + ( + cd dst && + modify s/5/43/ stuff && + git commit -a -m "Independent discovery of 5->43" ) ' test_expect_success 'git pull --rebase detects upstreamed changes' ' - (cd dst && -git pull --rebase && -test -z "$(git ls-files -u)" + ( + cd dst && + git pull --rebase && + test -z "$(git ls-files -u)" ) ' test_expect_success 'setup for avoiding reapplying old patches' ' - (cd dst && -test_might_fail git rebase --abort && -git reset --hard origin/master + ( + cd dst && + test_might_fail git rebase --abort && + git reset --hard origin/master ) && git clone --bare src src-replace.git && rm -rf src && mv src-replace.git src && - (cd dst && -modify s/2/22/ stuff && -git commit -a -m "Change 2" && -modify s/3/33/ stuff && -git commit -a -m "Change 3" && -modify s/4/44/ stuff && -git commit -a -m "Change 4" && -git push && - -modify s/44/55/ stuff && -git commit --amend -a -m "Modified Change 4" + ( + cd dst && + modify s/2/22/ stuff && + git commit -a -m "Change 2" && + modify s/3/33/ stuff && + git commit -a -m "Chan
[PATCH v3 06/14] t5520: remove spaces after redirect operator
The style for tests in Git is to have the redirect operator attached to the filename with no spaces. Fix test cases where this is not the case. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 004d5884cd..7bb9031140 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -243,10 +243,10 @@ test_expect_success 'fast-forward fails with conflicting work tree' ' test_expect_success '--rebase' ' git branch to-rebase && - echo modified again > file && + echo modified again >file && git commit -m file file && git checkout to-rebase && - echo new > file2 && + echo new >file2 && git add file2 && git commit -m "new file" && git tag before-rebase && @@ -542,10 +542,10 @@ test_expect_success '--rebase with rebased upstream' ' git checkout copy && git tag copy-orig && git reset --hard HEAD^ && - echo conflicting modification > file && + echo conflicting modification >file && git commit -m conflict file && git checkout to-rebase && - echo file > file2 && + echo file >file2 && git commit -m to-rebase file2 && git tag to-rebase-orig && git pull --rebase me copy && @@ -591,7 +591,7 @@ test_expect_success 'pull --rebase dies early with dirty working directory' ' test_config branch.to-rebase.remote me && test_config branch.to-rebase.merge refs/heads/copy && test_config branch.to-rebase.rebase true && - echo dirty >> file && + echo dirty >>file && git add file && test_must_fail git pull && test "$COPY" = "$(git rev-parse --verify me/copy)" && -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 07/14] t5520: use test_line_count where possible
Instead of rolling our own functionality to test the number of lines a command outputs, use test_line_count() which provides better debugging information in the case of a failure. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 7bb9031140..0ca4867e96 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -699,7 +699,8 @@ test_expect_success 'git pull --rebase does not reapply old patches' ' ( cd dst && test_must_fail git pull --rebase && - test 1 = $(find .git/rebase-apply -name "000*" | wc -l) + find .git/rebase-apply -name "000*" >patches && + test_line_count = 1 patches ) ' -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 11/14] t5520: don't put git in upstream of pipe
Before, if the invocation of git failed, it would be masked by the pipe since only the return code of the last element of a pipe is used. Rewrite the test to put the Git command on its own line so its return code is not masked. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 8b7e7ae55d..8ddf89e550 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -668,7 +668,8 @@ test_expect_success 'pull --rebase fails on corrupt HEAD' ' ( cd corrupt && test_commit one && - obj=$(git rev-parse --verify HEAD | sed "s#^..#&/#") && + git rev-parse --verify HEAD >head && + obj=$(sed "s#^..#&/#" head) && rm -f .git/objects/$obj && test_must_fail git pull --rebase ) -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 14/14] t5520: replace `! git` with `test_must_fail git`
Currently, if a Git command fails in an unexpected way, such as a segfault, it will be masked and ignored. Replace the ! with test_must_fail so that only expected failures pass. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index ef3dbc201a..602d996a33 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -537,7 +537,7 @@ test_expect_success 'pull --rebase=i' ' test_expect_success 'pull.rebase=invalid fails' ' git reset --hard before-preserve-rebase && test_config pull.rebase invalid && - ! git pull . copy + test_must_fail git pull . copy ' test_expect_success '--rebase=false create a new merge commit' ' @@ -572,7 +572,7 @@ test_expect_success REBASE_P \ test_expect_success '--rebase=invalid fails' ' git reset --hard before-preserve-rebase && - ! git pull --rebase=invalid . copy + test_must_fail git pull --rebase=invalid . copy ' test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-merge' ' -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 12/14] t5520: replace subshell cat comparison with test_cmp
We currently have many instances of `test = $(cat )` and
`test $(cat ) = `. In the case where this fails, it will be
difficult for a developer to debug since the output will be masked.
Replace these instances with invocations of test_cmp().
This change was done with the following GNU sed expressions:
s/\(\s*\)test \([^=]*\)= "$(cat \([^)]*\))"/\1echo \2>expect
\&\&\n\1test_cmp expect \3/
s/\(\s*\)test "$(cat \([^)]*\))" = \([^&]*\)\( &&\)\?$/\1echo \3
>expect \&\&\n\1test_cmp expect \2\4/
A future patch will clean up situations where we have multiple duplicate
statements within a test case. This is done to keep this patch purely
mechanical.
Signed-off-by: Denton Liu
---
t/t5520-pull.sh | 105
1 file changed, 70 insertions(+), 35 deletions(-)
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 8ddf89e550..c9e4eec004 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -15,8 +15,10 @@ test_pull_autostash () {
git add new_file &&
git pull "$@" . copy &&
test_cmp_rev HEAD^ copy &&
- test "$(cat new_file)" = dirty &&
- test "$(cat file)" = "modified again"
+ echo dirty >expect &&
+ test_cmp expect new_file &&
+ echo "modified again" >expect &&
+ test_cmp expect file
}
test_pull_autostash_fail () {
@@ -110,9 +112,11 @@ test_expect_success 'test . as a remote' '
echo updated >file &&
git commit -a -m updated &&
git checkout copy &&
- test "$(cat file)" = file &&
+ echo file >expect &&
+ test_cmp expect file &&
git pull &&
- test "$(cat file)" = updated &&
+ echo updated >expect &&
+ test_cmp expect file &&
git reflog -1 >reflog.actual &&
sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
echo "OBJID HEAD@{0}: pull: Fast-forward" >reflog.expected &&
@@ -125,9 +129,11 @@ test_expect_success 'the default remote . should not break
explicit pull' '
git commit -a -m modified &&
git checkout copy &&
git reset --hard HEAD^ &&
- test "$(cat file)" = file &&
+ echo file >expect &&
+ test_cmp expect file &&
git pull . second &&
- test "$(cat file)" = modified &&
+ echo modified >expect &&
+ test_cmp expect file &&
git reflog -1 >reflog.actual &&
sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
@@ -137,10 +143,12 @@ test_expect_success 'the default remote . should not
break explicit pull' '
test_expect_success 'fail if wildcard spec does not match any refs' '
git checkout -b test copy^ &&
test_when_finished "git checkout -f copy && git branch -D test" &&
- test "$(cat file)" = file &&
+ echo file >expect &&
+ test_cmp expect file &&
test_must_fail git pull . "refs/nonexisting1/*:refs/nonexisting2/*"
2>err &&
test_i18ngrep "no candidates for merging" err &&
- test "$(cat file)" = file
+ echo file >expect &&
+ test_cmp expect file
'
test_expect_success 'fail if no branches specified with non-default remote' '
@@ -148,11 +156,13 @@ test_expect_success 'fail if no branches specified with
non-default remote' '
test_when_finished "git remote remove test_remote" &&
git checkout -b test copy^ &&
test_when_finished "git checkout -f copy && git branch -D test" &&
- test "$(cat file)" = file &&
+ echo file >expect &&
+ test_cmp expect file &&
test_config branch.test.remote origin &&
test_must_fail git pull test_remote 2>err &&
test_i18ngrep "specify a branch on the command line" err &&
- test "$(cat file)" = file
+ echo file >expect &&
+ test_cmp expect file
'
test_expect_success 'fail if not on a branch' '
@@ -160,10 +170,12 @@ test_expect_success 'fail if not on a branch' '
test_when_finished "git remote remove origin" &&
git checkout HEAD^ &&
test_when_finished "git checkout -f copy" &&
- test "$(cat file)" = file &&
+ echo file >expect &&
+ test_cmp expect file &&
test_must_fail git pull 2>err &&
test_i18ngrep "not currently on a branch" err &&
- test "$(cat file)" = file
+ echo file >expect &&
+ test_cmp expect file
'
test_expect_success 'fail if no configuration for current branch' '
@@ -172,10 +184,12 @@ test_expect_success 'fail if no configuration for current
branch' '
git checkout -b test copy^ &&
test_when_finished "git checkout -f copy && git branch -D test" &&
test_config branch.test.remote test_remote &&
- test "$(cat file)" = file &&
+ echo file >expect &&
+ test_cmp expect file &&
test_must_fail git pull 2>err &&
test_i18ngrep "no tracking information" err &&
- test "$(cat file)" = file
+ ec
[PATCH v3 13/14] t5520: remove redundant lines in test cases
In the previous patches, the mechanical application of changes left some duplicate statements in the test case which were not strictly incorrect but were redundant and possibly misleading. Remove these duplicate statements so that it is clear that the intent behind the tests are that the content of the file stays the same throughout the whole test case. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 8 1 file changed, 8 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index c9e4eec004..ef3dbc201a 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -147,7 +147,6 @@ test_expect_success 'fail if wildcard spec does not match any refs' ' test_cmp expect file && test_must_fail git pull . "refs/nonexisting1/*:refs/nonexisting2/*" 2>err && test_i18ngrep "no candidates for merging" err && - echo file >expect && test_cmp expect file ' @@ -161,7 +160,6 @@ test_expect_success 'fail if no branches specified with non-default remote' ' test_config branch.test.remote origin && test_must_fail git pull test_remote 2>err && test_i18ngrep "specify a branch on the command line" err && - echo file >expect && test_cmp expect file ' @@ -174,7 +172,6 @@ test_expect_success 'fail if not on a branch' ' test_cmp expect file && test_must_fail git pull 2>err && test_i18ngrep "not currently on a branch" err && - echo file >expect && test_cmp expect file ' @@ -188,7 +185,6 @@ test_expect_success 'fail if no configuration for current branch' ' test_cmp expect file && test_must_fail git pull 2>err && test_i18ngrep "no tracking information" err && - echo file >expect && test_cmp expect file ' @@ -202,7 +198,6 @@ test_expect_success 'pull --all: fail if no configuration for current branch' ' test_cmp expect file && test_must_fail git pull --all 2>err && test_i18ngrep "There is no tracking information" err && - echo file >expect && test_cmp expect file ' @@ -215,7 +210,6 @@ test_expect_success 'fail if upstream branch does not exist' ' test_cmp expect file && test_must_fail git pull 2>err && test_i18ngrep "no such ref was fetched" err && - echo file >expect && test_cmp expect file ' @@ -685,10 +679,8 @@ test_expect_success 'pull --rebase fails on unborn branch with staged changes' ' git ls-files >actual && test_cmp expect actual && test_must_fail git pull --rebase .. master 2>err && - echo staged-file >expect && git ls-files >actual && test_cmp expect actual && - echo staged-file >expect && git show :staged-file >actual && test_cmp expect actual && test_i18ngrep "unborn branch with changes added to the index" err -- 2.24.0.rc0.197.g0926ab8072
[PATCH v3 09/14] t5520: use test_cmp_rev where possible
In case an invocation of `git rev-list` fails within the subshell, the failure will be masked. Remove the subshell and use test_cmp_rev() so that failures can be discovered. This change was done with the following sed expressions: s/test "$(git rev-parse.* \([^)]*\))" = "$(git rev-parse \([^)]*\))"/test_cmp_rev \1 \2/ s/test \([^ ]*\) = "$(git rev-parse.* \([^)]*\))"/test_cmp_rev \1 \2/ s/test "$(git rev-parse.* \([^)]*\))" != "$(git rev-parse.* \([^)]*\))"/test_cmp_rev ! \1 \2/ s/test \([^ ]*\) != "$(git rev-parse.* \([^)]*\))"/test_cmp_rev ! \1 \2/ Signed-off-by: Denton Liu --- t/t5520-pull.sh | 50 - 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 18225d8430..1af6ea06ee 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -230,7 +230,7 @@ test_expect_success 'fast-forwards working tree if branch head is updated' ' git pull . second:third 2>err && test_i18ngrep "fetch updated the current branch head" err && test "$(cat file)" = modified && - test "$(git rev-parse third)" = "$(git rev-parse second)" + test_cmp_rev third second ' test_expect_success 'fast-forward fails with conflicting work tree' ' @@ -241,7 +241,7 @@ test_expect_success 'fast-forward fails with conflicting work tree' ' test_must_fail git pull . second:third 2>err && test_i18ngrep "Cannot fast-forward your working tree" err && test "$(cat file)" = conflict && - test "$(git rev-parse third)" = "$(git rev-parse second)" + test_cmp_rev third second ' test_expect_success '--rebase' ' @@ -254,7 +254,7 @@ test_expect_success '--rebase' ' git commit -m "new file" && git tag before-rebase && git pull --rebase . copy && - test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && + test_cmp_rev HEAD^ copy && test new = "$(git show HEAD:file2)" ' @@ -266,7 +266,7 @@ test_expect_success '--rebase fast forward' ' git checkout to-rebase && git pull --rebase . ff && - test "$(git rev-parse HEAD)" = "$(git rev-parse ff)" && + test_cmp_rev HEAD ff && # The above only validates the result. Did we actually bypass rebase? git reflog -1 >reflog.actual && @@ -290,7 +290,7 @@ test_expect_success '--rebase --autostash fast forward' ' git checkout behind && echo dirty >file && git pull --rebase --autostash . to-rebase-ff && - test "$(git rev-parse HEAD)" = "$(git rev-parse to-rebase-ff)" + test_cmp_rev HEAD to-rebase-ff ' test_expect_success '--rebase with conflicts shows advice' ' @@ -328,7 +328,7 @@ test_expect_success 'failed --rebase shows advice' ' test_expect_success '--rebase fails with multiple branches' ' git reset --hard before-rebase && test_must_fail git pull --rebase . copy master 2>err && - test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" && + test_cmp_rev HEAD before-rebase && test_i18ngrep "Cannot rebase onto multiple branches" err && test modified = "$(git show HEAD:file)" ' @@ -380,7 +380,7 @@ test_expect_success 'pull.rebase' ' git reset --hard before-rebase && test_config pull.rebase true && git pull . copy && - test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && + test_cmp_rev HEAD^ copy && test new = "$(git show HEAD:file2)" ' @@ -398,7 +398,7 @@ test_expect_success 'branch.to-rebase.rebase' ' git reset --hard before-rebase && test_config branch.to-rebase.rebase true && git pull . copy && - test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && + test_cmp_rev HEAD^ copy && test new = "$(git show HEAD:file2)" ' @@ -407,14 +407,14 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' test_config pull.rebase true && test_config branch.to-rebase.rebase false && git pull . copy && - test "$(git rev-parse HEAD^)" != "$(git rev-parse copy)" && + test_cmp_rev ! HEAD^ copy && test new = "$(git show HEAD:file2)" ' test_expect_success 'pull --rebase warns on --verify-signatures' ' git reset --hard before-rebase && git pull --rebase --verify-signatures . copy 2>err && - test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && + test_cmp_rev HEAD^ copy && test new = "$(git show HEAD:file2)" && test_i18ngrep "ignoring --verify-signatures for rebase" err ' @@ -422,7 +422,7 @@ test_expect_success 'pull --rebase warns on --verify-signatures' ' test_expect_success 'pull --rebase does not warn on --no-verify-signatures' ' git reset --hard before-rebase && git pull --rebase --no-verify-signatures . copy 2>err && - test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" && + test_cmp_rev HEAD^ co
[PATCH v3 10/14] t5520: test single-line files by git with test_cmp
In case an invocation of a Git command fails within the subshell, the failure will be masked. Replace the subshell with a file-redirection and a call to test_cmp. This change was done with the following GNU sed expressions: s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/ s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/ A future patch will clean up situations where we have multiple duplicate statements within a test case. This is done to keep this patch purely mechanical. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 64 - 1 file changed, 48 insertions(+), 16 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 1af6ea06ee..8b7e7ae55d 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -255,7 +255,9 @@ test_expect_success '--rebase' ' git tag before-rebase && git pull --rebase . copy && test_cmp_rev HEAD^ copy && - test new = "$(git show HEAD:file2)" + echo new >expect && + git show HEAD:file2 >actual && + test_cmp expect actual ' test_expect_success '--rebase fast forward' ' @@ -330,7 +332,9 @@ test_expect_success '--rebase fails with multiple branches' ' test_must_fail git pull --rebase . copy master 2>err && test_cmp_rev HEAD before-rebase && test_i18ngrep "Cannot rebase onto multiple branches" err && - test modified = "$(git show HEAD:file)" + echo modified >expect && + git show HEAD:file >actual && + test_cmp expect actual ' test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' ' @@ -381,7 +385,9 @@ test_expect_success 'pull.rebase' ' test_config pull.rebase true && git pull . copy && test_cmp_rev HEAD^ copy && - test new = "$(git show HEAD:file2)" + echo new >expect && + git show HEAD:file2 >actual && + test_cmp expect actual ' test_expect_success 'pull --autostash & pull.rebase=true' ' @@ -399,7 +405,9 @@ test_expect_success 'branch.to-rebase.rebase' ' test_config branch.to-rebase.rebase true && git pull . copy && test_cmp_rev HEAD^ copy && - test new = "$(git show HEAD:file2)" + echo new >expect && + git show HEAD:file2 >actual && + test_cmp expect actual ' test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' @@ -408,14 +416,18 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' ' test_config branch.to-rebase.rebase false && git pull . copy && test_cmp_rev ! HEAD^ copy && - test new = "$(git show HEAD:file2)" + echo new >expect && + git show HEAD:file2 >actual && + test_cmp expect actual ' test_expect_success 'pull --rebase warns on --verify-signatures' ' git reset --hard before-rebase && git pull --rebase --verify-signatures . copy 2>err && test_cmp_rev HEAD^ copy && - test new = "$(git show HEAD:file2)" && + echo new >expect && + git show HEAD:file2 >actual && + test_cmp expect actual && test_i18ngrep "ignoring --verify-signatures for rebase" err ' @@ -423,7 +435,9 @@ test_expect_success 'pull --rebase does not warn on --no-verify-signatures' ' git reset --hard before-rebase && git pull --rebase --no-verify-signatures . copy 2>err && test_cmp_rev HEAD^ copy && - test new = "$(git show HEAD:file2)" && + echo new >expect && + git show HEAD:file2 >actual && + test_cmp expect actual && test_i18ngrep ! "verify-signatures" err ' @@ -445,7 +459,9 @@ test_expect_success 'pull.rebase=false create a new merge commit' ' git pull . copy && test_cmp_rev HEAD^1 before-preserve-rebase && test_cmp_rev HEAD^2 copy && - test file3 = "$(git show HEAD:file3.t)" + echo file3 >expect && + git show HEAD:file3.t >actual && + test_cmp expect actual ' test_expect_success 'pull.rebase=true flattens keep-merge' ' @@ -453,7 +469,9 @@ test_expect_success 'pull.rebase=true flattens keep-merge' ' test_config pull.rebase true && git pull . copy && test_cmp_rev HEAD^^ copy && - test file3 = "$(git show HEAD:file3.t)" + echo file3 >expect && + git show HEAD:file3.t >actual && + test_cmp expect actual ' test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' ' @@ -461,7 +479,9 @@ test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' ' test_config pull.rebase 1 && git pull . copy && test_cmp_rev HEAD^^ copy && - test file3 = "$(git show HEAD:file3.t)" + echo file3 >expect && + git show HEAD:file3.t >actual && + test_cmp expect actual ' test_expect_s
[PATCH v3 08/14] t5520: replace test -{n,z} with test-lib functions
When wrapping a Git command in a subshell within another command, we throw away the Git command's exit code. In case the Git command fails, we would like to know about it rather than the failure being silent. Extract Git commands so that their exit codes are not lost. Instead of using `test -n` or `test -z`, replace them respectively with invocations of test_file_not_empty() and test_must_be_empty() so that we get better debugging information in the case of a failure. Signed-off-by: Denton Liu --- t/t5520-pull.sh | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh index 0ca4867e96..18225d8430 100755 --- a/t/t5520-pull.sh +++ b/t/t5520-pull.sh @@ -206,15 +206,18 @@ test_expect_success 'fail if the index has unresolved entries' ' test_when_finished "git checkout -f copy && git branch -D third" && test "$(cat file)" = file && test_commit modified2 file && - test -z "$(git ls-files -u)" && + git ls-files -u >unmerged && + test_must_be_empty unmerged && test_must_fail git pull . second && - test -n "$(git ls-files -u)" && + git ls-files -u >unmerged && + test_file_not_empty unmerged && cp file expected && test_must_fail git pull . second 2>err && test_i18ngrep "Pulling is not possible because you have unmerged files." err && test_cmp expected file && git add file && - test -z "$(git ls-files -u)" && + git ls-files -u >unmerged && + test_must_be_empty unmerged && test_must_fail git pull . second 2>err && test_i18ngrep "You have not concluded your merge" err && test_cmp expected file @@ -667,7 +670,8 @@ test_expect_success 'git pull --rebase detects upstreamed changes' ' ( cd dst && git pull --rebase && - test -z "$(git ls-files -u)" + git ls-files -u >untracked && + test_must_be_empty untracked ) ' -- 2.24.0.rc0.197.g0926ab8072
Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
Jeff King writes: >> ... >> I agree with you that it did correctly sort them in ASCII order. > > What's the purpose of sorting them, though? I thought it was less for > aesthetics and more to to keep lines deterministic (to avoid two > branches adding the same line in different places, thus causing > weirdness when the two are merged). In that case, I think we care less > about the exact order and more that anybody can easily reproduce the > same sort (by running "10:!sort" or whatever you weird emacs-types would > type). In the ideal world, "sort" would have a handy option we can tell it to reshuffle the ASCII table in such a way that all punctuations come before alphanumeric, making sure "/" and "." are the first two letters in the alphabet, and everybody can use it to sort the lines reproducibly and also readably. But I do not know of such a widely used implementation of "sort", so... If we had known better, we would have used such a custom sort order to sort the index entries, making sure that slash sorts before any other byte ;-)
[RFC PATCH v2 0/3] format-patch --complete / am --exact
[I'm intentionally keeping the recipient list short to avoid hitting the Oracle spam filter on outgoing email, hopefully everybody on the git side who is interested will receive this via the mailing list and I will link this submission from the workflows list too.] Background: There seems to be a consensus in the Linux kernel development community that tracking patches, patchsets, reviews, and discussion of said patches is too difficult. One big problem is that there is often no reference to the email discussion in git history once the patch has been merged. In order to simplify the tracking of patches, I proposed in [1] that we include enough metadata about a patch to reconstruct the commit SHA1s when emailing patches; this means that, assuming a patchset is based on a publicly available parent SHA1, we can track email patches in git and use the git SHA1 as a stable reference to a particular submission or its corresponding discussion. I basically view this as a foundation on which we can build a richer kernel development experience without sacrificing the current email-based workflow. Since I started working on this feature, I also realised that 'git am' already has a mechanism to amend changelogs with a reference to the "Message-Id" of the email of a patch using the --message-id flag, and while this should IMHO be used a lot more for the kernel, it does not completely offset the utility of these patches. I'm sending out an early v2 to get more feedback on the implementation, exact choice of flags and terminology (--exact, --complete, "metadata", etc.), changelogs. Changes since v1: - moved metadata to the bottom of the diff - fixes to pass existing tests (0023, 3403, 4150, 4256, 5100) - handles format=flowed (best effort) - better changelogs - documentation - new tests Todo: - 'git am --no-exact' _with_ known metadata could append the original sha1 (and/or mail reference) to the changelog - UTF-8/non-ASCII encodings - 'git am' error handling (e.g. wrong base) - more tests: --range-diff, --base=auto, 'am -s', etc. - GPG-signed commits [2] Out of scope for now: - Ted's suggestion of a new flag for the base [3] - in-transit mangling - minisigs - empty commits and/or merge commits [4] [1] https://lore.kernel.org/workflows/b9fb52b8-8168-6bf0-9a72-1e6c44a28...@oracle.com/ [2] https://lore.kernel.org/workflows/56664222-6c29-09dc-ef78-7b380b113...@oracle.com/ [3] https://lore.kernel.org/workflows/20191017144708.gi25...@mit.edu/ [4] https://lore.kernel.org/workflows/xmqqeezc83i6@gitster-ct.c.googlers.com/
[RFC PATCH v2 2/3] mailinfo: collect commit metadata from mail
"Metadata" here is used for the raw commit metadata which gets included
in a patch when using 'git format-patch --complete'.
The new email format is roughly:
1. email headers (ends with a blank line)
2. changelog (ends with "---" or when the diff starts)
3. comments (optional; ends when the diff starts)
4. diff itself (ends when there are no more files/hunks)
5. metadata (optional; starts with "--" and then "commit [...]")
6. signature (optional; starts with "--")
Traditionally, the comments and signature were counted as part of the diff,
and although the metadata now appears between the diff and the signature
(and is extracted into its own file) the signature is also output together
with the diff; this breaks no existing test cases and seems to be the most
backwards compatible.
Signed-off-by: Vegard Nossum
Previous-version: 51bb531eb57320caf3761680ebf77c25b89b3719
---
Documentation/git-mailinfo.txt | 6 +-
builtin/am.c | 2 +-
builtin/mailinfo.c | 13 ++-
mailinfo.c | 154 -
mailinfo.h | 9 +-
t/t5100-mailinfo.sh| 22 +
t/t5100/meta-info0001 | 5 ++
t/t5100/meta-meta0001 | 23 +
t/t5100/meta-msg0001 | 6 ++
t/t5100/meta-patch0001 | 76
t/t5100/meta-samples.mbox | 133
11 files changed, 439 insertions(+), 10 deletions(-)
create mode 100644 t/t5100/meta-info0001
create mode 100644 t/t5100/meta-meta0001
create mode 100644 t/t5100/meta-msg0001
create mode 100644 t/t5100/meta-patch0001
create mode 100644 t/t5100/meta-samples.mbox
diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index 3bbc731f67..14a2cca08c 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -9,7 +9,7 @@ git-mailinfo - Extracts patch and authorship from a single
e-mail message
SYNOPSIS
[verse]
-'git mailinfo' [-k|-b] [-u | --encoding= | -n] [--[no-]scissors]
+'git mailinfo' [-k|-b] [-u | --encoding= | -n] [--[no-]scissors]
[]
DESCRIPTION
@@ -21,6 +21,10 @@ written out to the standard output to be used by 'git am'
to create a commit. It is usually not necessary to use this
command directly. See linkgit:git-am[1] instead.
+If specified, specifies a filename where commit metadata
+will be written. If the e-mail does not contain such information,
+this file will be empty.
+
OPTIONS
---
diff --git a/builtin/am.c b/builtin/am.c
index 8181c2aef3..4190383bba 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1159,7 +1159,7 @@ static int parse_mail(struct am_state *state, const char
*mail)
mi.input = xfopen(mail, "r");
mi.output = xfopen(am_path(state, "info"), "w");
- if (mailinfo(&mi, am_path(state, "msg"), am_path(state, "patch")))
+ if (mailinfo(&mi, am_path(state, "msg"), am_path(state, "patch"),
am_path(state, "meta")))
die("could not parse patch");
fclose(mi.input);
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index cfb667a594..6c0746fa8e 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -9,14 +9,14 @@
#include "mailinfo.h"
static const char mailinfo_usage[] =
- "git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=
| -n] [--scissors | --no-scissors] < mail >info";
+ "git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=
| -n] [--scissors | --no-scissors] [] < mail >info";
int cmd_mailinfo(int argc, const char **argv, const char *prefix)
{
const char *def_charset;
struct mailinfo mi;
int status;
- char *msgfile, *patchfile;
+ char *msgfile, *patchfile, *metafile;
setup_mailinfo(&mi);
@@ -47,7 +47,7 @@ int cmd_mailinfo(int argc, const char **argv, const char
*prefix)
argc--; argv++;
}
- if (argc != 3)
+ if (argc < 3 || argc > 4)
usage(mailinfo_usage);
mi.input = stdin;
@@ -56,10 +56,15 @@ int cmd_mailinfo(int argc, const char **argv, const char
*prefix)
msgfile = prefix_filename(prefix, argv[1]);
patchfile = prefix_filename(prefix, argv[2]);
- status = !!mailinfo(&mi, msgfile, patchfile);
+ metafile = NULL;
+ if (argc == 4)
+ metafile = prefix_filename(prefix, argv[3]);
+
+ status = !!mailinfo(&mi, msgfile, patchfile, metafile);
clear_mailinfo(&mi);
free(msgfile);
free(patchfile);
+ free(metafile);
return status;
}
diff --git a/mailinfo.c b/mailinfo.c
index b395adbdf2..173cb58f6b 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -825,10 +825,139 @@ static int handle_commit_msg(struct mailinfo *mi, struct
strbuf *line)
return 0;
}
-static void handle_patch(struct mailinfo *mi, const struct strbuf *line)
+enum patch_parser_state {
+ SKIP_UNTIL_DIFF,
+ NEW_DIFF_OR_HUNK,
+
[RFC PATCH v2 1/3] format-patch: add --complete
This option causes the raw commit data to be inserted between the
changelog and the diffstat when you run git-format-patch. With a
following patch to 'git am', this will allow the exact reconstruction
of the commit to the point where the sha1 will be the same.
There is also a new config option controlling the default behaviour:
format.complete
Signed-off-by: Vegard Nossum
Previous-version: 622a0469a4970c5daac0c0323e2d6a77b3bebbdb
---
Documentation/config/format.txt| 7 ++
Documentation/git-format-patch.txt | 9
builtin/log.c | 35 ++
3 files changed, 51 insertions(+)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index 40cad9278f..3a38679837 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -87,6 +87,13 @@ format.useAutoBase::
A boolean value which lets you enable the `--base=auto` option of
format-patch by default.
+format.complete::
+ Provides the default value for the `--complete` option to
+ format-patch. If true, the raw commit metadata (including the
+ SHA1) is included at the bottom of the diff, before the signature.
+ This allows a recipient who has all the parent commits and/or the
+ tree to reconstruct the commit with the same SHA1.
+
format.notes::
Provides the default value for the `--notes` option to
format-patch. Accepts a boolean value, or a ref which specifies
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index 2035d4d5d5..74fc6d8a8c 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -26,6 +26,7 @@ SYNOPSIS
[--no-notes | --notes[=]]
[--interdiff=]
[--range-diff= [--creation-factor=]]
+ [--[no-]complete]
[--progress]
[]
[ | ]
@@ -325,6 +326,14 @@ you can use `--suffix=-patch` to get
`0001-description-of-my-change-patch`.
range are always formatted as creation patches, independently
of this flag.
+--[no-]complete::
+ Include the raw commit metadata (including the SHA1) at the
+ bottom of the diff, before the signature. This allows a
+ recipient who has all the parent commits and/or the tree to
+ reconstruct the commit with the same SHA1. The default is
+ `--no-complete`, unless the `format.complete` configuration
+ option is set.
+
--progress::
Show progress reports on stderr as patches are generated.
diff --git a/builtin/log.c b/builtin/log.c
index 89873d2dc2..822a0838b6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -783,6 +783,8 @@ enum {
COVER_AUTO
};
+static int fmt_complete;
+
static int git_format_config(const char *var, const char *value, void *cb)
{
struct rev_info *rev = cb;
@@ -888,6 +890,10 @@ static int git_format_config(const char *var, const char
*value, void *cb)
}
return 0;
}
+ if (!strcmp(var, "format.complete")) {
+ fmt_complete = git_config_bool(var, value);
+ return 0;
+ }
return git_log_config(var, value, cb);
}
@@ -1490,6 +1496,23 @@ static void print_bases(struct base_tree_info *bases,
FILE *file)
oidclr(&bases->base_commit);
}
+static void print_meta(struct rev_info *opt, struct commit *commit)
+{
+ const char *buffer = get_commit_buffer(commit, NULL);
+ const char *subject;
+
+ fprintf(opt->diffopt.file, "--\n");
+ fprintf(opt->diffopt.file, "commit %s\n",
oid_to_hex(&commit->object.oid));
+
+ /*
+* TODO: hex-encode to avoid mailer mangling?
+*/
+ if (find_commit_subject(buffer, &subject))
+ fprintf(opt->diffopt.file, "%.*s", (int) (subject - buffer),
buffer);
+ else
+ fprintf(opt->diffopt.file, "%s", buffer);
+}
+
static const char *diff_title(struct strbuf *sb, int reroll_count,
const char *generic, const char *rerolled)
{
@@ -1622,6 +1645,8 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
N_("add a signature")),
OPT_STRING(0, "base", &base_commit, N_("base-commit"),
N_("add prerequisite tree info to the patch
series")),
+ OPT_BOOL(0, "complete", &fmt_complete,
+N_("include all the information necessary to
reconstruct commit exactly")),
OPT_FILENAME(0, "signature-file", &signature_file,
N_("add a signature from a file")),
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
@@ -1921,6 +1946,14 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
prepare_bases(&bases, base, list, nr);
}
+ if (fmt
[RFC PATCH v2 3/3] am: add --exact
This uses exact metadata when creating the commit object, hopefully
reconstructing the commit with the exact same SHA1.
Note: In order to be forwards compatible with new commit formats we
may want a new helper for creating a commit with the exact metadata
that is present (and then validating the result) as opposed to trying
to parse the metadata and pass it piecewise to commit_tree().
Previous-version: 3120370db89f32e07a082edb4722db8feef1
Cc: Paul Tan
Signed-off-by: Vegard Nossum
---
Documentation/git-am.txt | 9 +++-
builtin/am.c | 111 +++
t/t4150-am.sh| 30 +++
3 files changed, 138 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fc3b993c33..5b75596aaf 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox
SYNOPSIS
[verse]
-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]
+'git am' [--[no-]exact] [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]
[--[no-]3way] [--interactive] [--committer-date-is-author-date]
[--ignore-date] [--ignore-space-change | --ignore-whitespace]
[--whitespace=] [-C] [-p] [--directory=]
@@ -31,6 +31,13 @@ OPTIONS
supply this argument, the command reads from the standard input.
If you supply directories, they will be treated as Maildirs.
+-e::
+--[no-]exact::
+ Reconstruct the exact commit that the patch was generated from,
+ assuming the mail contains complete metadata (i.e. it was generated
+ using `git format-patch --complete`). This is only possible if all
+ the parent commits are available in the repository.
+
-s::
--signoff::
Add a `Signed-off-by:` line to the commit message, using
diff --git a/builtin/am.c b/builtin/am.c
index 4190383bba..c0fc27a2ae 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -118,6 +118,7 @@ struct am_state {
int allow_rerere_autoupdate;
const char *sign_commit;
int rebasing;
+ int exact;
};
/**
@@ -399,6 +400,9 @@ static void am_load(struct am_state *state)
state->rebasing = !!file_exists(am_path(state, "rebasing"));
+ read_state_file(&sb, state, "exact", 1);
+ state->exact = !strcmp(sb.buf, "t");
+
strbuf_release(&sb);
}
@@ -1005,6 +1009,8 @@ static void am_setup(struct am_state *state, enum
patch_format patch_format,
else
write_state_text(state, "applying", "");
+ write_state_bool(state, "exact", state->exact);
+
if (!get_oid("HEAD", &curr_head)) {
write_state_text(state, "abort-safety", oid_to_hex(&curr_head));
if (!state->rebasing)
@@ -1548,40 +1554,121 @@ static int fall_back_threeway(const struct am_state
*state, const char *index_pa
*/
static void do_commit(const struct am_state *state)
{
+ struct object_id meta_commit = {};
+ struct object_id meta_tree = {};
+
struct object_id tree, parent, commit;
const struct object_id *old_oid;
struct commit_list *parents = NULL;
- const char *reflog_msg, *author;
+ const char *reflog_msg, *author = NULL;
struct strbuf sb = STRBUF_INIT;
+ if (state->exact) {
+ /*
+* Scan meta file for parents + other data.
+*
+* TODO: Pass everything after the "commit ..." line
+* verbatim to the commit for forwards compatibility
+* (e.g. so we don't need to know about every type of
+* commit attribute that may appear in the future).
+*/
+
+ struct strbuf line = STRBUF_INIT;
+ FILE *fp = xfopen(am_path(state, "meta"), "r");
+
+ while (!strbuf_getline_lf(&line, fp)) {
+ const char *rest;
+
+ if (skip_prefix(line.buf, "commit ", &rest)) {
+ if (get_oid_hex(rest, &meta_commit))
+ die("invalid exact metadata (commit)");
+ } else if (skip_prefix(line.buf, "tree ", &rest)) {
+ if (get_oid_hex(rest, &meta_tree))
+ die("invalid exact metadata (tree)");
+ } else if (skip_prefix(line.buf, "parent ", &rest)) {
+ if (get_oid_hex(rest, &parent))
+ die("invalid exact metadata (parent)");
+
+
commit_list_insert(lookup_commit(the_repository, &parent), &parents);
+ } else if (skip_prefix(line.buf, "author ", &rest)) {
+ author = strdup(rest);
+ } else if (skip_prefix(line.buf, "committer ", &rest)) {
+ char *name_copy;
+
Re: email as a bona fide git transport
On 10/20/19 8:28 AM, Vegard Nossum wrote: On 10/20/19 5:17 AM, Willy Tarreau wrote: On Fri, Oct 18, 2019 at 03:14:56PM -0400, Theodore Y. Ts'o wrote: On Fri, Oct 18, 2019 at 06:50:51PM +0200, Vegard Nossum wrote: The problem I ran into with putting the metadata at the end was detecting where the diff ends. A comment in 'git apply' suggested that detecting the difference between "--" as a diff/signature separator and as part of the diff is nontrivial in the sense that you need to actually do some parsing and keep track of hunk sizes. Could we cheat by having "git format-patch" add a "Diff-size" in the header which gives the number of lines in the diff so git am can just count lines to find the Trailer section? Be careful with this, it starts like this and ends up with non-editable patches. I'd rather have git-am use best-effort detection of the end. Expect filesystem developers to come up with a format that uses extents ;-) Also when dealing with stable backports, I've done a lot of "cat foo.diff >> bar.patch" to fixup some patches in which I just had to move some parts around. Having to count lines and edit a counter somewhere is going to become really painful. I almost have some new patches ready for putting the metadata after the patch using a very bare-bones diff parser (it's actually not that bad), I just need to fix a few corner cases that are causing breakage in the git test suite. I sent v2 of the patches (with metadata _after_ the diff) to the git list here: https://public-inbox.org/git/20191022114518.32055-1-vegard.nos...@oracle.com/T/#u As I wrote in there, we could already today start using git am --message-id when applying patches and this would provide something that a bot could annotate with git notes pointing to lore/LKML/LWN/whatever. I think that would already be a pretty nice improvement over today's situation. Sadly, since the beginning of 2018, this was only used for a measly ~0.14% of all non-merge commits in the kernel: $ git rev-list --count --no-merges --since='2018-01-01' --grep 'Message-Id: ' linus/master 178 $ git rev-list --count --no-merges --since='2018-01-01' linus/master 130777 So how can we spread the word about --message-id and get maintainers to actually use it? I don't suppose it's reasonable to change the 'git am' default setting? Vegard
Re: [Outreachy] First contribution
El lun., 21 oct. 2019 a las 20:35, Emily Shaffer () escribió: > > On Mon, Oct 21, 2019 at 12:39:16PM +0200, Miriam R. wrote: > > Dear Git developers, > > I’m an Outreachy applicant, I would like to make my contribution to > > apply to this Outreachy internship period. > > Welcome, Miriam! Good to hear from you. > > > > > I have found this issue tagged as open and goodfirstissue: > > https://github.com/gitgitgadget/git/issues/230 > > > > But there is a PR from 4 months ago: > > https://github.com/gitgitgadget/git/pull/271 and I don't know how to > > find out if a patch including that change already exists or if it > > makes sense to do it. > > GitGitGadget exists to repackage PRs (which Git project doesn't use) > into emailed patches (which Git project does use) when the author writes > /submit on the PR comment chain. In that PR I see Johannes asking for a > /submit, but no submit; next I would check if a patch with the same > title came through in the mailing list by searching on the > public-inbox.org mirror: > > https://public-inbox.org/git/?q=is_directory+dir_exists > > Looks like, no, a patch with those hotwords wasn't mailed. Finally, I > would check the project to see if it's still an issue: > > $ cd my-git-dir/ > $ git grep is_directory > > I still see 30 instances of is_directory in the codebase, so looks like > we haven't made this change. :) > Thank you Emily!! Then I'll do this issue #230 :) > > > > In case this issue is not suitable for my first contribution, I have > > also found this: > > https://github.com/gitgitgadget/git/issues/379 > > This is also a fine change if you want to make it. > > Good luck, and remember it's fine to ask the mentor for the project you > ultimately want to help on for help, code review in advance, etc. Thank you for the advice!, I'm already in touch with Christian Couder. Best, Miriam > > - Emily
Re: email as a bona fide git transport
On Tue, Oct 22, 2019 at 02:11:22PM +0200, Vegard Nossum wrote:
>
> As I wrote in there, we could already today start using
>
> git am --message-id
>
> when applying patches and this would provide something that a bot could
> annotate with git notes pointing to lore/LKML/LWN/whatever. I think that
> would already be a pretty nice improvement over today's situation.
>
> Sadly, since the beginning of 2018, this was only used for a measly
> ~0.14% of all non-merge commits in the kernel:
>
> $ git rev-list --count --no-merges --since='2018-01-01' --grep 'Message-Id:
> ' linus/master
> 178
You might also want to count commits which have a link tag with a
Message-Id:
Link:
https://lore.kernel.org/r/c3438dad66a34a7d4e7509a5dd64c2326340a52a.1571647180.git.mbobrow...@mbobrowski.org
That's because some kernel developers have been using a hook script like this:
#!/bin/sh
# For .git/hooks/applypatch-msg
#
# You must have the following in .git/config:
# [am]
# messageid = true
. git-sh-setup
perl -pi -e 's|^Message-Id:\s*]+)>?$|Link:
https://lore.kernel.org/r/$1|g;' "$1"
test -x "$GIT_DIR/hooks/commit-msg" &&
exec "$GIT_DIR/hooks/commit-msg" ${1+"$@"}
:
as we had reached rough consensus that this was the best way to
incorprate the message id (since it could made to be a clickable link
in tools like gitk, for example). This rough consensus has only been
in place since around the time of the Maintainer's Summit in Lisbon,
so uptake is still probably a bit slow. I'd expect to see a lot more
of this in the next merge window, though.
- Ted
Re: [GSoC] Follow-up post
Hi Matheus, On Mon, Oct 21, 2019 at 7:14 PM Matheus Tavares Bernardino wrote: > > I wrote a small follow-up post to talk about the conclusion of my GSoC > project. I believe the main remaining tasks are now finally complete > :) If you would be interested in taking a look, the post is at > https://matheustavares.gitlab.io/posts/gsoc-follow-ups Thank you for the follow-up blog post! It is a great explanation of the work you did to come up with the current mt/threaded-grep-in-object-store! Best, Christian.
[PATCH 1/1] vreportf: Fix interleaving issues, remove 4096 limitation
From: Alexandr Miloslavskiy
This also fixes t5516 on Windows VS build.
For detailed explanation please refer to code comments in this commit.
There was a lot of back-and-forth already in vreportf():
d048a96e (2007-11-09) - 'char msg[256]' is introduced to avoid interleaving
389d1767 (2009-03-25) - Buffer size increased to 1024 to avoid truncation
625a860c (2009-11-22) - Buffer size increased to 4096 to avoid truncation
f4c3edc0 (2015-08-11) - Buffer removed to avoid truncation
b5a9e435 (2017-01-11) - Reverts f4c3edc0 to be able to replace control
chars before sending to stderr
This fix attempts to solve all issues:
1) avoid multiple fprintf() interleaving
2) avoid truncation
3) avoid char interleaving in fprintf() on some platforms
4) avoid buffer block interleaving when output is large
5) avoid out-of-order messages
6) replace control characters in output
Other commits worthy of notice:
9ac13ec9 (2006-10-11) - Another attempt to solve interleaving.
This is seemingly related to
d048a96e.
137a0d0e (2007-11-19) - Addresses out-of-order for display()
34df8aba (2009-03-10) - Switches xwrite() to fprintf() in recv_sideband()
to support UTF-8 emulation
eac14f89 (2012-01-14) - Removes the need for fprintf() for UTF-8 emulation,
so it's safe to use xwrite()
again
5e5be9e2 (2016-06-28) - recv_sideband() uses xwrite() again
Signed-off-by: Alexandr Miloslavskiy
---
usage.c | 154 +---
1 file changed, 148 insertions(+), 6 deletions(-)
diff --git a/usage.c b/usage.c
index 2fdb20086b..ccdd91a7b9 100644
--- a/usage.c
+++ b/usage.c
@@ -6,17 +6,159 @@
#include "git-compat-util.h"
#include "cache.h"
+static void replace_control_chars(char* str, size_t size, char replacement)
+{
+ size_t i;
+
+ for (i = 0; i < size; i++) {
+ if (iscntrl(str[i]) && str[i] != '\t' && str[i] != '\n')
+ str[i] = replacement;
+ }
+}
+
+/*
+ * Atomically report (prefix + vsnprintf(err, params) + '\n') to stderr.
+ * Always returns desired buffer size.
+ * Doesn't write to stderr if content didn't fit.
+ *
+ * This function composes everything into a single buffer before
+ * sending to stderr. This is to defeat various non-atomic issues:
+ * 1) If stderr is fully buffered:
+ *the ordering of stdout and stderr messages could be wrong,
+ *because stderr output waits for the buffer to become full.
+ * 2) If stderr has any type of buffering:
+ *buffer has fixed size, which could lead to interleaved buffer
+ *blocks when two threads/processes write at the same time.
+ * 3) If stderr is not buffered:
+ *There are two problems, one with atomic fprintf() and another
+ *for non-atomic fprintf(), and both occur depending on platform
+ *(see details below). If atomic, this function still writes 3
+ *parts, which could get interleaved with multiple threads. If
+ *not atomic, then fprintf() will basically write char-by-char,
+ *which leads to unreadable char-interleaved writes if two
+ *processes write to stderr at the same time (threads are OK
+ *because fprintf() usually locks file in current process). This
+ *for example happens in t5516 where 'git-upload-pack' detects
+ *an error, reports it to parent 'git fetch' and both die() at the
+ *same time.
+ *
+ *Behaviors, at the moment of writing:
+ *a) libc - fprintf()-interleaved
+ * fprintf() enables temporary stream buffering.
+ * See: buffered_vfprintf()
+ *b) VC++ - char-interleaved
+ * fprintf() enables temporary stream buffering, but only if
+ * stream was not set to no buffering. This has no effect,
+ * because stderr is not buffered by default, and git takes
+ * an extra step to ensure that in swap_osfhnd().
+ * See: _iob[_IOB_ENTRIES],
+ *__acrt_stdio_temporary_buffering_guard,
+ *has_any_buffer()
+ *c) MinGW - char-interleaved (console), full buffering (file)
+ * fprintf() obeys stderr buffering. But it uses old MSVCRT.DLL,
+ * which eventually calls _flsbuf(), which enables buffering unless
+ * isatty(stderr) or buffering is disabled. Buffering is not disabled
+ * by default for stderr. Therefore, buffering is enabled for
+ * file-redirected stderr.
+ * See: __mingw_vfprintf(),
+ *__pformat_wcputs(),
+ *_fputc_nolock(),
+ *_flsbuf(),
+ *_iob[_IOB_ENTRIES]
+ * 4) If stderr is line buffered: MinGW/VC++ will enable full
+ *buffering instead. See MSDN setvbuf().
+ */
+static size_t vreportf_buf(char *buf, size_t buf_size, const char *prefix,
const char *err, va_list params)
+{
+ int printf_ret = 0;
+ size_t prefix_size = 0;
+ size_t total_size = 0;
+
+ /*
+* NOTE
[PATCH 0/1] vreportf: Fix interleaving issues, remove 4096 limitation
This fixes t5516 on Windows. For detailed explanation please refer to code comments in this commit. There was a lot of back-and-forth already in vreportf(): d048a96e (2007-11-09) - 'char msg[256]' is introduced to avoid interleaving 389d1767 (2009-03-25) - Buffer size increased to 1024 to avoid truncation 625a860c (2009-11-22) - Buffer size increased to 4096 to avoid truncation f4c3edc0 (2015-08-11) - Buffer removed to avoid truncation b5a9e435 (2017-01-11) - Reverts f4c3edc0 to be able to replace control chars before sending to stderr This fix attempts to solve all issues: 1) avoid multiple fprintf() interleaving 2) avoid truncation 3) avoid char interleaving in fprintf() on some platforms 4) avoid buffer block interleaving when output is large 5) avoid Out-of-order messages 6) replace control characters in output Other commits worthy of notice: 9ac13ec9 (2006-10-11) - Another attempt to solve interleaving. This is seemingly related to d048a96e. 137a0d0e (2007-11-19) - Addresses out-of-order for display() 34df8aba (2009-03-10) - Switches xwrite() to fprintf() in recv_sideband() to support UTF-8 emulation eac14f89 (2012-01-14) - Removes the need for fprintf() for UTF-8 emulation, so it's safe to use xwrite() again 5e5be9e2 (2016-06-28) - recv_sideband() uses xwrite() again Signed-off-by: Alexandr Miloslavskiy alexandr.miloslavs...@syntevo.com [alexandr.miloslavs...@syntevo.com] Alexandr Miloslavskiy (1): vreportf: Fix interleaving issues, remove 4096 limitation usage.c | 154 +--- 1 file changed, 148 insertions(+), 6 deletions(-) base-commit: d966095db01190a2196e31195ea6fa0c722aa732 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-407%2FSyntevoAlex%2F%230194_t5516_fixes-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-407/SyntevoAlex/#0194_t5516_fixes-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/407 -- gitgitgadget
[PATCH v2 1/1] vreportf: Fix interleaving issues, remove 4096 limitation
From: Alexandr Miloslavskiy
This also fixes t5516 on Windows VS build.
For detailed explanation please refer to code comments in this commit.
There was a lot of back-and-forth already in vreportf():
d048a96e (2007-11-09) - 'char msg[256]' is introduced to avoid interleaving
389d1767 (2009-03-25) - Buffer size increased to 1024 to avoid truncation
625a860c (2009-11-22) - Buffer size increased to 4096 to avoid truncation
f4c3edc0 (2015-08-11) - Buffer removed to avoid truncation
b5a9e435 (2017-01-11) - Reverts f4c3edc0 to be able to replace control
chars before sending to stderr
This fix attempts to solve all issues:
1) avoid multiple fprintf() interleaving
2) avoid truncation
3) avoid char interleaving in fprintf() on some platforms
4) avoid buffer block interleaving when output is large
5) avoid out-of-order messages
6) replace control characters in output
Other commits worthy of notice:
9ac13ec9 (2006-10-11) - Another attempt to solve interleaving.
This is seemingly related to d048a96e.
137a0d0e (2007-11-19) - Addresses out-of-order for display()
34df8aba (2009-03-10) - Switches xwrite() to fprintf() in recv_sideband()
to support UTF-8 emulation
eac14f89 (2012-01-14) - Removes the need for fprintf() for UTF-8 emulation,
so it's safe to use xwrite() again
5e5be9e2 (2016-06-28) - recv_sideband() uses xwrite() again
Signed-off-by: Alexandr Miloslavskiy
---
usage.c | 154 +---
1 file changed, 148 insertions(+), 6 deletions(-)
diff --git a/usage.c b/usage.c
index 2fdb20086b..ccdd91a7b9 100644
--- a/usage.c
+++ b/usage.c
@@ -6,17 +6,159 @@
#include "git-compat-util.h"
#include "cache.h"
+static void replace_control_chars(char* str, size_t size, char replacement)
+{
+ size_t i;
+
+ for (i = 0; i < size; i++) {
+ if (iscntrl(str[i]) && str[i] != '\t' && str[i] != '\n')
+ str[i] = replacement;
+ }
+}
+
+/*
+ * Atomically report (prefix + vsnprintf(err, params) + '\n') to stderr.
+ * Always returns desired buffer size.
+ * Doesn't write to stderr if content didn't fit.
+ *
+ * This function composes everything into a single buffer before
+ * sending to stderr. This is to defeat various non-atomic issues:
+ * 1) If stderr is fully buffered:
+ *the ordering of stdout and stderr messages could be wrong,
+ *because stderr output waits for the buffer to become full.
+ * 2) If stderr has any type of buffering:
+ *buffer has fixed size, which could lead to interleaved buffer
+ *blocks when two threads/processes write at the same time.
+ * 3) If stderr is not buffered:
+ *There are two problems, one with atomic fprintf() and another
+ *for non-atomic fprintf(), and both occur depending on platform
+ *(see details below). If atomic, this function still writes 3
+ *parts, which could get interleaved with multiple threads. If
+ *not atomic, then fprintf() will basically write char-by-char,
+ *which leads to unreadable char-interleaved writes if two
+ *processes write to stderr at the same time (threads are OK
+ *because fprintf() usually locks file in current process). This
+ *for example happens in t5516 where 'git-upload-pack' detects
+ *an error, reports it to parent 'git fetch' and both die() at the
+ *same time.
+ *
+ *Behaviors, at the moment of writing:
+ *a) libc - fprintf()-interleaved
+ * fprintf() enables temporary stream buffering.
+ * See: buffered_vfprintf()
+ *b) VC++ - char-interleaved
+ * fprintf() enables temporary stream buffering, but only if
+ * stream was not set to no buffering. This has no effect,
+ * because stderr is not buffered by default, and git takes
+ * an extra step to ensure that in swap_osfhnd().
+ * See: _iob[_IOB_ENTRIES],
+ *__acrt_stdio_temporary_buffering_guard,
+ *has_any_buffer()
+ *c) MinGW - char-interleaved (console), full buffering (file)
+ * fprintf() obeys stderr buffering. But it uses old MSVCRT.DLL,
+ * which eventually calls _flsbuf(), which enables buffering unless
+ * isatty(stderr) or buffering is disabled. Buffering is not disabled
+ * by default for stderr. Therefore, buffering is enabled for
+ * file-redirected stderr.
+ * See: __mingw_vfprintf(),
+ *__pformat_wcputs(),
+ *_fputc_nolock(),
+ *_flsbuf(),
+ *_iob[_IOB_ENTRIES]
+ * 4) If stderr is line buffered: MinGW/VC++ will enable full
+ *buffering instead. See MSDN setvbuf().
+ */
+static size_t vreportf_buf(char *buf, size_t buf_size, const char *prefix,
const char *err, va_list params)
+{
+ int printf_ret = 0;
+ size_t prefix_size = 0;
+ size_t total_size = 0;
+
+ /*
+* NOTE: Can't use strbuf functions here, because it can be called when
+
[PATCH v2 0/1] vreportf: Fix interleaving issues, remove 4096 limitation
This fixes t5516 on Windows. For detailed explanation please refer to code comments in this commit. There was a lot of back-and-forth already in vreportf(): d048a96e (2007-11-09) - 'char msg[256]' is introduced to avoid interleaving 389d1767 (2009-03-25) - Buffer size increased to 1024 to avoid truncation 625a860c (2009-11-22) - Buffer size increased to 4096 to avoid truncation f4c3edc0 (2015-08-11) - Buffer removed to avoid truncation b5a9e435 (2017-01-11) - Reverts f4c3edc0 to be able to replace control chars before sending to stderr This fix attempts to solve all issues: 1) avoid multiple fprintf() interleaving 2) avoid truncation 3) avoid char interleaving in fprintf() on some platforms 4) avoid buffer block interleaving when output is large 5) avoid Out-of-order messages 6) replace control characters in output Other commits worthy of notice: 9ac13ec9 (2006-10-11) - Another attempt to solve interleaving. This is seemingly related to d048a96e. 137a0d0e (2007-11-19) - Addresses out-of-order for display() 34df8aba (2009-03-10) - Switches xwrite() to fprintf() in recv_sideband() to support UTF-8 emulation eac14f89 (2012-01-14) - Removes the need for fprintf() for UTF-8 emulation, so it's safe to use xwrite() again 5e5be9e2 (2016-06-28) - recv_sideband() uses xwrite() again Signed-off-by: Alexandr Miloslavskiy alexandr.miloslavs...@syntevo.com [alexandr.miloslavs...@syntevo.com] Alexandr Miloslavskiy (1): vreportf: Fix interleaving issues, remove 4096 limitation usage.c | 154 +--- 1 file changed, 148 insertions(+), 6 deletions(-) base-commit: d966095db01190a2196e31195ea6fa0c722aa732 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-407%2FSyntevoAlex%2F%230194_t5516_fixes-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-407/SyntevoAlex/#0194_t5516_fixes-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/407 Range-diff vs v1: 1: 5fd06fa881 ! 1: 54f0d6f6b5 vreportf: Fix interleaving issues, remove 4096 limitation @@ -23,12 +23,12 @@ Other commits worthy of notice: 9ac13ec9 (2006-10-11) - Another attempt to solve interleaving. -This is seemingly related to d048a96e. +This is seemingly related to d048a96e. 137a0d0e (2007-11-19) - Addresses out-of-order for display() 34df8aba (2009-03-10) - Switches xwrite() to fprintf() in recv_sideband() -to support UTF-8 emulation +to support UTF-8 emulation eac14f89 (2012-01-14) - Removes the need for fprintf() for UTF-8 emulation, -so it's safe to use xwrite() again +so it's safe to use xwrite() again 5e5be9e2 (2016-06-28) - recv_sideband() uses xwrite() again Signed-off-by: Alexandr Miloslavskiy -- gitgitgadget
Re: [git-for-windows] Git for Windows v2.24.0-rc0, was Re: [ANNOUNCE] Git v2.24.0-rc0
Hi Dscho, Install went Ok. Did a quick test on the config locations and `git config -l -show-origin` has 'lost' the ProgramData location as planned. The minor pedant did notice that the new location is listed slightly differently from the release notes. `file:C:/Program Files/Git/mingw64/../etc/gitconfig` --system, while the release notes simplify the path to C:/Program Files/Git/etc/gitconfig Dunno if that's worth a minor fix to the release notes to clarify for the broader Windows community. Maybe others can comment if they think it's even worth it. Philip On 21/10/2019 23:05, Johannes Schindelin wrote: Team, a couple of days later than I wanted, but at least it is now here: https://github.com/git-for-windows/git/releases/tag/v2.24.0-rc0.windows.1 Please test... Thank you, Johannes
Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
On Mon, Oct 21, 2019 at 09:54:42PM +, Johannes Schindelin via GitGitGadget
wrote:
> From: Johannes Schindelin
>
> Ever since worktrees were introduced, the `git_path()` function _really_
> needed to be called e.g. to get at the path to `logs/HEAD` (`HEAD` is
> specific to the worktree, and therefore so is its reflog). However, the
> wrong path is returned for `logs/HEAD.lock`.
>
> This does not matter as long as the Git executable is doing the asking,
> as the path for that `logs/HEAD.lock` file is constructed from
> `git_path("logs/HEAD")` by appending the `.lock` suffix.
>
> However, Git GUI just learned to use `--git-path` instead of appending
> relative paths to what `git rev-parse --git-dir` returns (and as a
> consequence not only using the correct hooks directory, but also using
> the correct paths in worktrees other than the main one). While it does
> not seem as if Git GUI in particular is asking for `logs/HEAD.lock`,
> let's be safe rather than sorry.
>
> Side note: Git GUI _does_ ask for `index.lock`, but that is already
> resolved correctly, due to `update_common_dir()` preferring to leave
> unknown paths in the (worktree-specific) git directory.
>
> Signed-off-by: Johannes Schindelin
> ---
> path.c | 8 +++-
> t/t1500-rev-parse.sh | 15 +++
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index e3da1f3c4e..5ff64e7a8a 100644
> --- a/path.c
> +++ b/path.c
> @@ -349,10 +349,16 @@ static int check_common(const char *unmatched, void
> *value, void *baton)
> static void update_common_dir(struct strbuf *buf, int git_dir_len,
> const char *common_dir)
> {
> - char *base = buf->buf + git_dir_len;
> + char *base = buf->buf + git_dir_len, *base2 = NULL;
> +
> + if (ends_with(base, ".lock"))
> + base = base2 = xstrndup(base, strlen(base) - 5);
Hm, this adds the magic number 5 and calls strlen(base) twice, because
ends_with() calls strip_suffix(), which calls strlen(). Calling
strip_suffix() directly would allow us to avoid the magic number and
the second strlen():
size_t len;
if (strip_suffix(base, ".lock", &len))
base = base2 = xstrndup(base, len);
> init_common_trie();
> if (trie_find(&common_trie, base, check_common, NULL) > 0)
> replace_dir(buf, git_dir_len, common_dir);
> +
> + free(base2);
> }
>
> void report_linked_checkout_garbage(void)
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 01abee533d..d318a1eeef 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
> test_cmp expect actual
> '
>
> +test_expect_success 'git-path in worktree' '
> + test_tick &&
> + git commit --allow-empty -m empty &&
> + git worktree add --detach wt &&
> + test_write_lines >expect \
> + "$(pwd)/.git/worktrees/wt/logs/HEAD" \
> + "$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
> + "$(pwd)/.git/worktrees/wt/index" \
> + "$(pwd)/.git/worktrees/wt/index.lock" &&
> + git -C wt rev-parse >actual \
> + --git-path logs/HEAD --git-path logs/HEAD.lock \
> + --git-path index --git-path index.lock &&
> + test_cmp expect actual
> +'
I think this test better fits into 't0060-path-utils.sh': it already
checks 'logs/HEAD' and 'index' in a different working tree (well, with
GIT_COMMON_DIR set, but that's the same), and it has a helper function
to make each of the two new .lock tests a one-liner.
> test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
> test_commit test_commit &&
> echo true >expect &&
> --
> gitgitgadget
Re: email as a bona fide git transport
On 10/22/19 3:53 PM, Theodore Y. Ts'o wrote:
On Tue, Oct 22, 2019 at 02:11:22PM +0200, Vegard Nossum wrote:
As I wrote in there, we could already today start using
git am --message-id
when applying patches and this would provide something that a bot could
annotate with git notes pointing to lore/LKML/LWN/whatever. I think that
would already be a pretty nice improvement over today's situation.
Sadly, since the beginning of 2018, this was only used for a measly
~0.14% of all non-merge commits in the kernel:
$ git rev-list --count --no-merges --since='2018-01-01' --grep 'Message-Id:
' linus/master
178
You might also want to count commits which have a link tag with a
Message-Id:
Link:
https://lore.kernel.org/r/c3438dad66a34a7d4e7509a5dd64c2326340a52a.1571647180.git.mbobrow...@mbobrowski.org
That's because some kernel developers have been using a hook script like this:
#!/bin/sh
# For .git/hooks/applypatch-msg
#
# You must have the following in .git/config:
# [am]
# messageid = true
. git-sh-setup
perl -pi -e 's|^Message-Id:\s*]+)>?$|Link: https://lore.kernel.org/r/$1|g;'
"$1"
test -x "$GIT_DIR/hooks/commit-msg" &&
exec "$GIT_DIR/hooks/commit-msg" ${1+"$@"}
:
as we had reached rough consensus that this was the best way to
incorprate the message id (since it could made to be a clickable link
in tools like gitk, for example). This rough consensus has only been
in place since around the time of the Maintainer's Summit in Lisbon,
so uptake is still probably a bit slow. I'd expect to see a lot more
of this in the next merge window, though.
Thanks, I was not aware of this!
Seems like something that should go in Documentation/maintainer/,
right?
The figure is much better, 16.7% on all non-merges since 2018-01-01.
This should help and we can maybe already do some interesting things
with git notes and lore/public-inbox.
Vegard
is commitGraph useful on the server side?
Hi, all: I've read the docs on commitGraph and it's not 100% clear to me if turning it on and generating commit graphs would be useful on the server-side. I know it's going to be enabled by default and automatically generated whenever "git gc" runs, so I'm trying to figure out if it'll be useful for git-daemon operations. Thanks in advance for your help. Best, -K
[PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
From: Derrick Stolee
While dogfooding, Johannes found a bug in the fetch.writeCommitGraph
config behavior. While his example initially happened during a clone
with --recurse-submodules, we found that it is actually a problem with
the first fetch after a new clone and no existing commit-graph file:
$ git clone test
$ cd test
$ git -c fetch.writeCommitGraph=true fetch origin
Computing commit graph generation numbers: 100% (12/12), done.
BUG: commit-graph.c:886: missing parent for commit
Aborted (core dumped)
In the repo I had cloned, there were really 60 commits to scan, but
only 12 were in the list to write when calling
compute_generation_numbers(). A commit in the list expects to see a
parent, but that parent is not in the list.
As an initial test, I converted the code in builtin/fetch.c that calls
write_commit_graph_reachable() to instead launch a "git commit-graph
write --reachable --split" process. That code worked, but is not how we
want the feature to work long-term.
That test did demonstrate that the issue must be something to do with
internal state of the 'git fetch' process.
The write_commit_graph() method in commit-graph.c ensures the commits we
plan to write are "closed under reachability" using close_reachable().
This method walks from the input commits, and uses the UNINTERESTING
flag to mark which commits have already been visited. This allows the
walk to take O(N) time, where N is the number of commits, instead of
O(P) time, where P is the number of paths. (The number of paths can be
exponential in the number of commits.)
However, the UNINTERESTING flag is used in lots of places in the
codebase. This flag usually means some barrier to stop a commit walk,
such as in revision-walking to compare histories. It is not often
cleared after the walk completes because the starting points of those
walks do not have the UNINTERESTING flag, and clear_commit_marks() would
stop immediately.
This is happening during a 'git fetch' call with a remote. The fetch
negotiation is comparing the remote refs with the local refs and marking
some commits as UNINTERESTING.
You may ask: did this feature ever work at all? Yes, it did, as long as
you had a commit-graph covering all of your local refs. My testing was
unfortunately limited to this scenario. The UNINTERESTING commits are
always part of the "old" commit-graph, and when we add new commits to a
top layer of the commit-graph chain those are not needed. If we happen
to merge layers of the chain, then the commits are added as a list, not
using a commit walk. Further, the test added for this config option in
t5510-fetch.sh uses local filesystem clones, which somehow avoids this
logic.
I tested running clear_commit_marks_many() to clear the UNINTERESTING
flag inside close_reachable(), but the tips did not have the flag, so
that did nothing.
Instead, I finally arrived on the conclusion that I should use a flag
that is not used in any other part of the code. In commit-reach.c, a
number of flags were defined for commit walk algorithms. The REACHABLE
flag seemed like it made the most sense, and it seems it was not
actually used in the file.
Add the REACHABLE flag to commit-graph.c and use it instead of
UNINTERESTING in close_reachable(). This fixes the bug in manual
testing.
I have failed to produce a test using the file:// protocol that
demonstrates this bug.
Reported-by: Johannes Schindelin
Signed-off-by: Derrick Stolee
---
commit-graph.c | 11 +++
commit-reach.c | 1 -
object.h | 3 ++-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index fc4a43b8d6..0aea7b2ae5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -41,6 +41,9 @@
#define GRAPH_MIN_SIZE (GRAPH_HEADER_SIZE + 4 * GRAPH_CHUNKLOOKUP_WIDTH \
+ GRAPH_FANOUT_SIZE + the_hash_algo->rawsz)
+/* Remember to update object flag allocation in object.h */
+#define REACHABLE (1u<<15)
+
char *get_commit_graph_filename(const char *obj_dir)
{
char *filename = xstrfmt("%s/info/commit-graph", obj_dir);
@@ -1030,11 +1033,11 @@ static void add_missing_parents(struct
write_commit_graph_context *ctx, struct c
{
struct commit_list *parent;
for (parent = commit->parents; parent; parent = parent->next) {
- if (!(parent->item->object.flags & UNINTERESTING)) {
+ if (!(parent->item->object.flags & REACHABLE)) {
ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1,
ctx->oids.alloc);
oidcpy(&ctx->oids.list[ctx->oids.nr],
&(parent->item->object.oid));
ctx->oids.nr++;
- parent->item->object.flags |= UNINTERESTING;
+ parent->item->object.flags |= REACHABLE;
}
}
}
@@ -1052,7 +1055,7 @@ static void close_reachable(struct
write_commit_graph_context *ctx)
display_progress(ctx->p
[PATCH 0/1] [v2.24.0-rc0 BUG] fetch.writeCommitGraph fails on first fetch
While dogfooding, Johannes found a bug in the fetch.writeCommitGraph config behavior. While his example initially happened during a clone with --recurse-submodules, we found that it is actually a problem with the first fetch after a new clone and no existing commit-graph file: $ git clone test $ cd test $ git -c fetch.writeCommitGraph=true fetch origin Computing commit graph generation numbers: 100% (12/12), done. BUG: commit-graph.c:886: missing parent for commit Aborted (core dumped) In the repo I had cloned, there were really 60 commits to scan, but only 12 were in the list to write when calling compute_generation_numbers(). A commit in the list expects to see a parent, but that parent is not in the list. The details above are the start of the commit message for [PATCH 1/1]. I tried to include as much detail as I could for how I investigated the problem and why I think this is the right solution. I would like help creating a test that demonstrates this problem. It does not appear to trigger on the simplest examples. The simple example I used for my testing was https://github.com/derrickstolee/numbers Thanks, -Stolee /cc johannes.schinde...@gmx.de, p...@peff.net Derrick Stolee (1): commit-graph: fix writing first commit-graph during fetch commit-graph.c | 11 +++ commit-reach.c | 1 - object.h | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-) base-commit: d966095db01190a2196e31195ea6fa0c722aa732 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-415%2Fderrickstolee%2Ffetch-first-write-fail-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-415/derrickstolee/fetch-first-write-fail-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/415 -- gitgitgadget
Re: [PATCH v2 3/9] ewah/bitmap: introduce bitmap_word_alloc()
> From: Jeff King > > In a following commit we will need to allocate a variable > number of bitmap words, instead of always 32, so let's add > bitmap_word_alloc() for this purpose. > > We will also always access at least one word for each bitmap, > so we want to make sure that at least one is always > allocated. The last paragraph is not true - we still can allocate 0. (We just ensure that when we grow, we grow to at least 1.) I think we should just delete the last paragraph - the first paragraph is sufficient. Other than that, all patches up to but not including the last one look good, except the ones that just add a new function because I'll have to review the last patch to see how they are used.
[PATCH 1/1] documentation: remove empty doc files
From: Heba Waly
Remove empty and redundant documentation files from the
Documentation/technical/ directory.
As part of moving the documentation from Documentation/technical/api-* to
header files, the following files are deleted because they include only
TODO messages with no documentation to be moved:
Documentation/technical/api-grep.txt
Documentation/technical/api-object-access.txt
Documentation/technical/api-quote.txt
Documentation/technical/api-xdiff-interface.txt
Signed-off-by: Heba Waly
---
Documentation/technical/api-grep.txt| 8
Documentation/technical/api-object-access.txt | 15 ---
Documentation/technical/api-quote.txt | 10 --
Documentation/technical/api-xdiff-interface.txt | 7 ---
4 files changed, 40 deletions(-)
delete mode 100644 Documentation/technical/api-grep.txt
delete mode 100644 Documentation/technical/api-object-access.txt
delete mode 100644 Documentation/technical/api-quote.txt
delete mode 100644 Documentation/technical/api-xdiff-interface.txt
diff --git a/Documentation/technical/api-grep.txt
b/Documentation/technical/api-grep.txt
deleted file mode 100644
index a69cc8964d..00
--- a/Documentation/technical/api-grep.txt
+++ /dev/null
@@ -1,8 +0,0 @@
-grep API
-
-
-Talk about , things like:
-
-* grep_buffer()
-
-(JC)
diff --git a/Documentation/technical/api-object-access.txt
b/Documentation/technical/api-object-access.txt
deleted file mode 100644
index 5b29622d00..00
--- a/Documentation/technical/api-object-access.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-object access API
-=
-
-Talk about and family, things like
-
-* read_sha1_file()
-* read_object_with_reference()
-* has_sha1_file()
-* write_sha1_file()
-* pretend_object_file()
-* lookup_{object,commit,tag,blob,tree}
-* parse_{object,commit,tag,blob,tree}
-* Use of object flags
-
-(JC, Shawn, Daniel, Dscho, Linus)
diff --git a/Documentation/technical/api-quote.txt
b/Documentation/technical/api-quote.txt
deleted file mode 100644
index e8a1bce94e..00
--- a/Documentation/technical/api-quote.txt
+++ /dev/null
@@ -1,10 +0,0 @@
-quote API
-=
-
-Talk about , things like
-
-* sq_quote and unquote
-* c_style quote and unquote
-* quoting for foreign languages
-
-(JC)
diff --git a/Documentation/technical/api-xdiff-interface.txt
b/Documentation/technical/api-xdiff-interface.txt
deleted file mode 100644
index 6296ecad1d..00
--- a/Documentation/technical/api-xdiff-interface.txt
+++ /dev/null
@@ -1,7 +0,0 @@
-xdiff interface API
-===
-
-Talk about our calling convention to xdiff library, including
-xdiff_emit_consume_fn.
-
-(Dscho, JC)
--
gitgitgadget
[PATCH 0/1] [Outreachy] documentation: remove empty doc files
Remove empty and redundant documentation files from the Documentation/technical/ directory. As part of moving the documentation from Documentation/technical/api-* to header files, the following files are deleted because they include only TODO messages with no documentation: Documentation/technical/api-grep.txt Documentation/technical/api-object-access.txt Documentation/technical/api-quote.txt Documentation/technical/api-xdiff-interface.txt Signed-off-by: Heba Waly heba.w...@gmail.com [heba.w...@gmail.com] Heba Waly (1): documentation: remove empty doc files Documentation/technical/api-grep.txt| 8 Documentation/technical/api-object-access.txt | 15 --- Documentation/technical/api-quote.txt | 10 -- Documentation/technical/api-xdiff-interface.txt | 7 --- 4 files changed, 40 deletions(-) delete mode 100644 Documentation/technical/api-grep.txt delete mode 100644 Documentation/technical/api-object-access.txt delete mode 100644 Documentation/technical/api-quote.txt delete mode 100644 Documentation/technical/api-xdiff-interface.txt base-commit: d966095db01190a2196e31195ea6fa0c722aa732 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-412%2FHebaWaly%2Fdelete_empty_docs-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-412/HebaWaly/delete_empty_docs-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/412 -- gitgitgadget
Re: email as a bona fide git transport
Vegard Nossum wrote: > I sent v2 of the patches (with metadata _after_ the diff) to the git > list here: > > https://public-inbox.org/git/20191022114518.32055-1-vegard.nos...@oracle.com/T/#u > > As I wrote in there, we could already today start using > >git am --message-id > > when applying patches and this would provide something that a bot could > annotate with git notes pointing to lore/LKML/LWN/whatever. I think that > would already be a pretty nice improvement over today's situation. > > Sadly, since the beginning of 2018, this was only used for a measly > ~0.14% of all non-merge commits in the kernel: --message-id helps provide a concrete reference, yes. However, being able to search for commit subjects in the mail archives is already implemented via cgit filter. An example is here: https://80x24.org/mirrors/git.git/commit/?id=8da56a484800023a545d7a7c022473f5aa9e720f The link at "userdiff: fix some corner cases in dts regex" makes a link to: https://public-inbox.org/git/?x=t&q=%22userdiff:+fix+some+corner+cases+in+dts+regex%22 (side note: not sure if that "x=t" to expand the whole message is good...) That link is generated by examples/cgit-commit-filter.lua in the public-inbox source: https://public-inbox.org/meta/1677253/s/?b=examples/cgit-commit-filter.lua My longer term plan is to be able to use the post-image blob OIDs from cgit to generate a search query for public-inbox such as: https://public-inbox.org/git/?q=dfpost:afc6b5b404+dfpost:072d58b69d+dfpost:4353b8220c+dfpost:333a625c70+dfpost:e187d356f6 Which finds all versions of the userdiff patch posted. But AFAIK there's no easy way to get at blob OIDs from cgit to a Lua filter...
Re: [PATCH 2/2] merge-recursive: fix merging a subdirectory into the root directory
Sorry for the long delay before getting back to this; the other stuff I was working on took longer than expected. On Mon, Oct 14, 2019 at 3:42 AM Johannes Schindelin wrote: > On Sat, 12 Oct 2019, Elijah Newren wrote: > > On Sat, Oct 12, 2019 at 1:37 PM Johannes Schindelin > > wrote: > > > > > > For the record: I am still a huge anti-fan of splitting `setup` test > > > cases from the test cases that do actual things, _unless_ it is > > > _one_, and _only one_, big, honking `setup` test case that is the > > > very first one in the test script. [...] > > The one thing I do agree with you on is test cases need to be > > optimized for when they report breakages, but that is precisely what > > led me to splitting setup and testing. > > To me, it is so not helpful _not_ to see the output of a `setup` that > succeeded, and only the output of the actual test that actually failed. > > It removes context. > > I need to understand the scenario where the breakage happens, and the > only way I can understand is when I understand the context. > > So the context needs to be as close as possible. I've updated the patch series with a change that I hope helps while still allowing the setup "steps" to be visibly differentiated from the testing steps. > > Way too many tests in the testsuite intersperse several setup and test > > cases together making it really hard to disentangle, understand what > > is going on, or even reverse engineer what is relevant. The absolute > > worst tests are the ones which just keep making additional changes to > > some existing repo to provide extra setup, causing all sorts of > > problems for skipping and resuming and understanding (to say nothing > > of test prerequisites that aren't always met). > > I agree with this sentiment, and have to point out that this is yet > another fallout of the way our test suite is implemented. If you look > e.g. at JUnit, there are no "setup test cases". There are specific setup > steps that you can define, there is even a teardown step you can define, > and those individual test cases? They can run in parallel, or > randomized, and they run in their own sandbox, to make sure that they > don't rely on side effects of unrelated test cases. > > We don't do that. We don't enforce the absence of side effects, and > therefore we have a megaton of them. > > But note how this actually speaks _against_ separating the setup from > the test? Because then you _explicitly_ want those test cases to rely on > one another. Which flies in the _face_ of trying to disentangling them. I agree that it is desirable to avoid side effects in the tests, but I'd like to point out that I'm not at all sure that your conclusion here is the only logical one to draw here in comparing to JUnit. As you point out, JUnit has clearly delineated setup steps for a test (as well as teardown steps), providing a place to keep them separate. Our testsuite lacks that, so how do folks try to get it? One logical way would be just inlining the setup steps in the test outside a test_expect_* block (which has been done in the past), but that has even worse problems. Another way, even if suboptimal, is placing those steps in their own test_expect_* block. You say just throw the setup and test together, but that breaks the separation. I think it's a case of the testsuite not providing the right abstractions and enough capability, leaving us to argue over which aspects of a more featureful test harness are most important to emulate. You clearly picked one, while I was focusing on another. Anyway, all that said, I think I have a nice compromise that I'll send out with V2. [...] > > > Makes sense, but the part that I am missing is > > > > > > test_path_is_file bar.c.t > > > > > > I.e. the _most_ important outcome of this test is: the rename was > > > detected, and the added file was correctly placed into the target > > > directory of the rename. > > > > That's a useful thing to add to the test, I'll add it. (It's kind of > > included in the 'git hash-object bar.c.t' half a dozen or so lines > > above, but this line helps document the expectation a bit better.) > > > > I'd be very reticent to include only this test_path_is_file check, as > > it makes no guarantee that it has the right contents or that we didn't > > also keep around another copy in a/b/bar.c.t, etc. > > I agree that it has to strike a balance. There are multiple aspects you > need to consider: > > - It needs to be easy to understand what the test case tries to ensure. > > - While it is important to verify that Git works correctly, you do not > want to make the test suite so slow as to be useless. I vividly > remember how Duy mentioned that he does not have the time to run the > test suite before contributing. That's how bad things are _already_. On this issue I'm having a harder time finding common ground. Perhaps there's something clever to be done, but I'm not seeing it. I can at least try to explain my pe
Re: [RFC/WIP] range-diff: show old/new blob OIDs in comments
Hi Eric,
On Thu, 17 Oct 2019, Eric Wong wrote:
> (WIP, mostly stream-of-concious notes + reasoning)
>
> When using "git format-patch --range-diff", the pre and
> post-image blob OIDs are in each email, while the exact
> commit OIDs are rarely shared via emails (only the tip
> commit from "git request-pull").
>
> These blob OIDs make it easy to search for or lookup the
> full emails which create them, or the blob itself once
> it's fetched via git.
>
> public-inbox indexes and allows querying specifically for blob
> OIDs via dfpre:/dfpost: since June 2017. As of Jan 2019,
> public-inbox also supports recreating blobs out of patch emails
> (querying internally with dfpre:/dfpost: and doing "git apply")
>
> Searching on these blob OIDs also makes it easier to find
> previous versions of the patch sets using any mail search
> engine.
>
> Future changes to public-inbox may allow generating custom
> diffs out of any blobs it can find or recreate.
>
> Most of this is pretty public-inbox-specific and would've
> made some future changes to public-inbox much easier
> (if we had this from the start of range-diff).
>
> Unfortunately, it won't help with cases where range-diffs
> are already published, but range-diff isn't too old.
I guess your patch won't hurt.
As to recreating blobs from mails: Wow. That's quite a length you're
going, and I think it is a shame that you have to. If only every
contribution came accompanied with a pullable branch in a public
repository.
Instead, we will have to rely on your centralized, non-distributed
service...
Ciao,
Dscho
>
> I'm also still learning my way around git's C internals, but
> using patch.{old,new}_oid_prefix seems alright...
>
> FIXME: tests, t3206 needs updating
>
> Not-signed-off-by: Eric Wong
> ---
> range-diff.c | 19 +++
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 7fed5a3b4b..85d2f1f58f 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -118,13 +118,24 @@ static int read_patches(const char *range, struct
> string_list *list)
> die(_("could not parse git header '%.*s'"),
> (int)len, line);
> strbuf_addstr(&buf, " ## ");
> if (patch.is_new > 0)
> - strbuf_addf(&buf, "%s (new)", patch.new_name);
> + strbuf_addf(&buf, "%s (new %s)",
> + patch.new_name,
> + patch.new_oid_prefix);
> else if (patch.is_delete > 0)
> - strbuf_addf(&buf, "%s (deleted)",
> patch.old_name);
> + strbuf_addf(&buf, "%s (deleted %s)",
> + patch.old_name,
> + patch.old_oid_prefix);
> else if (patch.is_rename)
> - strbuf_addf(&buf, "%s => %s", patch.old_name,
> patch.new_name);
> + strbuf_addf(&buf, "%s => %s (%s..%s)",
> + patch.old_name,
> + patch.new_name,
> + patch.old_oid_prefix,
> + patch.new_oid_prefix);
> else
> - strbuf_addstr(&buf, patch.new_name);
> + strbuf_addf(&buf, "%s (%s..%s)",
> + patch.new_name,
> + patch.old_oid_prefix,
> + patch.new_oid_prefix);
>
> free(current_filename);
> if (patch.is_delete > 0)
>
>
Re: [RFC/WIP] range-diff: show old/new blob OIDs in comments
On Tue, Oct 22, 2019 at 09:18:35PM +0200, Johannes Schindelin wrote: As to recreating blobs from mails: Wow. That's quite a length you're going, and I think it is a shame that you have to. If only every contribution came accompanied with a pullable branch in a public repository. Trouble is, those public repositories are transient and may be gone soon after the pull request is performed. The goal is to be able to reconstruct full code history prior to when it is merged into the official repository. Instead, we will have to rely on your centralized, non-distributed service... It's actually neither, because mirroring and distributing public-inbox repositories is already easy, and we hope to make it easier in the near future. -K
Re: is commitGraph useful on the server side?
On 10/22/2019 12:51 PM, Konstantin Ryabitsev wrote: > Hi, all: > > I've read the docs on commitGraph and it's not 100% clear to me if turning it > on and generating commit graphs would be useful on the server-side. I know > it's going to be enabled by default and automatically generated whenever "git > gc" runs, so I'm trying to figure out if it'll be useful for git-daemon > operations. > > Thanks in advance for your help. I've CC'd Taylor Blau for more information here. I'm biased, but I think the commit-graph is generally really good to have in almost all cases. I actually do not know of a good reason to _not_ have it. If you are managing reachability bitmaps, then most of the server-side stuff will use the bitmaps instead. However, creating those bitmaps will be slightly faster with the commit-graph. If you don't use bitmaps, then the commit-graph will help fetch negotiation and many other commit-walk experiences. If you have a lot of machinery around your server maintenance, then you can schedule commit-graph updates more frequently than bitmap computations, and you would get benefit by parsing commits faster in the zone "above" the bitmaps. Thanks, -Stolee
Re: [PATCH v2 9/9] pack-objects: improve partial packfile reuse
First, the commit message could probably be reordered to start with the
main point (reusing of packfiles at object granularity instead of
allowing reuse of only one contiguous region). To specific points:
> The dynamic array of `struct reused_chunk` is useful
> because we need to know not just the number of zero bits,
> but the accumulated offset due to missing objects.
The number of zero bits is irrelevant, I think. (I know I introduced
this idea, but at that time I was somehow confused that the offset was a
number of objects and not a number of bytes.)
> If a client both asks for a tag by sha1 and specifies
> "include-tag", we may end up including the tag in the reuse
> bitmap (due to the first thing), and then later adding it to
> the packlist (due to the second). This results in duplicate
> objects in the pack, which git chokes on. We should notice
> that we are already including it when doing the include-tag
> portion, and avoid adding it to the packlist.
I still don't understand this part. Going off what's written in the
commit message here, it seems to me that the issue is that (1) an object
can be added to the reuse bitmap without going through to_pack, and (2)
this is done when the client asks for a tag by SHA-1. But all objects
when specified by SHA-1 go through to_pack, don't they?
On to the review of the code. The ordering of - and + lines are
confusing, so I have just removed all - lines.
> +/*
> + * Record the offsets needed in our reused packfile chunks due to
> + * "gaps" where we omitted some objects.
> + */
> +static struct reused_chunk {
> + off_t start;
> + off_t offset;
> +} *reused_chunks;
> +static int reused_chunks_nr;
> +static int reused_chunks_alloc;
A chunk is a set of objects from the original packfile that we are
reusing; all objects in a chunk have the same relative position in the
original packfile and the generated packfile. (This does not mean that
the bytes are exactly the same or, even, that the last object in the
chunk has the same size in the original packfile and the generated
packfile.)
"start" is the offset of the first object of the chunk in the original
packfile.
"offset" is "start" minus the offset of the first object of the chunk in
the generated packfile. (I think it is easier to understand if this was
"new minus old" instead of "old minus new", that is, the negation of its
current value.)
So I would prefer:
/*
* A reused set of objects. All objects in a chunk have the same
* relative position in the original packfile and the generated
* packfile.
*/
struct reused_chunk {
/* The offset of the first object of this chunk in the original
* packfile. */
off_t original;
/* The offset of the first object of this chunk in the generated
* packfile minus "original". */
off_t difference;
}
> +static void record_reused_object(off_t where, off_t offset)
Straightforward, other than the renaming of parameters.
> +/*
> + * Binary search to find the chunk that "where" is in. Note
> + * that we're not looking for an exact match, just the first
> + * chunk that contains it (which implicitly ends at the start
> + * of the next chunk.
> + */
> +static off_t find_reused_offset(off_t where)
Also straightforward.
> +static void write_reused_pack_one(size_t pos, struct hashfile *out,
> + struct pack_window **w_curs)
> +{
> + off_t offset, next, cur;
> + enum object_type type;
> + unsigned long size;
> +
> + offset = reuse_packfile->revindex[pos].offset;
> + next = reuse_packfile->revindex[pos + 1].offset;
>
> + record_reused_object(offset, offset - hashfile_total(out));
This is where I understood what "offset" meant in struct reused_chunk.
>
> + cur = offset;
> + type = unpack_object_header(reuse_packfile, w_curs, &cur, &size);
> + assert(type >= 0);
>
> + if (type == OBJ_OFS_DELTA) {
> + off_t base_offset;
> + off_t fixup;
> +
> + unsigned char header[MAX_PACK_OBJECT_HEADER];
> + unsigned len;
> +
> + base_offset = get_delta_base(reuse_packfile, w_curs, &cur,
> type, offset);
> + assert(base_offset != 0);
> +
> + /* Convert to REF_DELTA if we must... */
> + if (!allow_ofs_delta) {
> + int base_pos = find_revindex_position(reuse_packfile,
> base_offset);
> + const unsigned char *base_sha1 =
> + nth_packed_object_sha1(reuse_packfile,
> +
> reuse_packfile->revindex[base_pos].nr);
> +
> + len = encode_in_pack_object_header(header,
> sizeof(header),
> +OBJ_REF_DELTA, size);
> + hashwrite(out, header, len);
> + hashwrite(out, base_sha1, 20);
> + copy_pack_data(out, reuse_packfile, w_curs, cur, next
Re: is commitGraph useful on the server side?
On Tue, Oct 22, 2019 at 03:44:28PM -0400, Derrick Stolee wrote: > I'm biased, but I think the commit-graph is generally really good to have > in almost all cases. I actually do not know of a good reason to _not_ have > it. A lot depends on how much you do on the server. If you're serving a web interface that runs things like `rev-list`, or `for-each-ref --contains`, etc, then you should see a big improvement. If you're _just_ serving fetches with `upload-pack`, you might see some small improvement during fetch negotiation. But I suspect it would be dwarfed by the cost of actually generating packs. Likewise, the traversal there will be dominated by accessing trees (and if that is expensive, then you ought to be using reachability bitmaps). But I agree that there's no reason _not_ to use them. -Peff
Re: is commitGraph useful on the server side?
[resending, looks like we lost Konstantin from the cc] On Tue, Oct 22, 2019 at 03:44:28PM -0400, Derrick Stolee wrote: > I'm biased, but I think the commit-graph is generally really good to have > in almost all cases. I actually do not know of a good reason to _not_ have > it. A lot depends on how much you do on the server. If you're serving a web interface that runs things like `rev-list`, or `for-each-ref --contains`, etc, then you should see a big improvement. If you're _just_ serving fetches with `upload-pack`, you might see some small improvement during fetch negotiation. But I suspect it would be dwarfed by the cost of actually generating packs. Likewise, the traversal there will be dominated by accessing trees (and if that is expensive, then you ought to be using reachability bitmaps). But I agree that there's no reason _not_ to use them. -Peff
Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
On Tue, Oct 22, 2019 at 05:28:55PM +, Derrick Stolee via GitGitGadget wrote: > However, the UNINTERESTING flag is used in lots of places in the > codebase. This flag usually means some barrier to stop a commit walk, > such as in revision-walking to compare histories. It is not often > cleared after the walk completes because the starting points of those > walks do not have the UNINTERESTING flag, and clear_commit_marks() would > stop immediately. Oof. Nicely explained, and your fix makes sense. The global-ness of revision flags always makes me nervous about doing more things in-process (this isn't the first such bug we've had). I have a dream of converting most uses of flags into using a commit-slab. That provides cheap access to an auxiliary structure, so each traversal, etc, could keep its own flag structure. I'm not sure if it would have a performance impact, though. Even though it's O(1), it is an indirect lookup, which could have some memory-access impact (though my guess is it would be lost in the noise). One of the sticking points is that all object types, not just commits, use flags. But we only assign slab ids to commits. I noticed recently that "struct object" has quite a few spare bits in it these days, because the switch to a 32-byte oid means 64-bit machines now have an extra 4 bytes of padding. I wonder if we could use that to store an index field. Anyway, that's getting far off the topic; clearly we need a fix in the meantime, and what you have here looks good to me. > I tested running clear_commit_marks_many() to clear the UNINTERESTING > flag inside close_reachable(), but the tips did not have the flag, so > that did nothing. Another option would be clear_object_flags(), which just walks all of the in-memory structs. Your REACHABLE solution is cheaper, though. > Instead, I finally arrived on the conclusion that I should use a flag > that is not used in any other part of the code. In commit-reach.c, a > number of flags were defined for commit walk algorithms. The REACHABLE > flag seemed like it made the most sense, and it seems it was not > actually used in the file. Yeah, being able to remove it from commit-reach.c surprised me for a moment. To further add to the confusion, builtin/fsck.c has its own REACHABLE flag (with a different bit and a totally different purpose). I don't think there's any practical problem there, though. > I have failed to produce a test using the file:// protocol that > demonstrates this bug. Hmm, from the description, it sounds like it should be easy. I might poke at it a bit. -Peff
Re: is commitGraph useful on the server side?
On Tue, Oct 22, 2019 at 04:06:16PM -0400, Jeff King wrote: I'm biased, but I think the commit-graph is generally really good to have in almost all cases. I actually do not know of a good reason to _not_ have it. A lot depends on how much you do on the server. If you're serving a web interface that runs things like `rev-list`, or `for-each-ref --contains`, etc, then you should see a big improvement. Ah, good to know, so something like cgit would see an improvement if there are commit graphs generated for the repos it serves. If you're _just_ serving fetches with `upload-pack`, you might see some small improvement during fetch negotiation. But I suspect it would be dwarfed by the cost of actually generating packs. Likewise, the traversal there will be dominated by accessing trees (and if that is expensive, then you ought to be using reachability bitmaps). We do generate bitmaps on a routine basis. OK, I think I'm convinced that enabling commitgraph and generating them regularly is going to be a net win. Thanks, everyone. -K
Re: [PATCH 1/1] config: add documentation to config.h
On Sun, Oct 20, 2019 at 09:35:17PM +1300, Heba Waly wrote: > On Sat, Oct 19, 2019 at 11:52 AM Emily Shaffer > wrote: > > > > On Fri, Oct 18, 2019 at 12:06:59AM +, Heba Waly via GitGitGadget wrote: > > > From: Heba Waly > > > > Hi Heba, > > > > Thanks for the patch! > > > > I'd like to highlight to the community that this is an Outreachy > > applicant and microproject. Heba, when you send the next version, I > > think you can add [Outreachy] manually to the PR subject line - that > > should draw the attention of those in the community who are invested in > > helping Outreachy applicants. > Good idea! I wanted to add it to the email subject but as I decided to > use gitgadget > I had no control over the subject. Hm, it looks like you already figured out how to add it to the title of the PR. :) > > > > > > This commit is copying and summarizing the documentation from > > > documentation/technical/api-config.txt to comments in config.h > > > > I think in the GitGitGadget PR you've got some great comments from Dscho > > about how to format your commit message; please take a look at those and > > feel free to reach out to me if you're still not sure what's missing or > > not. > Will do. > > > Signed-off-by: Heba Waly > > > > One thing I miss in this change is the removal of the contents of > > Documentation/technical/api-config.txt (or maybe the removal of the file > > itself). I'd prefer to see at least for api-config.txt to say something > > like "Please refer to comments in 'config.h'"; or, more drastically, for > > api-config.txt to be removed entirely. > > > > Having both pieces of documentation standing independently means that > > someone who's trying to add new information about the config API won't > > know where to add it; eventually they'll add something to config.h but > > not api-config.txt, or vice versa, and the two documents will go out of > > sync. So we want to move the documentation, rather than copy it. > That makes sense, thanks for the explanation. > I wasn't sure if it should be removed or not so I decided to leave it > until I'm asked otherwise. > So I assume api-config.html will be removed too? That shouldn't be tracked - this is generated from api-config.txt as part of the build. So don't worry about this part. > > > + > > > +/** > > > + * Value Parsing Helpers > > > + * - > > > > It may not make sense to have the header here in the middle of the doc. > > > > I wonder whether we need the headers at all anymore; or, whether it > > makes more sense to put this header in the long comment at the top with > > just the list of function names (so someone knows where to look), and > > leave the per-function explanations inline with the function they > > describe? > I see your point Emily, but in the CodingGuidelines file it was > advised to refer to strbuf.h > as a model for documentation, I noticed that strbuf.h used headers > this way so I decided > to replicate that. Ok! Sure. > > I made a couple of smallish comments about general formatting, but I'm > > also interested to know whether you were able to move the entire > > contents of api-config.txt across to here. Was there anything that you > > couldn't find a place for? > Yes, everything is moved. > > Thanks a lot for this change, and congrats on getting your first review > > out! Welcome! :) > > > > - Emily > > > Thanks a lot Emily for the detailed and helpful feedback! > > Heba
Re: [PATCH v2 1/1] config: move documentation to config.h
On Tue, Oct 22, 2019 at 07:05:06AM +, Heba Waly via GitGitGadget wrote: > From: Heba Waly > > Move the documentation from Documentation/technical/api-config.txt into > config.h This is still a little thin for what we usually want from commit messages. Try to imagine that five years from now, you find this commit by running `git blame` on config.h and then examining the commit which introduced all these comments with `git show ` - what would you want to know? Typically we want to know "why" the change was made, because the diff shows "what". We can see from the diff that you're moving comments from A to B, but if you explain why you did so (not "because my Outreachy mentor told me to" ;) but "because it is useful to see usage information next to code" or "this is best practice as described by blah blah") - I wouldn't be able to know that reasoning just from looking at your diff. > diff --git a/config.h b/config.h > index f0ed464004..02f78ffc2b 100644 > --- a/config.h > +++ b/config.h > @@ -4,6 +4,23 @@ > #include "hashmap.h" > #include "string-list.h" > > + > +/** > + * The config API gives callers a way to access Git configuration files > + * (and files which have the same syntax). See linkgit:git-config[1] for a Ah, here's another place where the Asciidoc link isn't going to do anything anymore. Otherwise I didn't still see anything jumping out. When the commit message is cleaned up I'm ready to add my Reviewed-by line. - Emily
Re: [PATCH 1/1] documentation: remove empty doc files
On Tue, Oct 22, 2019 at 06:19:35PM +, Heba Waly via GitGitGadget wrote: > From: Heba Waly > > Remove empty and redundant documentation files from the > Documentation/technical/ directory. > > As part of moving the documentation from Documentation/technical/api-* to > header files, the following files are deleted because they include only > TODO messages with no documentation to be moved: > Documentation/technical/api-grep.txt > Documentation/technical/api-object-access.txt > Documentation/technical/api-quote.txt > Documentation/technical/api-xdiff-interface.txt Same thing as I mentioned in your other review; what you've added to your commit message now doesn't say anything you didn't say with the diff. I can see that you removed empty documentation files; I can see that those files include only TODO. Maybe you can explain why it's a bad developer experience to stumble across these, and that those files sat untouched for years in the TODO(contributor-name) state. > > Signed-off-by: Heba Waly > --- > Documentation/technical/api-grep.txt| 8 > Documentation/technical/api-object-access.txt | 15 --- > Documentation/technical/api-quote.txt | 10 -- > Documentation/technical/api-xdiff-interface.txt | 7 --- > 4 files changed, 40 deletions(-) > delete mode 100644 Documentation/technical/api-grep.txt > delete mode 100644 Documentation/technical/api-object-access.txt > delete mode 100644 Documentation/technical/api-quote.txt > delete mode 100644 Documentation/technical/api-xdiff-interface.txt As for the content of this change, I absolutely approve. I've stumbled across some of these empty docs while looking for answers before and found it really demoralizing - the community is so interested in teaching me how to contribute that they've sat on a TODO for 12 years? :( I even held up api-grep.txt as a (bad) example in a talk I gave this year. I'm happy to see these files go. - Emily
Re: Git in Outreachy December 2019?
On Fri, Sep 20, 2019 at 06:47:01PM -0700, Emily Shaffer wrote: > On Fri, Sep 20, 2019 at 10:04:48AM -0700, Jonathan Tan wrote: > > > Prospective mentors need to sign up on that site, and should propose a > > > project they'd be willing to mentor. > > > > [snip] > > > > > I'm happy to discuss possible projects if anybody has an idea but isn't > > > sure how to develop it into a proposal. > > > > I'm new to Outreachy and programs like this, so does anyone have an > > opinion on my draft proposal below? It does not have any immediate > > user-facing benefit, but it does have a definite end point. > > I'd appreciate similar opinion if anybody has it - and I'd also really > feel more comfortable with a co-mentor. I know early on in this thread about Outreachy projects some folks expressed interest in comentoring. Is anybody still interested in doing so? For context, I've been in contact with 3 applicants who have either sent their first patch already or are getting ready to (and have needed some involved discussion or help) plus another few applicants who have inquired and may or may not send patches in the future. I've also received quite a few mails outside of my timezone working hours (I usually am awake/working 18:00GMT-02:00GMT), which I feel badly about not being able to respond to in a timely fashion. If anybody wants to comentor I would be so excited to have the help :) - Emily
[PATCH v2 1/3] merge-recursive: clean up get_renamed_dir_portion()
From: Elijah Newren
Dscho noted a few things making this function hard to follow.
Restructure it a bit and add comments to make it easier to follow. The
restructurings include:
* There was a special case if-check at the end of the function
checking whether someone just renamed a file within its original
directory, meaning that there could be no directory rename involved.
That check was slightly convoluted; it could be done in a more
straightforward fashion earlier in the function, and can be done
more cheaply too (no call to strncmp).
* The conditions for advancing end_of_old and end_of_new before
calling strchr were both confusing and unnecessary. If either
points at a '/', then they need to be advanced in order to find the
next '/'. If either doesn't point at a '/', then advancing them one
char before calling strchr() doesn't hurt. So, just rip out the
if conditions and advance both before calling strchr().
Signed-off-by: Elijah Newren
---
merge-recursive.c | 60 ---
1 file changed, 36 insertions(+), 24 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 22a12cfeba..f80e48f623 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1943,8 +1943,8 @@ static void get_renamed_dir_portion(const char *old_path,
const char *new_path,
char **old_dir, char **new_dir)
{
char *end_of_old, *end_of_new;
- int old_len, new_len;
+ /* Default return values: NULL, meaning no rename */
*old_dir = NULL;
*new_dir = NULL;
@@ -1955,43 +1955,55 @@ static void get_renamed_dir_portion(const char
*old_path, const char *new_path,
*"a/b/c/d" was renamed to "a/b/some/thing/else"
* so, for this example, this function returns "a/b/c/d" in
* *old_dir and "a/b/some/thing/else" in *new_dir.
-*
-* Also, if the basename of the file changed, we don't care. We
-* want to know which portion of the directory, if any, changed.
+*/
+
+ /*
+* If the basename of the file changed, we don't care. We want
+* to know which portion of the directory, if any, changed.
*/
end_of_old = strrchr(old_path, '/');
end_of_new = strrchr(new_path, '/');
-
if (end_of_old == NULL || end_of_new == NULL)
- return;
+ return; /* We haven't modified *old_dir or *new_dir yet. */
+
+ /* Find the first non-matching character traversing backwards */
while (*--end_of_new == *--end_of_old &&
end_of_old != old_path &&
end_of_new != new_path)
; /* Do nothing; all in the while loop */
+
/*
-* We've found the first non-matching character in the directory
-* paths. That means the current directory we were comparing
-* represents the rename. Move end_of_old and end_of_new back
-* to the full directory name.
+* If both got back to the beginning of their strings, then the
+* directory didn't change at all, only the basename did.
*/
- if (*end_of_old == '/')
- end_of_old++;
- if (*end_of_old != '/')
- end_of_new++;
- end_of_old = strchr(end_of_old, '/');
- end_of_new = strchr(end_of_new, '/');
+ if (end_of_old == old_path && end_of_new == new_path &&
+ *end_of_old == *end_of_new)
+ return; /* We haven't modified *old_dir or *new_dir yet. */
/*
-* It may have been the case that old_path and new_path were the same
-* directory all along. Don't claim a rename if they're the same.
+* We've found the first non-matching character in the directory
+* paths. That means the current characters we were looking at
+* were part of the first non-matching subdir name going back from
+* the end of the strings. Get the whole name by advancing both
+* end_of_old and end_of_new to the NEXT '/' character. That will
+* represent the entire directory rename.
+*
+* The reason for the increment is cases like
+*a/b/star/foo/whatever.c -> a/b/tar/foo/random.c
+* After dropping the basename and going back to the first
+* non-matching character, we're now comparing:
+*a/b/s and a/b/
+* and we want to be comparing:
+*a/b/star/ and a/b/tar/
+* but without the pre-increment, the one on the right would stay
+* a/b/.
*/
- old_len = end_of_old - old_path;
- new_len = end_of_new - new_path;
+ end_of_old = strchr(++end_of_old, '/');
+ end_of_new = strchr(++end_of_new, '/');
- if (old_len != new_len || strncmp(old_path, new_path, old_len)) {
- *old_dir = xstrndup(old_path, old_len);
- *new_dir = xstrndup(new_pa
[PATCH v2 3/3] t604[236]: do not run setup in separate tests
From: Elijah Newren
Transform the setup "tests" to setup functions, and have the actual
tests call the setup functions. Advantages:
* Should make life easier for people working with webby CI/PR builds
who have to abuse mice (and their own index finger as well) in
order to switch from viewing one testcase to another. Sounds
awful; hopefully this will improve things for them.
* Improves re-runnability: any failed test in any of these three
files can now be re-run in isolation, e.g.
./t6042* --ver --imm -x --run=21
whereas before it would require two tests to be specified to the
--run argument, the other needing to be picked out as the relevant
setup test from one or two tests before.
* Importantly, this still keeps the "setup" and "test" sections
somewhat separate to make it easier for readers to discern what is
just ancillary setup and what the intent of the test is.
Signed-off-by: Elijah Newren
---
t/t6042-merge-rename-corner-cases.sh | 111 +++---
t/t6043-merge-rename-directories.sh| 466 ++---
t/t6046-merge-skip-unneeded-updates.sh | 135 ---
3 files changed, 393 insertions(+), 319 deletions(-)
diff --git a/t/t6042-merge-rename-corner-cases.sh
b/t/t6042-merge-rename-corner-cases.sh
index c5b57f40c3..b047cf1c1c 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -5,7 +5,7 @@ test_description="recursive merge corner cases w/ renames but
not criss-crosses"
. ./test-lib.sh
-test_expect_success 'setup rename/delete + untracked file' '
+test_setup_rename_delete_untracked () {
test_create_repo rename-delete-untracked &&
(
cd rename-delete-untracked &&
@@ -29,9 +29,10 @@ test_expect_success 'setup rename/delete + untracked file' '
git commit -m track-people-instead-of-objects &&
echo "Myyy PRECIOUSSS" >ring
)
-'
+}
test_expect_success "Does git preserve Gollum's precious artifact?" '
+ test_setup_rename_delete_untracked &&
(
cd rename-delete-untracked &&
@@ -49,7 +50,7 @@ test_expect_success "Does git preserve Gollum's precious
artifact?" '
#
# We should be able to merge B & C cleanly
-test_expect_success 'setup rename/modify/add-source conflict' '
+test_setup_rename_modify_add_source () {
test_create_repo rename-modify-add-source &&
(
cd rename-modify-add-source &&
@@ -70,9 +71,10 @@ test_expect_success 'setup rename/modify/add-source
conflict' '
git add a &&
git commit -m C
)
-'
+}
test_expect_failure 'rename/modify/add-source conflict resolvable' '
+ test_setup_rename_modify_add_source &&
(
cd rename-modify-add-source &&
@@ -88,7 +90,7 @@ test_expect_failure 'rename/modify/add-source conflict
resolvable' '
)
'
-test_expect_success 'setup resolvable conflict missed if rename missed' '
+test_setup_break_detection_1 () {
test_create_repo break-detection-1 &&
(
cd break-detection-1 &&
@@ -110,9 +112,10 @@ test_expect_success 'setup resolvable conflict missed if
rename missed' '
git add a &&
git commit -m C
)
-'
+}
test_expect_failure 'conflict caused if rename not detected' '
+ test_setup_break_detection_1 &&
(
cd break-detection-1 &&
@@ -135,7 +138,7 @@ test_expect_failure 'conflict caused if rename not
detected' '
)
'
-test_expect_success 'setup conflict resolved wrong if rename missed' '
+test_setup_break_detection_2 () {
test_create_repo break-detection-2 &&
(
cd break-detection-2 &&
@@ -160,9 +163,10 @@ test_expect_success 'setup conflict resolved wrong if
rename missed' '
git add a &&
git commit -m E
)
-'
+}
test_expect_failure 'missed conflict if rename not detected' '
+ test_setup_break_detection_2 &&
(
cd break-detection-2 &&
@@ -182,7 +186,7 @@ test_expect_failure 'missed conflict if rename not
detected' '
# Commit B: rename a->b
# Commit C: rename a->b, add unrelated a
-test_expect_success 'setup undetected rename/add-source causes data loss' '
+test_setup_break_detection_3 () {
test_create_repo break-detection-3 &&
(
cd break-detection-3 &&
@@ -202,9 +206,10 @@ test_expect_success 'setup undetected rename/add-source
causes data loss' '
git add a &&
git commit -m C
)
-'
+}
test_expect_failure 'detect rename/add-source and preserve all data' '
+ test_setup_break_detection_3 &&
(
cd break-detection-3 &&
@@ -231,6 +236,7 @@ test_expect_failure 'detect rename/add-source and preserve
all data' '
'
test_expect_failure 'detect rename/add-source and preserve all data, merge
other way' '
+ test_
[PATCH v2 0/3] Dir rename fixes
This series improves a couple things found after looking into things Dscho
flagged:
* clarify and slightly restructure code in the get_renamed_dir_portion()
function
* extend support of detecting renaming/merging of one directory into
another to support the root directory as a target directory
First patch best viewed with a --histogram diff (sorry, gitgitgadget does
not yet know how to generate those).
Changes since v1:
* Incorporated code cleanups suggested by Dscho
* Fixed to work with an alternate rename-to-root-directory case (end_of_new
== NULL), with new testcase
* Added a new patch to the end of the series to stop making setup tests be
part of a separate test_expect_success block.
Elijah Newren (3):
merge-recursive: clean up get_renamed_dir_portion()
merge-recursive: fix merging a subdirectory into the root directory
t604[236]: do not run setup in separate tests
merge-recursive.c | 104 -
t/t6042-merge-rename-corner-cases.sh | 111 +++--
t/t6043-merge-rename-directories.sh| 568 -
t/t6046-merge-skip-unneeded-updates.sh | 135 +++---
4 files changed, 582 insertions(+), 336 deletions(-)
base-commit: 08da6496b61341ec45eac36afcc8f94242763468
Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-390%2Fnewren%2Fdir-rename-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-390/newren/dir-rename-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/390
Range-diff vs v1:
1: 8ae78679c9 = 1: 8ae78679c9 merge-recursive: clean up
get_renamed_dir_portion()
2: 37aee862e1 ! 2: a1e80e8fbb merge-recursive: fix merging a subdirectory
into the root directory
@@ -34,9 +34,41 @@
strbuf_grow(&new_path, newlen);
strbuf_addbuf(&new_path, &entry->new_dir);
@@
- *end_of_old == *end_of_new)
- return; /* We haven't modified *old_dir or *new_dir yet. */
+ */
+ end_of_old = strrchr(old_path, '/');
+ end_of_new = strrchr(new_path, '/');
+- if (end_of_old == NULL || end_of_new == NULL)
+- return; /* We haven't modified *old_dir or *new_dir yet. */
++
++ /*
++ * If end_of_old is NULL, old_path wasn't in a directory, so there
++ * could not be a directory rename (our rule elsewhere that a
++ * directory which still exists is not considered to have been
++ * renamed means the root directory can never be renamed -- because
++ * the root directory always exists).
++ */
++ if (end_of_old == NULL)
++ return; /* Note: *old_dir and *new_dir are still NULL */
++
++ /*
++ * If new_path contains no directory (end_of_new is NULL), then we
++ * have a rename of old_path's directory to the root directory.
++ */
++ if (end_of_new == NULL) {
++ *old_dir = xstrndup(old_path, end_of_old - old_path);
++ *new_dir = xstrdup("");
++ return;
++ }
+ /* Find the first non-matching character traversing backwards */
+ while (*--end_of_new == *--end_of_old &&
+@@
+ */
+ if (end_of_old == old_path && end_of_new == new_path &&
+ *end_of_old == *end_of_new)
+- return; /* We haven't modified *old_dir or *new_dir yet. */
++ return; /* Note: *old_dir and *new_dir are still NULL */
++
+ /*
+ * If end_of_new got back to the beginning of its string, and
+ * end_of_old got back to the beginning of some subdirectory, then
@@ -44,21 +76,19 @@
+ * needs slightly special handling.
+ *
+ * Note: There is no need to consider the opposite case, with a
-+ * rename/merge of the root directory into some subdirectory.
-+ * Our rule elsewhere that a directory which still exists is not
-+ * considered to have been renamed means the root directory can
-+ * never be renamed (because the root directory always exists).
++ * rename/merge of the root directory into some subdirectory
++ * because as noted above the root directory always exists so it
++ * cannot be considered to be renamed.
+ */
+ if (end_of_new == new_path &&
+ end_of_old != old_path && end_of_old[-1] == '/') {
-+ *old_dir = xstrndup(old_path, end_of_old-1 - old_path);
-+ *new_dir = xstrndup(new_path, end_of_new - new_path);
++ *old_dir = xstrndup(old_path, --end_of_old - old_path);
++ *new_dir = xstrdup("");
+ return;
+ }
-+
+
/*
* We've found the first non-matching character in the directory
- * paths. That means the current characters we were looking at
diff --git a/t/t6043-merge-rename-directories.sh
b/t/t6043-merge-rename-directories.sh
--- a/t/t6043-merge-rename-directories.sh
@@ -68,18 +98,18 @@
'
+# Testcase 12d
[PATCH v2 2/3] merge-recursive: fix merging a subdirectory into the root directory
From: Elijah Newren
We allow renaming all entries in e.g. a directory named z/ into a
directory named y/ to be detected as a z/ -> y/ rename, so that if the
other side of history adds any files to the directory z/ in the mean
time, we can provide the hint that they should be moved to y/.
There is no reason to not allow 'y/' to be the root directory, but the
code did not handle that case correctly. Add a testcase and the
necessary special checks to support this case.
Signed-off-by: Elijah Newren
---
merge-recursive.c | 52 -
t/t6043-merge-rename-directories.sh | 114
2 files changed, 163 insertions(+), 3 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index f80e48f623..ec60715368 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1931,6 +1931,16 @@ static char *apply_dir_rename(struct dir_rename_entry
*entry,
return NULL;
oldlen = strlen(entry->dir);
+ if (entry->new_dir.len == 0)
+ /*
+* If someone renamed/merged a subdirectory into the root
+* directory (e.g. 'some/subdir' -> ''), then we want to
+* avoid returning
+* '' + '/filename'
+* as the rename; we need to make old_path + oldlen advance
+* past the '/' character.
+*/
+ oldlen++;
newlen = entry->new_dir.len + (strlen(old_path) - oldlen) + 1;
strbuf_grow(&new_path, newlen);
strbuf_addbuf(&new_path, &entry->new_dir);
@@ -1963,8 +1973,26 @@ static void get_renamed_dir_portion(const char
*old_path, const char *new_path,
*/
end_of_old = strrchr(old_path, '/');
end_of_new = strrchr(new_path, '/');
- if (end_of_old == NULL || end_of_new == NULL)
- return; /* We haven't modified *old_dir or *new_dir yet. */
+
+ /*
+* If end_of_old is NULL, old_path wasn't in a directory, so there
+* could not be a directory rename (our rule elsewhere that a
+* directory which still exists is not considered to have been
+* renamed means the root directory can never be renamed -- because
+* the root directory always exists).
+*/
+ if (end_of_old == NULL)
+ return; /* Note: *old_dir and *new_dir are still NULL */
+
+ /*
+* If new_path contains no directory (end_of_new is NULL), then we
+* have a rename of old_path's directory to the root directory.
+*/
+ if (end_of_new == NULL) {
+ *old_dir = xstrndup(old_path, end_of_old - old_path);
+ *new_dir = xstrdup("");
+ return;
+ }
/* Find the first non-matching character traversing backwards */
while (*--end_of_new == *--end_of_old &&
@@ -1978,7 +2006,25 @@ static void get_renamed_dir_portion(const char
*old_path, const char *new_path,
*/
if (end_of_old == old_path && end_of_new == new_path &&
*end_of_old == *end_of_new)
- return; /* We haven't modified *old_dir or *new_dir yet. */
+ return; /* Note: *old_dir and *new_dir are still NULL */
+
+ /*
+* If end_of_new got back to the beginning of its string, and
+* end_of_old got back to the beginning of some subdirectory, then
+* we have a rename/merge of a subdirectory into the root, which
+* needs slightly special handling.
+*
+* Note: There is no need to consider the opposite case, with a
+* rename/merge of the root directory into some subdirectory
+* because as noted above the root directory always exists so it
+* cannot be considered to be renamed.
+*/
+ if (end_of_new == new_path &&
+ end_of_old != old_path && end_of_old[-1] == '/') {
+ *old_dir = xstrndup(old_path, --end_of_old - old_path);
+ *new_dir = xstrdup("");
+ return;
+ }
/*
* We've found the first non-matching character in the directory
diff --git a/t/t6043-merge-rename-directories.sh
b/t/t6043-merge-rename-directories.sh
index c966147d5d..32cdd1f493 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -4051,6 +4051,120 @@ test_expect_success '12c-check: Moving one directory
hierarchy into another w/ c
)
'
+# Testcase 12d, Rename/merge of subdirectory into the root
+# Commit O: a/b/subdir/foo
+# Commit A: subdir/foo
+# Commit B: a/b/subdir/foo, a/b/bar
+# Expected: subdir/foo, bar
+
+test_expect_success '12d-setup: Rename/merge subdir into the root, variant 1' '
+ test_create_repo 12d &&
+ (
+ cd 12d &&
+
+ mkdir -p a/b/subdir &&
+ test_commit a/b/subdir/foo &&
+
+ git branch O &&
+ git branch A &&
+ git branch B &&
+
+ git c
Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
On Tue, Oct 22, 2019 at 04:33:16PM -0400, Jeff King wrote:
> > I have failed to produce a test using the file:// protocol that
> > demonstrates this bug.
>
> Hmm, from the description, it sounds like it should be easy. I might
> poke at it a bit.
Hmph. I can reproduce it here, but it seems to depend on the repository.
If I do this:
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ecabbe1616..8d473a456f 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -583,6 +583,14 @@ test_expect_success 'fetch.writeCommitGraph' '
)
'
+test_expect_success 'fetch.writeCommitGraph with a bigger repo' '
+ git clone "$TEST_DIRECTORY/.." repo &&
+ (
+ cd repo &&
+ git -c fetch.writeCommitGraph fetch origin
+ )
+'
+
# configured prune tests
set_config_tristate () {
it reliably triggers the bug. But if I make a synthetic repo, even it
has a lot of commits (thousands or more), it doesn't trigger. I thought
maybe it had to do with having commits that were not at tips (since the
tip ones presumably _are_ fed into the graph generation process). But
that doesn't seem to help.
Puzzling...
-Peff
Re: [PATCH v2 1/1] ci(osx): use new location of the `perforce` cask
Hi Gábor, On Fri, 18 Oct 2019, SZEDER Gábor wrote: > On Thu, Oct 17, 2019 at 12:47:33PM +, Johannes Schindelin via > GitGitGadget wrote: > > From: Johannes Schindelin > > > > The CI builds are failing for Mac OS X due to a change in the > > s/CI/Azure Pipelines/ > > Our Travis CI builds are fine. For the moment ;-) > > location of the perforce cask. The command outputs the following > > error: > > > > + brew install caskroom/cask/perforce > > Error: caskroom/cask was moved. Tap homebrew/cask-cask instead. > > > > So let's try to call `brew cask install perforce` first (which is what > > that error message suggests, in a most round-about way). > > > > The "caskroom" way was added in 672f51cb (travis-ci: > > fix Perforce install on macOS, 2017-01-22) and the justification > > is that the call "brew cask install perforce" can fail due to a checksum > > mismatch: the recipe simply downloads the official Perforce distro, and > > whenever that is updated, the recipe needs to be updated, too. > > This paragraph is wrong, it mixes up things too much. > > Prior to 672f51cb we used to install the 'perforce' _package_ with > 'brew install perforce' (note: no 'cask' in there). The justification > for 672f51cb was that the command 'brew install perforce' simply > stopped working, after Homebrew folks decided that it's better to move > the 'perforce' package to a "cask". It was _their_ justification for > this move that 'brew install perforce' "can fail due to a checksum > mismatch ...", and casks can be installed without checksum > verification. And indeed, both 'brew cask install perforce' and 'brew > install caskroom/cask/perforce' printed something along the lines of: > > ==> No checksum defined for Cask perforce, skipping verification > > It's unclear to me why 672f51cb used 'brew install > caskroom/cask/perforce' instead of 'brew cask install perforce'. It > appears (by running both commands on old Travis CI macOS images) that > both commands worked all the same already back then. > > Anyway, as the error message at the top of the log message shows, > 'brew install caskroom/cask/perforce' has stopped working recently, > but 'brew cask install perforce' still does, so let's use that. If you don't mind, I am going to copy/edit these three paragraphs into the commit message, > Note that on Travis CI we explicitly specify which macOS image to use, > and nowadays we don't run 'brew update' during the build process [1], > so both commands work in our builds there. > > [1] f2f4715033 (ci: don't update Homebrew, 2019-07-03) > > > CI servers are typically fresh virtual machines, but not always. To > > accommodate for that, let's try harder if `brew cask install perforce` > > fails, by specifically pulling the latest `master` of the > > `homebrew-cask` repository. > > Homebrew didn't record a checksum for Perforce versions r17.1, r17.2 > and r18.1, so installing those still works fine. Our Travis CI images > install r18.1. > > However, when Homebrew updated to Perforce r19.1, they included the > checksum again for some reason (intentional or accidental; I didn't > look why). This worked fine for a while, until a couple of days ago > Perforce updated the r19.1 binaries in place, breaking those > checksums. > > If we were to still run 'brew update', then it would shortly fix the > checksum mismatch. But we don't run it, and we do not want to run it > because it takes ages. Falling back to pull from the 'homebrew-cask' > repository could be a reasonable and quick workaround. Okay, good. > > This will still fail, of course, when `homebrew-cask` falls behind > > Perforce's release schedule. But once it is updated, we can now simply > > re-run the failed jobs and they will pick up that update. > > In our CI builds we don't at all care what the checksums of the > Perforce binaries are, so I would really like to tell 'brew' to ignore > any checksum mismatch when installing 'perforce'. Alas, it appears > that 'brew' has no public options to turn of or to ignore checksum > verification. Sad, yet true, that we indeed have no command-line option to say "you know what, your checksum possibly mismatches, but we really don't care". > Now, let's take a step back. > > All 'brew cask install perforce' really does is run 'curl' to download > a tar.gz from the Perforce servers, verify its checksum, unpack it, > and put the executables somewhere on $PATH. That's not rocket > science, we could easily do that ourselves; we don't even have to deal > with a tar.gz, the 'p4' and 'p4d' binaries for mac are readily > available for download at: > > http://filehost.perforce.com/perforce/r19.1/bin.macosx1010x86_64/ > > And, in fact, that's what we have been doing in some of our Linux jobs > since the very beginning, so basically only the download URL has to be > adjusted. I'd rather not. Just because there is no better way on Linux, and just because the current `perforce` cask recipe happens to just download and un
Re: [PATCH v2 1/1] ci(osx): use new location of the `perforce` cask
Hi Junio, On Mon, 21 Oct 2019, Junio C Hamano wrote: > SZEDER Gábor writes: > > > On Thu, Oct 17, 2019 at 12:47:33PM +, Johannes Schindelin via > > GitGitGadget wrote: > >> From: Johannes Schindelin > >> > >> The CI builds are failing for Mac OS X due to a change in the > > > > s/CI/Azure Pipelines/ > > > > Our Travis CI builds are fine. > > > >> location of the perforce cask. The command outputs the following > >> error: > >> > >> + brew install caskroom/cask/perforce > >> Error: caskroom/cask was moved. Tap homebrew/cask-cask instead. > >> > >> So let's try to call `brew cask install perforce` first (which is what > >> that error message suggests, in a most round-about way). > >> > >> The "caskroom" way was added in 672f51cb (travis-ci: > >> fix Perforce install on macOS, 2017-01-22) and the justification > >> is that the call "brew cask install perforce" can fail due to a checksum > >> mismatch: the recipe simply downloads the official Perforce distro, and > >> whenever that is updated, the recipe needs to be updated, too. > > > > This paragraph is wrong, it mixes up things too much. > > > > Prior to 672f51cb we used to install the 'perforce' _package_ with > > 'brew install perforce' (note: no 'cask' in there). The justification > > for 672f51cb was that the command 'brew install perforce' simply > > stopped working, after Homebrew folks decided that it's better to move > > the 'perforce' package to a "cask". It was _their_ justification for > > this move that 'brew install perforce' "can fail due to a checksum > > mismatch ...", and casks can be installed without checksum > > verification. And indeed, both 'brew cask install perforce' and 'brew > > install caskroom/cask/perforce' printed something along the lines of: > > > > ==> No checksum defined for Cask perforce, skipping verification > > > > It's unclear to me why 672f51cb used 'brew install > > caskroom/cask/perforce' instead of 'brew cask install perforce'. It > > appears (by running both commands on old Travis CI macOS images) that > > both commands worked all the same already back then. > > > > Anyway, as the error message at the top of the log message shows, > > 'brew install caskroom/cask/perforce' has stopped working recently, > > but 'brew cask install perforce' still does, so let's use that. > > > > Note that on Travis CI we explicitly specify which macOS image to use, > > and nowadays we don't run 'brew update' during the build process [1], > > so both commands work in our builds there. > > ... > > Now, let's take a step back. > > > > All 'brew cask install perforce' really does is ... > > ... in fact, that's what we have been doing in some of our Linux jobs > > since the very beginning, so basically only the download URL has to be > > adjusted. > > This is already in 'next' X-<; reverting a merge is cheap but I > prefer to do so when we already have a replacement. I force-pushed (see https://github.com/gitgitgadget/git/pull/400), and once Stolee approves, he will submit v3. This will only change the commit message, though, as I disagree that hard-coding the URL would be an improvement: the nice thing about a package management system is that the user does not need to know the details (or need to know if the details change, like, ever). Ciao, Dscho > > Thanks for taking a closer look. >
[PATCH] pretty: Add "%aU"|"%au" option to output author's username
In many projects the number of contributors is low enough that users know
each other and the full email address doesn't need to be displayed.
Displaying only the author's username saves a lot of columns on the screen.
For example displaying "prarit" instead of "pra...@redhat.com" saves 11
columns.
Add a "%aU"|"%au" option that outputs the author's email username.
Also add tests for "%ae" and "%an".
Signed-off-by: Prarit Bhargava
---
Documentation/pretty-formats.txt | 3 +++
pretty.c | 5 +
t/t4202-log.sh | 16
3 files changed, 24 insertions(+)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index b87e2e83e6d0..479a15a8ab12 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -163,6 +163,9 @@ The placeholders are:
'%ae':: author email
'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
or linkgit:git-blame[1])
+'%au':: author username
+'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]
+ or linkgit:git-blame[1])
'%ad':: author date (format respects --date= option)
'%aD':: author date, RFC2822 style
'%ar':: author date, relative
diff --git a/pretty.c b/pretty.c
index b32f0369531c..2a5b93022050 100644
--- a/pretty.c
+++ b/pretty.c
@@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char
part,
strbuf_add(sb, mail, maillen);
return placeholder_len;
}
+ if (part == 'u' || part == 'U') { /* username */
+ maillen = strstr(s.mail_begin, "@") - s.mail_begin;
+ strbuf_add(sb, mail, maillen);
+ return placeholder_len;
+ }
if (!s.date_begin)
goto skip;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e803ba402e9e..2fee0c067197 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1729,4 +1729,20 @@ test_expect_success 'log --end-of-options' '
test_cmp expect actual
'
+test_expect_success 'log pretty %an %ae %au' '
+ git checkout -b anaeau &&
+ test_commit anaeau_test anaeau_test_file &&
+ git log --pretty="%an" > actual &&
+ git log --pretty="%ae" >> actual &&
+ git log --pretty="%au" >> actual &&
+ git log > full &&
+ name=$(cat full | grep "^Author: " | awk -F "Author: " " { print \$2 }
" | awk -F " <" " { print \$1 } ") &&
+ email=$(cat full | grep "^Author: " | awk -F "<" " { print \$2 } " |
awk -F ">" " { print \$1 } ") &&
+ username=$(cat full | grep "^Author: " | awk -F "<" " { print \$2 } " |
awk -F ">" " { print \$1 } " | awk -F "@" " { print \$1 } " ) &&
+ echo "${name}" > expect &&
+ echo "${email}" >> expect &&
+ echo "${username}" >> expect &&
+ test_cmp expect actual
+'
+
test_done
--
2.21.0
[PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit`
From: Johannes Schindelin In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), `git commit` learned to suggest to run `git cherry-pick --skip` when trying to cherry-pick an empty patch, but that was never tested for. Here is a test that verifies that a message is given to the user that contains the correct invocation. Signed-off-by: Johannes Schindelin --- t/t3510-cherry-pick-sequence.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 793bcc7fe3..5b94fdaa67 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -123,7 +123,8 @@ test_expect_success 'revert --skip to skip commit' ' test_expect_success 'skip "empty" commit' ' pristine_detach picked && test_commit dummy foo d && - test_must_fail git cherry-pick anotherpick && + test_must_fail git cherry-pick anotherpick 2>err && + test_i18ngrep "git cherry-pick --skip" err && git cherry-pick --skip && test_cmp_rev dummy HEAD ' -- gitgitgadget
[PATCH 0/3] commit: fix advice for empty commits during rebases
In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we introduced a helpful message that suggests to run git cherry-pick --skip (instead of the previous message that talked about git reset) when a cherry-pick failed due to an empty patch. However, the same message is displayed during a rebase, when the patch to-be-committed is empty. In this case, git reset would also have worked, but git cherry-pick --skip does not work. This is a regression introduced in this cycle. Let's be more careful here. Johannes Schindelin (3): cherry-pick: add test for `--skip` advice in `git commit` sequencer: export the function to get the path of `.git/rebase-merge/` commit: give correct advice for empty commit during a rebase builtin/commit.c| 33 - sequencer.c | 4 ++-- sequencer.h | 1 + t/t3403-rebase-skip.sh | 9 + t/t3510-cherry-pick-sequence.sh | 3 ++- 5 files changed, 38 insertions(+), 12 deletions(-) base-commit: d966095db01190a2196e31195ea6fa0c722aa732 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-417%2Fdscho%2Ffix-commit-advice-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-417/dscho/fix-commit-advice-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/417 -- gitgitgadget
[PATCH 3/3] commit: give correct advice for empty commit during a rebase
From: Johannes Schindelin
In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
`git commit` learned to suggest to run `git cherry-pick --skip` when
trying to cherry-pick an empty patch.
However, it was overlooked that there are more conditions than just a
`git cherry-pick` when this advice is printed (which originally
suggested the neutral `git reset`): the same can happen during a rebase.
Let's suggest the correct command, even during a rebase.
While at it, we adjust more places in `builtin/commit.c` that
incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
surely this must be a `cherry-pick` in progress.
Note: we take pains to handle the situation when a user runs a `git
cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
line in an interactive rebase). On the other hand, it is not possible to
run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
`sequencer/` exist, we still want to advise to use `git cherry-pick
--skip`.
Signed-off-by: Johannes Schindelin
---
builtin/commit.c | 33 -
t/t3403-rebase-skip.sh | 9 +
2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index e588bc6ad3..2beae13620 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to
conflict resolution.\
"git commit --allow-empty\n"
"\n");
+static const char empty_rebase_advice[] =
+N_("Otherwise, please use 'git rebase --skip'\n");
+
static const char empty_cherry_pick_advice_single[] =
N_("Otherwise, please use 'git cherry-pick --skip'\n");
@@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
static const char *cleanup_arg;
static enum commit_whence whence;
-static int sequencer_in_use;
+static int sequencer_in_use, rebase_in_progress;
static int use_editor = 1, include_status = 1;
static int have_option_m;
static struct strbuf message = STRBUF_INIT;
@@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
whence = FROM_CHERRY_PICK;
if (file_exists(git_path_seq_dir()))
sequencer_in_use = 1;
+ if (file_exists(git_path_rebase_merge_dir()))
+ rebase_in_progress = 1;
}
else
whence = FROM_COMMIT;
@@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char
**argv, const char *prefix
if (whence != FROM_COMMIT) {
if (whence == FROM_MERGE)
die(_("cannot do a partial commit during a merge."));
- else if (whence == FROM_CHERRY_PICK)
+ else if (whence == FROM_CHERRY_PICK) {
+ if (rebase_in_progress && !sequencer_in_use)
+ die(_("cannot do a partial commit during a
rebase."));
die(_("cannot do a partial commit during a
cherry-pick."));
+ }
}
if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
@@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file,
const char *prefix,
fputs(_(empty_amend_advice), stderr);
else if (whence == FROM_CHERRY_PICK) {
fputs(_(empty_cherry_pick_advice), stderr);
- if (!sequencer_in_use)
- fputs(_(empty_cherry_pick_advice_single),
stderr);
- else
+ if (sequencer_in_use)
fputs(_(empty_cherry_pick_advice_multi),
stderr);
+ else if (rebase_in_progress)
+ fputs(_(empty_rebase_advice), stderr);
+ else
+ fputs(_(empty_cherry_pick_advice_single),
stderr);
}
return 0;
}
@@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const
char *argv[],
if (amend && whence != FROM_COMMIT) {
if (whence == FROM_MERGE)
die(_("You are in the middle of a merge -- cannot
amend."));
- else if (whence == FROM_CHERRY_PICK)
+ else if (whence == FROM_CHERRY_PICK) {
+ if (rebase_in_progress && !sequencer_in_use)
+ die(_("You are in the middle of a rebase --
cannot amend."));
die(_("You are in the middle of a cherry-pick -- cannot
amend."));
+ }
}
if (fixup_message && squash_message)
die(_("Options --squash and --fixup cannot be used together"));
@@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
reduce_heads_replace(&parents);
} else {
if (!reflog_msg)
-
[PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/`
From: Johannes Schindelin
The presence of this path will be used in the next commit to fix an
incorrect piece of advice in `git commit` when in the middle of a
rebase.
Signed-off-by: Johannes Schindelin
---
sequencer.c | 4 ++--
sequencer.h | 1 +
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 9d5964fd81..5bd7e9d05a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,7 +47,7 @@ static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
-static GIT_PATH_FUNC(rebase_path, "rebase-merge")
+GIT_PATH_FUNC(git_path_rebase_merge_dir, "rebase-merge")
/*
* The file containing rebase commands, comments, and empty lines.
* This file is created by "git rebase -i" then edited by the user. As
@@ -218,7 +218,7 @@ static inline int is_rebase_i(const struct replay_opts
*opts)
static const char *get_dir(const struct replay_opts *opts)
{
if (is_rebase_i(opts))
- return rebase_path();
+ return git_path_rebase_merge_dir();
return git_path_seq_dir();
}
diff --git a/sequencer.h b/sequencer.h
index 574260f621..505852d7d1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -9,6 +9,7 @@ struct repository;
const char *git_path_commit_editmsg(void);
const char *git_path_seq_dir(void);
+const char *git_path_rebase_merge_dir(void);
const char *rebase_path_todo(void);
const char *rebase_path_todo_backup(void);
--
gitgitgadget
Re: [RFC PATCH 1/7] Makefile: alphabetically sort += lists
Hi, On Mon, 21 Oct 2019, Denton Liu wrote: > Hi Johannes, > > On Mon, Oct 21, 2019 at 08:44:40PM +0200, Johannes Schindelin wrote: > > Hi Junio, > > > > On Fri, 18 Oct 2019, Junio C Hamano wrote: > > > > > Denton Liu writes: > > > > > > > There are many += lists in the Makefile and, over time, they have gotten > > > > slightly out of order, alphabetically. Alphabetically sort all += lists > > > > to bring them back in order. > > > > ... > > > > > > Hmm. I like the general thrust, but ... > > > > > > > LIB_OBJS += combine-diff.o > > > > -LIB_OBJS += commit.o > > > > LIB_OBJS += commit-graph.o > > > > LIB_OBJS += commit-reach.o > > > > +LIB_OBJS += commit.o > > > > > > ... I do not particularly see this change (there may be similar > > > ones) desirable. I'd find it it be much more natural to sort > > > "commit-anything" after "commit", and that is true with or without > > > the common extension ".o" added to these entries. > > > > > > In short, flipping these entries because '.' sorts later than '-' is > > > making the result look "less sorted", at least to me. > > > > The problem with this argument is that it disagrees with ASCII, as `-` > > has code 0x2d while `.` has code 0x2e, i.e. it is lexicographically > > _larger_. > > > > So Denton's patch does the correct thing. > > I actually agree with Junio on this one. Without the prefixes, "commit" > would go before "commit-graph" so I think it would make more sense to > order with the prefixes removed instead of taking the naive ordering by > just sorting each block. That will make it harder on other contributors like me, who prefer to mark the lines in `vim` and then call `:sort` on them, and then not care about it any further. Any decision that makes automating tedious tasks harder puts more burden on human beings. I don't like that. Ciao, Dscho > > Thanks, > > Denton > > > > > Ciao, > > Dscho >
Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
On Tue, Oct 22, 2019 at 05:45:54PM -0400, Jeff King wrote:
> On Tue, Oct 22, 2019 at 04:33:16PM -0400, Jeff King wrote:
>
> > > I have failed to produce a test using the file:// protocol that
> > > demonstrates this bug.
> >
> > Hmm, from the description, it sounds like it should be easy. I might
> > poke at it a bit.
>
> Hmph. I can reproduce it here, but it seems to depend on the repository.
> If I do this:
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index ecabbe1616..8d473a456f 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -583,6 +583,14 @@ test_expect_success 'fetch.writeCommitGraph' '
> )
> '
>
> +test_expect_success 'fetch.writeCommitGraph with a bigger repo' '
> + git clone "$TEST_DIRECTORY/.." repo &&
> + (
> + cd repo &&
> + git -c fetch.writeCommitGraph fetch origin
> + )
> +'
> +
> # configured prune tests
>
> set_config_tristate () {
>
> it reliably triggers the bug. But if I make a synthetic repo, even it
> has a lot of commits (thousands or more), it doesn't trigger. I thought
> maybe it had to do with having commits that were not at tips (since the
> tip ones presumably _are_ fed into the graph generation process). But
> that doesn't seem to help.
>
> Puzzling...
Submodules?
$ cd ~/src/git/
$ git quotelog 86cfd61e6b
86cfd61e6b (sha1dc: optionally use sha1collisiondetection as a submodule,
2017-07-01)
$ git init --bare good.git
Initialized empty Git repository in /home/szeder/src/git/good.git/
$ git push -q good.git 86cfd61e6b^:refs/heads/master
$ git clone good.git good-clone
Cloning into 'good-clone'...
done.
$ git -c fetch.writeCommitGraph -C good-clone fetch origin
Computing commit graph generation numbers: 100% (46958/46958), done.
$ git init --bare bad.git
Initialized empty Git repository in /home/szeder/src/git/bad.git/
$ git push -q bad.git 86cfd61e6b:refs/heads/master
$ git clone bad.git bad-clone
Cloning into 'bad-clone'...
done.
$ git -c fetch.writeCommitGraph -C bad-clone fetch origin
Computing commit graph generation numbers: 100% (1/1), done.
BUG: commit-graph.c:886: missing parent
9936c1b52a39fa14fca04f937df3e75f7498ac66 for commit
86cfd61e6bc12745751c43b4f69886b290cd85cb
Aborted
In the cover letter Derrick mentioned that he used
https://github.com/derrickstolee/numbers for testing, and that repo
has a submodule as well.
Re: [PATCH v2 1/1] ci(osx): use new location of the `perforce` cask
Johannes Schindelin writes: >> This is already in 'next' X-<; reverting a merge is cheap but I >> prefer to do so when we already have a replacement. > > I force-pushed (see https://github.com/gitgitgadget/git/pull/400), and > once Stolee approves, he will submit v3. This will only change the > commit message, though, as I disagree that hard-coding the URL would be > an improvement: the nice thing about a package management system is that > the user does not need to know the details (or need to know if the > details change, like, ever). If this were meant for the upcoming release, I would rather see us copy a butt-ugly-but-known-working procedure if we have one this close to -rc1. If the hard-coded URL ever changes, the procedure we would be copying from would be broken anyway. But I agree 100% that we should take a conceptually cleaner approach for the longer term. Let's replace the original one with this and cook in 'next'---it would be ideal if the ugly-but-know-working one be updated to match in the meantime, but if it is bypassing package management for a reason (the upstream just publishes the URL to download from without packaging it properly, for example?), that would not be possible, and it is OK if that is the case. Thanks.
Re: [PATCH] pretty: Add "%aU"|"%au" option to output author's username
Prarit Bhargava writes:
> Subject: Re: [PATCH] pretty: Add "%aU"|"%au" option to output author's
> username
Downcase "Add" (see "git shortlog --no-merges -100 master" and
mimick the project convention).
> Add a "%aU"|"%au" option that outputs the author's email username.
Even though I personally do not see the use for it, I agree it would
make sense to have an option to show the local part only where the
e-mail address is shown.
I do not know if u/U is a good mnemonic; it hints too strongly that
it may come from GIT_{AUTHOR/COMMITTER}_NAME but that is not what
you are doing---isn't there a letter that better conveys that this
is about RFC 2822 local-part (cf. page 16 ieft.org/rfc/rfc2822.txt)?
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> index b87e2e83e6d0..479a15a8ab12 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -163,6 +163,9 @@ The placeholders are:
> '%ae':: author email
> '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
> or linkgit:git-blame[1])
> +'%au':: author username
> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]
> + or linkgit:git-blame[1])
> '%ad':: author date (format respects --date= option)
> '%aD':: author date, RFC2822 style
> '%ar':: author date, relative
> diff --git a/pretty.c b/pretty.c
> index b32f0369531c..2a5b93022050 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char
> part,
> strbuf_add(sb, mail, maillen);
> return placeholder_len;
> }
> + if (part == 'u' || part == 'U') { /* username */
> + maillen = strstr(s.mail_begin, "@") - s.mail_begin;
> + strbuf_add(sb, mail, maillen);
> + return placeholder_len;
> + }
I think users get %eu and %eU for free with this change, which should
be documented.
Re: [PATCH] pretty: Add "%aU"|"%au" option to output author's username
On 2019-10-22 at 23:28:47, Prarit Bhargava wrote:
> In many projects the number of contributors is low enough that users know
> each other and the full email address doesn't need to be displayed.
> Displaying only the author's username saves a lot of columns on the screen.
> For example displaying "prarit" instead of "pra...@redhat.com" saves 11
> columns.
>
> Add a "%aU"|"%au" option that outputs the author's email username.
I have no position on whether or not this is a useful change. I don't
think I'll end up using it, but I can imagine cases where it is useful,
such as certain corporate environments. It would be interesting to see
what others think.
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> index b87e2e83e6d0..479a15a8ab12 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -163,6 +163,9 @@ The placeholders are:
> '%ae':: author email
> '%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]
> or linkgit:git-blame[1])
> +'%au':: author username
> +'%aU':: author username (respecting .mailmap, see linkgit:git-shortlog[1]
> + or linkgit:git-blame[1])
I think we need to actually document what "username" means here. I
expect you'll have people think that this magically means their
$HOSTING_PLATFORM username, which it is not.
> diff --git a/pretty.c b/pretty.c
> index b32f0369531c..2a5b93022050 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -706,6 +706,11 @@ static size_t format_person_part(struct strbuf *sb, char
> part,
> strbuf_add(sb, mail, maillen);
> return placeholder_len;
> }
> + if (part == 'u' || part == 'U') { /* username */
> + maillen = strstr(s.mail_begin, "@") - s.mail_begin;
> + strbuf_add(sb, mail, maillen);
> + return placeholder_len;
> + }
This branch doesn't appear to do anything different for the mailmap and
non-mailmap cases. Perhaps adding an additional test that demonstrates
the difference would be a good idea.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 1/1] documentation: remove empty doc files
Emily Shaffer writes: > As for the content of this change, I absolutely approve. I've stumbled > across some of these empty docs while looking for answers before and > found it really demoralizing - the community is so interested in > teaching me how to contribute that they've sat on a TODO for 12 years? > :( I even held up api-grep.txt as a (bad) example in a talk I gave this > year. I'm happy to see these files go. I'd approve this move, too, especially if we accompanied deletion with addition (or verification of existence) of necessary docs elsewhere (perhaps in *.h headers).
[PATCH v3 0/1] ci: update caskroom/cask/perforce to new location
Running CI on Mac OS X in Azure Pipelines is currently broken due to a moved homebrew package. Change since v2: * The commit message was improved (thanks Gábor). Change since v1: -The step is now more robust (by pulling homebrew-cask and trying again if the pull failed). Thanks, -Stolee Johannes Schindelin (1): ci(osx): use new location of the `perforce` cask ci/install-dependencies.sh | 5 + 1 file changed, 5 insertions(+) base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-400%2Fderrickstolee%2Fci-caskroom-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-400/derrickstolee/ci-caskroom-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/400 Range-diff vs v2: 1: 372ab24acf ! 1: 9d80e845bf ci(osx): use new location of the `perforce` cask @@ -2,9 +2,8 @@ ci(osx): use new location of the `perforce` cask -The CI builds are failing for Mac OS X due to a change in the -location of the perforce cask. The command outputs the following -error: +The Azure Pipelines builds are failing for macOS due to a change in the +location of the perforce cask. The command outputs the following error: + brew install caskroom/cask/perforce Error: caskroom/cask was moved. Tap homebrew/cask-cask instead. @@ -12,11 +11,27 @@ So let's try to call `brew cask install perforce` first (which is what that error message suggests, in a most round-about way). -The "caskroom" way was added in 672f51cb (travis-ci: -fix Perforce install on macOS, 2017-01-22) and the justification -is that the call "brew cask install perforce" can fail due to a checksum -mismatch: the recipe simply downloads the official Perforce distro, and -whenever that is updated, the recipe needs to be updated, too. +Prior to 672f51cb we used to install the 'perforce' package with 'brew +install perforce' (note: no 'cask' in there). The justification for +672f51cb was that the command 'brew install perforce' simply stopped +working, after Homebrew folks decided that it's better to move the +'perforce' package to a "cask". Their justification for this move was +that 'brew install perforce' "can fail due to a checksum mismatch ...", +and casks can be installed without checksum verification. And indeed, +both 'brew cask install perforce' and 'brew install +caskroom/cask/perforce' printed something along the lines of: + + ==> No checksum defined for Cask perforce, skipping verification + +It is unclear why 672f51cb used 'brew install caskroom/cask/perforce' +instead of 'brew cask install perforce'. It appears (by running both +commands on old Travis CI macOS images) that both commands worked all +the same already back then. + +In any case, as the error message at the top of this commit message +shows, 'brew install caskroom/cask/perforce' has stopped working +recently, but 'brew cask install perforce' still does, so let's use +that. CI servers are typically fresh virtual machines, but not always. To accommodate for that, let's try harder if `brew cask install perforce` @@ -31,6 +46,7 @@ https://dev.azure.com/gitgitgadget/git/_build?definitionId=11&_a=summary will be finished once the next Perforce upgrade comes around. +Helped-by: SZEDER Gábor Signed-off-by: Johannes Schindelin Signed-off-by: Derrick Stolee -- gitgitgadget
[PATCH v3 1/1] ci(osx): use new location of the `perforce` cask
From: Johannes Schindelin
The Azure Pipelines builds are failing for macOS due to a change in the
location of the perforce cask. The command outputs the following error:
+ brew install caskroom/cask/perforce
Error: caskroom/cask was moved. Tap homebrew/cask-cask instead.
So let's try to call `brew cask install perforce` first (which is what
that error message suggests, in a most round-about way).
Prior to 672f51cb we used to install the 'perforce' package with 'brew
install perforce' (note: no 'cask' in there). The justification for
672f51cb was that the command 'brew install perforce' simply stopped
working, after Homebrew folks decided that it's better to move the
'perforce' package to a "cask". Their justification for this move was
that 'brew install perforce' "can fail due to a checksum mismatch ...",
and casks can be installed without checksum verification. And indeed,
both 'brew cask install perforce' and 'brew install
caskroom/cask/perforce' printed something along the lines of:
==> No checksum defined for Cask perforce, skipping verification
It is unclear why 672f51cb used 'brew install caskroom/cask/perforce'
instead of 'brew cask install perforce'. It appears (by running both
commands on old Travis CI macOS images) that both commands worked all
the same already back then.
In any case, as the error message at the top of this commit message
shows, 'brew install caskroom/cask/perforce' has stopped working
recently, but 'brew cask install perforce' still does, so let's use
that.
CI servers are typically fresh virtual machines, but not always. To
accommodate for that, let's try harder if `brew cask install perforce`
fails, by specifically pulling the latest `master` of the
`homebrew-cask` repository.
This will still fail, of course, when `homebrew-cask` falls behind
Perforce's release schedule. But once it is updated, we can now simply
re-run the failed jobs and they will pick up that update.
As for updating `homebrew-cask`: the beginnings of automating this in
https://dev.azure.com/gitgitgadget/git/_build?definitionId=11&_a=summary
will be finished once the next Perforce upgrade comes around.
Helped-by: SZEDER Gábor
Signed-off-by: Johannes Schindelin
Signed-off-by: Derrick Stolee
---
ci/install-dependencies.sh | 5 +
1 file changed, 5 insertions(+)
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index 85a9d6b15c..ce149ed39c 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -40,6 +40,11 @@ osx-clang|osx-gcc)
test -z "$BREW_INSTALL_PACKAGES" ||
brew install $BREW_INSTALL_PACKAGES
brew link --force gettext
+ brew cask install perforce || {
+ # Update the definitions and try again
+ git -C "$(brew
--repository)"/Library/Taps/homebrew/homebrew-cask pull &&
+ brew cask install perforce
+ } ||
brew install caskroom/cask/perforce
case "$jobname" in
osx-gcc)
--
gitgitgadget
Re: [PATCH v2 1/1] ci(osx): use new location of the `perforce` cask
On Wed, Oct 23, 2019 at 01:23:25AM +0200, Johannes Schindelin wrote: > On Fri, 18 Oct 2019, SZEDER Gábor wrote: > > > On Thu, Oct 17, 2019 at 12:47:33PM +, Johannes Schindelin via > > GitGitGadget wrote: > > > From: Johannes Schindelin > > > > > > The CI builds are failing for Mac OS X due to a change in the > > > > s/CI/Azure Pipelines/ > > > > Our Travis CI builds are fine. > > For the moment ;-) Touché. Believe it or not, I did wrote "at least for now" at the end of that sentence, but then deleted it. Serves me right, now there is some new breakage with gcc@8... :) > If you don't mind, I am going to copy/edit these three paragraphs into > the commit message, Sure, go ahead. > > In our CI builds we don't at all care what the checksums of the > > Perforce binaries are, so I would really like to tell 'brew' to ignore > > any checksum mismatch when installing 'perforce'. Alas, it appears > > that 'brew' has no public options to turn of or to ignore checksum > > verification. > > Sad, yet true, that we indeed have no command-line option to say "you > know what, your checksum possibly mismatches, but we really don't care". Actually, 'brew' does have some undocumented options, but I didn't even bothered to check, because it's not really sensible to rely on an undocumented option (especially when even the documented options break every once in a while...). > > Now, let's take a step back. > > > > All 'brew cask install perforce' really does is run 'curl' to download > > a tar.gz from the Perforce servers, verify its checksum, unpack it, > > and put the executables somewhere on $PATH. That's not rocket > > science, we could easily do that ourselves; we don't even have to deal > > with a tar.gz, the 'p4' and 'p4d' binaries for mac are readily > > available for download at: > > > > http://filehost.perforce.com/perforce/r19.1/bin.macosx1010x86_64/ > > > > And, in fact, that's what we have been doing in some of our Linux jobs > > since the very beginning, so basically only the download URL has to be > > adjusted. > > I'd rather not. > > Just because there is no better way on Linux, and just because the > current `perforce` cask recipe happens to just download and unpack that > file does not mean that this won't change. Yeah, I'm fairly sure that the way Homebrew installs Perforce will change, but if we download Perforce ourselves, then it won't matter at all. Downloading the Perforce binaries with 'wget' worked fairly well for almost four years, except from that server glitch a couple of weeks ago; I think downloading the macOS binaries from the same server would work just as well. OTOH, this is the fourth time that we have to tweak how we install Perforce via Homebrew. FWIW, it looks like this: https://github.com/szeder/git/blob/ci-osx-wget-perforce/ci/install-dependencies.sh#L11
[PATCH 0/1] Thyme two ficks sum Documentaton tyops and speling erors!
We have a number of typos and spelling errors that I spotted under Documentation/. It'd be nice if someone could double check that I placed the missing right parenthesis correctly in Documentation/technical/api-trace2.txt. Also, not sure if folks would be happy or unhappy with me un-splitting a word in commit-graph.txt. Elijah Newren (1): Documentation: fix a bunch of typos, both old and new Documentation/CodingGuidelines | 2 +- Documentation/RelNotes/1.7.0.2.txt | 2 +- Documentation/RelNotes/1.7.10.4.txt| 2 +- Documentation/RelNotes/1.7.12.3.txt| 2 +- Documentation/RelNotes/1.7.5.3.txt | 2 +- Documentation/RelNotes/1.8.0.txt | 2 +- Documentation/RelNotes/2.1.3.txt | 2 +- Documentation/RelNotes/2.10.0.txt | 2 +- Documentation/RelNotes/2.10.2.txt | 2 +- Documentation/RelNotes/2.11.1.txt | 2 +- Documentation/RelNotes/2.12.0.txt | 2 +- Documentation/RelNotes/2.13.3.txt | 4 ++-- Documentation/RelNotes/2.14.0.txt | 4 ++-- Documentation/RelNotes/2.16.0.txt | 2 +- Documentation/RelNotes/2.16.3.txt | 2 +- Documentation/RelNotes/2.17.0.txt | 2 +- Documentation/RelNotes/2.18.0.txt | 2 +- Documentation/RelNotes/2.19.0.txt | 2 +- Documentation/RelNotes/2.24.0.txt | 2 +- Documentation/RelNotes/2.3.3.txt | 2 +- Documentation/RelNotes/2.3.7.txt | 2 +- Documentation/RelNotes/2.4.3.txt | 2 +- Documentation/RelNotes/2.8.0.txt | 2 +- Documentation/RelNotes/2.9.3.txt | 2 +- Documentation/config/tag.txt | 2 +- Documentation/git-bisect-lk2009.txt| 2 +- Documentation/git-check-attr.txt | 2 +- Documentation/git-check-ignore.txt | 2 +- Documentation/git-filter-branch.txt| 2 +- Documentation/git-range-diff.txt | 2 +- Documentation/git-tag.txt | 2 +- Documentation/gitattributes.txt| 2 +- Documentation/gitmodules.txt | 2 +- Documentation/technical/api-trace2.txt | 14 +++--- Documentation/technical/commit-graph.txt | 12 ++-- .../technical/hash-function-transition.txt | 2 +- Documentation/technical/index-format.txt | 4 ++-- Documentation/technical/partial-clone.txt | 2 +- Documentation/technical/protocol-v2.txt| 2 +- Documentation/technical/rerere.txt | 2 +- 40 files changed, 54 insertions(+), 54 deletions(-) base-commit: d966095db01190a2196e31195ea6fa0c722aa732 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-418%2Fnewren%2Ftypo-fixes-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-418/newren/typo-fixes-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/418 -- gitgitgadget
[PATCH 1/1] Documentation: fix a bunch of typos, both old and new
From: Elijah Newren Signed-off-by: Elijah Newren --- Documentation/CodingGuidelines | 2 +- Documentation/RelNotes/1.7.0.2.txt | 2 +- Documentation/RelNotes/1.7.10.4.txt| 2 +- Documentation/RelNotes/1.7.12.3.txt| 2 +- Documentation/RelNotes/1.7.5.3.txt | 2 +- Documentation/RelNotes/1.8.0.txt | 2 +- Documentation/RelNotes/2.1.3.txt | 2 +- Documentation/RelNotes/2.10.0.txt | 2 +- Documentation/RelNotes/2.10.2.txt | 2 +- Documentation/RelNotes/2.11.1.txt | 2 +- Documentation/RelNotes/2.12.0.txt | 2 +- Documentation/RelNotes/2.13.3.txt | 4 ++-- Documentation/RelNotes/2.14.0.txt | 4 ++-- Documentation/RelNotes/2.16.0.txt | 2 +- Documentation/RelNotes/2.16.3.txt | 2 +- Documentation/RelNotes/2.17.0.txt | 2 +- Documentation/RelNotes/2.18.0.txt | 2 +- Documentation/RelNotes/2.19.0.txt | 2 +- Documentation/RelNotes/2.24.0.txt | 2 +- Documentation/RelNotes/2.3.3.txt | 2 +- Documentation/RelNotes/2.3.7.txt | 2 +- Documentation/RelNotes/2.4.3.txt | 2 +- Documentation/RelNotes/2.8.0.txt | 2 +- Documentation/RelNotes/2.9.3.txt | 2 +- Documentation/config/tag.txt | 2 +- Documentation/git-bisect-lk2009.txt| 2 +- Documentation/git-check-attr.txt | 2 +- Documentation/git-check-ignore.txt | 2 +- Documentation/git-filter-branch.txt| 2 +- Documentation/git-range-diff.txt | 2 +- Documentation/git-tag.txt | 2 +- Documentation/gitattributes.txt| 2 +- Documentation/gitmodules.txt | 2 +- Documentation/technical/api-trace2.txt | 14 +++--- Documentation/technical/commit-graph.txt | 12 ++-- .../technical/hash-function-transition.txt | 2 +- Documentation/technical/index-format.txt | 4 ++-- Documentation/technical/partial-clone.txt | 2 +- Documentation/technical/protocol-v2.txt| 2 +- Documentation/technical/rerere.txt | 2 +- 40 files changed, 54 insertions(+), 54 deletions(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index f45db5b727..d05a80fe9d 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -75,7 +75,7 @@ For shell scripts specifically (not exhaustive): - If you want to find out if a command is available on the user's $PATH, you should use 'type ', instead of 'which '. - The output of 'which' is not machine parseable and its exit code + The output of 'which' is not machine parsable and its exit code is not reliable across platforms. - We use POSIX compliant parameter substitutions and avoid bashisms; diff --git a/Documentation/RelNotes/1.7.0.2.txt b/Documentation/RelNotes/1.7.0.2.txt index fcb46ca6a4..73ed2b5278 100644 --- a/Documentation/RelNotes/1.7.0.2.txt +++ b/Documentation/RelNotes/1.7.0.2.txt @@ -34,7 +34,7 @@ Fixes since v1.7.0.1 * "git status" in 1.7.0 lacked the optimization we used to have in 1.6.X series to speed up scanning of large working tree. - * "gitweb" did not diagnose parsing errors properly while reading tis configuration + * "gitweb" did not diagnose parsing errors properly while reading its configuration file. And other minor fixes and documentation updates. diff --git a/Documentation/RelNotes/1.7.10.4.txt b/Documentation/RelNotes/1.7.10.4.txt index 326670df6e..57597f2bf3 100644 --- a/Documentation/RelNotes/1.7.10.4.txt +++ b/Documentation/RelNotes/1.7.10.4.txt @@ -7,7 +7,7 @@ Fixes since v1.7.10.3 * The message file for Swedish translation has been updated a bit. * A name taken from mailmap was copied into an internal buffer - incorrectly and could overun the buffer if it is too long. + incorrectly and could overrun the buffer if it is too long. * A malformed commit object that has a header line chomped in the middle could kill git with a NULL pointer dereference. diff --git a/Documentation/RelNotes/1.7.12.3.txt b/Documentation/RelNotes/1.7.12.3.txt index ecda427a35..4b822976b8 100644 --- a/Documentation/RelNotes/1.7.12.3.txt +++ b/Documentation/RelNotes/1.7.12.3.txt @@ -25,7 +25,7 @@ Fixes since v1.7.12.2 its Accept-Encoding header. * "git receive-pack" (the counterpart to "git push") did not give - progress output while processing objects it received to the puser + progress output while processing objects it received to the user when run over the smart-http protocol. * "git status" honored the ignore=dirty setting
Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
On 10/22/2019 7:35 PM, SZEDER Gábor wrote: > On Tue, Oct 22, 2019 at 05:45:54PM -0400, Jeff King wrote: >> Puzzling... > > Submodules? > > $ cd ~/src/git/ > $ git quotelog 86cfd61e6b > 86cfd61e6b (sha1dc: optionally use sha1collisiondetection as a submodule, > 2017-07-01) > $ git init --bare good.git > Initialized empty Git repository in /home/szeder/src/git/good.git/ > $ git push -q good.git 86cfd61e6b^:refs/heads/master > $ git clone good.git good-clone > Cloning into 'good-clone'... > done. > $ git -c fetch.writeCommitGraph -C good-clone fetch origin > Computing commit graph generation numbers: 100% (46958/46958), done. > $ git init --bare bad.git > Initialized empty Git repository in /home/szeder/src/git/bad.git/ > $ git push -q bad.git 86cfd61e6b:refs/heads/master > $ git clone bad.git bad-clone > Cloning into 'bad-clone'... > done. > $ git -c fetch.writeCommitGraph -C bad-clone fetch origin > Computing commit graph generation numbers: 100% (1/1), done. > BUG: commit-graph.c:886: missing parent > 9936c1b52a39fa14fca04f937df3e75f7498ac66 for commit > 86cfd61e6bc12745751c43b4f69886b290cd85cb > Aborted > > In the cover letter Derrick mentioned that he used > https://github.com/derrickstolee/numbers for testing, and that repo > has a submodule as well. I completely forgot that I put a submodule in that repo. It makes sense that the Git repo also has one. Thanks for the test! I'll get it into the test suite tomorrow. -Stolee
Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
On Tue, Oct 22, 2019 at 08:35:57PM -0400, Derrick Stolee wrote:
> > In the cover letter Derrick mentioned that he used
> > https://github.com/derrickstolee/numbers for testing, and that repo
> > has a submodule as well.
>
> I completely forgot that I put a submodule in that repo. It makes sense
> that the Git repo also has one. Thanks for the test! I'll get it into
> the test suite tomorrow.
Ah, right. Good catch Gábor. The test below fails for me without your
patch, and succeeds with it (the extra empty commit is necessary so that
we have a parent).
I admit I am puzzled, though, _why_ the presence of the submodule
matters. That is, from your explanation, I thought the issue was simply
that `fetch` walked (and marked) some commits, and the flags overlapped
with what the commit-graph code expected.
I could guess that the presence of the submodule triggers some analysis
for --recurse-submodules. But then we don't actually recurse (maybe
because they're not activated? In which case maybe we shouldn't be doing
that extra walk to look for submodules if there aren't any activated
ones in our local repo).
I'd also wonder if it would be possible to trigger in another way (say,
due to want/have negotiation, though I guess most of the walking there
is done on the server side). But it may not be worth digging too far
into it.
-Peff
---
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index ecabbe1616..1b092fec0b 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -583,6 +583,21 @@ test_expect_success 'fetch.writeCommitGraph' '
)
'
+test_expect_success 'fetch.writeCommitGraph with a submodule' '
+ git init has-submodule &&
+ (
+ cd has-submodule &&
+ git commit --allow-empty -m parent &&
+ git submodule add ../three &&
+ git commit -m "add submodule"
+ ) &&
+ git clone has-submodule submod-clone &&
+ (
+ cd submod-clone &&
+ git -c fetch.writeCommitGraph fetch origin
+ )
+'
+
# configured prune tests
set_config_tristate () {
Re: [PATCH 1/1] commit-graph: fix writing first commit-graph during fetch
On Tue, Oct 22, 2019 at 08:48:20PM -0400, Jeff King wrote: > I admit I am puzzled, though, _why_ the presence of the submodule > matters. That is, from your explanation, I thought the issue was simply > that `fetch` walked (and marked) some commits, and the flags overlapped > with what the commit-graph code expected. > > I could guess that the presence of the submodule triggers some analysis > for --recurse-submodules. But then we don't actually recurse (maybe > because they're not activated? In which case maybe we shouldn't be doing > that extra walk to look for submodules if there aren't any activated > ones in our local repo). Indeed, that seems to be it. If I do this: git init repo cd repo cat >.gitmodules <<\EOF [submodule "foo"] path = foo url = https://example.com EOF time git fetch /path/to/git.git then we end up traversing the whole git.git history a second time, even though we should know off the bat that there are no active submodules that we would recurse to. Doing this makes the problem go away: diff --git a/submodule.c b/submodule.c index 0f199c5137..0db2f18b93 100644 --- a/submodule.c +++ b/submodule.c @@ -1193,7 +1193,7 @@ static void calculate_changed_submodule_paths(struct repository *r, struct string_list_item *name; /* No need to check if there are no submodules configured */ - if (!submodule_from_path(r, NULL, NULL)) + if (!is_submodule_active(r, NULL)) return; argv_array_push(&argv, "--"); /* argv[0] program name */ but causes some tests to fail (I think that in some cases we're supposed to auto-initialize, and we'd probably need to cover that case, too). All of this is outside of your fix, of course, but: 1. I'm satisfied now that I understand why the test triggers the problem. 2. You may want have a real activated submodule in your test. Right now we'll trigger the submodule-recursion check even without that, but in the future we might do something like the hunk above. In which case your test wouldn't be checking anything interesting anymore. -Peff
Re: [RFC/WIP] range-diff: show old/new blob OIDs in comments
Johannes Schindelin wrote: > Hi Eric, > > > On Thu, 17 Oct 2019, Eric Wong wrote: > > > (WIP, mostly stream-of-concious notes + reasoning) > > > > When using "git format-patch --range-diff", the pre and > > post-image blob OIDs are in each email, while the exact > > commit OIDs are rarely shared via emails (only the tip > > commit from "git request-pull"). > > > > These blob OIDs make it easy to search for or lookup the > > full emails which create them, or the blob itself once > > it's fetched via git. > > > > public-inbox indexes and allows querying specifically for blob > > OIDs via dfpre:/dfpost: since June 2017. As of Jan 2019, > > public-inbox also supports recreating blobs out of patch emails > > (querying internally with dfpre:/dfpost: and doing "git apply") > > > > Searching on these blob OIDs also makes it easier to find > > previous versions of the patch sets using any mail search > > engine. > > > > Future changes to public-inbox may allow generating custom > > diffs out of any blobs it can find or recreate. > > > > Most of this is pretty public-inbox-specific and would've > > made some future changes to public-inbox much easier > > (if we had this from the start of range-diff). > > > > Unfortunately, it won't help with cases where range-diffs > > are already published, but range-diff isn't too old. > > I guess your patch won't hurt. Cool, will update tests and resend. > As to recreating blobs from mails: Wow. That's quite a length you're > going, and I think it is a shame that you have to. If only every contribution came accompanied with a pullable branch in a public > repository. What Konstantin said about git repos being transient. It wasn't too much work to recreate those blobs from scratch since git-apply has done it since 2005. Just add search :) We could get around transient repos with automatic mirroring bots which never deletes or overwrites anything published. That includes preserving pre-force-push data in case of force pushes. > Instead, we will have to rely on your centralized, non-distributed > service... I'm curious how you came to believe that, since that's the opposite of what public-inbox has always been intended to be. The only thing that's centralized and not reproducible by mirrors is the domain name (and I also have Tor .onion mirrors with no dependency on ICAAN). Memorable naming is a tricky problem in decentralized systems, though...
Re: [PATCH] test-progress: fix test failures on big-endian systems
Jeff King writes: > But here's where it gets tricky. In addition to catching any size > mismatches, this will also catch signedness problems. I.e., if we make > OPT_INTEGER() use "intp", then everybody passing in &unsigned_var now > gets a compiler warning. Which maybe is a good thing, I dunno. Hmph, true. I'd agree with back-burnering it for now. Perhaps we'd fix the signedness issue one by one in a preparatory series before converting the value field to a union, if we want to pursue this idea further (in which I am mildly interested, by the way), but it does sound like it should be given lower priority. > So that's where I gave up. Converting between signed and unsigned > variables needs to be done very carefully, as there are often subtle > impacts (e.g., loop terminations). And because we have so many sign > issues already, compiling with "-Wsign-compare", etc, isn't likely to > help. True. Thanks.
Re: [PATCH v2 0/2] Fix the speed of the CI (Visual Studio) tests
"Johannes Schindelin via GitGitGadget" writes: > Changes since v1: > > * Fixed typo "nore" -> "nor" in the commit message. > > Johannes Schindelin (2): > ci(visual-studio): use strict compile flags, and optimization > ci(visual-studio): actually run the tests in parallel > > azure-pipelines.yml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Thanks. I will take the change to 'master' directly, as cooking in 'next' won't give it any extra exposure when the topic touches only this file and the patch comes from those who do exercise azure CI build before sending it out to the list ;-)
Re: [PATCH v5 1/2] format-patch: create leading components of output directory
Bert Wesarg writes: > Please ignore this. Will rebase on 2.24-rc0 and will only include the > test changes. Thanks.
Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
Johannes Schindelin writes: > However, I think this is _really_ ugly and intrusive. The opposite of > what my goals were. > > So I think I'll just bite the bullet and use a temporary copy if the > argument ends in `.lock`. Sounds like a quite sensible design decision to me.
Re: [PATCH v2 1/1] config: move documentation to config.h
Emily Shaffer writes: >> ... >> +/** >> + * The config API gives callers a way to access Git configuration files >> + * (and files which have the same syntax). See linkgit:git-config[1] for a > > Ah, here's another place where the Asciidoc link isn't going to do > anything anymore. > > Otherwise I didn't still see anything jumping out. When the commit > message is cleaned up I'm ready to add my Reviewed-by line. Thanks. Your review(s) have been quite sensible and helpful.
Re: [PATCH] t7419: change test_must_fail to ! for grep
Denton Liu writes: > According to t/README, test_must_fail() should only be used to test for > failure in Git commands. Replace the invocations of > `test_must_fail grep` with `! grep`. > > Signed-off-by: Denton Liu > --- > *sigh* Here's another cleanup patch for 'dl/submodule-set-branch'. It's > inspired by Eric Sunshine's comments on the t5520 patchset from earlier. > It's definitely not urgent, though, and can wait for v2.25.0. True, but they are trivially correct and removes the risk of inexperienced developers copying and pasting this bad pattern to other tests that was added during this cycle, so I think it is a good idea to take it now. Thanks for being extra careful.
Re: [BUG] "--show-current-patch" return a mail instead of a patch
Jerome Pouiller writes:
> Hello all,
>
> I try to use "git am" to apply a patch sent using "git send-email". This
> patch does not apply properly. I try to use "git am --show-current-patch"
> to understand the problem. However, since original mail is encoded in quoted-
> printable, data returned by --show-current-patch is not a valid patch.
I agree that --show-current-patch is a misdesigned feature. We'd be
doing a better service to our users if we documented that the patch
and log message are found at .git/rebase-apply/{patch,msg} instead
of trying to hide the path.
Unfortunately, it is likely that those who added that feature have
built their tooling around it to depend on its output being the full
e-mail message "am" was fed (and split by "git mailsplit"). So I do
not think we will be changing the output to the patch file only.
But even then, the documentation can be fixed without any backward
compatibility issues. Perhaps like this?
Documentation/git-am.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 6f6c34b0f4..f63b70325c 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -172,7 +172,7 @@ default. You can use `--no-utf8` to override this.
untouched.
--show-current-patch::
- Show the patch being applied when "git am" is stopped because
+ Show the entire e-mail message "git am" has stopped at, because
of conflicts.
DISCUSSION
Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
"Johannes Schindelin via GitGitGadget" writes: > Note: we take pains to handle the situation when a user runs a `git > cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec` > line in an interactive rebase). On the other hand, it is not possible to > run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and > `sequencer/` exist, we still want to advise to use `git cherry-pick > --skip`. Good description of why the code does what it does, that future readers will surely appreciate. Nice.
Re: [PATCH 0/3] commit: fix advice for empty commits during rebases
"Johannes Schindelin via GitGitGadget"
writes:
> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we
> introduced a helpful message that suggests to run git cherry-pick --skip
> (instead of the previous message that talked about git reset) when a
> cherry-pick failed due to an empty patch.
>
> However, the same message is displayed during a rebase, when the patch
> to-be-committed is empty. In this case, git reset would also have worked,
> but git cherry-pick --skip does not work. This is a regression introduced in
> this cycle.
>
> Let's be more careful here.
>
> Johannes Schindelin (3):
> cherry-pick: add test for `--skip` advice in `git commit`
> sequencer: export the function to get the path of `.git/rebase-merge/`
> commit: give correct advice for empty commit during a rebase
Overall they looked nicely done. The last one may have started its
life as "let's fix advice for empty", but no longer is.
The old code used the sequencer_in_use boolean to tell between two
states ("are we doing a single pick, or a range pick?"), but now we
have another boolean rebase_in_progress, and depending on the shape
of the if statements existing codepaths happen to have, these two
are combined in different ways to deal with new states. E.g. some
places say
if (rebase_in_progress && !sequencer_in_use)
we are doing a rebase;
else
we are doing a cherry-pick;
and some others say
if (sequencer_in_use)
we are doing a multi pick;
else if (rebase_in_progress)
we are doing a rebase;
else
we are doing a single pick;
I wonder if it makes the resulting logic simpler to reason about if
these combination of two variables are rewritten to use a single
variable that enumerates three (or is it four, counting "we are
doing neither a cherry-pick or a rebase"?) possible state.
Other than that, looked sensible. Will queue.
Thanks.
Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
SZEDER Gábor writes: >> +char *base = buf->buf + git_dir_len, *base2 = NULL; >> + >> +if (ends_with(base, ".lock")) >> +base = base2 = xstrndup(base, strlen(base) - 5); > > Hm, this adds the magic number 5 and calls strlen(base) twice, because > ends_with() calls strip_suffix(), which calls strlen(). Calling > strip_suffix() directly would allow us to avoid the magic number and > the second strlen(): > > size_t len; > if (strip_suffix(base, ".lock", &len)) > base = base2 = xstrndup(base, len); Makes sense, and is easy to squash in.
Re: [PATCH v5 13/17] read-tree: show progress by default
Derrick Stolee writes: >> I'm slightly wary of changing the output of plumbing commands >> like this. If a script wants progress output it can already get >> it by passing --verbose. With this change a script that does not >> want that output now has to pass --no-verbose. > > If a script is calling this, then won't stderr not be a terminal window, and > isatty(2) return 0? Unless the script tries to capture the error output and react differently depending on the error message from the plumbing (which is not localized), iow most of the time, standard error stream is left unredirected and likely to be connected to the terminal if the script is driven from a terminal command line. > Or, if the script is run with stderr passing through to > a terminal, then the user would see progress while running the script, which > seems like a side-effect but not one that will cause a broken script. It will show unwanted output to the end users, no? That is the complaint about having to pass --no-verbose, if I understand correctly, if the script does not want to show the progress output.
Re: [PATCH v5 00/17] New sparse-checkout builtin and "cone" mode
"Derrick Stolee via GitGitGadget" writes: > V4 UPDATE: Rebased on latest master to include ew/hashmap and > ds/include-exclude in the base. > > This series makes the sparse-checkout feature more user-friendly. While > there, I also present a way to use a limited set of patterns to gain a > significant performance boost in very large repositories. > ... > Updates in V5: > > * The 'set' subcommand now enables the core.sparseCheckout config setting >(unless the checkout fails). > > > * If the in-process unpack_trees() fails with the new patterns, the >index.lock file is rolled back before the replay of the old >sparse-checkout patterns. > > > * Some documentation fixes, f(d)open->xf(d)open calls, and other nits. >Thanks everyone! Thanks. I quickly scanned the changes between the last round and this one, and all I saw were pure improvements ;-) Not that I claim to have read the previous round very carefully, though. Will replace and queue.
Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
SZEDER Gábor writes: >> - b9317d55a3 added two new keys to the trie: 'logs/refs/rewritten' >> and 'logs/refs/worktree', next to the already existing >> 'logs/refs/bisect'. This resulted in a trie node with the path >> 'logs/refs', which didn't exist before, and which doesn't have a > > Oops, I missed the trailing slash, that must be 'logs/refs/'! > >> value attached. A query for 'logs/refs/' finds this node and then >> hits that one callsite of the match function which doesn't check >> for the value's existence, and thus invokes the match function >> with NULL as value. Given that the trie is maintained by hand in common_list[], I wonder if we can mechanically catch errors like the one b9317d55a3 added, by perhaps having a self-test function that a t/helper/ program calls to perform consistency check after the "git" gets built. Thanks.
Re: [RFC/WIP] range-diff: show old/new blob OIDs in comments
Eric Wong writes: > What Konstantin said about git repos being transient. > It wasn't too much work to recreate those blobs from > scratch since git-apply has done it since 2005. ;-) > We could get around transient repos with automatic mirroring > bots which never deletes or overwrites anything published. > That includes preserving pre-force-push data in case of > force pushes. > >> Instead, we will have to rely on your centralized, non-distributed >> service... > > I'm curious how you came to believe that, since that's the > opposite of what public-inbox has always been intended to be. I think the (mis)perception comes from the fact that the website and the newsfeed you give are both too easy to use and directly attract end users, instead of enticing them to keep their own mirrors for offline use. Thanks for injecting dose of sanity.
