Re: [PATCH v2 18/18] completion: support branch-diff
On Fri, May 04, 2018 at 05:35:11PM +0200, Johannes Schindelin wrote:
> Tab completion of `branch-diff` is very convenient, especially given
> that the revision arguments that need to be passed to `git branch-diff`
> are typically more complex than, say, your grandfather's `git log`
> arguments.
>
> Without this patch, we would only complete the `branch-diff` part but
> not the options and other arguments.
>
> This of itself may already be slightly disruptive for well-trained
> fingers that assume that `git braorimas` would expand to
> `git branch origin/master`, as we now no longer automatically append a
> space after completing `git branch`: this is now ambiguous.
>
> Signed-off-by: Johannes Schindelin
> ---
> contrib/completion/git-completion.bash | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 01dd9ff07a2..45addd525ac 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1496,6 +1496,24 @@ _git_format_patch ()
> __git_complete_revlist
> }
>
> +__git_branch_diff_options="
> + --no-patches --creation-weight= --dual-color
> +"
> +
> +_git_branch_diff ()
> +{
> + case "$cur" in
> + --*)
> + __gitcomp "
You should use __gitcomp_builtin so you don't have to maintain
$__git_branch_diff_options here. Something like this
-- 8< --
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 45addd525a..4745631daf 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1496,18 +1496,11 @@ _git_format_patch ()
__git_complete_revlist
}
-__git_branch_diff_options="
- --no-patches --creation-weight= --dual-color
-"
-
_git_branch_diff ()
{
case "$cur" in
--*)
- __gitcomp "
- $__git_branch_diff_options
- $__git_diff_common_options
- "
+ __gitcomp_builtin branch-diff "$__git_diff_common_options"
return
;;
esac
-- 8< --
> + $__git_branch_diff_options
> + $__git_diff_common_options
> + "
> + return
> + ;;
> + esac
> + __git_complete_revlist
> +}
> +
> _git_fsck ()
> {
> case "$cur" in
> --
> 2.17.0.409.g71698f11835
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sun, May 6, 2018 at 6:53 AM, Jacob Keller wrote: > On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic > wrote: >> Hi Dscho, >> >> On 05/05/2018 23:57, Johannes Schindelin wrote: >>> >>> > > This builtin does not do a whole lot so far, apart from showing a >>> > > usage that is oddly similar to that of `git tbdiff`. And for a >>> > > good reason: the next commits will turn `branch-diff` into a >>> > > full-blown replacement for `tbdiff`. >>> > >>> > One minor point about the name: will it become annoying as a tab >>> > completion conflict with git-branch? >>> >>> I did mention this in the commit message of 18/18: >>> >>> Without this patch, we would only complete the `branch-diff` part but >>> not the options and other arguments. >>> >>> This of itself may already be slightly disruptive for well-trained >>> fingers that assume that `git braorimas` would expand to >>> `git branch origin/master`, as we now no longer automatically append a >>> space after completing `git branch`: this is now ambiguous. >>> >>> > It feels really petty complaining about the name, but I just want >>> > to raise the point, since it will never be easier to change than >>> > right now. >>> >>> I do hear you. Especially since I hate `git cherry` every single >>> time that I try to tab-complete `git cherry-pick`. >>> >>> > (And no, I don't really have another name in mind; I'm just >>> > wondering if "subset" names like this might be a mild annoyance in >>> > the long run). >>> >>> They totally are, and if you can come up with a better name, I am >>> really interested in changing it before this hits `next`, even. >> >> I gave this just a quick glance so might be I`m missing something >> obvious or otherwise well-known here, bur why not `diff-branch` instead? >> >> From user interface perspective, I would (personally) rather expect a >> command that does "diff of branches" to belong to "diff family" of >> commands (just operating on branches, instead of "branch" command >> knowing to "diff itself"), and I see we already have `diff-files`, >> `diff-index` and `diff-tree`, for what that`s worth. >> >> Heck, I might even expect something like `git diff --branch ...` to work, >> but I guess that is yet a different matter :) >> >> Thanks, Buga > > I like diff-branch, though I suppose that also conflicts with diff too. How about interdiff? -- Duy
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Todd, On Sat, 5 May 2018, Todd Zullinger wrote: > I wrote: > > Would it be possible and reasonable to teach 'git branch' to > > call this as a subcommand, i.e. as 'git branch diff'? Then > > the completion wouldn't offer git branch-diff. > > Of course right after I sent this, it occurred to me that > 'git branch diff' would make mask the ability to create a > branch named diff. Using 'git branch --diff ...' wouldn't > suffer that problem. Yep, I immediately thought of --diff instead of diff when I read your previous mail on that matter. And I like this idea! Of course, it will complicate the code to set up the pager a bit (for `branch-diff`, I could default to "on" all the time). But IIRC we recently changed the --list cmdmode to set the pager to "auto", so I'll just copy that. > It does add a bit more overhead to the 'git branch' command, > in terms of documentation and usage. I'm not sure it's too > much though. The git-branch summary wouldn't change much: > > -git-branch - List, create, or delete branches > +git-branch - List, create, delete, or diff branches Indeed. Unless I hear objections, I will work on moving to `git branch --diff` (it might take a while, though, I will be traveling for work this week). Ciao, Johannes
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Duy, On Sun, 6 May 2018, Duy Nguyen wrote: > On Sun, May 6, 2018 at 6:53 AM, Jacob Keller wrote: > > On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic > > wrote: > >> > >> On 05/05/2018 23:57, Johannes Schindelin wrote: > >>> > >>> > > This builtin does not do a whole lot so far, apart from showing a > >>> > > usage that is oddly similar to that of `git tbdiff`. And for a > >>> > > good reason: the next commits will turn `branch-diff` into a > >>> > > full-blown replacement for `tbdiff`. > >>> > > >>> > One minor point about the name: will it become annoying as a tab > >>> > completion conflict with git-branch? > >>> > >>> I did mention this in the commit message of 18/18: > >>> > >>> Without this patch, we would only complete the `branch-diff` part but > >>> not the options and other arguments. > >>> > >>> This of itself may already be slightly disruptive for well-trained > >>> fingers that assume that `git braorimas` would expand > >>> to > >>> `git branch origin/master`, as we now no longer automatically append a > >>> space after completing `git branch`: this is now ambiguous. > >>> > >>> > It feels really petty complaining about the name, but I just want > >>> > to raise the point, since it will never be easier to change than > >>> > right now. > >>> > >>> I do hear you. Especially since I hate `git cherry` every single > >>> time that I try to tab-complete `git cherry-pick`. > >>> > >>> > (And no, I don't really have another name in mind; I'm just > >>> > wondering if "subset" names like this might be a mild annoyance in > >>> > the long run). > >>> > >>> They totally are, and if you can come up with a better name, I am > >>> really interested in changing it before this hits `next`, even. > >> > >> I gave this just a quick glance so might be I`m missing something > >> obvious or otherwise well-known here, bur why not `diff-branch` instead? > >> > >> From user interface perspective, I would (personally) rather expect a > >> command that does "diff of branches" to belong to "diff family" of > >> commands (just operating on branches, instead of "branch" command > >> knowing to "diff itself"), and I see we already have `diff-files`, > >> `diff-index` and `diff-tree`, for what that`s worth. > >> > >> Heck, I might even expect something like `git diff --branch ...` to work, > >> but I guess that is yet a different matter :) > >> > >> Thanks, Buga > > > > I like diff-branch, though I suppose that also conflicts with diff too. > > How about interdiff? No. An interdiff is well defined as the diff you would get by first applying the first of two patches in reverse and then the second patch forward. In other words, it turns two revisions of a patch into the diff between the result of applying both revisions. I tried very hard to avoid using that term in my patch series (tbdiff used the term incorrectly: what it called an interdiff is a diff of two patches, where a patch is an author line followed by the commit message followed by the commit diff). Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Buga, On Sun, 6 May 2018, Igor Djordjevic wrote: > On 05/05/2018 23:57, Johannes Schindelin wrote: > > > > > > This builtin does not do a whole lot so far, apart from showing a > > > > usage that is oddly similar to that of `git tbdiff`. And for a > > > > good reason: the next commits will turn `branch-diff` into a > > > > full-blown replacement for `tbdiff`. > > > > > > One minor point about the name: will it become annoying as a tab > > > completion conflict with git-branch? > > > > I did mention this in the commit message of 18/18: > > > > Without this patch, we would only complete the `branch-diff` part but > > not the options and other arguments. > > > > This of itself may already be slightly disruptive for well-trained > > fingers that assume that `git braorimas` would expand to > > `git branch origin/master`, as we now no longer automatically append a > > space after completing `git branch`: this is now ambiguous. > > > > > It feels really petty complaining about the name, but I just want > > > to raise the point, since it will never be easier to change than > > > right now. > > > > I do hear you. Especially since I hate `git cherry` every single > > time that I try to tab-complete `git cherry-pick`. > > > > > (And no, I don't really have another name in mind; I'm just > > > wondering if "subset" names like this might be a mild annoyance in > > > the long run). > > > > They totally are, and if you can come up with a better name, I am > > really interested in changing it before this hits `next`, even. > > I gave this just a quick glance so might be I`m missing something > obvious or otherwise well-known here, bur why not `diff-branch` instead? I think that is just turning the problem from `branch` to `diff`. Of course, we have precedent with diff-index and diff-files. Except that they don't auto-complete (because they are low-level commands) and I *would* like the subcommand discussed in this here patch series to auto-complete. I think Todd's idea to shift it from a full-blown builtin to a cmdmode of `branch` makes tons of sense. Ciao, Dscho
Re: [PATCH v2 05/18] branch-diff: also show the diff between patches
Hi Buga, On Sun, 6 May 2018, Igor Djordjevic wrote: > On 04/05/2018 17:34, Johannes Schindelin wrote: > > Just like tbdiff, we now show the diff between matching patches. This is > > a "diff of two diffs", so it can be a bit daunting to read for the > > beginner. > > > > And just like tbdiff, we now also accept the `--no-patches` option > > (which is actually equivalent to the diff option `-s`). > > A quick nit - would `--no-patch` (singular form) option name be more > aligned with diff `-s` option it resembles? The reason I used `--no-patches` is that tbdiff called it that way. But you're right, the functionality is already available via -s, and we *do* make this a distinct thing from tbdiff. So I'll simply drop support for --no-patches. Ciao, Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Junio, On Sun, 6 May 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > On Sat, 5 May 2018, Jeff King wrote: > > > >> On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote: > >> > >> > This builtin does not do a whole lot so far, apart from showing a usage > >> > that is oddly similar to that of `git tbdiff`. And for a good reason: > >> > the next commits will turn `branch-diff` into a full-blown replacement > >> > for `tbdiff`. > >> > >> One minor point about the name: will it become annoying as a tab > >> completion conflict with git-branch? > > If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) > but I think the 't' in there stands for "topic", not "Thomas's". > > How about "git topic-diff"? Or `git topic-branch-diff`? But then, we do not really use the term `topic branch` a lot in Git, *and* the operation in question is not really about showing differences between topic branches, but between revisions of topic branches. So far, the solution I like best is to use `git branch --diff <...>`, which also neatly side-steps the problem of cluttering the top-level command list (because tab completion). Ciao, Dscho
Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
Hi Junio, On Sun, 6 May 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Johannes Schindelin (17): > > Add a function to solve least-cost assignment problems > > Add a new builtin: branch-diff > > Perhaps retitling these to > > hungarian: a function to solve least-cost assignment problems > branch-diff: a new builtin to compare iterations of a topic > > may serve as good precedents to changes other people may later make > to these files. Especially the second one is already consistent > with the several changes that are listed below ;-) I like it! They are retitled locally, in preparation for whenever I send out the next iteration. Ciao, Dscho
Re: [GSoC] A blog about 'git stash' project
Hi Sebi, On Friday 04 May 2018 03:18 AM, Paul-Sebastian Ungureanu wrote: > Hello everybody, > > The community bonding period started. It is well known that for a > greater rate of success, it is recommended to send weekly reports > regarding project status. But, what would be a good form for such a > report? I, for one, think that starting a blog is one of the best > options because all the content will be stored in one place. Without > further ado, I would like you to present my blog [1]. > > Any feedback is greatly appreciated! Thank you! > The blog looks pretty well written. I also read your proposal. It also seems to be pretty much well written. I like the way you explain things. Particularly, you seem to be explaining the problem and the way you're about to approach it well. The plan seems pretty good. I just thought of suggesting one thing which might possibly be redundant. I think you're aware of the fact that the Git project has Travis-CI builds enabled[1] which you could take advantage of to ensure your changes pass in various text environments. If you're interested in testing your changes (which I suspect you are), you might also be interested in 'git-test'[2], a tool built by Michael Haggerty. Unlike the Travis-CI tests which test only the tip of the change, 'git-test' would help you ensure that every single commit you make doesn't break the test suite (which is both a nice thing and is expected here). Sorry for the off-topic info about the tests in this mail :-) Hope you're able to achieve your goal as planned and have a great time during this summer of code! References: [1]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L70 [2]: https://github.com/mhagger/git-test Regards, Kaartic QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! signature.asc Description: OpenPGP digital signature
[PATCH] refs: handle null-oid for pseudorefs
According to the documentation on `git update-ref`, it is possible to
"specify 40 '0' or an empty string as to make sure that the
ref you are creating does not exist." But in the code for pseudorefs, we
do not implement this. If we fail to read the old ref, we immediately
die. A failure to read would actually be a good thing if we have been
given the null-oid.
With the null-oid, allow -- and even require -- the ref-reading to fail.
This implements the "make sure that the ref ... does not exist" part of
the documentation.
Since we have a `strbuf err` for collecting errors, let's use it and
signal an error to the caller instead of dying hard.
Reported-by: Rafael Ascensão
Helped-by: Rafael Ascensão
Signed-off-by: Martin Ågren
---
(David's twopensource-address bounced, so I'm trying instead the one he
most recently posted from.)
t/t1400-update-ref.sh | 7 +++
refs.c| 19 +++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 664a3a4e4e..bd41f86f22 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -457,6 +457,13 @@ test_expect_success 'git cat-file blob master@{2005-05-26
23:42}:F (expect OTHER
test OTHER = $(git cat-file blob "master@{2005-05-26 23:42}:F")
'
+test_expect_success 'create pseudoref with old oid null, but do not overwrite'
'
+ git update-ref PSEUDOREF $A $Z &&
+ test_when_finished "git update-ref -d PSEUDOREF" &&
+ test $A = $(cat .git/PSEUDOREF) &&
+ test_must_fail git update-ref PSEUDOREF $A $Z
+'
+
a=refs/heads/a
b=refs/heads/b
c=refs/heads/c
diff --git a/refs.c b/refs.c
index 8b7a77fe5e..3669190499 100644
--- a/refs.c
+++ b/refs.c
@@ -666,10 +666,21 @@ static int write_pseudoref(const char *pseudoref, const
struct object_id *oid,
if (old_oid) {
struct object_id actual_old_oid;
- if (read_ref(pseudoref, &actual_old_oid))
- die("could not read ref '%s'", pseudoref);
- if (oidcmp(&actual_old_oid, old_oid)) {
- strbuf_addf(err, "unexpected sha1 when writing '%s'",
pseudoref);
+ if (read_ref(pseudoref, &actual_old_oid)) {
+ if (!is_null_oid(old_oid)) {
+ strbuf_addf(err, "could not read ref '%s'",
+ pseudoref);
+ rollback_lock_file(&lock);
+ goto done;
+ }
+ } else if (is_null_oid(old_oid)) {
+ strbuf_addf(err, "ref '%s' already exists",
+ pseudoref);
+ rollback_lock_file(&lock);
+ goto done;
+ } else if (oidcmp(&actual_old_oid, old_oid)) {
+ strbuf_addf(err, "unexpected sha1 when writing '%s'",
+ pseudoref);
rollback_lock_file(&lock);
goto done;
}
--
2.17.0.411.g9fd64c8e46
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Dscho, On 06/05/2018 14:10, Johannes Schindelin wrote: > > > > > > This builtin does not do a whole lot so far, apart from showing a > > > > > usage that is oddly similar to that of `git tbdiff`. And for a > > > > > good reason: the next commits will turn `branch-diff` into a > > > > > full-blown replacement for `tbdiff`. > > > > > > > > One minor point about the name: will it become annoying as a tab > > > > completion conflict with git-branch? > > > > > > I did mention this in the commit message of 18/18: > > > > > > Without this patch, we would only complete the `branch-diff` part but > > > not the options and other arguments. > > > > > > This of itself may already be slightly disruptive for well-trained > > > fingers that assume that `git braorimas` would expand > > > to > > > `git branch origin/master`, as we now no longer automatically append a > > > space after completing `git branch`: this is now ambiguous. > > > > > > > It feels really petty complaining about the name, but I just want > > > > to raise the point, since it will never be easier to change than > > > > right now. > > > > > > I do hear you. Especially since I hate `git cherry` every single > > > time that I try to tab-complete `git cherry-pick`. > > > > > > > (And no, I don't really have another name in mind; I'm just > > > > wondering if "subset" names like this might be a mild annoyance in > > > > the long run). > > > > > > They totally are, and if you can come up with a better name, I am > > > really interested in changing it before this hits `next`, even. > > > > I gave this just a quick glance so might be I`m missing something > > obvious or otherwise well-known here, bur why not `diff-branch` instead? > > I think that is just turning the problem from `branch` to `diff`. > > Of course, we have precedent with diff-index and diff-files. Except that > they don't auto-complete (because they are low-level commands) and I > *would* like the subcommand discussed in this here patch series to > auto-complete. Yeah, I did suspect it might be something like this (those other ones not auto-completing, where we do want it here), thanks for elaborating. > I think Todd's idea to shift it from a full-blown builtin to a cmdmode > of `branch` makes tons of sense. I don`t know, I still find it a bit strange that in order to "diff something", you go to "something" and tell it to "diff itself" - not because it`s a weird concept (OOP, anyone? :]), but because we already have "diff" command that can accept different things, thus just teaching it to accept additional "something" (branch, in this case), seems more natural (to me) - "branch diff" being just another "diff" mode of operation. What about that side thought you left out from my original message, making it `git diff --branch` instead? But if "branch diff" is considered to be too special-cased mode of "diff" so that supporting it from `diff` itself would make it feel awkward in both usage and maintenance (in terms of many other regular `diff` specific options being unsupported), I guess I would understand having it outside `diff` altogether (and implemented as proposed `git branch --diff`, or something)... for the time being, at least :) Regards, Buga
[PATCH 0/5] getting rid of most "static struct lock_file"s
This series addresses two classes of "static struct lock_file", removing the staticness: Those locks that already live inside a function, and those that can simply be moved into the function they are used from. The first three patches are some cleanups I noticed along the way, where we first take a lock using LOCK_DIE_ON_ERROR, then check whether we got it. After this series, we have a small number of "static struct lock_file" left, namely those locks that are used from within more than one function. Thus, after this series, when one stumbles upon a static lock_file, it should be a clear warning that the lock is being used by more than one function. Martin Martin Ågren (5): t/helper/test-write-cache: clean up lock-handling refs.c: do not die if locking fails in `write_pseudoref()` refs.c: drop dead code checking lock status in `delete_pseudoref()` lock_file: make function-local locks non-static lock_file: move static locks into functions t/helper/test-scrap-cache-tree.c | 4 ++-- t/helper/test-write-cache.c | 14 +- apply.c | 2 +- builtin/add.c| 3 +-- builtin/describe.c | 2 +- builtin/difftool.c | 2 +- builtin/gc.c | 2 +- builtin/merge.c | 4 ++-- builtin/mv.c | 2 +- builtin/read-tree.c | 3 +-- builtin/receive-pack.c | 2 +- builtin/rm.c | 3 +-- bundle.c | 2 +- fast-import.c| 2 +- refs.c | 12 refs/files-backend.c | 2 +- rerere.c | 3 +-- shallow.c| 2 +- 18 files changed, 27 insertions(+), 39 deletions(-) -- 2.17.0.411.g9fd64c8e46
[PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`
If we could not take the lock, we add an error to the `strbuf err` and
return. However, this code is dead. The reason is that we take the lock
using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
error-handling to actually kick in.
We could instead just drop the dead code and die here. But everything is
prepared for gently propagating the error, so let's do that instead.
There is similar dead code in `delete_pseudoref()`, but let's save that
for the next patch.
While at it, make the lock non-static.
Signed-off-by: Martin Ågren
---
refs.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 8b7a77fe5e..8c50b8b139 100644
--- a/refs.c
+++ b/refs.c
@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const
struct object_id *oid,
{
const char *filename;
int fd;
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const
struct object_id *oid,
strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
filename = git_path("%s", pseudoref);
- fd = hold_lock_file_for_update_timeout(&lock, filename,
- LOCK_DIE_ON_ERROR,
+ fd = hold_lock_file_for_update_timeout(&lock, filename, 0,
get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
--
2.17.0.411.g9fd64c8e46
[PATCH 4/5] lock_file: make function-local locks non-static
These `struct lock_file`s are local to their respective functions and we
can drop their staticness.
Signed-off-by: Martin Ågren
---
apply.c| 2 +-
builtin/describe.c | 2 +-
builtin/difftool.c | 2 +-
builtin/gc.c | 2 +-
builtin/merge.c| 4 ++--
builtin/receive-pack.c | 2 +-
bundle.c | 2 +-
fast-import.c | 2 +-
refs/files-backend.c | 2 +-
shallow.c | 2 +-
10 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/apply.c b/apply.c
index 7e5792c996..07b14d1127 100644
--- a/apply.c
+++ b/apply.c
@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state,
struct patch *list)
{
struct patch *patch;
struct index_state result = { NULL };
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
int res;
/* Once we start supporting the reverse patch, it may be
diff --git a/builtin/describe.c b/builtin/describe.c
index b5afc45846..8bd95cf371 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char
*prefix)
suffix = broken;
}
} else if (dirty) {
- static struct lock_file index_lock;
+ struct lock_file index_lock = LOCK_INIT;
struct rev_info revs;
struct argv_array args = ARGV_ARRAY_INIT;
int fd, result;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..162806f238 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks,
const char *prefix,
continue;
if (!indices_loaded) {
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
strbuf_reset(&buf);
strbuf_addf(&buf, "%s/wtindex", tmpdir);
if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124eaa..274660d9dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -234,7 +234,7 @@ static int need_to_gc(void)
/* return NULL on success, else hostname running the gc */
static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
{
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
char my_host[HOST_NAME_MAX + 1];
struct strbuf sb = STRBUF_INIT;
struct stat st;
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..de62b2c5c6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct
commit_list *common,
struct commit_list *remoteheads,
struct commit *head)
{
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
const char *head_arg = "HEAD";
hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct
commit_list *remoteheads)
{
struct object_id result_tree, result_commit;
struct commit_list *parents, **pptr = &parents;
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4b68a28e92..d3cf36cef5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)
static int command_singleton_iterator(void *cb_data, struct object_id *oid);
static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
{
- static struct lock_file shallow_lock;
+ struct lock_file shallow_lock = LOCK_INIT;
struct oid_array extra = OID_ARRAY_INIT;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
uint32_t mask = 1 << (cmd->index % 32);
diff --git a/bundle.c b/bundle.c
index 902c9b5448..160bbfdc64 100644
--- a/bundle.c
+++ b/bundle.c
@@ -409,7 +409,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info
*revs)
int create_bundle(struct bundle_header *header, const char *path,
int argc, const char **argv)
{
- static struct lock_file lock;
+ struct lock_file lock = LOCK_INIT;
int bundle_fd = -1;
int bundle_to_stdout;
int ref_count = 0;
diff --git a/fast-import.c b/fast-import.c
index 05d1079d23..09443c1a9d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1817,7 +1817,7 @@ static void dump_marks_helper(FILE *f,
static void dump_marks(void)
{
- static struct lock_file mark_lock;
+ struct lock_file mark_lock = LOCK_INIT;
FILE *f;
[PATCH 5/5] lock_file: move static locks into functions
Each of these `struct lock_file`s is used from within a single function.
Move them into the respective functions to make the scope clearer. While
doing so, we can drop the staticness.
After this commit, the remaing occurences of "static struct lock_file"
are locks that are used from within different functions. That is, they
need to remain static. (Short of more intrusive changes like passing
around pointers to non-static locks.)
Signed-off-by: Martin Ågren
---
t/helper/test-scrap-cache-tree.c | 4 ++--
builtin/add.c| 3 +--
builtin/mv.c | 2 +-
builtin/read-tree.c | 3 +--
builtin/rm.c | 3 +--
rerere.c | 3 +--
6 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b..393f1604ff 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -4,10 +4,10 @@
#include "tree.h"
#include "cache-tree.h"
-static struct lock_file index_lock;
-
int cmd__scrap_cache_tree(int ac, const char **av)
{
+ struct lock_file index_lock = LOCK_INIT;
+
setup_git_directory();
hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9a..8a155dd41e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
return 0;
}
-static struct lock_file lock_file;
-
static const char ignore_error[] =
N_("The following paths are ignored by one of your .gitignore files:\n");
@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+ struct lock_file lock_file = LOCK_INIT;
git_config(add_config, NULL);
diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..b4692409e3 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)
return path;
}
-static struct lock_file lock_file;
#define SUBMODULE_WITH_GITDIR ((const char *)1)
static void prepare_move_submodule(const char *src, int first,
@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+ struct lock_file lock_file = LOCK_INIT;
git_config(git_default_config, NULL);
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index bf87a2710b..ebc43eb805 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -107,8 +107,6 @@ static int git_read_tree_config(const char *var, const char
*value, void *cb)
return git_default_config(var, value, cb);
}
-static struct lock_file lock_file;
-
int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
{
int i, stage = 0;
@@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char
*unused_prefix)
struct tree_desc t[MAX_UNPACK_TREES];
struct unpack_trees_options opts;
int prefix_set = 0;
+ struct lock_file lock_file = LOCK_INIT;
const struct option read_tree_options[] = {
{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
N_("write resulting index to "),
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee81..65b448ef8e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int
index_only)
return errs;
}
-static struct lock_file lock_file;
-
static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
static int ignore_unmatch = 0;
@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {
int cmd_rm(int argc, const char **argv, const char *prefix)
{
+ struct lock_file lock_file = LOCK_INIT;
int i;
struct pathspec pathspec;
char *seen;
diff --git a/rerere.c b/rerere.c
index 18cae2d11c..e0862e2778 100644
--- a/rerere.c
+++ b/rerere.c
@@ -703,10 +703,9 @@ static int merge(const struct rerere_id *id, const char
*path)
return ret;
}
-static struct lock_file index_lock;
-
static void update_paths(struct string_list *update)
{
+ struct lock_file index_lock = LOCK_INIT;
int i;
hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
--
2.17.0.411.g9fd64c8e46
[PATCH 1/5] t/helper/test-write-cache: clean up lock-handling
Die in case writing the index fails, so that the caller can notice
(instead of, say, being impressed by how performant the writing is).
While at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we
do not need to worry about whether we succeeded. Also, we can move the
`struct lock_file` into the function and drop the staticness.
Signed-off-by: Martin Ågren
---
t/helper/test-write-cache.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c
index 017dc30380..8837717d36 100644
--- a/t/helper/test-write-cache.c
+++ b/t/helper/test-write-cache.c
@@ -2,22 +2,18 @@
#include "cache.h"
#include "lockfile.h"
-static struct lock_file index_lock;
-
int cmd__write_cache(int argc, const char **argv)
{
- int i, cnt = 1, lockfd;
+ struct lock_file index_lock = LOCK_INIT;
+ int i, cnt = 1;
if (argc == 2)
cnt = strtol(argv[1], NULL, 0);
setup_git_directory();
read_cache();
for (i = 0; i < cnt; i++) {
- lockfd = hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
- if (0 <= lockfd) {
- write_locked_index(&the_index, &index_lock,
COMMIT_LOCK);
- } else {
- rollback_lock_file(&index_lock);
- }
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
+ if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
+ die("unable to write index file");
}
return 0;
--
2.17.0.411.g9fd64c8e46
[PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`
After taking the lock we check whether we got it and die otherwise. But
since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have
died.
Unlike in the previous patch, this function is not prepared for
indicating errors via a `strbuf err`, so let's just drop the dead code.
Any improved error-handling can be added later.
While at it, make the lock non-static and reduce its scope.
Signed-off-by: Martin Ågren
---
refs.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 8c50b8b139..7abd30dfe1 100644
--- a/refs.c
+++ b/refs.c
@@ -689,20 +689,17 @@ static int write_pseudoref(const char *pseudoref, const
struct object_id *oid,
static int delete_pseudoref(const char *pseudoref, const struct object_id
*old_oid)
{
- static struct lock_file lock;
const char *filename;
filename = git_path("%s", pseudoref);
if (old_oid && !is_null_oid(old_oid)) {
- int fd;
+ struct lock_file lock = LOCK_INIT;
struct object_id actual_old_oid;
- fd = hold_lock_file_for_update_timeout(
+ hold_lock_file_for_update_timeout(
&lock, filename, LOCK_DIE_ON_ERROR,
get_files_ref_lock_timeout_ms());
- if (fd < 0)
- die_errno(_("Could not open '%s' for writing"),
filename);
if (read_ref(pseudoref, &actual_old_oid))
die("could not read ref '%s'", pseudoref);
if (oidcmp(&actual_old_oid, old_oid)) {
--
2.17.0.411.g9fd64c8e46
Re: [PATCH v2 07/18] branch-diff: indent the diffs just like tbdiff
On 4 May 2018 at 17:34, Johannes Schindelin wrote:
> @@ -353,6 +358,7 @@ static void output(struct string_list *a, struct
> string_list *b,
> int cmd_branch_diff(int argc, const char **argv, const char *prefix)
> {
> struct diff_options diffopt = { NULL };
> + struct strbuf four_spaces = STRBUF_INIT;
> double creation_weight = 0.6;
> struct option options[] = {
> OPT_SET_INT(0, "no-patches", &diffopt.output_format,
> @@ -371,6 +377,9 @@ int cmd_branch_diff(int argc, const char **argv, const
> char *prefix)
>
> diff_setup(&diffopt);
> diffopt.output_format = DIFF_FORMAT_PATCH;
> + diffopt.output_prefix = output_prefix_cb;
> + strbuf_addstr(&four_spaces, "");
> + diffopt.output_prefix_data = &four_spaces;
>
> argc = parse_options(argc, argv, NULL, options,
> builtin_branch_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
You end up leaking the buffer of `four_spaces`. Granted, that's not a
big memory leak, but still. ;-) This was the only leak that
LeakSanitizer found in v2 when running the new test-script and playing
around with this a bit. This looks really good!
Martin
Re: [PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line
On 5 May 2018 at 04:43, Taylor Blau wrote: > Take advantage of 'git-grep(1)''s new option, '--column' in order to > teach Peff's 'git-jump' script how to jump to the correct column for any > given match. > > 'git-grep(1)''s output is in the correct format for Vim's jump list, so > no additional cleanup is necessary. > diff --git a/contrib/git-jump/README b/contrib/git-jump/README > index 4484bda410..7630e16854 100644 > # use the silver searcher for git jump grep > -git config jump.grepCmd "ag --column" > +git config jump.grepCmd "ag" I think this change originates from Ævar's comment that it "also makes sense to update the example added in 007d06aa57 [...] which seems to have added jump.grepCmd as a workaround for not having this" [1]. Somehow though, this approach seems a bit backwards to me. I do believe that your series reduces the reasons for using `jump.grepCmd`, but regressing this example usage of `jump.grepCmd` seems a bit hostile. If someone wants to use `ag`, wouldn't we want to hint that they will probably want to use `--column`? Is there some other `ag --column --foo` that we can give, where `--foo` is not yet in `git grep`? ;-) Martin [1] https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com/
Re: [PATCH] refs: handle null-oid for pseudorefs
LGTM.
(This is the current best address to reach me, but do not expect fast
responses over the next few days as I'm out of town)
On Sun, 2018-05-06 at 15:35 +0200, Martin Ågren wrote:
> According to the documentation on `git update-ref`, it is possible to
> "specify 40 '0' or an empty string as to make sure that
> the
> ref you are creating does not exist." But in the code for pseudorefs,
> we
> do not implement this. If we fail to read the old ref, we immediately
> die. A failure to read would actually be a good thing if we have been
> given the null-oid.
>
> With the null-oid, allow -- and even require -- the ref-reading to
> fail.
> This implements the "make sure that the ref ... does not exist" part
> of
> the documentation.
>
> Since we have a `strbuf err` for collecting errors, let's use it and
> signal an error to the caller instead of dying hard.
>
> Reported-by: Rafael Ascensão
> Helped-by: Rafael Ascensão
> Signed-off-by: Martin Ågren
> ---
> (David's twopensource-address bounced, so I'm trying instead the one
> he
> most recently posted from.)
>
> t/t1400-update-ref.sh | 7 +++
> refs.c| 19 +++
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 664a3a4e4e..bd41f86f22 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -457,6 +457,13 @@ test_expect_success 'git cat-file blob
> master@{2005-05-26 23:42}:F (expect OTHER
> test OTHER = $(git cat-file blob "master@{2005-05-26
> 23:42}:F")
> '
>
> +test_expect_success 'create pseudoref with old oid null, but do not
> overwrite' '
> + git update-ref PSEUDOREF $A $Z &&
> + test_when_finished "git update-ref -d PSEUDOREF" &&
> + test $A = $(cat .git/PSEUDOREF) &&
> + test_must_fail git update-ref PSEUDOREF $A $Z
> +'
> +
> a=refs/heads/a
> b=refs/heads/b
> c=refs/heads/c
> diff --git a/refs.c b/refs.c
> index 8b7a77fe5e..3669190499 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -666,10 +666,21 @@ static int write_pseudoref(const char
> *pseudoref, const struct object_id *oid,
> if (old_oid) {
> struct object_id actual_old_oid;
>
> - if (read_ref(pseudoref, &actual_old_oid))
> - die("could not read ref '%s'", pseudoref);
> - if (oidcmp(&actual_old_oid, old_oid)) {
> - strbuf_addf(err, "unexpected sha1 when
> writing '%s'", pseudoref);
> + if (read_ref(pseudoref, &actual_old_oid)) {
> + if (!is_null_oid(old_oid)) {
> + strbuf_addf(err, "could not read ref
> '%s'",
> + pseudoref);
> + rollback_lock_file(&lock);
> + goto done;
> + }
> + } else if (is_null_oid(old_oid)) {
> + strbuf_addf(err, "ref '%s' already exists",
> + pseudoref);
> + rollback_lock_file(&lock);
> + goto done;
> + } else if (oidcmp(&actual_old_oid, old_oid)) {
> + strbuf_addf(err, "unexpected sha1 when
> writing '%s'",
> + pseudoref);
> rollback_lock_file(&lock);
> goto done;
> }
Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`
Re making the lock static, I wonder about the following case:
if (read_ref(pseudoref, &actual_old_oid))
die("could not read ref '%s'", pseudoref);
I think this calls exit(), and then atexit tries to clean up the lock
files. But since lock is no longer static, the stack may have been
destroyed (I don't actually know whether this is true, so maybe someone
else does).
On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:
> If we could not take the lock, we add an error to the `strbuf err`
> and
> return. However, this code is dead. The reason is that we take the
> lock
> using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
> error-handling to actually kick in.
>
> We could instead just drop the dead code and die here. But everything
> is
> prepared for gently propagating the error, so let's do that instead.
>
> There is similar dead code in `delete_pseudoref()`, but let's save
> that
> for the next patch.
>
> While at it, make the lock non-static.
>
> Signed-off-by: Martin Ågren
> ---
> refs.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8b7a77fe5e..8c50b8b139 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref,
> const struct object_id *oid,
> {
> const char *filename;
> int fd;
> - static struct lock_file lock;
> + struct lock_file lock = LOCK_INIT;
> struct strbuf buf = STRBUF_INIT;
> int ret = -1;
>
> @@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref,
> const struct object_id *oid,
> strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
>
> filename = git_path("%s", pseudoref);
> - fd = hold_lock_file_for_update_timeout(&lock, filename,
> -LOCK_DIE_ON_ERROR,
> + fd = hold_lock_file_for_update_timeout(&lock, filename, 0,
> get_files_ref_lock_ti
> meout_ms());
> if (fd < 0) {
> strbuf_addf(err, "could not open '%s' for writing:
> %s",
Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`
Same concern here about staticness.
On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:
> After taking the lock we check whether we got it and die otherwise.
> But
> since we take the lock using `LOCK_DIE_ON_ERROR`, we would already
> have
> died.
>
> Unlike in the previous patch, this function is not prepared for
> indicating errors via a `strbuf err`, so let's just drop the dead
> code.
> Any improved error-handling can be added later.
>
> While at it, make the lock non-static and reduce its scope.
>
> Signed-off-by: Martin Ågren
> ---
> refs.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8c50b8b139..7abd30dfe1 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -689,20 +689,17 @@ static int write_pseudoref(const char
> *pseudoref, const struct object_id *oid,
>
> static int delete_pseudoref(const char *pseudoref, const struct
> object_id *old_oid)
> {
> - static struct lock_file lock;
> const char *filename;
>
> filename = git_path("%s", pseudoref);
>
> if (old_oid && !is_null_oid(old_oid)) {
> - int fd;
> + struct lock_file lock = LOCK_INIT;
> struct object_id actual_old_oid;
>
> - fd = hold_lock_file_for_update_timeout(
> + hold_lock_file_for_update_timeout(
> &lock, filename, LOCK_DIE_ON_ERROR,
> get_files_ref_lock_timeout_ms());
> - if (fd < 0)
> - die_errno(_("Could not open '%s' for
> writing"), filename);
> if (read_ref(pseudoref, &actual_old_oid))
> die("could not read ref '%s'", pseudoref);
> if (oidcmp(&actual_old_oid, old_oid)) {
Re: [PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`
On 6 May 2018 at 17:48, David Turner wrote:
> On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:
>> While at it, make the lock non-static.
> Re making the lock static, I wonder about the following case:
>
> if (read_ref(pseudoref, &actual_old_oid))
>
> die("could not read ref '%s'", pseudoref);
>
> I think this calls exit(), and then atexit tries to clean up the lock
> files. But since lock is no longer static, the stack may have been
> destroyed (I don't actually know whether this is true, so maybe someone
> else does).
Right. After commit 076aa2cbda (tempfile: auto-allocate tempfiles on
heap, 2017-09-05) this is safe though. Quite a few locks have already
been moved to the stack, e.g., in 14bca6c63c (sequencer: make lockfiles
non-static, 2018-02-27) and 02ae242fdd (checkout-index: simplify locking
logic, 2017-10-05). I could add a note to the commit message to make
this clear, like "After 076aa2cbda, locks no longer need to be static."
Martin
Re: [PATCH 4/5] lock_file: make function-local locks non-static
On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote:
> These `struct lock_file`s are local to their respective functions and we
> can drop their staticness.
>
> Signed-off-by: Martin Ågren
> ---
> apply.c| 2 +-
> builtin/describe.c | 2 +-
> builtin/difftool.c | 2 +-
> builtin/gc.c | 2 +-
> builtin/merge.c| 4 ++--
> builtin/receive-pack.c | 2 +-
> bundle.c | 2 +-
> fast-import.c | 2 +-
> refs/files-backend.c | 2 +-
> shallow.c | 2 +-
> 10 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 7e5792c996..07b14d1127 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state
> *state, struct patch *list)
> {
> struct patch *patch;
> struct index_state result = { NULL };
> - static struct lock_file lock;
> + struct lock_file lock = LOCK_INIT;
Is it really safe to do this? I vaguely remember something about
(global) linked list and signal handling which could trigger any time
and probably at atexit() time too (i.e. die()). You don't want to
depend on stack-based variables in that case.
--
Duy
Re: [PATCH 4/5] lock_file: make function-local locks non-static
On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen wrote:
> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote:
>> These `struct lock_file`s are local to their respective functions and we
>> can drop their staticness.
>>
>> Signed-off-by: Martin Ågren
>> ---
>> apply.c| 2 +-
>> builtin/describe.c | 2 +-
>> builtin/difftool.c | 2 +-
>> builtin/gc.c | 2 +-
>> builtin/merge.c| 4 ++--
>> builtin/receive-pack.c | 2 +-
>> bundle.c | 2 +-
>> fast-import.c | 2 +-
>> refs/files-backend.c | 2 +-
>> shallow.c | 2 +-
>> 10 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/apply.c b/apply.c
>> index 7e5792c996..07b14d1127 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state
>> *state, struct patch *list)
>> {
>> struct patch *patch;
>> struct index_state result = { NULL };
>> - static struct lock_file lock;
>> + struct lock_file lock = LOCK_INIT;
>
> Is it really safe to do this? I vaguely remember something about
> (global) linked list and signal handling which could trigger any time
> and probably at atexit() time too (i.e. die()). You don't want to
> depend on stack-based variables in that case.
So I dug in a bit more about this. The original implementation does
not allow stack-based lock files at all in 415e96c8b7 ([PATCH]
Implement git-checkout-cache -u to update stat information in the
cache. - 2005-05-15). The situation has changed since 422a21c6a0
(tempfile: remove deactivated list entries - 2017-09-05). At the end
of that second commit, Jeff mentioned "We can clean them up
individually" which I guess is what these patches do. Though I do not
know if we need to make sure to call "release" function or something/
Either way you need more explanation and assurance than just "we can
drop their staticness" in the commit mesage.
--
Duy
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
Hi Taylor
On 05/05/18 03:43, Taylor Blau wrote:
>
> Teach 'git-grep(1)' a new option, '--column', to show the column
> number of the first match on a non-context line.
>
> For example:
>
> $ git grep -n --column foo | head -n3
> .clang-format:51:14:# myFunction(foo, bar, baz);
> .clang-format:64:7:# int foo();
> .clang-format:75:8:# void foo()
>
> Signed-off-by: Taylor Blau
> ---
> Documentation/git-grep.txt | 5 -
> builtin/grep.c | 1 +
> t/t7810-grep.sh| 22 ++
> 3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 18b494731f..5409a24399 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
> [-v | --invert-match] [-h|-H] [--full-name]
> [-E | --extended-regexp] [-G | --basic-regexp]
> [-P | --perl-regexp]
> -[-F | --fixed-strings] [-n | --line-number]
> +[-F | --fixed-strings] [-n | --line-number] [--column]
> [-l | --files-with-matches] [-L | --files-without-match]
> [(-O | --open-files-in-pager) []]
> [-z | --null]
> @@ -169,6 +169,9 @@ providing this option will cause it to die.
> --line-number::
> Prefix the line number to matching lines.
>
> +--column::
> + Prefix the 1-indexed column number of the first match on non-context
> lines.
> +
I think it would be useful to explain what the column number actually is
so that users know how to consume it. Is it a count of bytes, multi-byte
characters or graphemes? It would probably be worth testing with a file
that contains multi-byte characters to check for future regressions.
Best Wishes
Phillip
> -l::
> --files-with-matches::
> --name-only::
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 5f32d2ce84..5c83f17759 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char
> *prefix)
> GREP_PATTERN_TYPE_PCRE),
> OPT_GROUP(""),
> OPT_BOOL('n', "line-number", &opt.linenum, N_("show line
> numbers")),
> + OPT_BOOL(0, "column", &opt.columnnum, N_("show column number of
> first match")),
> OPT_NEGBIT('h', NULL, &opt.pathname, N_("don't show
> filenames"), 1),
> OPT_BIT('H', NULL, &opt.pathname, N_("show filenames"), 1),
> OPT_NEGBIT(0, "full-name", &opt.relative,
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 1797f632a3..a03c3416e7 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -99,6 +99,28 @@ do
> test_cmp expected actual
> '
>
> + test_expect_success "grep -w $L (with --column)" '
> + {
> + echo ${HC}file:5:foo mmap bar
> + echo ${HC}file:14:foo_mmap bar mmap
> + echo ${HC}file:5:foo mmap bar_mmap
> + echo ${HC}file:14:foo_mmap bar mmap baz
> + } >expected &&
> + git grep --column -w -e mmap $H >actual &&
> + test_cmp expected actual
> + '
> +
> + test_expect_success "grep -w $L (with --{line,column}-number)" '
> + {
> + echo ${HC}file:1:5:foo mmap bar
> + echo ${HC}file:3:14:foo_mmap bar mmap
> + echo ${HC}file:4:5:foo mmap bar_mmap
> + echo ${HC}file:5:14:foo_mmap bar mmap baz
> + } >expected &&
> + git grep -n --column -w -e mmap $H >actual &&
> + test_cmp expected actual
> + '
> +
> test_expect_success "grep -w $L" '
> {
> echo ${HC}file:1:foo mmap bar
>
Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash
Hi Johannes, sorry it's taken me a while to look at this. I think it
mostly makes sense to me, the code is well documented. I've got one
comment below
On 27/04/18 21:48, Johannes Schindelin wrote:
>
> During a series of fixup/squash commands, the interactive rebase builds
> up a commit message with comments. This will be presented to the user in
> the editor if at least one of those commands was a `squash`.
>
> In any case, the commit message will be cleaned up eventually, removing
> all those intermediate comments, in the final step of such a
> fixup/squash chain.
>
> However, if the last fixup/squash command in such a chain fails with
> merge conflicts, and if the user then decides to skip it (or resolve it
> to a clean worktree and then continue the rebase), the current code
> fails to clean up the commit message.
>
> This commit fixes that behavior.
>
> The fix is quite a bit more involved than meets the eye because it is
> not only about the question whether we are `git rebase --skip`ing a
> fixup or squash. It is also about removing the skipped fixup/squash's
> commit message from the accumulated commit message. And it is also about
> the question whether we should let the user edit the final commit
> message or not ("Was there a squash in the chain *that was not
> skipped*?").
>
> For example, in this case we will want to fix the commit message, but
> not open it in an editor:
>
> pick<- succeeds
> fixup <- succeeds
> squash <- fails, will be skipped
>
> This is where the newly-introduced `current-fixups` file comes in real
> handy. A quick look and we can determine whether there was a non-skipped
> squash. We only need to make sure to keep it up to date with respect to
> skipped fixup/squash commands. As a bonus, we can even avoid committing
> unnecessarily, e.g. when there was only one fixup, and it failed, and
> was skipped.
>
> To fix only the bug where the final commit message was not cleaned up
> properly, but without fixing the rest, would have been more complicated
> than fixing it all in one go, hence this commit lumps together more than
> a single concern.
>
> For the same reason, this commit also adds a bit more to the existing
> test case for the regression we just fixed.
>
> The diff is best viewed with --color-moved.
>
> Signed-off-by: Johannes Schindelin
> ---
> sequencer.c| 113 -
> t/t3418-rebase-continue.sh | 35 ++--
> 2 files changed, 131 insertions(+), 17 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 56166b0d6c7..cec180714ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2779,19 +2779,16 @@ static int continue_single_pick(void)
> return run_command_v_opt(argv, RUN_GIT_CMD);
> }
>
> -static int commit_staged_changes(struct replay_opts *opts)
> +static int commit_staged_changes(struct replay_opts *opts,
> + struct todo_list *todo_list)
> {
> unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
> + unsigned int final_fixup = 0, is_clean;
>
> if (has_unstaged_changes(1))
> return error(_("cannot rebase: You have unstaged changes."));
> - if (!has_uncommitted_changes(0)) {
> - const char *cherry_pick_head = git_path_cherry_pick_head();
>
> - if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> - return error(_("could not remove CHERRY_PICK_HEAD"));
> - return 0;
> - }
> + is_clean = !has_uncommitted_changes(0);
>
> if (file_exists(rebase_path_amend())) {
> struct strbuf rev = STRBUF_INIT;
> @@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts
> *opts)
> if (get_oid_hex(rev.buf, &to_amend))
> return error(_("invalid contents: '%s'"),
> rebase_path_amend());
> - if (oidcmp(&head, &to_amend))
> + if (!is_clean && oidcmp(&head, &to_amend))
> return error(_("\nYou have uncommitted changes in your "
> "working tree. Please, commit them\n"
> "first and then run 'git rebase "
> "--continue' again."));
> + /*
> + * When skipping a failed fixup/squash, we need to edit the
> + * commit message, the current fixup list and count, and if it
> + * was the last fixup/squash in the chain, we need to clean up
> + * the commit message and if there was a squash, let the user
> + * edit it.
> + */
> + if (is_clean && !oidcmp(&head, &to_amend) &&
> + opts->current_fixup_count > 0 &&
> + file_exists(rebase_path_stopped_sha())) {
> + const char *p = opts->current_fixups.buf;
> + int len = opts->current_fix
Re: [PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'
On Sat, May 05 2018, Taylor Blau wrote:
> + test_expect_success "grep -w $L (with --{line,column}-number)" '
It's now --column in v4 but this still refers to v3 --column-number.
Re: [PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line
On Sun, May 06 2018, Martin Ågren wrote:
> On 5 May 2018 at 04:43, Taylor Blau wrote:
>> Take advantage of 'git-grep(1)''s new option, '--column' in order to
>> teach Peff's 'git-jump' script how to jump to the correct column for any
>> given match.
>>
>> 'git-grep(1)''s output is in the correct format for Vim's jump list, so
>> no additional cleanup is necessary.
>
>> diff --git a/contrib/git-jump/README b/contrib/git-jump/README
>> index 4484bda410..7630e16854 100644
>
>> # use the silver searcher for git jump grep
>> -git config jump.grepCmd "ag --column"
>> +git config jump.grepCmd "ag"
>
> I think this change originates from Ævar's comment that it "also makes
> sense to update the example added in 007d06aa57 [...] which seems to
> have added jump.grepCmd as a workaround for not having this" [1].
>
> Somehow though, this approach seems a bit backwards to me. I do believe
> that your series reduces the reasons for using `jump.grepCmd`, but
> regressing this example usage of `jump.grepCmd` seems a bit hostile. If
> someone wants to use `ag`, wouldn't we want to hint that they will
> probably want to use `--column`?
>
> Is there some other `ag --column --foo` that we can give, where `--foo`
> is not yet in `git grep`? ;-)
>
> Martin
>
> [1] https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com/
Yeah it doesn't make sense to drop --column here, FWIW what I had in
mind was something closer to:
diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..357f79371a 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -25,6 +25,13 @@ git-jump will feed this to the editor:
foo.c:2: printf("hello word!\n");
---
+Or, when running 'git jump grep' column numbers will also be emitted,
+e.g. `git jump grep "hello"' would return:
+
+---
+foo.c:2:10: printf("hello word!\n");
+---
+
Obviously this trivial case isn't that interesting; you could just open
`foo.c` yourself. But when you have many changes scattered across a
project, you can use the editor's support to "jump" from point to point.
I.e. let's note what the output format is now like for 'grep', and no
need to change the jump.grepCmd.
The above patch may be incorrect when it comes to the line numbe /
column number / format, I just wrote that by hand.
Weak option parsing in `git submodule`
I recently faced the consequence of the weak option parsing done by in `git submodule`. Initially tried to add a new submodule using the `git submodule add` sub-command in the following way, $ git submodule add ./foo/bar This cloned the submodule into a new directory named 'bar' in the present directory. This was initially confusing as I expected `git submodule` to use the actual directory itself as it resides inside a sub-directory the main project and thus avoiding the creation of a new clone. Then I came to know that `git submodule` wasn't smart enough yet to identify this, by reading the documentation. Then, after realizing that I would have to specify the path in the command to avoid the redundant clone, I corrected the command without consulting the documentation (thoroughly) to, $ git submodule add ./foo/bar --path ./foo/bar expecting that the path needs to be specified after an argument. This is what triggered the weird consequence of weak option parsing. The output I got for the above command was: The following path is ignored by one of your .gitignore files: --path Use -f if you really want to add it. That really confused me pretty much (being a person who doesn't know much about how `git submodule` works). Instead of complaining about an inexistent option '--path', it considers '--path' to be the argument which seems to be bad. It doesn't even complain about the extra argument. I also dug to find the reason behind this totally weird message. It seems to be a consequence of the following lousy check ($sm_path is the normalized argument): if test -z "$force" && ! git add --dry-run --ignore-missing --no-warn-embedded-repo "$sm_path" > /dev/null 2>&1 then eval_gettextln "The following path is ignored by one of your .gitignore files: \$sm_path Use -f if you really want to add it." >&2 exit 1 fi The lack of checking for the reason behind why `git add` fails seems to be the reason behind that weird message. Ways to fix this: 1. Fix this particular issue by adding a '--' after the '--no-warn- embedded-repo' option in the above check. But that would also require that we allow other parts of the script to accept weird paths such as '--path'. Not so useful/helpful. 2. Check for the actual return value of `git add` in the check and act accordingly. Also, check if there are unnecessary arguments for `submodule add`. 3. Tighten option parsing in the `git-submodule` script to ensure this doesn't happen in the long term and helps users to get more relevant error messages. I find 3 to be a long term solution but not sure if it's worth trying as there are efforts towards porting `git submodule` to C. So, I guess we should at least do 2 as a short-term solution. Thoughts?? -- Sivaraam QUOTE: “The most valuable person on any team is the person who makes everyone else on the team more valuable, not the person who knows the most.” - Joel Spolsky Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance!
[bug] Multiline value should error if the next line is section
## Environment OS: Arch Linux Git version: git@next d54016d9e ## Reproduction Steps (1) Create the following `git.config`, ``` [alias] tree = --no-pager log --graph \ --date=format:'%Y-%m-%d' \ --pretty=format:'%C(auto,dim)%ad %<(7,trunc) %an %Creset%m %h %s %Cgreen%d%Creset' \ --exclude="*/production" \ --exclude="*/dev-*" \ -n 20 \ [user] name = Shulhan email = m...@kilabit.info ``` (2) Run `git config -f git.config -l` ## Expected Result Error message, fatal: bad config line 9 at git.config ## Actual Result The command print the following output, ``` alias.tree=--no-pager log --graph --date=format:'%Y-%m-%d' --pretty=format:'%C(auto,dim)%ad %<(7,trunc) %an %Creset%m %h %s %Cgreen%d%Creset' --exclude=*/production --exclude=*/dev-* -n 20 [user] alias.name=Shulhan alias.email=m...@kilabit.info ```
Re: [PATCH 4/5] lock_file: make function-local locks non-static
On 6 May 2018 at 19:42, Duy Nguyen wrote: > On Sun, May 6, 2018 at 7:26 PM, Duy Nguyen wrote: >> On Sun, May 6, 2018 at 4:10 PM, Martin Ågren wrote: >>> These `struct lock_file`s are local to their respective functions and we >>> can drop their staticness. >>> - static struct lock_file lock; >>> + struct lock_file lock = LOCK_INIT; >> >> Is it really safe to do this? I vaguely remember something about >> (global) linked list and signal handling which could trigger any time >> and probably at atexit() time too (i.e. die()). You don't want to >> depend on stack-based variables in that case. > > So I dug in a bit more about this. The original implementation does > not allow stack-based lock files at all in 415e96c8b7 ([PATCH] > Implement git-checkout-cache -u to update stat information in the > cache. - 2005-05-15). The situation has changed since 422a21c6a0 > (tempfile: remove deactivated list entries - 2017-09-05). At the end > of that second commit, Jeff mentioned "We can clean them up > individually" which I guess is what these patches do. Though I do not > know if we need to make sure to call "release" function or something/ > Either way you need more explanation and assurance than just "we can > drop their staticness" in the commit mesage. Thank you Duy for your comments. How about I write the commit message like so: After 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we can have lockfiles on the stack. These `struct lock_file`s are local to their respective functions and we can drop their staticness. Each of these users either commits or rolls back the lock in every codepath, with these possible exceptions: * We bail using a call to `die()` or `exit()`. The lock will be cleaned up automatically. * We return early from a function `cmd_foo()` in builtin/, i.e., we are just about to exit. The lock will be cleaned up automatically. If I have missed some codepath where we do not exit, yet leave a locked lock around, that was so also before this patch. If we would later re-enter the same function, then before this patch, we would be retaking a lock for the very same `struct lock_file`, which feels awkward, but to the best of my reading has well-defined behavior. Whereas after this patch, we would attempt to take the lock with a completely fresh `struct lock_file`. In both cases, the result would simply be that the lock can not be taken, which is a situation we already handle.
Re: [bug] Multiline value should error if the next line is section
Hi Shulhan Thank you for your report. I'm abbreviating a bit: On 6 May 2018 at 21:03, Shulhan wrote: > [alias] > tree = --no-pager log --graph \ > -n 20 \ > [user] > name = Shulhan > > (2) Run `git config -f git.config -l` > > The command print the following output, > > alias.tree=--no-pager log --graph -n 20 [user] > alias.name=Shulhan Small mistake, big consequences. :-) This behavior looks correct to me, though. It seems very hard to me to second-guess what the user meant. For example, what if that third line contained a "="? Like: [alias] huh = !dd \ bs=1024 ... Should Git guess that the backslash on the second line was a mistake? Or maybe not, because alias.bs = "1024 ..." would be a useless alias? I think such guessing would be theoretically possible, but especially if Git guesses wrong, that could be very frustrating to fight against. Martin
[PATCH 2/2] Documentation: render revisions correctly under Asciidoctor
When creating a literal block from an indented block without any sort of delimiters, Asciidoctor strips off all leading whitespace, resulting in a misrendered chart. Use an explicit literal block to indicate to Asciidoctor that we want to keep the leading whitespace. Signed-off-by: brian m. carlson --- A future direction we could go here would actually be to turn this into a table, which might render more acceptably in all forms. But I think this patch provides a useful improvement and we can switch to a table later if desired. Documentation/revisions.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index dfcc49c72c..8f60c9f431 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -345,6 +345,7 @@ Here are a handful of examples using the Loeliger illustration above, with each step in the notation's expansion and selection carefully spelt out: + Args Expanded argumentsSelected commits DG H D D F G H I J D F @@ -367,3 +368,4 @@ spelt out: = B ^B^1 ^B^2 ^B^3 = B ^D ^E ^F B F^! D = F ^I ^J D G H D F +
[PATCH 1/2] Documentation: use 8-space tabs with Asciidoctor
Asciidoctor expands tabs at the beginning of a line. However, it does not expand them into 8 spaces by default. Since we use 8-space tabs, tell Asciidoctor that we want 8 spaces by setting the tabsize attribute. This ensures that our ASCII art renders properly. Signed-off-by: brian m. carlson --- Documentation/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/Makefile b/Documentation/Makefile index 6232143cb9..bcd216d96c 100644 --- a/Documentation/Makefile +++ b/Documentation/Makefile @@ -184,7 +184,7 @@ ASCIIDOC = asciidoctor ASCIIDOC_CONF = ASCIIDOC_HTML = xhtml5 ASCIIDOC_DOCBOOK = docbook45 -ASCIIDOC_EXTRA += -acompat-mode +ASCIIDOC_EXTRA += -acompat-mode -atabsize=8 ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;' DBLATEX_COMMON =
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
On Sun, May 6, 2018 at 8:21 AM, Johannes Schindelin wrote: > On Sun, 6 May 2018, Junio C Hamano wrote: >> Johannes Schindelin writes: >> > On Sat, 5 May 2018, Jeff King wrote: >> >> One minor point about the name: will it become annoying as a tab >> >> completion conflict with git-branch? >> >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) >> but I think the 't' in there stands for "topic", not "Thomas's". >> How about "git topic-diff"? > > Or `git topic-branch-diff`? > > But then, we do not really use the term `topic branch` a lot in Git, *and* > the operation in question is not really about showing differences between > topic branches, but between revisions of topic branches. > > So far, the solution I like best is to use `git branch --diff <...>`, > which also neatly side-steps the problem of cluttering the top-level > command list (because tab completion). Let's, please, not fall into the trap of polluting git-branch with utterly unrelated functionality, as has happened a few times with other Git commands. Let's especially not do so merely for the sake of tab-completion. git-branch is for branch management; it's not for diff'ing. Of the suggestions thus far, Junio's git-topic-diff seems the least worse, and doesn't suffer from tab-completion problems. Building on Duy's suggestion: git-interdiff could be a superset of the current git-branch-diff: # standard interdiff git interdiff womp-v1 womp-v2 # 'tbdiff'-like output git interdiff --topic womp-v1 womp-v2 (Substitute "--topic" by any other better name.)
Re: [bug] Multiline value should error if the next line is section
On Sun, May 06, 2018 at 10:03:10PM +0200, Martin Ågren wrote: > This behavior looks correct to me, though. It seems very hard to me to > second-guess what the user meant. For example, what if that third line > contained a "="? Like: > > [alias] > huh = !dd \ > bs=1024 ... > > Should Git guess that the backslash on the second line was a mistake? > Or maybe not, because alias.bs = "1024 ..." would be a useless alias? > > I think such guessing would be theoretically possible, but especially if > Git guesses wrong, that could be very frustrating to fight against. I agree that trying to guess what the user wanted here is likely impossible. Furthermore, Git intentionally ignores unknown options. For example, I have advice and diff options set in my .gitconfig that would not be valid on the Git shipped with a base CentOS 6 (which, unfortunately, I sometimes have to use). It's very convenient for users working across a variety of systems that unknown options are simply ignored, even if that means sometimes mistakes are not caught. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
On Fri, May 04, 2018 at 05:34:27PM +0200, Johannes Schindelin wrote: > The incredibly useful `git-tbdiff` tool to compare patch series (say, to see > what changed between two iterations sent to the Git mailing list) is slightly > less useful for this developer due to the fact that it requires the > `hungarian` > and `numpy` Python packages which are for some reason really hard to build in > MSYS2. So hard that I even had to give up, because it was simply easier to > reimplement the whole shebang as a builtin command. I just want to say thanks for writing this. I use tbdiff extensively at work and having this built-in and much faster will really help. I did a once-over of v1 and I'll probably take a look at v2 or v3 (whatever's the latest) later in the week. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [bug] Multiline value should error if the next line is section
On Sun, 6 May 2018 22:03:10 +0200 Martin Ågren wrote: > Hi Shulhan > > Thank you for your report. I'm abbreviating a bit: > > On 6 May 2018 at 21:03, Shulhan wrote: > > [alias] > > tree = --no-pager log --graph \ > > -n 20 \ > > [user] > > name = Shulhan > > > > (2) Run `git config -f git.config -l` > > > > The command print the following output, > > > > alias.tree=--no-pager log --graph -n 20 [user] > > alias.name=Shulhan > > Small mistake, big consequences. :-) > > This behavior looks correct to me, though. It seems very hard to me to > second-guess what the user meant. For example, what if that third line > contained a "="? Like: > > [alias] > huh = !dd \ > bs=1024 ... > > Should Git guess that the backslash on the second line was a mistake? > Or maybe not, because alias.bs = "1024 ..." would be a useless alias? The context of multiline next value that I reported before was about section, not variable. > > I think such guessing would be theoretically possible, but especially > if Git guesses wrong, that could be very frustrating to fight against. > I'm not familiar with git config parser, obviously :), but checking the start of next multiline value that start with '[' maybe not impossible. Git should not guessed, but report error at the offending line: either user forgot to enclosed the variable with double quote or they missplace the backslash.
[PATCH 00/28] Hash-independent tests (part 2)
This is part 2 in the series to make tests hash independent. This series introduces an SHA1 prerequisite which checks if the hash in use is SHA-1, and can be used to skip the test if it is not. Additionally, because NewHash will be 256-bit, I introduced aliases for the test constants $_x40 and $_z40 which will be less confusing when the hash isn't 40 hex characters long. I opted to leave the old names in place for the moment to prevent any potential conflicts with other series and will clean up any stragglers later. Several tests are skipped because of SHA-1-specific dependencies: some of these are core tests which test basic expected hash values, some depend on colliding short names, and some depend on specially named object (the pack tests). Elsewhere, tests are modified to compute values using git hash-object and git rev-parse. There is one cleanup patch that fixes indentation so we can use an indented heredoc in the following patch. The order in this series is by patch type, then roughly by test number. All of these tests pass with an alternate 160-bit hash as well as SHA-1. An additional series cleaning up the remainder of the tests is required to make the entire testsuite pass with an alternate 160-bit hash. brian m. carlson (28): t/test-lib: add an SHA1 prerequisite t/test-lib: introduce ZERO_OID t: switch $_z40 to $ZERO_OID t/test-lib: introduce FULL_HEX t: switch $_x40 to $FULL_HEX t: annotate with SHA1 prerequisite t1007: annotate with SHA1 prerequisite t1512: skip test if not using SHA-1 t4044: skip test if not using SHA-1 t: skip pack tests if not using SHA-1 t2203: abstract away SHA-1-specific constants t3103: abstract away SHA-1-specific constants t3702: abstract away SHA-1-specific constants t3905: abstract away SHA-1-specific constants t4007: abstract away SHA-1-specific constants t4008: abstract away SHA-1-specific constants t4014: abstract away SHA-1-specific constants t4020: abstract away SHA-1-specific constants t4022: abstract away SHA-1-specific constants t4029: fix test indentation t4029: abstract away SHA-1-specific constants t4030: abstract away SHA-1-specific constants t/lib-diff-alternative: abstract away SHA-1-specific constants t4205: sort log output in a hash-independent way t4042: abstract away SHA-1-specific constants t4045: abstract away SHA-1-specific constants t4208: abstract away SHA-1-specific constants t5300: abstract away SHA-1-specific constants t/diff-lib.sh | 4 +- t/lib-diff-alternative.sh | 12 -- t/t-basic.sh| 24 ++-- t/t0090-cache-tree.sh | 2 +- t/t1000-read-tree-m-3way.sh | 2 +- t/t1001-read-tree-m-2way.sh | 2 +- t/t1002-read-tree-m-u-2way.sh | 2 +- t/t1006-cat-file.sh | 8 ++-- t/t1007-hash-object.sh | 16 t/t1012-read-tree-df.sh | 2 +- t/t1400-update-ref.sh | 2 +- t/t1407-worktree-ref-store.sh | 8 ++-- t/t1450-fsck.sh | 4 +- t/t1501-work-tree.sh| 6 +-- t/t1512-rev-parse-disambiguation.sh | 6 +++ t/t1601-index-bogus.sh | 2 +- t/t1700-split-index.sh | 2 +- t/t2011-checkout-invalid-head.sh| 2 +- t/t2025-worktree-add.sh | 8 ++-- t/t2027-worktree-list.sh| 2 +- t/t2107-update-index-basic.sh | 4 +- t/t2201-add-update-typechange.sh| 16 t/t2203-add-intent.sh | 14 +++ t/t3100-ls-tree-restrict.sh | 2 +- t/t3101-ls-tree-dirname.sh | 2 +- t/t3103-ls-tree-misc.sh | 3 +- t/t3200-branch.sh | 4 +- t/t3510-cherry-pick-sequence.sh | 8 ++-- t/t3702-add-edit.sh | 7 ++-- t/t3905-stash-include-untracked.sh | 12 +++--- t/t4002-diff-basic.sh | 2 +- t/t4006-diff-mode.sh| 2 +- t/t4007-rename-3.sh | 17 + t/t4008-diff-break-rewrite.sh | 59 - t/t4014-format-patch.sh | 11 +++--- t/t4020-diff-external.sh| 18 + t/t4022-diff-rewrite.sh | 5 ++- t/t4027-diff-submodule.sh | 6 +-- t/t4029-diff-trailing-space.sh | 38 ++- t/t4030-diff-textconv.sh| 5 ++- t/t4042-diff-textconv-caching.sh| 16 +--- t/t4044-diff-index-unique-abbrev.sh | 6 +++ t/t4045-diff-relative.sh| 6 ++- t/t4046-diff-unmerged.sh| 14 +++ t/t4054-diff-bogus-tree.sh | 12 +++--- t/t4058-diff-duplicates.sh | 12 +++--- t/t4150-am.sh | 4 +- t/t4200-rerere.sh | 2 +- t/t4201-shortlog.sh | 2 +- t/t4205-log-pretty-formats.sh | 8 ++-- t/t4208-log-magic-pathspec.sh | 3 +- t/t5150-request-pull.sh | 2 +- t/t5300-pack-object.sh
[PATCH 01/28] t/test-lib: add an SHA1 prerequisite
There are some basic tests in our codebase that test that we get fixed SHA-1 values. These are valuable because they make sure that our SHA-1 implementation is free of bugs, but obviously these tests will fail with a different hash. There are also tests which intentionally produce objects that have collisions when truncated to a certain length to test our handling of these cases. These tests, too, will fail with a different hash. Add an SHA1 prerequisite to annotate both of these types of tests and disable them when we're using a different hash. In the future, we can create versions of these tests which handle both SHA-1 and NewHash. Signed-off-by: brian m. carlson --- t/test-lib.sh | 7 +++ 1 file changed, 7 insertions(+) diff --git a/t/test-lib.sh b/t/test-lib.sh index ea2bbaaa7a..fce728d2ea 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1212,3 +1212,10 @@ test_lazy_prereq TIME_T_IS_64BIT 'test-tool date time_t-is64bit' test_lazy_prereq CURL ' curl --version ' + +# SHA1 is a test if the hash algorithm in use is SHA-1. This is both for tests +# which will not work with other hash algorithms and tests that work but don't +# test anything meaningful (e.g. special values which cause short collisions). +test_lazy_prereq SHA1 ' + test $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 +'
[PATCH 06/28] t0000: annotate with SHA1 prerequisite
Since this is a core test that tests basic functionality, annotate the assertions that have dependencies on SHA-1 with the appropriate prerequisite. Signed-off-by: brian m. carlson --- t/t-basic.sh | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/t/t-basic.sh b/t/t-basic.sh index 7fd87dd544..af61d083b4 100755 --- a/t/t-basic.sh +++ b/t/t-basic.sh @@ -839,7 +839,7 @@ test_expect_success 'writing tree out with git write-tree' ' ' # we know the shape and contents of the tree and know the object ID for it. -test_expect_success 'validate object ID of a known tree' ' +test_expect_success SHA1 'validate object ID of a known tree' ' test "$tree" = 7bb943559a305bdd6bdee2cef6e5df2413c3d30a ' @@ -882,7 +882,7 @@ test_expect_success 'showing stage with git ls-files --stage' ' git ls-files --stage >current ' -test_expect_success 'validate git ls-files output for a known tree' ' +test_expect_success SHA1 'validate git ls-files output for a known tree' ' cat >expected <<-\EOF && 100644 f87290f8eb2cbbea7857214459a0739927eab154 0 path0 12 15a98433ae33114b085f3eb3bb03b832b3180a01 0 path0sym @@ -900,7 +900,7 @@ test_expect_success 'writing tree out with git write-tree' ' tree=$(git write-tree) ' -test_expect_success 'validate object ID for a known tree' ' +test_expect_success SHA1 'validate object ID for a known tree' ' test "$tree" = 087704a96baf1c2d1c869a8b084481e121c88b5b ' @@ -908,7 +908,7 @@ test_expect_success 'showing tree with git ls-tree' ' git ls-tree $tree >current ' -test_expect_success 'git ls-tree output for a known tree' ' +test_expect_success SHA1 'git ls-tree output for a known tree' ' cat >expected <<-\EOF && 100644 blob f87290f8eb2cbbea7857214459a0739927eab154path0 12 blob 15a98433ae33114b085f3eb3bb03b832b3180a01path0sym @@ -924,7 +924,7 @@ test_expect_success 'showing tree with git ls-tree -r' ' git ls-tree -r $tree >current ' -test_expect_success 'git ls-tree -r output for a known tree' ' +test_expect_success SHA1 'git ls-tree -r output for a known tree' ' cat >expected <<-\EOF && 100644 blob f87290f8eb2cbbea7857214459a0739927eab154path0 12 blob 15a98433ae33114b085f3eb3bb03b832b3180a01path0sym @@ -943,7 +943,7 @@ test_expect_success 'showing tree with git ls-tree -r -t' ' git ls-tree -r -t $tree >current ' -test_expect_success 'git ls-tree -r output for a known tree' ' +test_expect_success SHA1 'git ls-tree -r output for a known tree' ' cat >expected <<-\EOF && 100644 blob f87290f8eb2cbbea7857214459a0739927eab154path0 12 blob 15a98433ae33114b085f3eb3bb03b832b3180a01path0sym @@ -964,7 +964,7 @@ test_expect_success 'writing partial tree out with git write-tree --prefix' ' ptree=$(git write-tree --prefix=path3) ' -test_expect_success 'validate object ID for a known tree' ' +test_expect_success SHA1 'validate object ID for a known tree' ' test "$ptree" = 21ae8269cacbe57ae09138dcc3a2887f904d02b3 ' @@ -972,7 +972,7 @@ test_expect_success 'writing partial tree out with git write-tree --prefix' ' ptree=$(git write-tree --prefix=path3/subp3) ' -test_expect_success 'validate object ID for a known tree' ' +test_expect_success SHA1 'validate object ID for a known tree' ' test "$ptree" = 3c5e5399f3a333eddecce7a9b9465b63f65f51e2 ' @@ -1006,7 +1006,7 @@ test_expect_success 'git read-tree followed by write-tree should be idempotent' test "$newtree" = "$tree" ' -test_expect_success 'validate git diff-files output for a know cache/work tree state' ' +test_expect_success SHA1 'validate git diff-files output for a know cache/work tree state' ' cat >expected <<\EOF && :100644 100644 f87290f8eb2cbbea7857214459a0739927eab154 M path0 :12 12 15a98433ae33114b085f3eb3bb03b832b3180a01 M path0sym @@ -1033,21 +1033,21 @@ test_expect_success 'no diff after checkout and git update-index --refresh' ' P=087704a96baf1c2d1c869a8b084481e121c88b5b -test_expect_success 'git commit-tree records the correct tree in a commit' ' +test_expect_success SHA1 'git commit-tree records the correct tree in a commit' ' commit0=$(echo NO | git commit-tree $P) && tree=$(git show --pretty=raw $commit0 | sed -n -e "s/^tree //p" -e "/^author /q") && test "z$tree" = "z$P" ' -test_expect_success 'git commit-tree records the correct parent in a commit' ' +test_expect_success SHA1 'git commit-tree records the correct parent in a commit' ' commit1=$(echo NO | git commit-tree $P -p $commit0) && parent=$(git show --pretty=raw $commit1 | sed -n -e "s/^parent //p
[PATCH 03/28] t: switch $_z40 to $ZERO_OID
Switch all uses of $_z40 to $ZERO_OID so that they work correctly with larger hashes. This commit was created by using the following sed command to modify all files in the t directory except t/test-lib.sh: sed -i 's/\$_z40/$ZERO_OID/g' Signed-off-by: brian m. carlson --- t/t1006-cat-file.sh| 8 ++--- t/t1400-update-ref.sh | 2 +- t/t1407-worktree-ref-store.sh | 8 ++--- t/t1450-fsck.sh| 4 +-- t/t1501-work-tree.sh | 6 ++-- t/t1601-index-bogus.sh | 2 +- t/t1700-split-index.sh | 2 +- t/t2011-checkout-invalid-head.sh | 2 +- t/t2025-worktree-add.sh| 8 ++--- t/t2027-worktree-list.sh | 2 +- t/t2107-update-index-basic.sh | 4 +-- t/t2201-add-update-typechange.sh | 16 - t/t2203-add-intent.sh | 6 ++-- t/t3200-branch.sh | 4 +-- t/t4002-diff-basic.sh | 2 +- t/t4014-format-patch.sh| 2 +- t/t4020-diff-external.sh | 10 +++--- t/t4027-diff-submodule.sh | 6 ++-- t/t4046-diff-unmerged.sh | 14 t/t4054-diff-bogus-tree.sh | 12 +++ t/t4058-diff-duplicates.sh | 12 +++ t/t4150-am.sh | 4 +-- t/t4200-rerere.sh | 2 +- t/t5516-fetch-push.sh | 22 ++-- t/t5527-fetch-odd-refs.sh | 2 +- t/t5571-pre-push-hook.sh | 8 ++--- t/t6120-describe.sh| 2 +- t/t6300-for-each-ref.sh| 2 +- t/t6301-for-each-ref-errors.sh | 2 +- t/t7009-filter-branch-null-sha1.sh | 2 +- t/t7011-skip-worktree-reading.sh | 2 +- t/t7064-wtstatus-pv2.sh| 58 +++--- 32 files changed, 119 insertions(+), 119 deletions(-) diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 2ac3b940c6..13dd510b2e 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -236,8 +236,8 @@ test_expect_success "--batch-check for an empty line" ' ' test_expect_success 'empty --batch-check notices missing object' ' - echo "$_z40 missing" >expect && - echo "$_z40" | git cat-file --batch-check="" >actual && + echo "$ZERO_OID missing" >expect && + echo "$ZERO_OID" | git cat-file --batch-check="" >actual && test_cmp expect actual ' @@ -294,8 +294,8 @@ test_expect_success 'setup blobs which are likely to delta' ' test_expect_success 'confirm that neither loose blob is a delta' ' cat >expect <<-EOF && - $_z40 - $_z40 + $ZERO_OID + $ZERO_OID EOF git cat-file --batch-check="%(deltabase)" actual && test_cmp expect actual diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 664a3a4e4e..f6dc05654e 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -6,7 +6,7 @@ test_description='Test git update-ref and basic ref logging' . ./test-lib.sh -Z=$_z40 +Z=$ZERO_OID m=refs/heads/master n_dir=refs/heads/gu diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh index 2211f9831f..4623ae15c4 100755 --- a/t/t1407-worktree-ref-store.sh +++ b/t/t1407-worktree-ref-store.sh @@ -50,13 +50,13 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' ' ' test_expect_success 'for_each_reflog()' ' - echo $_z40 > .git/logs/PSEUDO-MAIN && + echo $ZERO_OID > .git/logs/PSEUDO-MAIN && mkdir -p .git/logs/refs/bisect && - echo $_z40 > .git/logs/refs/bisect/random && + echo $ZERO_OID > .git/logs/refs/bisect/random && - echo $_z40 > .git/worktrees/wt/logs/PSEUDO-WT && + echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT && mkdir -p .git/worktrees/wt/logs/refs/bisect && - echo $_z40 > .git/worktrees/wt/logs/refs/bisect/wt-random && + echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random && $RWT for-each-reflog | cut -c 42- | sort >actual && cat >expected <<-\EOF && diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index cb4b66e29d..91fd71444d 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -713,7 +713,7 @@ test_expect_success 'fsck notices dangling objects' ' test_expect_success 'fsck $name notices bogus $name' ' test_must_fail git fsck bogus && - test_must_fail git fsck $_z40 + test_must_fail git fsck $ZERO_OID ' test_expect_success 'bogus head does not fallback to all heads' ' @@ -723,7 +723,7 @@ test_expect_success 'bogus head does not fallback to all heads' ' blob=$(git rev-parse :foo) && test_when_finished "git rm --cached foo" && remove_object $blob && - test_must_fail git fsck $_z40 >out 2>&1 && + test_must_fail git fsck $ZERO_OID >out 2>&1 && ! grep $blob out ' diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh index 9c0bc65250..afcdfafe45 100755 --- a/t/t1501-work-tree.sh +++ b/t/t1501-work-tree.sh @@ -238,10 +238,10 @@ test_expect_suc
[PATCH 05/28] t: switch $_x40 to $FULL_HEX
Switch all uses of $_x40 to $FULL_HEX so that they work correctly with
larger hashes. This commit was created by using the following sed
command to modify all files in the t directory except t/test-lib.sh:
sed -i 's/\$_x40/$FULL_HEX/g'
Signed-off-by: brian m. carlson
---
t/diff-lib.sh | 4 ++--
t/t0090-cache-tree.sh | 2 +-
t/t1000-read-tree-m-3way.sh | 2 +-
t/t1001-read-tree-m-2way.sh | 2 +-
t/t1002-read-tree-m-u-2way.sh | 2 +-
t/t1012-read-tree-df.sh | 2 +-
t/t3100-ls-tree-restrict.sh | 2 +-
t/t3101-ls-tree-dirname.sh | 2 +-
t/t3510-cherry-pick-sequence.sh | 8
t/t4002-diff-basic.sh | 2 +-
t/t4006-diff-mode.sh| 2 +-
t/t4014-format-patch.sh | 2 +-
t/t4201-shortlog.sh | 2 +-
t/t5150-request-pull.sh | 2 +-
t/t6006-rev-list-format.sh | 4 ++--
t/t6012-rev-list-simplify.sh| 2 +-
t/t6111-rev-list-treesame.sh| 2 +-
t/t7506-status-submodule.sh | 2 +-
t/t9010-svn-fe.sh | 14 +++---
t/t9300-fast-import.sh | 6 +++---
20 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/t/diff-lib.sh b/t/diff-lib.sh
index c211dc40ee..506ef4c289 100644
--- a/t/diff-lib.sh
+++ b/t/diff-lib.sh
@@ -1,6 +1,6 @@
:
-sanitize_diff_raw='/^:/s/ '"\($_x40\)"' '"\($_x40\)"' \([A-Z]\)[0-9]* / \1 \2
\3# /'
+sanitize_diff_raw='/^:/s/ '"\($FULL_HEX\)"' '"\($FULL_HEX\)"' \([A-Z]\)[0-9]*
/ \1 \2 \3# /'
compare_diff_raw () {
# When heuristics are improved, the score numbers would change.
# Ignore them while comparing.
@@ -12,7 +12,7 @@ compare_diff_raw () {
test_cmp .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
}
-sanitize_diff_raw_z='/^:/s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*$/ X X \1#/'
+sanitize_diff_raw_z='/^:/s/ '"$FULL_HEX"' '"$FULL_HEX"' \([A-Z]\)[0-9]*$/ X X
\1#/'
compare_diff_raw_z () {
# When heuristics are improved, the score numbers would change.
# Ignore them while comparing.
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 4ae0995cd9..26f12facf0 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -9,7 +9,7 @@ cache-tree extension.
cmp_cache_tree () {
test-tool dump-cache-tree | sed -e '/#(ref)/d' >actual &&
- sed "s/$_x40/SHA/" filtered &&
+ sed "s/$FULL_HEX/SHA/" filtered &&
test_cmp "$1" filtered
}
diff --git a/t/t1000-read-tree-m-3way.sh b/t/t1000-read-tree-m-3way.sh
index 3c4d2d6045..d85056238e 100755
--- a/t/t1000-read-tree-m-3way.sh
+++ b/t/t1000-read-tree-m-3way.sh
@@ -128,7 +128,7 @@ cat >expected <<\EOF
EOF
check_result () {
- git ls-files --stage | sed -e 's/ '"$_x40"' / X /' >current &&
+ git ls-files --stage | sed -e 's/ '"$FULL_HEX"' / X /' >current &&
test_cmp expected current
}
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 5ededd8e40..01378f7bcd 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -30,7 +30,7 @@ read_tree_twoway () {
compare_change () {
sed -n >current \
-e '/^--- /d; /^+++ /d; /^@@ /d;' \
- -e 's/^\([-+][0-7][0-7][0-7][0-7][0-7][0-7]\) '"$_x40"' /\1 X /p' \
+ -e 's/^\([-+][0-7][0-7][0-7][0-7][0-7][0-7]\) '"$FULL_HEX"' /\1 X
/p' \
"$1"
test_cmp expected current
}
diff --git a/t/t1002-read-tree-m-u-2way.sh b/t/t1002-read-tree-m-u-2way.sh
index 7ca2e65d10..bafbe49971 100755
--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh
@@ -16,7 +16,7 @@ compare_change () {
-e '1{/^diff --git /d;}' \
-e '2{/^index /d;}' \
-e '/^--- /d; /^+++ /d; /^@@ /d;' \
- -e 's/^\(.[0-7][0-7][0-7][0-7][0-7][0-7]\) '"$_x40"' /\1 X /' "$1"
+ -e 's/^\(.[0-7][0-7][0-7][0-7][0-7][0-7]\) '"$FULL_HEX"' /\1 X /'
"$1"
test_cmp expected current
}
diff --git a/t/t1012-read-tree-df.sh b/t/t1012-read-tree-df.sh
index a6a04b6b90..26a4089f1e 100755
--- a/t/t1012-read-tree-df.sh
+++ b/t/t1012-read-tree-df.sh
@@ -32,7 +32,7 @@ settree () {
checkindex () {
git ls-files -s |
- sed "s|^[0-7][0-7]* $_x40 \([0-3]\) |\1 |" >current &&
+ sed "s|^[0-7][0-7]* $FULL_HEX \([0-3]\) |\1 |" >current &&
cat >expect &&
test_cmp expect current
}
diff --git a/t/t3100-ls-tree-restrict.sh b/t/t3100-ls-tree-restrict.sh
index 325114f8fe..f7b0ad774e 100755
--- a/t/t3100-ls-tree-restrict.sh
+++ b/t/t3100-ls-tree-restrict.sh
@@ -32,7 +32,7 @@ test_expect_success \
echo $tree'
test_output () {
-sed -e "s/ $_x40 / X /" check
+sed -e "s/ $FULL_HEX / X /" check
test_cmp expected check
}
diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh
index 327ded4000..316efabbae 100755
--- a/t/t3101-ls-tree-dirname.sh
+++ b/t/t3101-ls-tree-dirname.sh
@@ -40,7 +40,7 @@ test_expect_success 'setup' '
'
test_output () {
- sed -e "s/ $_x40
[PATCH 09/28] t4044: skip test if not using SHA-1
This test relies on objects with colliding short names which are necessarily dependent on the hash used. Skip the test if we're not using SHA-1. Signed-off-by: brian m. carlson --- t/t4044-diff-index-unique-abbrev.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t4044-diff-index-unique-abbrev.sh b/t/t4044-diff-index-unique-abbrev.sh index d5ce72be63..647905e01f 100755 --- a/t/t4044-diff-index-unique-abbrev.sh +++ b/t/t4044-diff-index-unique-abbrev.sh @@ -3,6 +3,12 @@ test_description='test unique sha1 abbreviation on "index from..to" line' . ./test-lib.sh +if ! test_have_prereq SHA1 +then + skip_all='not using SHA-1 for objects' + test_done +fi + cat >expect_initial <
[PATCH 02/28] t/test-lib: introduce ZERO_OID
Currently we have a variable, $_z40, which contains the all-zero object ID. However, with NewHash, we'll have an all-zero object ID which is longer than 40 hex characters. In such a case, $_z40 will be a confusing name. Create a $ZERO_OID variable which will always reflect the all-zeros object ID, regardless of the length of the current hash. Signed-off-by: brian m. carlson --- t/test-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index fce728d2ea..58c2ea52c6 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -184,6 +184,7 @@ _x40="$_x35$_x05" # Zero SHA-1 _z40= +ZERO_OID=$_z40 EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904 EMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 @@ -195,7 +196,7 @@ LF=' # when case-folding filenames u200c=$(printf '\342\200\214') -export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB +export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID # Each test should start with something like this, after copyright notices: #
[PATCH 12/28] t3103: abstract away SHA-1-specific constants
Adjust the test so that it uses variables and command substitution for trees instead of hard-coded hashes. This also has the benefit of making it more obvious how the test works. Signed-off-by: brian m. carlson --- t/t3103-ls-tree-misc.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh index 09dcf043fd..14520913af 100755 --- a/t/t3103-ls-tree-misc.sh +++ b/t/t3103-ls-tree-misc.sh @@ -17,7 +17,8 @@ test_expect_success 'setup' ' ' test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' - rm -f .git/objects/5f/cffbd6e4c5c5b8d81f5e9314b20e338e35 && + tree=$(git rev-parse HEAD:a) && + rm -f .git/objects/$(echo $tree | sed -e "s,^\(..\),\1/,") && test_must_fail git ls-tree -r HEAD '
[PATCH 11/28] t2203: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t2203-add-intent.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 1797f946b9..04d840a544 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -27,8 +27,8 @@ test_expect_success 'git status' ' test_expect_success 'git status with porcelain v2' ' git status --porcelain=v2 | grep -v "^?" >actual && - nam1=d00491fd7e5bb6fa28c517a0bb32b8b506539d4d && - nam2=ce013625030ba8dba906f756967f9e9ca394464a && + nam1=$(echo 1 | git hash-object --stdin) && + nam2=$(git hash-object elif) && cat >expect <<-EOF && 1 DA N... 100644 00 100644 $nam1 $ZERO_OID 1.t 1 A. N... 00 100644 100644 $ZERO_OID $nam2 elif @@ -181,7 +181,7 @@ test_expect_success 'rename detection finds the right names' ' EOF test_cmp expected.2 actual.2 && - hash=12f00e90b6ef79117ce6e650416b8cf517099b78 && + hash=$(git hash-object third) && git status --porcelain=v2 | grep -v "^?" >actual.3 && cat >expected.3 <<-EOF && 2 .R N... 100644 100644 100644 $hash $hash R100 third first @@ -212,7 +212,7 @@ test_expect_success 'double rename detection in status' ' EOF test_cmp expected.2 actual.2 && - hash=12f00e90b6ef79117ce6e650416b8cf517099b78 && + hash=$(git hash-object third) && git status --porcelain=v2 | grep -v "^?" >actual.3 && cat >expected.3 <<-EOF && 2 R. N... 100644 100644 100644 $hash $hash R100 second first
[PATCH 14/28] t3905: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t3905-stash-include-untracked.sh | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh index 3ea5b9bb3f..c073514385 100755 --- a/t/t3905-stash-include-untracked.sh +++ b/t/t3905-stash-include-untracked.sh @@ -34,25 +34,26 @@ test_expect_success 'stash save --include-untracked cleaned the untracked files' git status --porcelain >actual && test_cmp expect actual ' - +tracked=$(git rev-parse --short $(echo 1 | git hash-object --stdin)) +untracked=$(git rev-parse --short $(echo untracked | git hash-object --stdin)) cat > expect.diff < expect <
[PATCH 16/28] t4008: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4008-diff-break-rewrite.sh | 59 +++ 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh index 9dd1bc5e16..76481302c3 100755 --- a/t/t4008-diff-break-rewrite.sh +++ b/t/t4008-diff-break-rewrite.sh @@ -27,29 +27,32 @@ Further, with -B and -M together, these should turn into two renames. test_expect_success setup ' cat "$TEST_DIRECTORY"/diff-lib/README >file0 && cat "$TEST_DIRECTORY"/diff-lib/COPYING >file1 && + blob0_id=$(git hash-object file0) && + blob1_id=$(git hash-object file1) && git update-index --add file0 file1 && git tag reference $(git write-tree) ' test_expect_success 'change file1 with copy-edit of file0 and remove file0' ' sed -e "s/git/GIT/" file0 >file1 && + blob2_id=$(git hash-object file1) && rm -f file0 && git update-index --remove file0 file1 ' test_expect_success 'run diff with -B (#1)' ' git diff-index -B --cached reference >current && - cat >expect <<-\EOF && - :100644 00 548142c327a6790ff8821d67c2ee1eff7a656b52 D file0 - :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 2fbedd0b5d4b8126e4750c3bee305e8ff79f80ec M100 file1 + cat >expect <<-EOF && + :100644 00 $blob0_id $ZERO_OID Dfile0 + :100644 100644 $blob1_id $blob2_id M100 file1 EOF compare_diff_raw expect current ' test_expect_success 'run diff with -B and -M (#2)' ' git diff-index -B -M reference >current && - cat >expect <<-\EOF && - :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 2fbedd0b5d4b8126e4750c3bee305e8ff79f80ec R100 file0 file1 + cat >expect <<-EOF && + :100644 100644 $blob0_id $blob2_id R100 file0 file1 EOF compare_diff_raw expect current ' @@ -66,18 +69,18 @@ test_expect_success 'swap file0 and file1' ' test_expect_success 'run diff with -B (#3)' ' git diff-index -B reference >current && - cat >expect <<-\EOF && - :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 6ff87c4664981e4397625791c8ea3bbb5f2279a3 M100 file0 - :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 548142c327a6790ff8821d67c2ee1eff7a656b52 M100 file1 + cat >expect <<-EOF && + :100644 100644 $blob0_id $blob1_id M100 file0 + :100644 100644 $blob1_id $blob0_id M100 file1 EOF compare_diff_raw expect current ' test_expect_success 'run diff with -B and -M (#4)' ' git diff-index -B -M reference >current && - cat >expect <<-\EOF && - :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 6ff87c4664981e4397625791c8ea3bbb5f2279a3 R100 file1 file0 - :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 548142c327a6790ff8821d67c2ee1eff7a656b52 R100 file0 file1 + cat >expect <<-EOF && + :100644 100644 $blob1_id $blob1_id R100 file1 file0 + :100644 100644 $blob0_id $blob0_id R100 file0 file1 EOF compare_diff_raw expect current ' @@ -85,14 +88,15 @@ test_expect_success 'run diff with -B and -M (#4)' ' test_expect_success 'make file0 into something completely different' ' rm -f file0 && test_ln_s_add frotz file0 && + link_oid=$(printf frotz | git hash-object --stdin) && git update-index file1 ' test_expect_success 'run diff with -B (#5)' ' git diff-index -B reference >current && - cat >expect <<-\EOF && - :100644 12 548142c327a6790ff8821d67c2ee1eff7a656b52 67be421f88824578857624f7b3dc75e99a8a1481 T file0 - :100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 548142c327a6790ff8821d67c2ee1eff7a656b52 M100 file1 + cat >expect <<-EOF && + :100644 12 $blob0_id $link_oid Tfile0 + :100644 100644 $blob1_id $blob0_id M100 file1 EOF compare_diff_raw expect current ' @@ -103,9 +107,9 @@ test_expect_success 'run diff with -B -M (#6)' ' # file0 changed from regular to symlink. file1 is the same as the preimage # of file0. Because the change does not make file0 disappear, file1 is # denoted as a copy of file0 - cat >expect <<-\EOF && - :100644 12 548142c327a6790ff8821d67c2ee1eff7a656b52 67be421f88824578857624f7b3dc75e99a8a1481 T file0 - :100644 100644 548142c327a6790ff8821d67c2ee1eff7a656b52 548142c327a6790ff8821d67c2ee1eff7a656b52 C file0 file1 + cat >expect <<-EOF && + :100644 12 $blob0_id $link_oid Tfile0 + :100644 100644 $blob0_id $blob0_id Cfile0 file1 EOF compare_diff_raw expect current ' @@ -115,9 +119,9 @@ test_expect_success 'run diff with -M (#7)' ' # This should
[PATCH 10/28] t: skip pack tests if not using SHA-1
These tests rely on creating packs with specially named objects which are necessarily dependent on the hash used. Skip these tests if we're not using SHA-1. Signed-off-by: brian m. carlson --- t/t5308-pack-detect-duplicates.sh | 6 ++ t/t5309-pack-delta-cycles.sh | 6 ++ 2 files changed, 12 insertions(+) diff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh index 156ae9e9d3..6845c1f3c3 100755 --- a/t/t5308-pack-detect-duplicates.sh +++ b/t/t5308-pack-detect-duplicates.sh @@ -4,6 +4,12 @@ test_description='handling of duplicate objects in incoming packfiles' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-pack.sh +if ! test_have_prereq SHA1 +then + skip_all='not using SHA-1 for objects' + test_done +fi + # The sha1s we have in our pack. It's important that these have the same # starting byte, so that they end up in the same fanout section of the index. # That lets us make sure we are exercising the binary search with both sets. diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh index 3e7861b075..491556dad9 100755 --- a/t/t5309-pack-delta-cycles.sh +++ b/t/t5309-pack-delta-cycles.sh @@ -4,6 +4,12 @@ test_description='test index-pack handling of delta cycles in packfiles' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-pack.sh +if ! test_have_prereq SHA1 +then + skip_all='not using SHA-1 for objects' + test_done +fi + # Two similar-ish objects that we have computed deltas between. A=01d7713666f4de822776c7622c10f1b07de280dc B=e68fe8129b546b101aee9510c5328e7f21ca1d18
[PATCH 07/28] t1007: annotate with SHA1 prerequisite
Since this is a core test that tests basic functionality, annotate the
assertions that have dependencies on SHA-1 with the appropriate
prerequisite.
Signed-off-by: brian m. carlson
---
t/t1007-hash-object.sh | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index 532682f51c..a37753047e 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -9,13 +9,13 @@ echo_without_newline() {
}
test_blob_does_not_exist() {
- test_expect_success 'blob does not exist in database' "
+ test_expect_success SHA1 'blob does not exist in database' "
test_must_fail git cat-file blob $1
"
}
test_blob_exists() {
- test_expect_success 'blob exists in database' "
+ test_expect_success SHA1 'blob exists in database' "
git cat-file blob $1
"
}
@@ -73,19 +73,19 @@ test_expect_success "Can't use --path with --no-filters" '
push_repo
-test_expect_success 'hash a file' '
+test_expect_success SHA1 'hash a file' '
test $hello_sha1 = $(git hash-object hello)
'
test_blob_does_not_exist $hello_sha1
-test_expect_success 'hash from stdin' '
+test_expect_success SHA1 'hash from stdin' '
test $example_sha1 = $(git hash-object --stdin < example)
'
test_blob_does_not_exist $example_sha1
-test_expect_success 'hash a file and write to database' '
+test_expect_success SHA1 'hash a file and write to database' '
test $hello_sha1 = $(git hash-object -w hello)
'
@@ -161,7 +161,7 @@ pop_repo
for args in "-w --stdin" "--stdin -w"; do
push_repo
- test_expect_success "hash from stdin and write to database ($args)" '
+ test_expect_success SHA1 "hash from stdin and write to database
($args)" '
test $example_sha1 = $(git hash-object $args < example)
'
@@ -176,14 +176,14 @@ example"
sha1s="$hello_sha1
$example_sha1"
-test_expect_success "hash two files with names on stdin" '
+test_expect_success SHA1 "hash two files with names on stdin" '
test "$sha1s" = "$(echo_without_newline "$filenames" | git hash-object
--stdin-paths)"
'
for args in "-w --stdin-paths" "--stdin-paths -w"; do
push_repo
- test_expect_success "hash two files with names on stdin and write to
database ($args)" '
+ test_expect_success SHA1 "hash two files with names on stdin and write
to database ($args)" '
test "$sha1s" = "$(echo_without_newline "$filenames" | git
hash-object $args)"
'
[PATCH 04/28] t/test-lib: introduce FULL_HEX
Currently we have a variable, $_x40, which contains a regex that matches a full 40-character hex constant. However, with NewHash, we'll have object IDs that are longer than 40 characters. In such a case, $_x40 will be a confusing name. Create a $FULL_HEX variable which will always reflect a regex matching the appropriate object ID, regardless of the length of the current hash. Signed-off-by: brian m. carlson --- t/test-lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 58c2ea52c6..6d09bd99df 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -184,6 +184,7 @@ _x40="$_x35$_x05" # Zero SHA-1 _z40= +FULL_HEX="$_x40" ZERO_OID=$_z40 EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904 EMPTY_BLOB=e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 @@ -196,7 +197,7 @@ LF=' # when case-folding filenames u200c=$(printf '\342\200\214') -export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID +export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB ZERO_OID FULL_HEX # Each test should start with something like this, after copyright notices: #
[PATCH 20/28] t4029: fix test indentation
We typically indent our tests with a single tab, partially so that we can take advantage of indented heredocs. Make this change and move the quote marks to be in the typical position for our tests. Signed-off-by: brian m. carlson --- t/t4029-diff-trailing-space.sh | 33 - 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh index 3ccc237a8d..f4e18cb8d3 100755 --- a/t/t4029-diff-trailing-space.sh +++ b/t/t4029-diff-trailing-space.sh @@ -18,22 +18,21 @@ index 5f6a263..8cb8bae 100644 EOF exit 1 -test_expect_success \ -"$test_description" \ -'printf "\nx\n" > f && - git add f && - git commit -q -m. f && - printf "\ny\n" > f && - git config --bool diff.suppressBlankEmpty true && - git diff f > actual && - test_cmp exp actual && - perl -i.bak -p -e "s/^\$/ /" exp && - git config --bool diff.suppressBlankEmpty false && - git diff f > actual && - test_cmp exp actual && - git config --bool --unset diff.suppressBlankEmpty && - git diff f > actual && - test_cmp exp actual - ' +test_expect_success "$test_description" ' + printf "\nx\n" > f && + git add f && + git commit -q -m. f && + printf "\ny\n" > f && + git config --bool diff.suppressBlankEmpty true && + git diff f > actual && + test_cmp exp actual && + perl -i.bak -p -e "s/^\$/ /" exp && + git config --bool diff.suppressBlankEmpty false && + git diff f > actual && + test_cmp exp actual && + git config --bool --unset diff.suppressBlankEmpty && + git diff f > actual && + test_cmp exp actual +' test_done
[PATCH 21/28] t4029: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4029-diff-trailing-space.sh | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t4029-diff-trailing-space.sh b/t/t4029-diff-trailing-space.sh index f4e18cb8d3..eaa56521e8 100755 --- a/t/t4029-diff-trailing-space.sh +++ b/t/t4029-diff-trailing-space.sh @@ -6,7 +6,7 @@ test_description='diff honors config option, diff.suppressBlankEmpty' . ./test-lib.sh -cat <<\EOF > exp || +cat <<\EOF >expected || diff --git a/f b/f index 5f6a263..8cb8bae 100644 --- a/f @@ -20,9 +20,12 @@ exit 1 test_expect_success "$test_description" ' printf "\nx\n" > f && + before=$(git rev-parse --short $(git hash-object f)) && git add f && git commit -q -m. f && printf "\ny\n" > f && + after=$(git rev-parse --short $(git hash-object f)) && + sed -e "s/^index .*/index $before..$after 100644/" expected >exp && git config --bool diff.suppressBlankEmpty true && git diff f > actual && test_cmp exp actual &&
[PATCH 13/28] t3702: abstract away SHA-1-specific constants
Strip out the index lines in the diff before comparing them, as these will differ between hash algorithms. This leads to a smaller, simpler change than editing the index line. Signed-off-by: brian m. carlson --- t/t3702-add-edit.sh | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/t/t3702-add-edit.sh b/t/t3702-add-edit.sh index 3cb74ca296..1be5cd5756 100755 --- a/t/t3702-add-edit.sh +++ b/t/t3702-add-edit.sh @@ -40,7 +40,6 @@ test_expect_success 'setup' ' cat > expected-patch << EOF diff --git a/file b/file -index b9834b5..9020acb 100644 --- a/file +++ b/file @@ -1,11 +1,6 @@ @@ -80,7 +79,6 @@ EOF cat > expected << EOF diff --git a/file b/file -index b9834b5..ef6e94c 100644 --- a/file +++ b/file @@ -1,10 +1,12 @@ @@ -100,7 +98,7 @@ EOF echo "#!$SHELL_PATH" >fake-editor.sh cat >> fake-editor.sh <<\EOF -mv -f "$1" orig-patch && +egrep -v '^index' "$1" >orig-patch && mv -f patch "$1" EOF @@ -113,7 +111,8 @@ test_expect_success 'add -e' ' git add -e && test_cmp second-part file && test_cmp orig-patch expected-patch && - git diff --cached > out && + git diff --cached >actual && + egrep -v "^index " actual >out && test_cmp out expected '
[PATCH 15/28] t4007: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs and uses the ZERO_OID variable instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4007-rename-3.sh | 17 + 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh index dae327fabb..b187b7f6c6 100755 --- a/t/t4007-rename-3.sh +++ b/t/t4007-rename-3.sh @@ -17,6 +17,7 @@ test_expect_success 'prepare reference tree' ' echo $tree ' +blob=$(git hash-object "$TEST_DIRECTORY/diff-lib/COPYING") test_expect_success 'prepare work tree' ' cp path0/COPYING path1/COPYING && git update-index --add --remove path0/COPYING path1/COPYING @@ -26,8 +27,8 @@ test_expect_success 'prepare work tree' ' # path1 both have COPYING and the latter is a copy of path0/COPYING. # Comparing the full tree with cache should tell us so. -cat >expected <<\EOF -:100644 100644 6ff87c4664981e4397625791c8ea3bbb5f2279a3 6ff87c4664981e4397625791c8ea3bbb5f2279a3 C100 path0/COPYING path1/COPYING +cat >expectedexpected expected expected <
[PATCH 08/28] t1512: skip test if not using SHA-1
This test relies on objects with colliding short names which are necessarily dependent on the hash used. Skip the test if we're not using SHA-1. Signed-off-by: brian m. carlson --- t/t1512-rev-parse-disambiguation.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh index 711704ba5a..6537f30c9e 100755 --- a/t/t1512-rev-parse-disambiguation.sh +++ b/t/t1512-rev-parse-disambiguation.sh @@ -22,6 +22,12 @@ one tagged as v1.0.0. They all have one regular file each. . ./test-lib.sh +if ! test_have_prereq SHA1 +then + skip_all='not using SHA-1 for objects' + test_done +fi + test_expect_success 'blob and tree' ' test_tick && (
[PATCH 28/28] t5300: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for object IDs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t5300-pack-object.sh | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh index 65ff60f2ee..9e66637a19 100755 --- a/t/t5300-pack-object.sh +++ b/t/t5300-pack-object.sh @@ -466,9 +466,11 @@ test_expect_success !PTHREADS,C_LOCALE_OUTPUT 'pack-objects --threads=N or pack. test_expect_success \ 'fake a SHA1 hash collision' \ -'test -f .git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67 && - cp -f .git/objects/9d/235ed07cd19811a6ceb342de82f190e49c9f68 \ - .git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67' +'long_a=$(git hash-object a | sed -e "s!^..!&/!") && + long_b=$(git hash-object b | sed -e "s!^..!&/!") && + test -f .git/objects/$long_b && + cp -f .git/objects/$long_a \ + .git/objects/$long_b' test_expect_success \ 'make sure index-pack detects the SHA1 collision' \
[PATCH 26/28] t4045: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using
hard-coded hashes.
Signed-off-by: brian m. carlson
---
t/t4045-diff-relative.sh | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 6471a68701..36f8ed8a81 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -8,6 +8,7 @@ test_expect_success 'setup' '
echo content >file1 &&
mkdir subdir &&
echo other content >subdir/file2 &&
+ blob=$(git hash-object subdir/file2) &&
git add . &&
git commit -m one
'
@@ -17,10 +18,11 @@ check_diff () {
shift
expect=$1
shift
+ short_blob=$(git rev-parse --short $blob)
cat >expected <<-EOF
diff --git a/$expect b/$expect
new file mode 100644
- index 000..25c05ef
+ index 000..$short_blob
--- /dev/null
+++ b/$expect
@@ -0,0 +1 @@
@@ -68,7 +70,7 @@ check_raw () {
expect=$1
shift
cat >expected <<-EOF
- :00 100644
25c05ef3639d2d270e7fe765a67668f098092bc5 A $expect
+ :00 100644 $blob A $expect
EOF
test_expect_success "--raw $*" "
git -C '$dir' diff --no-abbrev --raw $* HEAD^ >actual &&
[PATCH 17/28] t4014: abstract away SHA-1-specific constants
Adjust the test so that it computes values for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4014-format-patch.sh | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 163d64fc32..eb34d0faab 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -578,7 +578,9 @@ test_expect_success 'excessive subject' ' rm -rf patches/ && git checkout side && + before=$(git rev-parse --short $(git hash-object file)) && for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >>file && + after=$(git rev-parse --short $(git hash-object file)) && git update-index file && git commit -m "This is an excessively long subject line for a message due to the habit some projects have of not having a short, one-line subject at the start of the commit message, but rather sticking a whole paragraph right at the start as the only thing in the commit message. It had better not become the filename for the patch." && git format-patch -o patches/ master..side && @@ -586,7 +588,6 @@ test_expect_success 'excessive subject' ' ' test_expect_success 'cover-letter inherits diff options' ' - git mv file foo && git commit -m foo && git format-patch --no-renames --cover-letter -1 && @@ -616,7 +617,7 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' ' ' cat > expect << EOF -index 40f36c6..2dc5c23 100644 +index $before..$after 100644 --- a/file +++ b/file @@ -13,4 +13,20 @@ C @@ -640,7 +641,7 @@ test_expect_success 'format-patch respects -U' ' cat > expect << EOF diff --git a/file b/file -index 40f36c6..2dc5c23 100644 +index $before..$after 100644 --- a/file +++ b/file @@ -14,3 +14,19 @@ C
[PATCH 23/28] t/lib-diff-alternative: abstract away SHA-1-specific constants
Adjust the test code so that it computes variables for blobs instead of using hard-coded hashes. This makes t4033 and t4050 (the patience and histogram tests) pass. Signed-off-by: brian m. carlson --- t/lib-diff-alternative.sh | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/t/lib-diff-alternative.sh b/t/lib-diff-alternative.sh index 8b4dbf22d2..8d1e408bb5 100644 --- a/t/lib-diff-alternative.sh +++ b/t/lib-diff-alternative.sh @@ -59,9 +59,11 @@ int main(int argc, char **argv) } EOF - cat >expect <<\EOF + file1=$(git rev-parse --short $(git hash-object file1)) + file2=$(git rev-parse --short $(git hash-object file2)) + cat >expectexpect <
[PATCH 25/28] t4042: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4042-diff-textconv-caching.sh | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh index 04a44d5c61..bf33aedf4b 100755 --- a/t/t4042-diff-textconv-caching.sh +++ b/t/t4042-diff-textconv-caching.sh @@ -15,9 +15,13 @@ test_expect_success 'setup' ' echo bar content 1 >bar.bin && git add . && git commit -m one && + foo1=$(git rev-parse --short HEAD:foo.bin) && + bar1=$(git rev-parse --short HEAD:bar.bin) && echo foo content 2 >foo.bin && echo bar content 2 >bar.bin && git commit -a -m two && + foo2=$(git rev-parse --short HEAD:foo.bin) && + bar2=$(git rev-parse --short HEAD:bar.bin) && echo "*.bin diff=magic" >.gitattributes && git config diff.magic.textconv ./helper && git config diff.magic.cachetextconv true @@ -25,14 +29,14 @@ test_expect_success 'setup' ' cat >expect
[PATCH 22/28] t4030: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4030-diff-textconv.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index aad6c7f78d..4cb9f0e523 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -148,7 +148,8 @@ test_expect_success 'diffstat does not run textconv' ' # restore working setup echo file diff=foo >.gitattributes -cat >expect.typechange <<'EOF' +symlink=$(git rev-parse --short $(printf frotz | git hash-object --stdin)) +cat >expect.typechange
[PATCH 18/28] t4020: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4020-diff-external.sh | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh index 49d3f54b29..fd2140700e 100755 --- a/t/t4020-diff-external.sh +++ b/t/t4020-diff-external.sh @@ -13,6 +13,7 @@ test_expect_success setup ' test_tick && echo second >file && + before=$(git rev-parse --short $(git hash-object file)) && git add file && git commit -m second && @@ -180,9 +181,12 @@ test_expect_success 'no diff with -diff' ' echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file test_expect_success 'force diff with "diff"' ' + after=$(git rev-parse --short $(git hash-object file)) && echo >.gitattributes "file diff" && git diff >actual && - test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual + sed -e "s/^index .*/index $before..$after 100644/" \ + "$TEST_DIRECTORY"/t4020/diff.NUL >expected-diff && + test_cmp expected-diff actual ' test_expect_success 'GIT_EXTERNAL_DIFF with more than one changed files' ' @@ -237,7 +241,7 @@ test_expect_success 'diff --cached' ' git update-index --assume-unchanged file && echo second >file && git diff --cached >actual && - test_cmp "$TEST_DIRECTORY"/t4020/diff.NUL actual + test_cmp expected-diff actual ' test_expect_success 'clean up crlf leftovers' '
[PATCH 19/28] t4022: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for blobs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4022-diff-rewrite.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh index cb51d9f9d4..0f1287a8ce 100755 --- a/t/t4022-diff-rewrite.sh +++ b/t/t4022-diff-rewrite.sh @@ -13,6 +13,7 @@ test_expect_success setup ' "nopqrstuvwxyzabcdefghijklmNOPQRSTUVWXYZABCDEFGHIJKLM" \ <"$TEST_DIRECTORY"/../COPYING >test && echo "to be deleted" >test2 && + blob=$(git rev-parse --short $(git hash-object test2)) && git add test2 ' @@ -27,7 +28,7 @@ test_expect_success 'detect rewrite' ' cat >expect
[PATCH 24/28] t4205: sort log output in a hash-independent way
This test enumerates log entries and then sorts them. For SHA-1, this produces results that happen to sort in the order specified in the test, but for other hash algorithms they sort differently. Ensure we sort the log entries in a hash-independent way by sorting on the ref name instead of the object ID. Signed-off-by: brian m. carlson --- t/t4205-log-pretty-formats.sh | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 591f35daaf..2052cadb11 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -516,22 +516,22 @@ test_expect_success 'log decoration properly follows tag chain' ' git commit --amend -m shorter && git log --no-walk --tags --pretty="%H %d" --decorate=full >actual && cat <<-EOF >expected && - $head1 (tag: refs/tags/tag2) $head2 (tag: refs/tags/message-one) $old_head1 (tag: refs/tags/message-two) + $head1 (tag: refs/tags/tag2) EOF - sort actual >actual1 && + sort -k3 actual >actual1 && test_cmp expected actual1 ' test_expect_success 'clean log decoration' ' git log --no-walk --tags --pretty="%H %D" --decorate=full >actual && cat >expected <<-EOF && - $head1 tag: refs/tags/tag2 $head2 tag: refs/tags/message-one $old_head1 tag: refs/tags/message-two + $head1 tag: refs/tags/tag2 EOF - sort actual >actual1 && + sort -k3 actual >actual1 && test_cmp expected actual1 '
[PATCH 27/28] t4208: abstract away SHA-1-specific constants
Adjust the test so that it computes variables for object IDs instead of using hard-coded hashes. Signed-off-by: brian m. carlson --- t/t4208-log-magic-pathspec.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh index a1705f70cf..62f335b2d9 100755 --- a/t/t4208-log-magic-pathspec.sh +++ b/t/t4208-log-magic-pathspec.sh @@ -45,8 +45,9 @@ test_expect_success 'git log -- :' ' ' test_expect_success 'git log HEAD -- :/' ' + initial=$(git rev-parse --short HEAD^) && cat >expected <<-EOF && - 24b24cf initial + $initial initial EOF (cd sub && git log --oneline HEAD -- :/ >../actual) && test_cmp expected actual
[PATCH] mailmap: update brian m. carlson's email address
The order of addresses in the mailmap file was reversed, leading to git preferring the crustytoothpaste.ath.cx address, which is obsolete, over the crustytoothpaste.net address, which is current. Switch the order of the addresses so that git log displays the correct address. Signed-off-by: brian m. carlson --- .mailmap | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.mailmap b/.mailmap index 7c71e88ea5..df7cf6313c 100644 --- a/.mailmap +++ b/.mailmap @@ -25,8 +25,8 @@ Ben Walton Benoit Sigoure Bernt Hansen Brandon Casey -brian m. carlson Brian M. Carlson -brian m. carlson +brian m. carlson Brian M. Carlson +brian m. carlson Bryan Larsen Bryan Larsen Cheng Renquan
Re: [bug] Multiline value should error if the next line is section
> On Sun, 6 May 2018 22:03:10 +0200 > Martin Ågren wrote: > > On 6 May 2018 at 21:03, Shulhan wrote: > > > [alias] > > > tree = --no-pager log --graph \ > > > -n 20 \ > > > [user] > > > name = Shulhan > > > > > > (2) Run `git config -f git.config -l` > > > > > > The command print the following output, > > > > > > alias.tree=--no-pager log --graph -n 20 [user] > > > alias.name=Shulhan > > > > Small mistake, big consequences. :-) > > > > This behavior looks correct to me, though. It seems very hard to me to > > second-guess what the user meant. For example, what if that third line > > contained a "="? Like: > > > > [alias] > > huh = !dd \ > > bs=1024 ... > > > > Should Git guess that the backslash on the second line was a mistake? > > Or maybe not, because alias.bs = "1024 ..." would be a useless alias? > > The context of multiline next value that I reported before was > about section, not variable. > > > > > I think such guessing would be theoretically possible, but especially > > if Git guesses wrong, that could be very frustrating to fight against. > > > > I'm not familiar with git config parser, obviously :), but checking > the start of next multiline value that start with '[' maybe not > impossible. Git should not guessed, but report error at the > offending line: either user forgot to enclosed the variable with > double quote or they missplace the backslash. But it's not an error; as far as the config file syntax is concerned, it's perfectly valid, even if it's not what you intended. Reporting it as error would be just guessing.
Re: [PATCH v10 18/36] merge-recursive: add get_directory_renames()
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 30894c1cc7..22c5e8e5c9 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
> + struct tree *tree)
> +{
> + struct hashmap *dir_renames;
> + struct hashmap_iter iter;
> + struct dir_rename_entry *entry;
> + int i;
> +
> + /*
> + * Typically, we think of a directory rename as all files from a
> + * certain directory being moved to a target directory. However,
> + * what if someone first moved two files from the original
> + * directory in one commit, and then renamed the directory
> + * somewhere else in a later commit? At merge time, we just know
> + * that files from the original directory went to two different
> + * places, and that the bulk of them ended up in the same place.
> + * We want each directory rename to represent where the bulk of the
> + * files from that directory end up; this function exists to find
> + * where the bulk of the files went.
> + *
> + * The first loop below simply iterates through the list of file
> + * renames, finding out how often each directory rename pair
> + * possibility occurs.
> + */
> + dir_renames = xmalloc(sizeof(struct hashmap));
Please use xmalloc(sizeof(*dir_renames)) instead, to avoid repeating the
data type.
> + dir_rename_init(dir_renames);
> + for (i = 0; i < pairs->nr; ++i) {
> + struct string_list_item *item;
> + int *count;
> + struct diff_filepair *pair = pairs->queue[i];
> + char *old_dir, *new_dir;
> +
> + /* File not part of directory rename if it wasn't renamed */
> + if (pair->status != 'R')
> + continue;
> +
> + get_renamed_dir_portion(pair->one->path, pair->two->path,
> + &old_dir,&new_dir);
> + if (!old_dir)
> + /* Directory didn't change at all; ignore this one. */
> + continue;
> +
> + entry = dir_rename_find_entry(dir_renames, old_dir);
> + if (!entry) {
> + entry = xmalloc(sizeof(struct dir_rename_entry));
Similarly: xmalloc(sizeof(*entry))
Re: [PATCH 04/28] t/test-lib: introduce FULL_HEX
On Sun, May 6, 2018 at 7:17 PM, brian m. carlson
wrote:
> Currently we have a variable, $_x40, which contains a regex that matches
> a full 40-character hex constant. However, with NewHash, we'll have
> object IDs that are longer than 40 characters. In such a case, $_x40
> will be a confusing name. Create a $FULL_HEX variable which will always
> reflect a regex matching the appropriate object ID, regardless of the
> length of the current hash.
Bikeshedding: $FULL_HEX doesn't convey much. Perhaps $OID_REGEX? (And
$_x05 and $_x35 can be named $OID_REGEX_SHORT and
$OID_REGEX_{something}, respectively? Or perhaps they don't need
renaming?)
> Signed-off-by: brian m. carlson
[RFC] Other chunks for commit-graph, part 2 - reachability indexes
Hello, In previous email I wrote: JN> As this post is getting long, I'll post other ideas, about commit JN> labeling for faster reachability queries in a separate email. This is this email. Here is second part of series dedicated to discussing what other data, like various reachability indexes / labeling could be added to the commit-graph file (as new chunks) to make Git even faster. By reachability I mean answering the question whether commit A can reach commit B, or in other words if commit B is an ancestor of commit A. This type of query is used in many Git operations. The commit-graph has one such reachability index built-in already, in the form of generation numbers; this index is called level or topological level of node / vertex in the literature / articles about graph algorithms. > 2. The generation number of the commit. Commits with no parents have >generation number 1; commits with parents have generation number one >more than the maximum generation number of its parents. I have played a bit with various reachability indexes, starting with the ones described in "Reachability Queries in Very Large Graphs: A Fast Refined Online Search Approach" (FELINE index) paper by Renê R. Veloso, Loïc Cerf, Wagner Meira Jr and Mohammed J. Zaki (2014), available in the PDF form at https://openproceedings.org/EDBT/2014/paper_166.pdf I have started working on Jupyter Notebook on Google Colaboratory to find out how much speedup we can get using those indexes for Git large-ish commit graphs (which turns out to be quite specific type of graph, more about which later), namely git.git and Linux kernel ones. The Jupyter Notebook (which runs on Google cloud, but can be also run locally) uses Python kernel, NetworkX librabry for graph manipulation, and matplotlib (via NetworkX) for display. Available at: https://colab.research.google.com/drive/1V-U7_slu5Z3s5iEEMFKhLXtaxSu5xyzg https://drive.google.com/file/d/1V-U7_slu5Z3s5iEEMFKhLXtaxSu5xyzg/view?usp=sharing At current size it started to be a bit unwieldy, especially recomputing it from the start, so I am thinking about splitting it up and moving it to GitHub; then I can e.g. put code and data in separate files, so they do not have to be recalculated (cloning full Linux repository takes quite a bit of time). I have started the process: first step which is copy of Colaboratory notebook is now available as the following repo: https://github.com/jnareb/git-commit-graph-ext https://github.com/jnareb/git-commit-graph-ext/blob/master/Graphs_FELINE_index.ipynb Let's start with the reminder from part 1: > Requirements and recommendations about possible new chunks > == > > Because the main goal of commit-graph feature is better performance in > large repositories, any proposed new chunks (or, at higher level, every > proposed piece of new data) needs to have the following properties. > > > 1. First, it needs to have bounded and sensible size. This means that > the size of new proposed chunk should be constant, proportional to the > number of commits, or at worst proportional to the number of edges. > > From the existing chunks, OIDF (OID Fanout) has constant size, both OIDL > (OID Lookup) and CGET/CDAT (Commit Data) has size proportional to the > number of commits, while not always present EDGE (Large Edge List) has > size proportional to the number of "excess" edges in "huge vertices" > (octopus merges). > > > 2. Second, we want fast access, in most cases fast random access. This > means using fixed-size records. All currently existing chunks have > records (columns) of fixed and aligned size. > > This restriction means that idexes where we have variable amount of data > per vertex (per commit) are discouraged. > > > 3. Third, it needs to be reasonably fast to create, and fast to update. > This means time to create the chunk to be proportional to number of > commits, or sum of number of commits and edges (which for commit graph > and other sparse graphs is proprtional to the number of nodes / commits > anyway). In my opinion time proportional to n*lug(n), where 'n' is the > number of commits, is also acceptable. Times proportional to n^2 or n^3 > are not acceptable. > > It is also strongly preferred that time to update the chunk is > proportional to the number of new commits, so that incremental > (append-only) update is possible. The generation numbers index has this > property. Though as Ævar said in response, which I agree with: ÆB> If hypothetically there was some data that significantly sped up Git ÆB> and the algorithm to generate it was ridiculously expensive, that ÆB> would be fine when combined with the ability to fetch it ÆB> pre-generated from the server. There could always be an option where ÆB> you trust the server and optionally don't verify the data, or where ÆB> it's much cheaper to verify than compute. https://public-inbox.org/git/86h8nm2pv8@gmail.com/T/#mdbc6a4ef052ae95
Re: [PATCH 14/28] t3905: abstract away SHA-1-specific constants
On Sun, May 6, 2018 at 7:17 PM, brian m. carlson wrote: > Adjust the test so that it computes variables for blobs instead of using > hard-coded hashes. > > Signed-off-by: brian m. carlson > --- > diff --git a/t/t3905-stash-include-untracked.sh > b/t/t3905-stash-include-untracked.sh > @@ -34,25 +34,26 @@ test_expect_success 'stash save --include-untracked > cleaned the untracked files' > git status --porcelain >actual && > test_cmp expect actual > ' > - > +tracked=$(git rev-parse --short $(echo 1 | git hash-object --stdin)) > +untracked=$(git rev-parse --short $(echo untracked | git hash-object > --stdin)) You lost the blank line following the previous test. > cat > expect.diff < diff --git a/HEAD b/HEAD > new file mode 100644 > -index 000..d00491f > +index 000..$tracked > --- /dev/null > +++ b/HEAD
Re: [PATCH 16/28] t4008: abstract away SHA-1-specific constants
On Sun, May 6, 2018 at 7:17 PM, brian m. carlson wrote: > Adjust the test so that it computes variables for blobs instead of using > hard-coded hashes. > > Signed-off-by: brian m. carlson > --- > diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh > @@ -27,29 +27,32 @@ Further, with -B and -M together, these should turn into > two renames. > + blob0_id=$(git hash-object file0) && > + blob1_id=$(git hash-object file1) && > + blob2_id=$(git hash-object file1) && > + link_oid=$(printf frotz | git hash-object --stdin) && Inconsistency nit: For the blobs, you tacked on "_id" but for the link you added "_oid".
Re: [PATCH v2 13/18] color: provide inverted colors, too
Hi Peff, On Sun, 6 May 2018, Jeff King wrote: > On Sun, May 06, 2018 at 02:35:44AM -0400, Jeff King wrote: > > > You'd have to introduce GIT_COLOR_REVERSE. I don't think we have a > > constant for it yet, but it's \x[7m. > > Heh, of course you knew that already, as I just noticed your patch is > using the reverse attribute internally (I had thought at first glance > you were just specifying the background independently). > > So really, I guess all I am arguing for is having GIT_COLOR_INV (or > REVERSE) as a constant, and then teaching the code to combine it with > the existing "new" color. It's perfectly OK to have: > > \x1b[7m\x1b[36m > > instead of: > > \x1b[7;36m > > It's two extra bytes, but I doubt anybody cares. Yep, I agree that it is a small price to pay for the benefit of simply using the reverse of diff.color.old (and .new). While at it, I also changed the hunk header colors: they are *also* simply the same ones, with the outer one having background and foreground reversed. Ciao, Dscho
Re: [PATCH v2 18/18] completion: support branch-diff
Hi Duy,
On Sun, 6 May 2018, Duy Nguyen wrote:
> On Fri, May 04, 2018 at 05:35:11PM +0200, Johannes Schindelin wrote:
> > Tab completion of `branch-diff` is very convenient, especially given
> > that the revision arguments that need to be passed to `git branch-diff`
> > are typically more complex than, say, your grandfather's `git log`
> > arguments.
> >
> > Without this patch, we would only complete the `branch-diff` part but
> > not the options and other arguments.
> >
> > This of itself may already be slightly disruptive for well-trained
> > fingers that assume that `git braorimas` would expand to
> > `git branch origin/master`, as we now no longer automatically append a
> > space after completing `git branch`: this is now ambiguous.
> >
> > Signed-off-by: Johannes Schindelin
> > ---
> > contrib/completion/git-completion.bash | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/contrib/completion/git-completion.bash
> > b/contrib/completion/git-completion.bash
> > index 01dd9ff07a2..45addd525ac 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -1496,6 +1496,24 @@ _git_format_patch ()
> > __git_complete_revlist
> > }
> >
> > +__git_branch_diff_options="
> > + --no-patches --creation-weight= --dual-color
> > +"
> > +
> > +_git_branch_diff ()
> > +{
> > + case "$cur" in
> > + --*)
> > + __gitcomp "
>
> You should use __gitcomp_builtin so you don't have to maintain
> $__git_branch_diff_options here. Something like this
>
> -- 8< --
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 45addd525a..4745631daf 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1496,18 +1496,11 @@ _git_format_patch ()
> __git_complete_revlist
> }
>
> -__git_branch_diff_options="
> - --no-patches --creation-weight= --dual-color
> -"
> -
> _git_branch_diff ()
> {
> case "$cur" in
> --*)
> - __gitcomp "
> - $__git_branch_diff_options
> - $__git_diff_common_options
> - "
> + __gitcomp_builtin branch-diff "$__git_diff_common_options"
> return
> ;;
> esac
> -- 8< --
Does this really work? I have this instead, for now, and verified that it
works:
-- snipsnap --
diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 01dd9ff07a2..c498c053881 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1205,13 +1205,14 @@ _git_bisect ()
_git_branch ()
{
- local i c=1 only_local_ref="n" has_r="n"
+ local i c=1 only_local_ref="n" has_r="n" diff_mode="n"
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
-d|--delete|-m|--move) only_local_ref="y" ;;
-r|--remotes) has_r="y" ;;
+ --diff) diff_mode="y" ;;
esac
((c++))
done
@@ -1221,11 +1222,22 @@ _git_branch ()
__git_complete_refs --cur="${cur##--set-upstream-to=}"
;;
--*)
+ if [ $diff_mode = "y" ]; then
+ __gitcomp "
+ --creation-factor= --dual-color
+ $__git_diff_common_options
+ "
+ return
+ fi
__gitcomp_builtin branch "--no-color --no-abbrev
--no-track --no-column
"
;;
*)
+ if [ $diff_mode = "y" ]; then
+ __git_complete_revlist
+ return
+ fi
if [ $only_local_ref = "y" -a $has_r = "n" ]; then
__gitcomp_direct "$(__git_heads "" "$cur" " ")"
else
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Buga, On Sun, 6 May 2018, Igor Djordjevic wrote: > On 06/05/2018 14:10, Johannes Schindelin wrote: > > > I think Todd's idea to shift it from a full-blown builtin to a cmdmode > > of `branch` makes tons of sense. > > I don`t know, I still find it a bit strange that in order to "diff > something", you go to "something" and tell it to "diff itself" - not > because it`s a weird concept (OOP, anyone? :]), but because we already > have "diff" command that can accept different things, thus just teaching > it to accept additional "something" (branch, in this case), seems more > natural (to me) - "branch diff" being just another "diff" mode of > operation. You also have to call `git branch` to list branches. And to rename branches. And to delete them. So why not also compare them at the same time? > What about that side thought you left out from my original message, > making it `git diff --branch` instead? I really did not like this, as all of the `git diff` options really are about comparing two revisions, not two *sets* of revisions. Further, if I put my unsuspecting user hat on, I would ask myself how you can compare branches with one another? That is what I would expect `git diff --branch` to do, not to compare two versions of *the same* branch. So `git diff --branch` does not at all convey the same to me as `git branch --diff`, and I find that the latter does match better what this patch series tries to achieve. I briefly considered `git branch --compare` instead, but then rejected it: it would again sound more like I try to compare two separate (and likely unrelated) branches with one another, and that simply does not make much sense, and tbdiff would not help with that, anyway. > But if "branch diff" is considered to be too special-cased mode of > "diff" so that supporting it from `diff` itself would make it feel > awkward in both usage and maintenance (in terms of many other regular > `diff` specific options being unsupported), I guess I would understand > having it outside `diff` altogether (and implemented as proposed `git > branch --diff`, or something)... for the time being, at least :) The branch diff is not even a special-cased mode of diff. It is *way* more complicated than that. It tries to find 1:1 correspondences between *sets* of commits, and then only outputs a "sort" of a diff between the commits that correspond with each other. I say "sort" of a diff because that diff does not look like `git diff ` at all! So I think it would just be confusing to add that mode to `git diff`. Ciao, Dscho
Re: [PATCH v2 13/18] color: provide inverted colors, too
Jeff King writes:
> On Sun, May 06, 2018 at 02:35:44AM -0400, Jeff King wrote:
>
>> You'd have to introduce GIT_COLOR_REVERSE. I don't think we have a
>> constant for it yet, but it's \x[7m.
>
> Heh, of course you knew that already, as I just noticed your patch is
> using the reverse attribute internally (I had thought at first glance
> you were just specifying the background independently).
I somehow suspected as such, but I also thought so and reacted "what
about us whose terminal is black-on-white unlike most others?",
before looking up what 7 meant ;-)
> So really, I guess all I am arguing for is having GIT_COLOR_INV (or
> REVERSE) as a constant, and then teaching the code to combine it with
> the existing "new" color. It's perfectly OK to have:
>
> \x1b[7m\x1b[36m
>
> instead of:
>
> \x1b[7;36m
>
> It's two extra bytes, but I doubt anybody cares.
I do not think two extra bytes will be missed, but it was not
immediately obvious to me how much flexibility or simplicity weu are
gaining by combining values from multiple configuration variables.
With a "letters on a new line is painted with ${new}, in addition,
the leading plus is further annotated with ${tbdiffNew}" (similarly
to "old") scheme, the user can take advantage of the fact that there
is no ${reset} between ${new} and ${tbdiffNew} and set tbdiffNew and
tbdiffOld to a same value (that does not change the color but
changes some other aspect of the appearance, like "reverse" or
"underline"). Since only pre-designed combination can be used (your
example works only because you chose to allow combination by
annotating the leading "+" with ${new}${tbdiffNew}), we'd need to
(1) establish a convention to paint things with similar meanings in
the same color, modifyable by individual command (e.g. you could say
anything new is by default green with "color.new=green", and then
"color.frotz.new=blink" "color.status.new=" "color.diff.new=blue"
would make frotz, status and diff subcommands to show new things in
blinking green, normal green, and blue), and (2) push the codebase
to adopt such color combination as a preferred design pattern if we
want the resulting system to be useful.
I guess you are getting simpler configuration, which is a big plus,
but to make a truly useful combining convention, we'd need to
rethink and find a way to transition existing configurations to the
new world, which may not be feasible.
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Johannes Schindelin writes:
>> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-)
>> but I think the 't' in there stands for "topic", not "Thomas's".
>>
>> How about "git topic-diff"?
>
> Or `git topic-branch-diff`?
Yeah something along that line, which is about comparing each step
in two iterations of a single topic. It would be wonderful if it
also supported a short-hand
$ git tbdiff --reflog 1.day.ago js/branch-diff
that turned into:
$ git tbdiff js/branch-diff..js/branch-diff@{1.day.ago} \
js/branch-diff@{1.day.ago}..js/branch-diff
That compares "what was on the topic a day ago" with "what is new on
the topic since that time", which is exactly what an individual
contributor wants when reviewing how the topic was polished, I would
say.
[Footnote]
A variant I often use when accepting a rerolled series is
$ git checkout js/branch-diff
$ git checkout master...
$ git am ./+js-branch-diff-v2
$ git tbdiff ..@{-1} @{-1}..
so this is not only for individual contributors but also helps
integrators.
Re: [PATCH 00/28] Hash-independent tests (part 2)
On Sun, May 6, 2018 at 7:17 PM, brian m. carlson wrote: > This series introduces an SHA1 prerequisite which checks if the hash in > use is SHA-1, and can be used to skip the test if it is not. > Additionally, because NewHash will be 256-bit, I introduced aliases for > the test constants $_x40 and $_z40 which will be less confusing when the > hash isn't 40 hex characters long. I opted to leave the old names in > place for the moment to prevent any potential conflicts with other > series and will clean up any stragglers later. > > Several tests are skipped because of SHA-1-specific dependencies: some > of these are core tests which test basic expected hash values, some > depend on colliding short names, and some depend on specially named > object (the pack tests). Was I wrong to expect this series to annotate[1] tests t3404 "short SHA-1 setup" t3404 "short SHA-1 collide" with the SHA1 prerequisite? [1]: https://public-inbox.org/git/CAPig+cR==snfgdhwqpdvw75fuxxg-vsq5tz_or7sy_c0l94...@mail.gmail.com/T/#m7bb98bd57a3189bb5fe01993b22b0c480a601259
Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
Hi Todd,
On Sat, 5 May 2018, Todd Zullinger wrote:
> > @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const
> > char *prefix)
> > struct string_list branch1 = STRING_LIST_INIT_DUP;
> > struct string_list branch2 = STRING_LIST_INIT_DUP;
> >
> > + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> > +
> > diff_setup(&diffopt);
> > diffopt.output_format = DIFF_FORMAT_PATCH;
> > diffopt.flags.suppress_diff_headers = 1;
>
> Should this also (or only) check color.diff.frag?
This code is not querying diff.color.frag, it is setting it. Without
any way to override it.
Having thought about it longer, and triggered by Peff's suggestion to
decouple the "reverse" part from the actual color, I fixed this by
- *not* setting .frag to magenta,
- using the reverse method also to mark outer *hunk headers* (not only the
outer -/+ markers).
- actually calling git_diff_ui_config()...
> I thought that color.diff.* was preferred over diff.color.*, though
> that doesn't seem to be entirely true in all parts of the current
> codebase.
>
> In testing this series it seems that setting color.diff
> options to change the various colors read earlier in this
> patch via diff_get_color_opt, as well as the 'frag' slot,
> are ignored. Setting them via diff.color. does work.
In my tests, it did not even work via diff.color.. But I think I
fixed this (at least my local testing confirms this) by calling
git_diff_ui_config().
> The later patch adding a man page documents branch-diff as
> using `diff.color.*` and points to git-config(1), but the
> config docs only list color.diff.
In the current form (`git branch --diff`), I refrained from going into
*so* much detail ;-) But the gist still holds, and now the code should
support it, too.
The current work in progress can be pulled as `branch-diff` from
https://github.com/dscho/git, if I could ask you to test?
Ciao,
Dscho
Re: [PATCH v2 07/18] branch-diff: indent the diffs just like tbdiff
Hi Martin,
On Sun, 6 May 2018, Martin Ågren wrote:
> On 4 May 2018 at 17:34, Johannes Schindelin
> wrote:
> > @@ -353,6 +358,7 @@ static void output(struct string_list *a, struct
> > string_list *b,
> > int cmd_branch_diff(int argc, const char **argv, const char *prefix)
> > {
> > struct diff_options diffopt = { NULL };
> > + struct strbuf four_spaces = STRBUF_INIT;
> > double creation_weight = 0.6;
> > struct option options[] = {
> > OPT_SET_INT(0, "no-patches", &diffopt.output_format,
> > @@ -371,6 +377,9 @@ int cmd_branch_diff(int argc, const char **argv, const
> > char *prefix)
> >
> > diff_setup(&diffopt);
> > diffopt.output_format = DIFF_FORMAT_PATCH;
> > + diffopt.output_prefix = output_prefix_cb;
> > + strbuf_addstr(&four_spaces, "");
> > + diffopt.output_prefix_data = &four_spaces;
> >
> > argc = parse_options(argc, argv, NULL, options,
> > builtin_branch_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
>
> You end up leaking the buffer of `four_spaces`. Granted, that's not a
> big memory leak, but still. ;-) This was the only leak that
> LeakSanitizer found in v2 when running the new test-script and playing
> around with this a bit. This looks really good!
Good point. Fixed.
Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Eric, On Sun, 6 May 2018, Eric Sunshine wrote: > On Sun, May 6, 2018 at 8:21 AM, Johannes Schindelin > wrote: > > On Sun, 6 May 2018, Junio C Hamano wrote: > >> Johannes Schindelin writes: > >> > On Sat, 5 May 2018, Jeff King wrote: > >> >> One minor point about the name: will it become annoying as a tab > >> >> completion conflict with git-branch? > >> > >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-) > >> but I think the 't' in there stands for "topic", not "Thomas's". > >> How about "git topic-diff"? > > > > Or `git topic-branch-diff`? > > > > But then, we do not really use the term `topic branch` a lot in Git, *and* > > the operation in question is not really about showing differences between > > topic branches, but between revisions of topic branches. > > > > So far, the solution I like best is to use `git branch --diff <...>`, > > which also neatly side-steps the problem of cluttering the top-level > > command list (because tab completion). > > Let's, please, not fall into the trap of polluting git-branch with > utterly unrelated functionality, as has happened a few times with > other Git commands. Let's especially not do so merely for the sake of > tab-completion. git-branch is for branch management; it's not for > diff'ing. I totally disagree. `git branch` is *the* command to work with branches. Yes, you can manage branches. But you can also list them. And now you can also compare them. > Of the suggestions thus far, Junio's git-topic-diff seems the least > worse, and doesn't suffer from tab-completion problems. Except that this is too limited a view. Have you seen one of the more important tidbits in the cover letter, the one about Git for Windows' *branch thicket*? In this case, it is not *one* topic branch that we are talking about. And even worse: what this patch series introduces is not at all a feature to compare topic branches! Instead, it is a way to compare iterations of patch series, versions of topic branches, changes introduced into a topic branch by rebasing it, etc. And `git topic-diff` simply does not say this. It says something different, something that my patches cannot fulfill. > Building on Duy's suggestion: git-interdiff could be a superset of the > current git-branch-diff: > > # standard interdiff > git interdiff womp-v1 womp-v2 > # 'tbdiff'-like output > git interdiff --topic womp-v1 womp-v2 No, no, and no. An interdiff is an interdiff is an interdiff. See e.g. https://www.tutorialspoint.com/unix_commands/interdiff.htm for details. The operation introduced by this patch series, or for that matter tbdiff, *never ever* produced an interdiff. Get this "interdiff" label out of your mind immediately when you think about this here operation. One of my commit messages even talks about this, and says *why* we do not generate interdiffs: they are in general not even well-defined. Take my --rebase-merges patch series, for example. It is so long-running that at some stages, all I did was to resolve merge conflicts incurred from rebasing to `master`. That was literally all. Now, if you tried to produce an interdiff, you would *already fail in the first step*, as the previous overall diff does not apply in reverse on current `master`. Out of all the options so far, the one that I liked was `git branch --diff`. Seriously. I do not understand why you think that this is abusing the `git branch` command. It is no less abusing it than `git branch --edit-description`! And that is a *very good* command, and it is *very good* that it is an option to `git branch`. It makes a total lot of sense, I have never had to think "wait, in which Git command is this implemented already?" And I would expect the exact same thing to happen with `git branch --diff`. Ciao, Johannes
Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
Hi Brian, On Sun, 6 May 2018, brian m. carlson wrote: > On Fri, May 04, 2018 at 05:34:27PM +0200, Johannes Schindelin wrote: > > The incredibly useful `git-tbdiff` tool to compare patch series (say, > > to see what changed between two iterations sent to the Git mailing > > list) is slightly less useful for this developer due to the fact that > > it requires the `hungarian` and `numpy` Python packages which are for > > some reason really hard to build in MSYS2. So hard that I even had to > > give up, because it was simply easier to reimplement the whole shebang > > as a builtin command. > > I just want to say thanks for writing this. I use tbdiff extensively at > work and having this built-in and much faster will really help. > > I did a once-over of v1 and I'll probably take a look at v2 or v3 > (whatever's the latest) later in the week. Thank you so much! Dscho
Re: [PATCH 04/28] t/test-lib: introduce FULL_HEX
On Sun, May 06, 2018 at 07:53:42PM -0400, Eric Sunshine wrote:
> On Sun, May 6, 2018 at 7:17 PM, brian m. carlson
> wrote:
> > Currently we have a variable, $_x40, which contains a regex that matches
> > a full 40-character hex constant. However, with NewHash, we'll have
> > object IDs that are longer than 40 characters. In such a case, $_x40
> > will be a confusing name. Create a $FULL_HEX variable which will always
> > reflect a regex matching the appropriate object ID, regardless of the
> > length of the current hash.
>
> Bikeshedding: $FULL_HEX doesn't convey much. Perhaps $OID_REGEX? (And
> $_x05 and $_x35 can be named $OID_REGEX_SHORT and
> $OID_REGEX_{something}, respectively? Or perhaps they don't need
> renaming?)
I agree that $OID_REGEX is better. Thinking about it, I'm wondering if
$_xoid might be shorter and familiar enough to people who are used to
$_x40. I'll wait for other people to chime in, and then reroll.
I don't think the short forms will end up needing renaming, but I'll try
to pick something sane in line with the others should that be necessary.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 14/28] t3905: abstract away SHA-1-specific constants
On Sun, May 06, 2018 at 08:03:27PM -0400, Eric Sunshine wrote: > On Sun, May 6, 2018 at 7:17 PM, brian m. carlson > wrote: > > Adjust the test so that it computes variables for blobs instead of using > > hard-coded hashes. > > > > Signed-off-by: brian m. carlson > > --- > > diff --git a/t/t3905-stash-include-untracked.sh > > b/t/t3905-stash-include-untracked.sh > > @@ -34,25 +34,26 @@ test_expect_success 'stash save --include-untracked > > cleaned the untracked files' > > git status --porcelain >actual && > > test_cmp expect actual > > ' > > - > > +tracked=$(git rev-parse --short $(echo 1 | git hash-object --stdin)) > > +untracked=$(git rev-parse --short $(echo untracked | git hash-object > > --stdin)) > > You lost the blank line following the previous test. So I did. Will fix. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 16/28] t4008: abstract away SHA-1-specific constants
On Sun, May 06, 2018 at 08:07:46PM -0400, Eric Sunshine wrote: > On Sun, May 6, 2018 at 7:17 PM, brian m. carlson > wrote: > > Adjust the test so that it computes variables for blobs instead of using > > hard-coded hashes. > > > > Signed-off-by: brian m. carlson > > --- > > diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh > > @@ -27,29 +27,32 @@ Further, with -B and -M together, these should turn > > into two renames. > > + blob0_id=$(git hash-object file0) && > > + blob1_id=$(git hash-object file1) && > > + blob2_id=$(git hash-object file1) && > > + link_oid=$(printf frotz | git hash-object --stdin) && > > Inconsistency nit: For the blobs, you tacked on "_id" but for the link > you added "_oid". Yes, that was intentional. I want them to line up nicely because it makes reading the heredocs much easier. Maybe it would be better if I called it "linkf_id" or "slink_id" or something. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 00/28] Hash-independent tests (part 2)
On Sun, May 06, 2018 at 09:49:45PM -0400, Eric Sunshine wrote: > On Sun, May 6, 2018 at 7:17 PM, brian m. carlson > wrote: > > This series introduces an SHA1 prerequisite which checks if the hash in > > use is SHA-1, and can be used to skip the test if it is not. > > Additionally, because NewHash will be 256-bit, I introduced aliases for > > the test constants $_x40 and $_z40 which will be less confusing when the > > hash isn't 40 hex characters long. I opted to leave the old names in > > place for the moment to prevent any potential conflicts with other > > series and will clean up any stragglers later. > > > > Several tests are skipped because of SHA-1-specific dependencies: some > > of these are core tests which test basic expected hash values, some > > depend on colliding short names, and some depend on specially named > > object (the pack tests). > > Was I wrong to expect this series to annotate[1] tests > > t3404 "short SHA-1 setup" > t3404 "short SHA-1 collide" > > with the SHA1 prerequisite? t3404 is in the next series. I realize it's slightly out of order, but I tended to fix them in roughly the order they showed up in parallel test output, which is why it's in the next series. There are some more involved ones which ended up in the next series as well. I figured reviewers would appreciate a shorter series over a longer one. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 1/2] Documentation: use 8-space tabs with Asciidoctor
"brian m. carlson" writes: > Asciidoctor expands tabs at the beginning of a line. However, it does > not expand them into 8 spaces by default. Since we use 8-space tabs, > tell Asciidoctor that we want 8 spaces by setting the tabsize attribute. > This ensures that our ASCII art renders properly. > > Signed-off-by: brian m. carlson > --- > Documentation/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Wonderful. Thanks. > > diff --git a/Documentation/Makefile b/Documentation/Makefile > index 6232143cb9..bcd216d96c 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -184,7 +184,7 @@ ASCIIDOC = asciidoctor > ASCIIDOC_CONF = > ASCIIDOC_HTML = xhtml5 > ASCIIDOC_DOCBOOK = docbook45 > -ASCIIDOC_EXTRA += -acompat-mode > +ASCIIDOC_EXTRA += -acompat-mode -atabsize=8 > ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions > ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;' > DBLATEX_COMMON =
Re: [PATCH] mailmap: update brian m. carlson's email address
"brian m. carlson" writes:
> The order of addresses in the mailmap file was reversed, leading to git
> preferring the crustytoothpaste.ath.cx address, which is obsolete, over
> the crustytoothpaste.net address, which is current. Switch the order of
> the addresses so that git log displays the correct address.
>
> Signed-off-by: brian m. carlson
> ---
I initially reacted to "was reversed" with "yikes, did we break the
mailmap reader and we need to update the file?", but apparently that
is not what this patch is about. I think what is happening here is
that cdb6b5ac (".mailmap: Combine more (name, email) to individual
persons", 2013-08-12) removed
-Brian M. Carlson
and then added these two lines
+brian m. carlson Brian M. Carlson
+brian m. carlson
where *.net address did not come from any other entry for you in the
file. I guess the author of the patch saw that you were sending
your messages from the .net address and tried to help by unifying
the two addresses, without knowing your preference and recorded two
reversed entries.
Will queue as-is for now, but if you want to update the log message
I do not mind taking a reroll.
Thanks.
> .mailmap | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/.mailmap b/.mailmap
> index 7c71e88ea5..df7cf6313c 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -25,8 +25,8 @@ Ben Walton
> Benoit Sigoure
> Bernt Hansen
> Brandon Casey
> -brian m. carlson Brian M. Carlson
>
> -brian m. carlson
>
> +brian m. carlson Brian M. Carlson
>
> +brian m. carlson
>
> Bryan Larsen
> Bryan Larsen
> Cheng Renquan
Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish
Ævar Arnfjörð Bjarmason writes:
> Right, and I'm with you so far, this makes sense to me for all existing
> uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same
> as rev-parse v2.17.0^{tree}^{tree}...
More importantly, you could spell v2.17.0 part of the above with a
short hexadecimal string. And that string should be naming some
tree-ish, the most important thing being that it is *NOT* required
to be a tree (and practically, it is likely that the user has a
tree-ish that is *NOT* a tree).
I guess I have a reaction to the title
"get_short_oid/peel_onion: ^{tree} should be tree"
"X^{tree}" should *RESULT* in a tree, but it should *REQUIRE* X to
be a tree-ish. It is unclear "should be tree" is about the former
and I read (perhaps mis-read) it as saying "it should require X to
be a tree"---that statement is utterly incorrect as we agreed above.
> case-by-case[1]:
>
> ^{tag}:
> 7452b4b5786778d5d87f5c90a94fab8936502e20
I take it as "git rev-parse 7452^{tag}" output (similarly ^{$type}
for the rest)? That probably is desirable, as blobs, trees and
commits cannot be peeled down to a tag.
> ^{commit}:
> hint: 74521eee4c commit 2007-12-01 - git-gui: install-sh from automake
> does not like -m755
> hint: 745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for
> check_refname_component
If 7452 points at a commit, that tag itself should also be given as
a possible object the user may have meant in the "hint" thing. I
agree it is a good idea to exclude trees and blobs from the hint,
for the same reason why I think it makes sense to exclude blobs,
trees and commits from hints for a X in "X^{tag}" above.
> ^{tree}:
> hint: 7452336aa3 tree
> hint: 74524f384d tree
> hint: 7452813bcd tree
> hint: 7452b1a701 tree
> hint: 7452b73c42 tree
> hint: 7452ca1557 tree
Again, if there is a commit or a tag (that points at a commit or a
tree) whose name begins with 7452, it should be included in the hint
above. Not having blobs in the hint of course makes sense, as a
blob cannot be X in "X^{tree}".
> And[2]:
>
> core.disambiguate=tag:
> [same as ^{tag]
> core.disambiguate=commit:
> [same as ^{commit}]
When core.disambiguate tells us to "interprete hexadecimal literals
to name commit objects only", giving only commits in hints: section
makes sense, because we are explicitly saying that "when I say 7452,
I do not mean any tag whose name begins with 7452", so "sorry, your
request is not explicit enough---there are two commits and a tag
that begin with that prefix" is not helpful---it should stop at "you
may have meant one of these two commits" and not mention any tag.
Re: [PATCH 2/2] Documentation: render revisions correctly under Asciidoctor
On 6 May 2018 at 22:42, brian m. carlson wrote: > When creating a literal block from an indented block without any sort of > delimiters, Asciidoctor strips off all leading whitespace, resulting in > a misrendered chart. Use an explicit literal block to indicate to > Asciidoctor that we want to keep the leading whitespace. Excellent. These two patches fix my original problem and seem like the obviously correct approach (in hindsight ;-) ). I wonder if the diagrams earlier in the file should be tackled also. Right now, one has a huge number of dots instead of the four you added; the other has none. They do render fine though, so that'd only be about consistency in the original .txt-file.
Re: What's cooking in git.git (Apr 2018, #04; Mon, 30)
Elijah Newren writes: > Hi Junio, > > On Sun, Apr 29, 2018 at 8:25 PM, Junio C Hamano wrote: >> * en/rename-directory-detection-reboot (2018-04-25) 36 commits > >> >> Reboot of an attempt to detect wholesale directory renames and use >> it while merging. >> >> Will merge to 'next'. > > Usually you have a mini-release-note in your "What's cooking" emails > next to the series, so I'm assuming from the text here that you might > just be intending to re-use the release note you used with the > original series. For the rebooted series, it is probably worth adding > something to the release notes about how it also fixes the > unnecessary-update issue reported by Linus Thanks.
Re: [PATCH v2 13/18] color: provide inverted colors, too
Hi Junio,
On Mon, 7 May 2018, Junio C Hamano wrote:
> Jeff King writes:
>
> > So really, I guess all I am arguing for is having GIT_COLOR_INV (or
> > REVERSE) as a constant, and then teaching the code to combine it with
> > the existing "new" color. It's perfectly OK to have:
> >
> > \x1b[7m\x1b[36m
> >
> > instead of:
> >
> > \x1b[7;36m
> >
> > It's two extra bytes, but I doubt anybody cares.
>
> I do not think two extra bytes will be missed, but it was not
> immediately obvious to me how much flexibility or simplicity weu are
> gaining by combining values from multiple configuration variables.
> With a "letters on a new line is painted with ${new}, in addition,
> the leading plus is further annotated with ${tbdiffNew}" (similarly
> to "old") scheme, the user can take advantage of the fact that there
> is no ${reset} between ${new} and ${tbdiffNew} and set tbdiffNew and
> tbdiffOld to a same value (that does not change the color but
> changes some other aspect of the appearance, like "reverse" or
> "underline"). Since only pre-designed combination can be used (your
> example works only because you chose to allow combination by
> annotating the leading "+" with ${new}${tbdiffNew}), we'd need to
> (1) establish a convention to paint things with similar meanings in
> the same color, modifyable by individual command (e.g. you could say
> anything new is by default green with "color.new=green", and then
> "color.frotz.new=blink" "color.status.new=" "color.diff.new=blue"
> would make frotz, status and diff subcommands to show new things in
> blinking green, normal green, and blue), and (2) push the codebase
> to adopt such color combination as a preferred design pattern if we
> want the resulting system to be useful.
>
> I guess you are getting simpler configuration, which is a big plus,
> but to make a truly useful combining convention, we'd need to
> rethink and find a way to transition existing configurations to the
> new world, which may not be feasible.
I really do not like the sound of that much complexity. It strikes me as
yet another instance of Yer Ain't Gonna Need It. In *particular* because
nested diffs are a special thing: you *already* get overwhelmed with
too much information, and adding colors to the fray won't help.
What does help is to keep the colors, so that they can mean the same thing
in inner vs outer diffs, but reverse foreground and background to make the
outer diff "stick out more".
Should my assessment be wrong, I think it'll still be relatively easy to
add support for config settings, *then*, not before we know it is needed.
Ciao,
Dscho
Re: [PATCH v2 02/18] Add a new builtin: branch-diff
Hi Junio,
On Mon, 7 May 2018, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-)
> >> but I think the 't' in there stands for "topic", not "Thomas's".
> >>
> >> How about "git topic-diff"?
> >
> > Or `git topic-branch-diff`?
>
> Yeah something along that line, which is about comparing each step
> in two iterations of a single topic. It would be wonderful if it
> also supported a short-hand
>
> $ git tbdiff --reflog 1.day.ago js/branch-diff
>
> that turned into:
>
> $ git tbdiff js/branch-diff..js/branch-diff@{1.day.ago} \
> js/branch-diff@{1.day.ago}..js/branch-diff
Or even easier: `git tbdiff js/branch-diff@{1.day.ago}...js/branch-diff`.
> That compares "what was on the topic a day ago" with "what is new on
> the topic since that time", which is exactly what an individual
> contributor wants when reviewing how the topic was polished, I would
> say.
It would be easy to introduce, but I am wary about its usefulness.
Unless you re-generate the branch from patches (which I guess you do a
lot, but I don't), you are likely to compare incomplete patch series: say,
when you call `git rebase -i` to reword 05/18's commit message, your
command will only compare 05--18 of the patch series.
Worse, if js/branch-diff needs to be uprooted (e.g. because it now depends
on some different patch, or because it already depended on a separate
patch series that was now updated), your `git branch --diff` call will
compare more than just my patches: it will assume that those dependencies
are part of the patch series, because they changed, too.
> [Footnote]
>
> A variant I often use when accepting a rerolled series is
>
> $ git checkout js/branch-diff
> $ git checkout master...
> $ git am ./+js-branch-diff-v2
> $ git tbdiff ..@{-1} @{-1}..
>
> so this is not only for individual contributors but also helps
> integrators.
Yes, and I also pointed out (twice) that it will help interested parties
follow what I do with my merging-rebases in Git for Windows.
Ciao,
Dscho
main url for linking to git source?
The git-scm.com site currently links to https://github.com/git/git for the (non-tarball) source code. Somebody raised the question[1] of whether it should point to kernel.org instead. Do people find one interface more or less pleasing than the other? Do we want to prefer kernel.org as more "official" or less commercial? I could see arguments both ways, so I thought I'd take a straw poll of what people on the list think. -Peff [1] https://github.com/git/git-scm.com/pull/1202
