Re: [BUG] gitignore documentation inconsistent with actual behaviour
On 20 Oct 2018, at 00:26, Duy Nguyen wrote: >Which way should we go? I'm leaning towards the second one... Not sure how much my opinion is worth, but the second option does feel more friendly (from a usage perspective) as well as more straight-forward (from a re-implementation perspective). There's a third option too, though, right? The 'rsync' behaviour mentioned earlier? It wouldn't matter either way in any of the examples i listed, but is there ever a conceivable use case for something like `foo**bar`, where the `**` matches across slashes? (I can't think of any reason i'd need that personally, but then again i don't understand why these people are using `**` the way they are in the first place.) dana
Re: [BUG] gitignore documentation inconsistent with actual behaviour
On Thu, Oct 11, 2018 at 05:19:06AM -0500, dana wrote:
> Hello,
>
> I'm a contributor to ripgrep, which is a grep-like tool that supports using
> gitignore files to control which files are searched in a repo (or any other
> directory tree). ripgrep's support for the patterns in these files is based on
> git's official documentation, as seen here:
>
> https://git-scm.com/docs/gitignore
>
> One of the most common reports on the ripgrep bug tracker is that it does not
> allow patterns like the following real-world examples, where a ** is used
> along
> with other text within the same path component:
>
> **/**$$*.java
> **.orig
> **local.properties
> !**.sha1
>
> The reason it doesn't allow them is that the gitignore documentation
> explicitly
> states that they're invalid:
>
> ...
I've checked the code and run some tests. There is a twist here. "**"
is only special when matched in "pathname" mode. That is when the
pattern contains at least one slash. In your patterns above, that only
applies to the first pattern.
When '**' is special, if it's neither '**/', '/**/' or '/**', it _is_
considered invalid (i.e. bad pattern) and the pattern will not match
anything.
The confusion comes from when '**' is not special for the remaining
three patterns, it's considered as regular '*' and still matches
stuff.
So, I think we have two options. The document could be clarified with
something like this
-- 8< --
diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index d107daaffd..500cd43939 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -100,7 +100,8 @@ PATTERN FORMAT
a shell glob pattern and checks for a match against the
pathname relative to the location of the `.gitignore` file
(relative to the toplevel of the work tree if not from a
- `.gitignore` file).
+ `.gitignore` file). Note that the "two consecutive asterisks" rule
+ below does not apply.
- Otherwise, Git treats the pattern as a shell glob: "`*`" matches
anything except "`/`", "`?`" matches any one character except "`/`"
@@ -129,7 +130,8 @@ full pathname may have special meaning:
matches zero or more directories. For example, "`a/**/b`"
matches "`a/b`", "`a/x/b`", "`a/x/y/b`" and so on.
- - Other consecutive asterisks are considered invalid.
+ - Other consecutive asterisks are considered invalid and the pattern
+ is ignored.
NOTES
-
-- 8< --
Or we could make the behavior consistent. If '**' is invalid, just
consider it two separate regular '*'. Then all four of your patterns
will behave the same way. The change for that is quite simple
-- 8< --
diff --git a/wildmatch.c b/wildmatch.c
index d074c1be10..64087bf02c 100644
--- a/wildmatch.c
+++ b/wildmatch.c
@@ -104,8 +104,10 @@ static int dowild(const uchar *p, const uchar *text,
unsigned int flags)
dowild(p + 1, text, flags) ==
WM_MATCH)
return WM_MATCH;
match_slash = 1;
- } else
- return WM_ABORT_MALFORMED;
+ } else {
+ /* without WM_PATHNAME, '*' == '**' */
+ match_slash = flags & WM_PATHNAME ? 0 :
1;
+ }
} else
/* without WM_PATHNAME, '*' == '**' */
match_slash = flags & WM_PATHNAME ? 0 : 1;
-- 8< --
Which way should we go? I'm leaning towards the second one...
--
Duy
Re: Shouldn't git be able to apply diffs that it created with --ignore-whitespace?
On Thu, Oct 18, 2018 at 03:12:09PM -0500, Mahmoud Al-Qudsi wrote: > Hello again all, > > I think I've previously broached this subject before, but I think I perhaps > wasn't clear enough about what I was trying to do or why I feel that git is at > fault here. > > (I'm running git 2.19.1) > > Starting with a fully-committed, not-dirty codebase, I open(ed) a poorly > formatted, mixed-whitespace file (that I absolutely did not author!) under > version control and make some very localized changes. My editor, being very > smart and helpful, fixes up the line ending on save, and I exit. > > At this point, my source file contains a) deliberate changes I want, and b) > whitespace changes I wish I could commit but that should not be a part of my > patch. > > Shouldn't the following workflow be supported: > > ~> git diff -w > foo.diff > ~> git reset --hard > ~> git apply [--ignore-whitespace] < foo.diff In general, git diff -w doesn't produce a patch that can be applied. That's because it ignores all whitespace changes in a particular way. The diff output is rendered as if the whitespace in the lines were written as it is in the postimage (the changed file), not the preimage (the original file). This is useful because usually if you're, say, indenting a block of code, you want to see the output properly indented with the new lines of code (say, the loop or conditional) you wrote around it. And since this feature is designed for visual inspection, it makes sense to do it this way. However, it essentially means that your changes can't, in general, be applied. There are, of course, situations in which this might have worked in the past, and it may indeed work for some situations still, but it won't in the general case. git apply --ignore-whitespace only modifies context lines, so it doesn't affect the actual content lines in the diff. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [RFC PATCH 2/3] Documentation/git-rev-list: s///
On Sun, Oct 14, 2018 at 4:25 PM Junio C Hamano wrote:
>
> Matthew DeVore writes:
>
> > -List commits that are reachable by following the `parent` links from the
> > -given commit(s), but exclude commits that are reachable from the one(s)
> > -given with a '{caret}' in front of them. The output is given in reverse
> > -chronological order by default.
> > +List objects that are reachable by following references from the given
> > +object(s), but exclude objects that are reachable from the one(s) given
> > +with a '{caret}' in front of them.
> >
> > +By default, only commit objects are shown, and the commits are shown in
> > +reverse chronological order. The '--object' flag causes non-commit objects
> > +to also be shown.
> >
> > +You can think of this as a set operation. Objects given on the command
> > +line form a set of objects that are reachable from any of them, and then
> > +objects reachable from any of the ones given with '{caret}' in front are
> > +subtracted from that set. The remaining objects are what come out in the
> > command's output. Various other options and paths parameters can be used
> > to further limit the result.
>
> I am not sure if this is a good rewrite. It gives a false
> impression as if you'd not see anything if I did this:
>
> git rev-list --objects ^master md/filter-trees:t/valgrind
>
Oh that's interesting. So my mental model conflicts with the command's
behavior. It actually is surprising behavior because:
# this shows all files that were modified in the HEAD commit
git rev-list --objects ^HEAD~1^{tree} HEAD:
# but this shows *all* files at HEAD
git rev-list --objects ^HEAD~1 HEAD:
Which means that ^commit and ^non-commit are treated inherently
differently. Maybe I should fix this bug before clarifying this
documentation...
> It is more like "this is a set operation across commits. We also
> show objects that are reachable from the commits in the resulting
> set and are not reachable from the commits in the set that were
> excluded when --objects option is given".
>
That would be correct though it wouldn't tell that you can use
"--objects ^foo-tree bar-tree." Without fixing the above bug, I could
add to your wording something to the effect of "You can also use trees
to include and exclude sets of objects rather than commits." Which
implies that mixing "--objects ^commit tree" on the command line is
undefined.
Re: [PATCH 1/1] archive: init archivers before determining format
On Fri, Oct 19, 2018 at 04:19:28PM -0700, stead...@google.com wrote:
> diff --git a/builtin/archive.c b/builtin/archive.c
> index e74f675390..dd3283a247 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv,
>* it.
>*/
> if (name_hint) {
> - const char *format = archive_format_from_filename(name_hint);
> + const char *format;
> + init_tar_archiver();
> + init_zip_archiver();
> + format = archive_format_from_filename(name_hint);
> if (format)
> packet_write_fmt(fd[1], "argument --format=%s\n",
> format);
Hrm. This code was added back in 56baa61d01 (archive: move file
extension format-guessing lower, 2011-06-21), and your example
invocation worked back then!
Unfortunately it was broken by the very next patch in the series,
08716b3c11 (archive: refactor file extension format-guessing,
2011-06-21). I guess that's what I get for not adding regression tests.
It's probably worth mentioning those points in the commit message.
Does this work with configured archiver extensions, too? I think so,
because we load them via init_tar_archiver().
Can we avoid repeating the list of archivers here? This needs to stay in
sync with the list in write_archive(). I know there are only two, but
can we factor out an init_archivers() call or something?
We also should probably just call it unconditionally when we start the
archiver command (I don't think there are any other bugs like this
lurking, but it doesn't cost very much to initialize these; it makes
sense to just do it early).
Other than those minor points (and the lack of test), your fix looks
good to me.
-Peff
Re: [PATCH 0/1] Fix format detection when archiving remotely
On Fri, Oct 19, 2018 at 04:19:27PM -0700, stead...@google.com wrote: > Currently, git-archive does not properly determine the desired archive > format when both --output and --remote are provided, because > run_remote_archiver() does not initialize the archivers prior to calling > archive_format_from_filename(). This results in the remote archiver > always returning a TAR file, regardless of the requested format. > > This patch initializes the TAR and ZIP archivers before calling > archive_format_from_filename(), which fixes format detection. It seems like some of this content could be in the commit message of the actual patch. > Steps to reproduce: > > ∫ git version > git version 2.19.1.568.g152ad8e336-goog > ∫ cd ~/src/git > ∫ git archive --output ~/good.zip HEAD > ∫ file ~/good.zip > /home/steadmon/good.zip: Zip archive data, at least v1.0 to extract > ∫ git archive --output ~/bad.zip --remote=. HEAD > ∫ file ~/bad.zip > /home/steadmon/bad.zip: POSIX tar archive And this could be in a test script in the actual patch. :) -Peff
[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them
At a checkpoint, fast-import resets all branches and tags on disk to the
OID that we have in memory. If --force is not given, only branch resets
that result in fast forwards with respect to the state on disk are
allowed.
With this approach, even for refs that fast-import has not been
commanded to change for a long time (or ever), at each checkpoint, we
will systematically reset them to the last state this process knows
about, a state which may have been set before the previous checkpoint.
Any changes made to these refs by a different process since the last
checkpoint will be overwritten.
1> is one process, 2> is another:
1> $ git fast-import --force
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master $B
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point master points again to $A.
This problem is mitigated somewhat for branches when --force is not
specified (the default), as requiring a fast forward means in practice
that concurrent external changes are likely to be preserved. But it's
not foolproof either:
1> $ git fast-import
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master refs/heads/master~1
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point, master points again to $A, not $A~1 as requested by the
second process.
We now keep track of whether branches and tags have been changed by this
fast-import process since our last checkpoint (or startup). At the next
checkpoint, only refs and tags that new commands have changed are
written to disk.
---
fast-import.c | 14 +++
t/t9300-fast-import.sh | 55 ++
2 files changed, 69 insertions(+)
Changed since v1:
The testcases failed to enforce the new behavior.
- t9300: Fixed tests to properly interleave operations between the
fast-import in the background and the concurrent "git branch" and "git
tag" command.
I misremembered how background_import_then_checkpoint() works: it
return only after the whole set of commands provided in input_file
(and now input_file2) are run by fast-import.
In v2, the competing git commands are started first in the background
and are run after a delay of 1 second. input_file is sent right away
to fast-import, followed 2 seconds later by input_file2. That is:
second 0: input_file
second 1: git branch or git tag command
second 2: input_file2
- t9300: Use --force for the branch testcase: without it, earlier
versions of Git don't fail this testcase (as resetting V3 to U is not
a FF, so the second checkpoint skips updating it in earlier versions
of Git, failing to demonstrate the problematic behavior).
Checked that the new testcases do fail with earlier versions of Git.
TODO: The testcases still rely on sleep(1).
diff --git a/fast-import.c b/fast-import.c
index 95600c78e048..d25be5c83110 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -250,6 +250,7 @@ struct branch {
uintmax_t num_notes;
unsigned active : 1;
unsigned delete : 1;
+ unsigned changed : 1;
unsigned pack_id : PACK_ID_BITS;
struct object_id oid;
};
@@ -257,6 +258,7 @@ struct branch {
struct tag {
struct tag *next_tag;
const char *name;
+ unsigned changed : 1;
unsigned int pack_id;
struct object_id oid;
};
@@ -728,6 +730,7 @@ static struct branch *new_branch(const char *name)
b->branch_tree.versions[1].mode = S_IFDIR;
b->num_notes = 0;
b->active = 0;
+ b->changed = 0;
b->pack_id = MAX_PACK_ID;
branch_table[hc] = b;
branch_count++;
@@ -1715,6 +1718,10 @@ static int update_branch(struct branch *b)
struct object_id old_oid;
struct strbuf err = STRBUF_INIT;
+ if (!b->changed)
+ return 0;
+ b->changed = 0;
+
if (is_null_oid(>oid)) {
if (b->delete)
delete_ref(NULL, b->name, NULL, 0);
@@ -1780,6 +1787,10 @@ static void dump_tags(void)
goto cleanup;
}
for (t = first_tag; t; t = t->next_tag) {
+ if (!t->changed)
+ continue;
+ t->changed = 0;
+
strbuf_reset(_name);
strbuf_addf(_name, "refs/tags/%s", t->name);
@@ -2813,6 +2824,7 @@ static void parse_new_commit(const char *arg)
if (!store_object(OBJ_COMMIT, _data, NULL, >oid, next_mark))
b->pack_id = pack_id;
+ b->changed = 1;
b->last_commit = object_count_by_type[OBJ_COMMIT];
}
@@ -2894,6 +2906,7 @@ static void parse_new_tag(const char *arg)
t->pack_id = MAX_PACK_ID;
else
t->pack_id = pack_id;
+ t->changed = 1;
}
static void parse_reset_branch(const char *arg)
@@ -2914,6
[PATCH 1/1] archive: init archivers before determining format
When passing both --remote and --output to git-archive, initialize the
archivers before attempting to determine the format from the output
filename. Without initialization, the format cannot be determined.
Signed-off-by: Josh Steadmon
---
builtin/archive.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/archive.c b/builtin/archive.c
index e74f675390..dd3283a247 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -45,7 +45,10 @@ static int run_remote_archiver(int argc, const char **argv,
* it.
*/
if (name_hint) {
- const char *format = archive_format_from_filename(name_hint);
+ const char *format;
+ init_tar_archiver();
+ init_zip_archiver();
+ format = archive_format_from_filename(name_hint);
if (format)
packet_write_fmt(fd[1], "argument --format=%s\n",
format);
}
--
2.19.1.568.g152ad8e336-goog
[PATCH 0/1] Fix format detection when archiving remotely
Currently, git-archive does not properly determine the desired archive format when both --output and --remote are provided, because run_remote_archiver() does not initialize the archivers prior to calling archive_format_from_filename(). This results in the remote archiver always returning a TAR file, regardless of the requested format. This patch initializes the TAR and ZIP archivers before calling archive_format_from_filename(), which fixes format detection. Steps to reproduce: ∫ git version git version 2.19.1.568.g152ad8e336-goog ∫ cd ~/src/git ∫ git archive --output ~/good.zip HEAD ∫ file ~/good.zip /home/steadmon/good.zip: Zip archive data, at least v1.0 to extract ∫ git archive --output ~/bad.zip --remote=. HEAD ∫ file ~/bad.zip /home/steadmon/bad.zip: POSIX tar archive (apply patch and build) ∫ ./bin-wrappers/git archive --output ~/fixed.zip --remote=. HEAD ∫ file ~/fixed.zip /home/steadmon/fixed.zip: Zip archive data, at least v1.0 to extract Josh Steadmon (1): archive: init archivers before determining format builtin/archive.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) -- 2.19.1.568.g152ad8e336-goog
[PATCH] fetch-pack: be more precise in parsing v2 response
Each section in a protocol v2 response is followed by either a DELIM
packet (indicating more sections to follow) or a FLUSH packet
(indicating none to follow). But when parsing the "acknowledgments"
section, do_fetch_pack_v2() is liberal in accepting both, but determines
whether to continue reading or not based solely on the contents of the
"acknowledgments" section, not on whether DELIM or FLUSH was read.
There is no issue with a protocol-compliant server, but can result in
confusing error messages when communicating with a server that
serves unexpected additional sections. Consider a server that sends
"new-section" after "acknowledgments":
- client writes request
- client reads the "acknowledgments" section which contains no "ready",
then DELIM
- since there was no "ready", client needs to continue negotiation, and
writes request
- client reads "new-section", and reports to the end user "expected
'acknowledgments', received 'new-section'"
For the person debugging the involved Git implementation(s), the error
message is confusing in that "new-section" was not received in response
to the latest request, but to the first one.
One solution is to always continue reading after DELIM, but in this
case, we can do better. We know from the protocol that "ready" means at
least the packfile section is coming (hence, DELIM) and that no "ready"
means that no sections are to follow (hence, FLUSH). So teach
process_acks() to enforce this.
Signed-off-by: Jonathan Tan
---
fetch-pack.c | 12 ++
t/t5702-protocol-v2.sh | 50 ++
2 files changed, 62 insertions(+)
diff --git a/fetch-pack.c b/fetch-pack.c
index b3ed7121bc..9691046e64 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1248,6 +1248,18 @@ static int process_acks(struct fetch_negotiator
*negotiator,
reader->status != PACKET_READ_DELIM)
die(_("error processing acks: %d"), reader->status);
+ /*
+* If an "acknowledgments" section is sent, a packfile is sent if and
+* only if "ready" was sent in this section. The other sections
+* ("shallow-info" and "wanted-refs") are sent only if a packfile is
+* sent. Therefore, a DELIM is expected if "ready" is sent, and a FLUSH
+* otherwise.
+*/
+ if (received_ready && reader->status != PACKET_READ_DELIM)
+ die(_("expected packfile to be sent after 'ready'"));
+ if (!received_ready && reader->status != PACKET_READ_FLUSH)
+ die(_("expected no other sections to be sent after no
'ready'"));
+
/* return 0 if no common, 1 if there are common, or 2 if ready */
return received_ready ? 2 : (received_ack ? 1 : 0);
}
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 8360188c01..51009ca391 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -512,6 +512,56 @@ test_expect_success 'push with http:// and a config of v2
does not request v2' '
! grep "git< version 2" log
'
+test_expect_success 'when server sends "ready", expect DELIM' '
+ rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
+
+ git clone "$HTTPD_URL/smart/http_parent" http_child &&
+
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" two &&
+
+ # After "ready" in the acknowledgments section, pretend that a FLUSH
+ # () was sent instead of a DELIM (0001).
+ printf "/ready/,$ s/0001//" \
+ >"$HTTPD_ROOT_PATH/one-time-sed" &&
+
+ test_must_fail git -C http_child -c protocol.version=2 \
+ fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
+ test_i18ngrep "expected packfile to be sent after .ready." err
+'
+
+test_expect_success 'when server does not send "ready", expect FLUSH' '
+ rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
+
+ git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
+
+ git clone "$HTTPD_URL/smart/http_parent" http_child &&
+
+ test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" two &&
+
+ # Create many commits to extend the negotiation phase across multiple
+ # requests, so that the server does not send "ready" in the first
+ # request.
+ for i in $(test_seq 1 32)
+ do
+ test_commit -C http_child c$i
+ done &&
+
+ # After the acknowledgments section, pretend that a DELIM
+ # (0001) was sent instead of a FLUSH ().
+ printf "/acknowledgments/,$ s//0001/" \
+ >"$HTTPD_ROOT_PATH/one-time-sed" &&
+
+ GIT_TRACE_PACKET="$(pwd)/log" test_must_fail git -C http_child \
+ -c protocol.version=2 \
+ fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
+
Re: [PATCH v2 13/13] commit-graph: specify OID version for SHA-256
On Thu, Oct 18, 2018 at 09:03:22AM -0400, Derrick Stolee wrote: > We should coordinate before incrementing the version number. I was working > on making the file formats incremental (think split-index) but couldn't come > up with a way to do it without incrementing the file format. It would be > best to combine these features into v2 of each format. For the moment, I'm happy to leave things as they are, and I'll interpret version 1 of the hash version field as whatever hash is in use elsewhere in .git. If you're going to bump the version in the future, feel free to CC me on the patches, and we'll make sure that we serialize the format_version field in the file then. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH] alias: detect loops in mixed execution mode
On Fri, Oct 19, 2018 at 10:28:22AM +0200, Ævar Arnfjörð Bjarmason wrote: > > - string_list_append(_list, *argv[0]); > > + add_cmd_history(, _list, *argv[0]); > > > > /* > > * It could be an alias -- this works around the insanity > > Just to sanity check an assumption of mine: One thing I didn't do is use > sq_quote_buf() and sq_dequote_to_argv() like we do for > CONFIG_DATA_ENVIRONMENT. This is because in the case of config we need > to deal with: > > $ git config alias.cfgdump > !env > $ git -c x.y=z -c "foo.bar='baz'" cfgdump|grep baz > GIT_CONFIG_PARAMETERS='x.y=z' 'foo.bar='\''baz'\''' > > But in this case I don't see how a command-name would ever contain > whitespace. So we skip quoting and just delimit by space. Alias names cannot currently contain whitespace, because it's not allowed in the key. However, we've discussed making the syntax alias..command, which would then make it possible. Whether anyone would use that is a different question, but hey, apparently some people think "My Documents" is a good name for a directory. ;) -Peff
Re: [PATCH] alias: detect loops in mixed execution mode
On Thu, Oct 18, 2018 at 10:57:39PM +, Ævar Arnfjörð Bjarmason wrote:
> Add detection for aliasing loops in cases where one of the aliases
> re-invokes git as a shell command. This catches cases like:
>
> [alias]
> foo = !git bar
> bar = !git foo
>
> Before this change running "git {foo,bar}" would create a
> forkbomb. Now using the aliasing loop detection and call history
> reporting added in 82f71d9a5a ("alias: show the call history when an
> alias is looping", 2018-09-16) and c6d75bc17a ("alias: add support for
> aliases of an alias", 2018-09-16) we'll instead report:
>
> fatal: alias loop detected: expansion of 'foo' does not terminate:
> foo <==
> bar ==>
The regular alias expansion can generally assume that there's no
conditional recursion going on, because it's expanding everything
itself. But when we involve multiple processes, things get trickier.
For instance, I could do this:
[alias]
countdown = "!f() { echo \"$@\"; test \"$1\" -gt 0 && git countdown
$(($1-1)); }; f"
which works now, but not with your patch.
Now obviously that's a silly toy example, but are there real cases which
might trigger this? Some plausible ones I can think of:
- an alias which handles some special cases, then chains to itself for
the simpler one (or to another alias or script, which ends up
chaining back to the original)
- an alias that runs a git command, which then spawns a hook or other
user-controlled script, which incidentally uses that same alias
I'd guess this sort of thing is pretty rare. But I wonder if we're
crossing the line of trying to assume too much about what the user's
arbitrary code does.
A simple depth counter can limit the fork bomb, and with a high enough
depth would be unlikely to trigger a false positive. It could also
protect non-aliases more reasonably, too (e.g., if you have a 1000-deep
git process hierarchy, there's a good chance you've found an infinite
loop in git itself).
> +static void init_cmd_history(struct strbuf *env, struct string_list
> *cmd_list)
> +{
> + const char *old = getenv(COMMAND_HISTORY_ENVIRONMENT);
> + struct strbuf **cmd_history, **ptr;
> +
> + if (!old || !*old)
> + return;
> +
> + strbuf_addstr(env, old);
> + strbuf_rtrim(env);
> +
> + cmd_history = strbuf_split_buf(old, strlen(old), ' ', 0);
> + for (ptr = cmd_history; *ptr; ptr++) {
> + strbuf_rtrim(*ptr);
> + string_list_append(cmd_list, (*ptr)->buf);
> + }
> + strbuf_list_free(cmd_history);
Maybe string_list_split() would be a little simpler?
-Peff
Re: Bug with "git mv" and submodules, and with "git submodule add something --force"
Stefan Beller wrote: > Maybe for now we can do with just an update of the documentation/bugs > section and say we cannot move files in and out of submodules? I think we have some existing logic to prevent "git add"-ing a file within a submodule to the superproject, for example. So "git mv" should learn the same trick. And perhaps the trick needs to be moved down a layer (e.g. into the index API). Hints? Thanks, Jonathan
Re: Bug with "git mv" and submodules, and with "git submodule add something --force"
On Fri, Oct 19, 2018 at 5:43 AM Juergen Vogl wrote: > > Hi there, > > tested on both, git 2.18 and git 2.19.1: > > moving a file with `git mv` from a project to a submodule results in an > **undefined state** of the local repository. Luckily we do have a submodule in git.git itself, so we can try it out here as well (I'll use a separate worktree to not hose my main repo): git worktree add ../testgit && cd ../testgit git checkout v2.19.1 && make install git --version git version 2.19.1 git submodule update --init Cloning into '/home/sbeller/testgit/sha1collisiondetection'... Submodule path 'sha1collisiondetection': checked out '232357eb2ea0397388254a4b188333a227bf5b10' git mv cache.h sha1collisiondetection/ git status HEAD detached at v2.19.1 Changes to be committed: (use "git reset HEAD ..." to unstage) deleted:sha1collisiondetection renamed:cache.h -> sha1collisiondetection/cache.h Untracked files: (use "git add ..." to include in what will be committed) sha1collisiondetection/.gitignore sha1collisiondetection/.travis.yml sha1collisiondetection/LICENSE.txt sha1collisiondetection/Makefile sha1collisiondetection/README.md sha1collisiondetection/lib/ sha1collisiondetection/src/ sha1collisiondetection/test/ sha1collisiondetection/vs2015/ > It breaks up the submodule (it's still in .gitmodules, but not > accessable via `git submodule`), and is not reversible on local repository. This seems like what I just did. However reversing can be done via: git checkout -f git status HEAD detached at v2.19.1 nothing to commit, working tree clean git submodule status 232357eb2ea0397388254a4b188333a227bf5b10 sha1collisiondetection (stable-v1.0.3-31-g232357e) So I think it's just "git-mv" that doesn't respect submodule boundaries, which we would want to address. The man page of git mv (https://git-scm.com/docs/git-mv) actually has a short note about submodules, but that is about moving *a* submodule not about moving things in and out. Maybe for now we can do with just an update of the documentation/bugs section and say we cannot move files in and out of submodules? > > Either `git mv submodule/file .` which is just running the command in reverse, (i.e. swapping destination and target), git mv cache.h sha1collisiondetection/ git mv sha1collisiondetection/cache.h cache.h git status HEAD detached at v2.19.1 Changes to be committed: (use "git reset HEAD ..." to unstage) deleted:sha1collisiondetection Untracked files: (use "git add ..." to include in what will be committed) sha1collisiondetection/ which seems like the submodule is gone, > nor deleting the folder works. For the > locale repo the submodule is gone. Yup, that seems to be the case. > But: trying to add it with `git > submodule add` also do not work and results in an error message (with > and without `--force` flag): Would checkout out a state where the submodule still exists (such as HEAD) and then running "git submodule update --init" fix it instead? > > $ git submodule add g...@github.com:---/wiki-public.git public > --force > A git directory for 'public' is found locally with remote(s): > origing...@github.com:---/wiki-public.git > If you want to reuse this local git directory instead of cloning again from > g...@github.com:---/wiki-public.git > use the '--force' option. If the local git directory is not the correct repo > or you are unsure what this means choose another name with the '--name' > option. > > Therefore, it's in a undefined, broken state. > > > Another bug I've got by testing upper line: > * --force will be used as folder name * when used in `git submodule add > g...@github.com:someone/some.git --force`: Yes, the order of arguments is important, the options (such as --force) goes before the URL and path. > /usr/libexec/git-core/git-submodule: line 273: cd: --: invalid option > cd: usage: cd [-L|-P] [dir] > Unable to checkout submodule '--force' Gah! We'd need to escape the path after the options, i.e. cd -- --force would cd into that directory, but I am unsure if that is accepted by all shells. Thanks, Stefan
Re: [PATCH 19/19] submodule: don't add submodule as odb for push
> In push_submodule(), because we do not actually need access to objects
> in the submodule, do not invoke add_submodule_odb().
> (for_each_remote_ref_submodule() does not require access to those
> objects, and the actual push is done by spawning another process,
> which handles object access by itself.)
The code looks good - my analysis is the same as that in my review of
the previous version [1].
Can you mention, in the commit message, the tests that exercise the
functionality here (and say that they still pass)?
[1]
https://public-inbox.org/git/20181011230028.200488-1-jonathanta...@google.com/
>
> Signed-off-by: Stefan Beller
> ---
> submodule.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 7305ae2e10..e623e6bf7f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1024,9 +1024,6 @@ static int push_submodule(const char *path,
> const struct string_list *push_options,
> int dry_run)
> {
> - if (add_submodule_odb(path))
> - return 1;
> -
> if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
> struct child_process cp = CHILD_PROCESS_INIT;
> argv_array_push(, "push");
> --
> 2.19.0
>
Re: [PATCH 18/19] submodule: use submodule repos for object lookup
> This converts the 'show_submodule_header' function to use
> the repository API properly, such that the submodule objects
> are not added to the main object store.
There is also a side effect in that the submodule now needs to pass all
the checks done by repo_init() instead of merely having the "objects/"
directory. Can you add information about this to the commit message?
Also, which tests exercise this functionality? Mention them in the
commit message.
> +/*
> + * Initialize 'out' based on the provided submodule path.
> + *
> + * Unlike repo_submodule_init, this tolerates submodules not present
> + * in .gitmodules. This function exists only to preserve historical behavior,
> + *
> + * Returns 0 on success, -1 when the submodule is not present.
> + */
> +static int open_submodule(struct repository *out, const char *path)
> +{
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (submodule_to_gitdir(, path) || repo_init(out, sb.buf, NULL)) {
> + strbuf_release();
> + return -1;
> + }
> +
> + out->submodule_prefix = xstrdup(path);
> + out->submodule_prefix = xstrfmt("%s%s/",
> + the_repository->submodule_prefix ?
> + the_repository->submodule_prefix :
> + "", path);
You seem to say here [1] that we don't need submodule_prefix, but you're
instead setting it twice? :-)
[1]
https://public-inbox.org/git/cagz79kztxonmuyx+ehg0q3ss2m-etkf0yiw3e40u3vct4qm...@mail.gmail.com/
> +/*
> + * Helper function to display the submodule header line prior to the full
> + * summary output.
> + *
> + * If it can locate the submodule git directory it will create a repository
> + * handle for the submodule and lookup both the left and right commits and
> + * put them into the left and right pointers.
> */
> -static void show_submodule_header(struct diff_options *o, const char *path,
> +static void show_submodule_header(struct diff_options *o,
> + const char *path,
> struct object_id *one, struct object_id *two,
> unsigned dirty_submodule,
> + struct repository *sub,
> struct commit **left, struct commit **right,
> struct commit_list **merge_bases)
> {
Location of the submodule git directory is done by the caller of this
function, not by this function. Also this function doesn't create any
repository handles. The documentation probably needs to be updated. Also
mention what happens if sub is NULL.
> @@ -563,16 +596,20 @@ void show_submodule_summary(struct diff_options *o,
> const char *path,
> struct rev_info rev;
> struct commit *left = NULL, *right = NULL;
> struct commit_list *merge_bases = NULL;
> + struct repository subrepo, *sub =
> +
> + if (open_submodule(, path) < 0)
> + sub = NULL;
Handling of the subrepo and *sub seems clumsy - it might be better to
just let open_submodule() return a struct repository pointer.
Previous 17 patches look good - most are the same as the previous
version, which I already was happy with, and I see that the patch I
requested in [2] was added.
[2]
https://public-inbox.org/git/20181011221114.186377-1-jonathanta...@google.com/
Re: [PATCH 0/1] commit-reach: fix first-parent heuristic
On 10/18/2018 10:31 PM, Junio C Hamano wrote: "Derrick Stolee via GitGitGadget" writes: I originally reported this fix [1] after playing around with the trace2 series for measuring performance. Since trace2 isn't merging quickly, I pulled the performance fix patch out and am sending it on its own. The only difference here is that we don't have the tracing to verify the performance fix in the test script. Thanks for sending this separately. What's the current status of the tracing patch series, by the way? I'm still working on it. I hope to reroll and submit a new version next week. We are currently dogfooding a version of it with our gvfs users. Jeff
[PATCH v3 3/8] merge-recursive: increase marker length with depth of recursion
Later patches in this series will modify file collision conflict
handling (e.g. from rename/add and rename/rename(2to1) conflicts) so
that multiply nested conflict markers can arise even before considering
conflicts in the virtual merge base. Including the virtual merge base
will provide a way to get triply (or higher) nested conflict markers.
This new way to get nested conflict markers will force the need for a
more general mechanism to extend the length of conflict markers in order
to differentiate between different nestings.
Along with this change to conflict marker length handling, we want to
make sure that we don't regress handling for other types of conflicts
with nested conflict markers. Add a more involved testcase using
merge.conflictstyle=diff3, where not only does the virtual merge base
contain conflicts, but its virtual merge base does as well (i.e. a case
with triply nested conflict markers). While there are multiple
reasonable ways to handle nested conflict markers in the virtual merge
base for this type of situation, the easiest approach that dovetails
well with the new needs for the file collision conflict handling is to
require that the length of the conflict markers increase with each
subsequent nesting.
Subsequent patches which change the rename/add and rename/rename(2to1)
conflict handling will modify the extra_marker_size flag appropriately
for their new needs.
Signed-off-by: Elijah Newren
---
ll-merge.c| 4 +-
ll-merge.h| 1 +
merge-recursive.c | 25 +++--
t/t6036-recursive-corner-cases.sh | 151 ++
4 files changed, 172 insertions(+), 9 deletions(-)
diff --git a/ll-merge.c b/ll-merge.c
index 0e2800f7bb..aabc1b5c2e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -384,7 +384,9 @@ int ll_merge(mmbuffer_t *result_buf,
if (opts->virtual_ancestor) {
if (driver->recursive)
driver = find_ll_merge_driver(driver->recursive);
- marker_size += 2;
+ }
+ if (opts->extra_marker_size) {
+ marker_size += opts->extra_marker_size;
}
return driver->fn(driver, result_buf, path, ancestor, ancestor_label,
ours, our_label, theirs, their_label,
diff --git a/ll-merge.h b/ll-merge.h
index b72b19921e..5b4e158502 100644
--- a/ll-merge.h
+++ b/ll-merge.h
@@ -11,6 +11,7 @@ struct ll_merge_options {
unsigned virtual_ancestor : 1;
unsigned variant : 2; /* favor ours, favor theirs, or union merge */
unsigned renormalize : 1;
+ unsigned extra_marker_size;
long xdl_opts;
};
diff --git a/merge-recursive.c b/merge-recursive.c
index 73b5710386..f795c92a69 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1058,7 +1058,8 @@ static int merge_3way(struct merge_options *o,
const struct diff_filespec *a,
const struct diff_filespec *b,
const char *branch1,
- const char *branch2)
+ const char *branch2,
+ const int extra_marker_size)
{
mmfile_t orig, src1, src2;
struct ll_merge_options ll_opts = {0};
@@ -1066,6 +1067,7 @@ static int merge_3way(struct merge_options *o,
int merge_status;
ll_opts.renormalize = o->renormalize;
+ ll_opts.extra_marker_size = extra_marker_size;
ll_opts.xdl_opts = o->xdl_opts;
if (o->call_depth) {
@@ -1300,6 +1302,7 @@ static int merge_mode_and_contents(struct merge_options
*o,
const char *filename,
const char *branch1,
const char *branch2,
+ const int extra_marker_size,
struct merge_file_info *result)
{
if (o->branch1 != branch1) {
@@ -1310,7 +1313,8 @@ static int merge_mode_and_contents(struct merge_options
*o,
*/
return merge_mode_and_contents(o, one, b, a,
filename,
- branch2, branch1, result);
+ branch2, branch1,
+ extra_marker_size, result);
}
result->merge = 0;
@@ -1351,7 +1355,8 @@ static int merge_mode_and_contents(struct merge_options
*o,
int ret = 0, merge_status;
merge_status = merge_3way(o, _buf, one, a, b,
- branch1, branch2);
+ branch1, branch2,
+ extra_marker_size);
if ((merge_status < 0) || !result_buf.ptr)
ret = err(o, _("Failed to execute internal
merge"));
@@ -1640,7
[PATCH v3 5/8] merge-recursive: fix rename/add conflict handling
This makes the rename/add conflict handling make use of the new
handle_file_collision() function, which fixes several bugs and improves
things for the rename/add case significantly. Previously, rename/add
would:
* Not leave any higher order stage entries in the index, making it
appear as if there were no conflict.
* Would place the rename file at the colliding path, and move the
added file elsewhere, which combined with the lack of higher order
stage entries felt really odd. It's not clear to me why the
rename should take precedence over the add; if one should be moved
out of the way, they both probably should.
* In the recursive case, it would do a two way merge of the added
file and the version of the renamed file on the renamed side,
completely excluding modifications to the renamed file on the
unrenamed side of history.
Use the new handle_file_collision() to fix all of these issues.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 137 +--
t/t6036-recursive-corner-cases.sh| 24 ++---
t/t6042-merge-rename-corner-cases.sh | 4 +-
3 files changed, 101 insertions(+), 64 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 24d979022e..0805095168 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -186,6 +186,7 @@ static int oid_eq(const struct object_id *a, const struct
object_id *b)
enum rename_type {
RENAME_NORMAL = 0,
RENAME_VIA_DIR,
+ RENAME_ADD,
RENAME_DELETE,
RENAME_ONE_FILE_TO_ONE,
RENAME_ONE_FILE_TO_TWO,
@@ -228,6 +229,7 @@ static inline void setup_rename_conflict_info(enum
rename_type rename_type,
struct stage_data *src_entry1,
struct stage_data *src_entry2)
{
+ int ostage1 = 0, ostage2;
struct rename_conflict_info *ci;
/*
@@ -264,18 +266,22 @@ static inline void setup_rename_conflict_info(enum
rename_type rename_type,
dst_entry2->rename_conflict_info = ci;
}
- if (rename_type == RENAME_TWO_FILES_TO_ONE) {
- /*
-* For each rename, there could have been
-* modifications on the side of history where that
-* file was not renamed.
-*/
- int ostage1 = o->branch1 == branch1 ? 3 : 2;
- int ostage2 = ostage1 ^ 1;
+ /*
+* For each rename, there could have been
+* modifications on the side of history where that
+* file was not renamed.
+*/
+ if (rename_type == RENAME_ADD ||
+ rename_type == RENAME_TWO_FILES_TO_ONE) {
+ ostage1 = o->branch1 == branch1 ? 3 : 2;
ci->ren1_other.path = pair1->one->path;
oidcpy(>ren1_other.oid, _entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
+ }
+
+ if (rename_type == RENAME_TWO_FILES_TO_ONE) {
+ ostage2 = ostage1 ^ 1;
ci->ren2_other.path = pair2->one->path;
oidcpy(>ren2_other.oid, _entry2->stages[ostage2].oid);
@@ -1559,7 +1565,6 @@ static struct diff_filespec *filespec_from_entry(struct
diff_filespec *target,
return target;
}
-#if 0 // #if-0-ing avoids unused function warning; will make live in next
commit
static int handle_file_collision(struct merge_options *o,
const char *collide_path,
const char *prev_path1,
@@ -1575,6 +1580,20 @@ static int handle_file_collision(struct merge_options *o,
char *alt_path = NULL;
const char *update_path = collide_path;
+ /*
+* It's easiest to get the correct things into stage 2 and 3, and
+* to make sure that the content merge puts HEAD before the other
+* branch if we just ensure that branch1 == o->branch1. So, simply
+* flip arguments around if we don't have that.
+*/
+ if (branch1 != o->branch1) {
+ return handle_file_collision(o, collide_path,
+prev_path2, prev_path1,
+branch2, branch1,
+b_oid, b_mode,
+a_oid, a_mode);
+ }
+
/*
* In the recursive case, we just opt to undo renames
*/
@@ -1678,7 +1697,38 @@ static int handle_file_collision(struct merge_options *o,
*/
return mfi.clean;
}
-#endif
+
+static int handle_rename_add(struct merge_options *o,
+struct rename_conflict_info *ci)
+{
+ /* a was renamed to c, and a separate c was added. */
+ struct diff_filespec *a = ci->pair1->one;
+ struct diff_filespec *c = ci->pair1->two;
+ char *path = c->path;
+ char
[PATCH v3 4/8] merge-recursive: new function for better colliding conflict resolutions
There are three conflict types that represent two (possibly entirely unrelated) files colliding at the same location: * add/add * rename/add * rename/rename(2to1) These three conflict types already share more similarity than might be immediately apparent from their description: (1) the handling of the rename variants already involves removing any entries from the index corresponding to the original file names[*], thus only leaving entries in the index for the colliding path; (2) likewise, any trace of the original file name in the working tree is also removed. So, in all three cases we're left with how to represent two colliding files in both the index and the working copy. [*] Technically, this isn't quite true because rename/rename(2to1) conflicts in the recursive (o->call_depth > 0) case do an "unrename" since about seven years ago. But even in that case, Junio felt compelled to explain that my decision to "unrename" wasn't necessarily the only or right answer -- search for "Comment from Junio" in t6036 for details. My initial motivation for looking at these three conflict types was that if the handling of these three conflict types is the same, at least in the limited set of cases where a renamed file is unmodified on the side of history where the file is not renamed, then a significant performance improvement for rename detection during merges is possible. However, while that served as motivation to look at these three types of conflicts, the actual goal of this new function is to try to improve the handling for all three cases, not to merely make them the same as each other in that special circumstance. === Handling the working tree === The previous behavior for these conflict types in regards to the working tree (assuming the file collision occurs at 'foo') was: * add/add does a two-way merge of the two files and records it as 'foo'. * rename/rename(2to1) records the two different files into two new uniquely named files (foo~HEAD and foo~$MERGE), while removing 'foo' from the working tree. * rename/add records the two different files into two different locations, recording the add at foo~$SIDE and, oddly, recording the rename at foo (why is the rename more important than the add?) So, the question for what to write to the working tree boils down to whether the two colliding files should be two-way merged and recorded in place, or recorded into separate files. As per discussion on the git mailing lit, two-way merging was deemed to always be preferred, as that makes these cases all more like content conflicts that users can handle from within their favorite editor, IDE, or merge tool. Note that since renames already involve a content merge, rename/add and rename/rename(2to1) conflicts could result in nested conflict markers. === Handling of the index === For a typical rename, unpack_trees() would set up the index in the following fashion: old_path new_path stage1: 5ca1ab1e stage2: f005ba11 stage3: b0a710ad And merge-recursive would rewrite this to new_path stage1: 5ca1ab1e stage2: f005ba11 stage3: b0a710ad Removing old_path from the index means the user won't have to `git rm old_path` manually every time a renamed path has a content conflict. It also means they can use `git checkout [--ours|--theirs|--conflict|-m] new_path`, `git diff [--ours|--theirs]` and various other commands that would be difficult otherwise. This strategy becomes a problem when we have a rename/add or rename/rename(2to1) conflict, however, because then we have only three slots to store blob sha1s and we need either four or six. Previously, this was handled by continuing to delete old_path from the index, and just outright ignoring any blob shas from old_path. That had the downside of deleting any trace of changes made to old_path on the other side of history. This function instead does a three-way content merge of the renamed file, and stores the blob sha1 for that at either stage2 or stage3 for new_path (depending on which side the rename came from). That has the advantage of bringing information about changes on both sides and still allows for easy resolution (no need to git rm old_path, etc.), but does have the downside that if the content merge had conflict markers, then what we store in the index is the sha1 of a blob with conflict markers. While that is a downside, it seems less problematic than the downsides of any obvious alternatives, and certainly makes more sense than the previous handling. Further, it has a precedent in that when we do recursive merges, we may accept a file with conflict markers as the resolution for the merge of the merge-bases, which will then show up in the index of the outer merge at stage 1 if a conflict exists at the outer level. Signed-off-by: Elijah Newren --- merge-recursive.c | 121 ++ 1 file changed, 121 insertions(+) diff
[PATCH v3 8/8] merge-recursive: improve rename/rename(1to2)/add[/add] handling
When we have a rename/rename(1to2) conflict, each of the renames can
collide with a file addition. Each of these rename/add conflicts suffered
from the same kinds of problems that normal rename/add suffered from.
Make the code use handle_file_conflicts() as well so that we get all the
same fixes and consistent behavior between the different conflict types.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 154 +--
t/t6042-merge-rename-corner-cases.sh | 29 +++--
t/t6043-merge-rename-directories.sh | 24 +++--
3 files changed, 113 insertions(+), 94 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c78b347112..5986b6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1709,80 +1709,17 @@ static int handle_rename_add(struct merge_options *o,
ci->dst_entry1->stages[other_stage].mode);
}
-static int handle_file(struct merge_options *o,
- struct diff_filespec *rename,
- int stage,
- struct rename_conflict_info *ci)
-{
- char *dst_name = rename->path;
- struct stage_data *dst_entry;
- const char *cur_branch, *other_branch;
- struct diff_filespec other;
- struct diff_filespec *add;
- int ret;
-
- if (stage == 2) {
- dst_entry = ci->dst_entry1;
- cur_branch = ci->branch1;
- other_branch = ci->branch2;
- } else {
- dst_entry = ci->dst_entry2;
- cur_branch = ci->branch2;
- other_branch = ci->branch1;
- }
-
- add = filespec_from_entry(, dst_entry, stage ^ 1);
- if (add) {
- int ren_src_was_dirty = was_dirty(o, rename->path);
- char *add_name = unique_path(o, rename->path, other_branch);
- if (update_file(o, 0, >oid, add->mode, add_name))
- return -1;
-
- if (ren_src_was_dirty) {
- output(o, 1, _("Refusing to lose dirty file at %s"),
- rename->path);
- }
- /*
-* Because the double negatives somehow keep confusing me...
-*1) update_wd iff !ren_src_was_dirty.
-*2) no_wd iff !update_wd
-*3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty
-*/
- remove_file(o, 0, rename->path, ren_src_was_dirty);
- dst_name = unique_path(o, rename->path, cur_branch);
- } else {
- if (dir_in_way(rename->path, !o->call_depth, 0)) {
- dst_name = unique_path(o, rename->path, cur_branch);
- output(o, 1, _("%s is a directory in %s adding as %s
instead"),
- rename->path, other_branch, dst_name);
- } else if (!o->call_depth &&
- would_lose_untracked(rename->path)) {
- dst_name = unique_path(o, rename->path, cur_branch);
- output(o, 1, _("Refusing to lose untracked file at %s; "
- "adding as %s instead"),
- rename->path, dst_name);
- }
- }
- if ((ret = update_file(o, 0, >oid, rename->mode, dst_name)))
- ; /* fall through, do allow dst_name to be released */
- else if (stage == 2)
- ret = update_stages(o, rename->path, NULL, rename, add);
- else
- ret = update_stages(o, rename->path, NULL, add, rename);
-
- if (dst_name != rename->path)
- free(dst_name);
-
- return ret;
-}
-
static int handle_rename_rename_1to2(struct merge_options *o,
struct rename_conflict_info *ci)
{
/* One file was renamed in both branches, but to different names. */
+ struct merge_file_info mfi;
+ struct diff_filespec other;
+ struct diff_filespec *add;
struct diff_filespec *one = ci->pair1->one;
struct diff_filespec *a = ci->pair1->two;
struct diff_filespec *b = ci->pair2->two;
+ char *path_desc;
output(o, 1, _("CONFLICT (rename/rename): "
"Rename \"%s\"->\"%s\" in branch \"%s\" "
@@ -1790,15 +1727,16 @@ static int handle_rename_rename_1to2(struct
merge_options *o,
one->path, a->path, ci->branch1,
one->path, b->path, ci->branch2,
o->call_depth ? _(" (left unresolved)") : "");
- if (o->call_depth) {
- struct merge_file_info mfi;
- struct diff_filespec other;
- struct diff_filespec *add;
- if (merge_mode_and_contents(o, one, a, b, one->path,
- ci->branch1, ci->branch2,
- o->call_depth * 2, ))
-
[PATCH v3 1/8] Add testcases for consistency in file collision conflict handling
Add testcases dealing with file collisions for the following types of
conflicts:
* add/add
* rename/add
* rename/rename(2to1)
All these conflict types simplify down to two files "colliding"
and should thus be handled similarly. This means that rename/add and
rename/rename(2to1) conflicts need to be modified to behave the same as
add/add conflicts currently do: the colliding files should be two-way
merged (instead of the current behavior of writing the two colliding
files out to separate temporary unique pathnames). Add testcases which
check this; subsequent commits will fix the conflict handling to make
these tests pass.
Signed-off-by: Elijah Newren
---
t/t6042-merge-rename-corner-cases.sh | 162 +++
1 file changed, 162 insertions(+)
diff --git a/t/t6042-merge-rename-corner-cases.sh
b/t/t6042-merge-rename-corner-cases.sh
index b97aca7fa2..b6fed2cb9a 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -937,4 +937,166 @@ test_expect_failure 'mod6-check: chains of
rename/rename(1to2) and rename/rename
)
'
+test_conflicts_with_adds_and_renames() {
+ sideL=$1
+ sideR=$2
+ expect=$3
+
+ # Setup:
+ # L
+ # / \
+ # master ?
+ # \ /
+ # R
+ #
+ # Where:
+ # Both L and R have files named 'three' which collide. Each of
+ # the colliding files could have been involved in a rename, in
+ # which case there was a file named 'one' or 'two' that was
+ # modified on the opposite side of history and renamed into the
+ # collision on this side of history.
+ #
+ # Questions:
+ # 1) The index should contain both a stage 2 and stage 3 entry
+ # for the colliding file. Does it?
+ # 2) When renames are involved, the content merges are clean, so
+ # the index should reflect the content merges, not merely the
+ # version of the colliding file from the prior commit. Does
+ # it?
+ # 3) There should be a file in the worktree named 'three'
+ # containing the two-way merged contents of the content-merged
+ # versions of 'three' from each of the two colliding
+ # files. Is it present?
+ # 4) There should not be any three~* files in the working
+ # tree
+ test_expect_success "setup simple $sideL/$sideR conflict" '
+ test_create_repo simple_${sideL}_${sideR} &&
+ (
+ cd simple_${sideL}_${sideR} &&
+
+ # Create some related files now
+ for i in $(test_seq 1 10)
+ do
+ echo Random base content line $i
+ done >file_v1 &&
+ cp file_v1 file_v2 &&
+ echo modification >>file_v2 &&
+
+ cp file_v1 file_v3 &&
+ echo more stuff >>file_v3 &&
+ cp file_v3 file_v4 &&
+ echo yet more stuff >>file_v4 &&
+
+ # Use a tag to record both these files for simple
+ # access, and clean out these untracked files
+ git tag file_v1 $(git hash-object -w file_v1) &&
+ git tag file_v2 $(git hash-object -w file_v2) &&
+ git tag file_v3 $(git hash-object -w file_v3) &&
+ git tag file_v4 $(git hash-object -w file_v4) &&
+ git clean -f &&
+
+ # Setup original commit (or merge-base), consisting of
+ # files named "one" and "two" if renames were involved.
+ touch irrelevant_file &&
+ git add irrelevant_file &&
+ if [ $sideL = "rename" ]
+ then
+ git show file_v1 >one &&
+ git add one
+ fi &&
+ if [ $sideR = "rename" ]
+ then
+ git show file_v3 >two &&
+ git add two
+ fi &&
+ test_tick && git commit -m initial &&
+
+ git branch L &&
+ git branch R &&
+
+ # Handle the left side
+ git checkout L &&
+ if [ $sideL = "rename" ]
+ then
+ git mv one three
+ else
+ git show file_v2 >three &&
+ git add three
+ fi &&
+ if [ $sideR = "rename" ]
+ then
+ git show file_v4 >two &&
+
[PATCH v3 6/8] merge-recursive: improve handling for rename/rename(2to1) conflicts
This makes the rename/rename(2to1) conflicts use the new
handle_file_collision() function. Since that function was based
originally on the rename/rename(2to1) handling code, the main
differences here are in what was added. In particular:
* Instead of storing files at collide_path~HEAD and collide_path~MERGE,
the files are two-way merged and recorded at collide_path.
* Instead of recording the version of the renamed file that existed
on the renamed side in the index (thus ignoring any changes that
were made to the file on the side of history without the rename),
we do a three-way content merge on the renamed path, then store
that at either stage 2 or stage 3.
* Note that since the content merge for each rename may have conflicts,
and then we have to merge the two renamed files, we can end up with
nested conflict markers.
Signed-off-by: Elijah Newren
---
merge-recursive.c| 104 ---
t/t6036-recursive-corner-cases.sh| 12 +---
t/t6042-merge-rename-corner-cases.sh | 38 ++
t/t6043-merge-rename-directories.sh | 83 -
4 files changed, 89 insertions(+), 148 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 0805095168..ead6054a75 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -696,27 +696,6 @@ static int update_stages(struct merge_options *opt, const
char *path,
return 0;
}
-static int update_stages_for_stage_data(struct merge_options *opt,
- const char *path,
- const struct stage_data *stage_data)
-{
- struct diff_filespec o, a, b;
-
- o.mode = stage_data->stages[1].mode;
- oidcpy(, _data->stages[1].oid);
-
- a.mode = stage_data->stages[2].mode;
- oidcpy(, _data->stages[2].oid);
-
- b.mode = stage_data->stages[3].mode;
- oidcpy(, _data->stages[3].oid);
-
- return update_stages(opt, path,
-is_null_oid() ? NULL : ,
-is_null_oid() ? NULL : ,
-is_null_oid() ? NULL : );
-}
-
static void update_entry(struct stage_data *entry,
struct diff_filespec *o,
struct diff_filespec *a,
@@ -1870,7 +1849,6 @@ static int handle_rename_rename_2to1(struct merge_options
*o,
char *path_side_2_desc;
struct merge_file_info mfi_c1;
struct merge_file_info mfi_c2;
- int ret;
output(o, 1, _("CONFLICT (rename/rename): "
"Rename %s->%s in %s. "
@@ -1878,81 +1856,22 @@ static int handle_rename_rename_2to1(struct
merge_options *o,
a->path, c1->path, ci->branch1,
b->path, c2->path, ci->branch2);
- remove_file(o, 1, a->path, o->call_depth ||
would_lose_untracked(a->path));
- remove_file(o, 1, b->path, o->call_depth ||
would_lose_untracked(b->path));
-
path_side_1_desc = xstrfmt("version of %s from %s", path, a->path);
path_side_2_desc = xstrfmt("version of %s from %s", path, b->path);
if (merge_mode_and_contents(o, a, c1, >ren1_other, path_side_1_desc,
o->branch1, o->branch2,
- o->call_depth * 2, _c1) ||
+ 1 + o->call_depth * 2, _c1) ||
merge_mode_and_contents(o, b, >ren2_other, c2, path_side_2_desc,
o->branch1, o->branch2,
- o->call_depth * 2, _c2))
+ 1 + o->call_depth * 2, _c2))
return -1;
free(path_side_1_desc);
free(path_side_2_desc);
- if (o->call_depth) {
- /*
-* If mfi_c1.clean && mfi_c2.clean, then it might make
-* sense to do a two-way merge of those results. But, I
-* think in all cases, it makes sense to have the virtual
-* merge base just undo the renames; they can be detected
-* again later for the non-recursive merge.
-*/
- remove_file(o, 0, path, 0);
- ret = update_file(o, 0, _c1.oid, mfi_c1.mode, a->path);
- if (!ret)
- ret = update_file(o, 0, _c2.oid, mfi_c2.mode,
- b->path);
- } else {
- char *new_path1 = unique_path(o, path, ci->branch1);
- char *new_path2 = unique_path(o, path, ci->branch2);
- output(o, 1, _("Renaming %s to %s and %s to %s instead"),
- a->path, new_path1, b->path, new_path2);
- if (was_dirty(o, path))
- output(o, 1, _("Refusing to lose dirty file at %s"),
- path);
- else if (would_lose_untracked(path))
- /*
-* Only
[PATCH v3 2/8] t6036, t6042: testcases for rename collision of already conflicting files
When a single file is renamed, it can also be modified, yielding the possibility of that renamed file having content conflicts. If two different such files are renamed into the same location, then two-way merging those files may result in nested conflicts. Add a testcase that makes sure we get this case correct, and uses different lengths of conflict markers to differentiate between the different nestings. Also add another case with an extra (i.e. third) level of conflict markers due to using merge.conflictstyle=diff3 and the virtual merge base also having conflicts present. Signed-off-by: Elijah Newren --- t/t6036-recursive-corner-cases.sh| 194 +++ t/t6042-merge-rename-corner-cases.sh | 118 2 files changed, 312 insertions(+) diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh index e1cef58f2a..78138a7fb4 100755 --- a/t/t6036-recursive-corner-cases.sh +++ b/t/t6036-recursive-corner-cases.sh @@ -1402,4 +1402,198 @@ test_expect_failure 'check conflicting modes for regular file' ' ) ' +# Setup: +# L1---L2 +# / \ / \ +# masterX? +# \ / \ / +# R1---R2 +# +# Where: +# master has two files, named 'b' and 'a' +# branches L1 and R1 both modify each of the two files in conflicting ways +# +# L2 is a merge of R1 into L1; more on it later. +# R2 is a merge of L1 into R1; more on it later. +# +# X is an auto-generated merge-base used when merging L2 and R2. +# since X is a merge of L1 and R1, it has conflicting versions of each file +# +# More about L2 and R2: +# - both resolve the conflicts in 'b' and 'a' differently +# - L2 renames 'b' to 'm' +# - R2 renames 'a' to 'm' +# +# In the end, in file 'm' we have four different conflicting files (from +# two versions of 'b' and two of 'a'). In addition, if +# merge.conflictstyle is diff3, then the base version also has +# conflict markers of its own, leading to a total of three levels of +# conflict markers. This is a pretty weird corner case, but we just want +# to ensure that we handle it as well as practical. + +test_expect_success "setup nested conflicts" ' + test_create_repo nested_conflicts && + ( + cd nested_conflicts && + + # Create some related files now + for i in $(test_seq 1 10) + do + echo Random base content line $i + done >initial && + + cp initial b_L1 && + cp initial b_R1 && + cp initial b_L2 && + cp initial b_R2 && + cp initial a_L1 && + cp initial a_R1 && + cp initial a_L2 && + cp initial a_R2 && + + test_write_lines b b_L1 >>b_L1 && + test_write_lines b b_R1 >>b_R1 && + test_write_lines b b_L2 >>b_L2 && + test_write_lines b b_R2 >>b_R2 && + test_write_lines a a_L1 >>a_L1 && + test_write_lines a a_R1 >>a_R1 && + test_write_lines a a_L2 >>a_L2 && + test_write_lines a a_R2 >>a_R2 && + + # Setup original commit (or merge-base), consisting of + # files named "b" and "a" + cp initial b && + cp initial a && + echo b >>b && + echo a >>a && + git add b a && + test_tick && git commit -m initial && + + git branch L && + git branch R && + + # Handle the left side + git checkout L && + mv -f b_L1 b && + mv -f a_L1 a && + git add b a && + test_tick && git commit -m "version L1 of files" && + git tag L1 && + + # Handle the right side + git checkout R && + mv -f b_R1 b && + mv -f a_R1 a && + git add b a && + test_tick && git commit -m "verson R1 of files" && + git tag R1 && + + # Create first merge on left side + git checkout L && + test_must_fail git merge R1 && + mv -f b_L2 b && + mv -f a_L2 a && + git add b a && + git mv b m && + test_tick && git commit -m "left merge, rename b->m" && + git tag L2 && + + # Create first merge on right side + git checkout R && + test_must_fail git merge L1 && + mv -f b_R2 b && + mv -f a_R2 a && + git add b a && + git mv a m && + test_tick && git commit -m "right merge, rename a->m" && + git tag R2 + ) +' + +test_expect_failure "check nested conflicts" ' + ( + cd nested_conflicts && + + git clean -f && +
[PATCH v3 7/8] merge-recursive: use handle_file_collision for add/add conflicts
This results in no-net change of behavior, it simply ensures that all
file-collision conflict handling types are being handled the same by
calling the same function.
Signed-off-by: Elijah Newren
---
merge-recursive.c | 29 +
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index ead6054a75..c78b347112 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3355,14 +3355,27 @@ static int process_entry(struct merge_options *o,
clean_merge = -1;
}
} else if (a_oid && b_oid) {
- /* Case C: Added in both (check for same permissions) and */
- /* case D: Modified in both, but differently. */
- int is_dirty = 0; /* unpack_trees would have bailed if dirty */
- clean_merge = handle_content_merge(o, path, is_dirty,
- o_oid, o_mode,
- a_oid, a_mode,
- b_oid, b_mode,
- NULL);
+ if (!o_oid) {
+ /* Case C: Added in both (check for same permissions) */
+ output(o, 1,
+ _("CONFLICT (add/add): Merge conflict in %s"),
+ path);
+ clean_merge = handle_file_collision(o,
+ path, NULL, NULL,
+ o->branch1,
+ o->branch2,
+ a_oid, a_mode,
+ b_oid, b_mode);
+ } else {
+ /* case D: Modified in both, but differently. */
+ int is_dirty = 0; /* unpack_trees would have bailed if
dirty */
+ clean_merge = handle_content_merge(o, path,
+ is_dirty,
+ o_oid, o_mode,
+ a_oid, a_mode,
+ b_oid, b_mode,
+ NULL);
+ }
} else if (!o_oid && !a_oid && !b_oid) {
/*
* this entry was deleted altogether. a_mode == 0 means
--
2.19.1.1079.gae8ff35a65
[PATCH v3 0/8] Improve path collision conflict resolutions
This series depends on en/merge-cleanup-more and is built on that series. (It merges cleanly to master, next, and pu). This series makes all the "file collision" conflict types be handled consistently; making them all behave like add/add (as suggested by Jonathan[1] and Junio[2]). These types are: * add/add * rename/add * rename/rename(2to1) * each rename/add piece of a rename/rename(1to2)/add[/add] conflict Changes since v2: * Removed RFC label (en/merge-cleanup-more is now pu -- in fact, v2 of that series is in pu; also, I'm starting to build other series on this one which has increased my confidence in it) * patch for increasing marker length with depth of recursion has been pulled from the en/merge-cleanup-more series and added to this series * Fixed an incorrect sentence in a commit message that was leftover from v1. [1] https://public-inbox.org/git/20180312213521.gb58...@aiede.svl.corp.google.com/ [2] https://public-inbox.org/git/capc5davu8vv9rdgon8jixeo3ycdvqq38yszzc-cpo+aqcak...@mail.gmail.com Elijah Newren (8): Add testcases for consistency in file collision conflict handling t6036, t6042: testcases for rename collision of already conflicting files merge-recursive: increase marker length with depth of recursion merge-recursive: new function for better colliding conflict resolutions merge-recursive: fix rename/add conflict handling merge-recursive: improve handling for rename/rename(2to1) conflicts merge-recursive: use handle_file_collision for add/add conflicts merge-recursive: improve rename/rename(1to2)/add[/add] handling ll-merge.c | 4 +- ll-merge.h | 1 + merge-recursive.c| 528 --- t/t6036-recursive-corner-cases.sh| 379 ++- t/t6042-merge-rename-corner-cases.sh | 333 - t/t6043-merge-rename-directories.sh | 107 +++--- 6 files changed, 1060 insertions(+), 292 deletions(-) -- 2.19.1.1036.gce51225f01.dirty
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 01:23:06PM -0400, Ben Peart wrote: > > > Okay. In any case, --no-quiet probably ought to be mentioned alongside > > > the "reset.quiet" option (and perhaps in git-reset.txt to as a way to > > > reverse "reset.quiet"). > [...] > Makes sense. I'll update the docs to say: > > -q:: > --quiet:: > --no-quiet:: > Be quiet, only report errors. > + > With --no-quiet report errors and unstaged changes after reset. I think we should be explicit that "--no-quiet" is already the default, which makes it easy to mention the config option. Something like: -q:: --quiet:: --no-quiet:: Be quiet, only report errors. The default behavior respects the `reset.quiet` config option, or `--no-quiet` if that is not set. I don't know if we need to mention the "unstaged changes" thing. We may grow other non-error messages (or may even have some now, I didn't check). But I'm OK including it, too. -Peff
Re: unused parameters in merge-recursive.c
On Fri, Oct 19, 2018 at 10:58:19AM -0700, Elijah Newren wrote: > > In most cases I've been trying to determine the "bug versus cruft" thing > > myself, but I fear that merge-recursive exceeds my abilities here. ;) > > These ones all look like cruft to me. I dug through them and tried > looking through history and old submissions for my guesses and how > they ended up here; details below. Good, that makes things easier. :) > > static int handle_rename_via_dir(struct merge_options *o, > > struct diff_filepair *pair, > > -const char *rename_branch, > > -const char *other_branch) > > +const char *rename_branch) > > Given the similarity in function signature to handle_rename_delete(), > it's possible I copied the function and then started editing. Whether > I was lazily doing that, or if I really added that parameter because I > thought I was going to add an informational message to the user that > used it, or something else, I don't know. But I agree, it's just not > needed and could be added back later if someone did find a use for it. Yeah, this was the one I was most worried about. Thanks for confirming. I'm preparing a bunch of similar cleanups, so I'll roll this into that series. -Peff
git ls-files --with-tree documentation
--with-tree= When using --error-unmatch to expand the user supplied (i.e. path pattern) arguments to paths, pretend that paths which were removed in the index since the named are still present. Using this option with -s or -u options does not make any sense. This seems to say that it only affects it when --error-unmatch is used, but in fact it goes deeper; for example I can use it to list files that are present in either the current work tree or some other branch: joey@darkstar:/tmp/v> git checkout foo joey@darkstar:/tmp/v> git ls-files --with-tree=master in-foo in-master joey@darkstar:/tmp/v> git ls-files in-foo joey@darkstar:/tmp/v> git ls-tree master 100644 blob 0242cc10fdf4e9afdfd0928c2a209d4545780168in-master This is very useful behavior, but I'm not sure if I should rely on it behaving this way in the future, given the documentation. t/t3060-ls-files-with-tree.sh does indeed test that it "should add entries from named tree", and it does it without using --error-unmatch. How about changing the documentation to something like this to make more explicit what it does. --with-tree= Treat all files in the as if they were present in the index. When using --error-unmatch to expand the user supplied (i.e. path pattern) arguments to paths, this has the effect that paths which were removed in the index since the named are still present. Using this option with -s or -u options does not make any sense. -- see shy jo
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Hi, Le 19/10/2018 à 14:46, SZEDER Gábor a écrit : > On Fri, Oct 19, 2018 at 03:02:22PM +0900, Junio C Hamano wrote: >> Two large set of topics on "rebase in C" and "rebase -i in C" are >> now in 'next'. > > I see occasional failures in 't5520-pull.sh': > > […] > > When running t5520 in a loop, it tends to fail between 10-40 > iterations, even when the machine is not under heavy load. > > It appears that these failures started with commit 5541bd5b8f (rebase: > default to using the builtin rebase, 2018-08-08), i.e. tip of > 'pk/rebase-in-c-6-final', but it's a "flip the big switch" commit, so > not very useful... > I can reproduce this. I also tried to run this specific test under valgrind, and found out that some cases I did not targeted with --valgrind-only failed. The same thing happens with t3404, which did not crash with valgrind before. Here is a log: > expecting success: > HEAD=$(git rev-parse HEAD) && > set_fake_editor && > git rebase -i -p HEAD^ && > git update-index --refresh && > git diff-files --quiet && > git diff-index --quiet --cached HEAD -- && > test $HEAD = $(git rev-parse HEAD) > > +++ git rev-parse HEAD > ++ HEAD=d2d5ba71c6d0266f26238e804f77f026984ae0d9 > ++ set_fake_editor > ++ write_script fake-editor.sh > ++ echo '#!/bin/sh' > ++ cat > ++ chmod +x fake-editor.sh > +++ pwd > ++ test_set_editor '/tmp/git-alban/trash > directory.t3404-rebase-interactive/fake-editor.sh' > ++ FAKE_EDITOR='/tmp/git-alban/trash > directory.t3404-rebase-interactive/fake-editor.sh' > ++ export FAKE_EDITOR > ++ EDITOR='"$FAKE_EDITOR"' > ++ export EDITOR > ++ git rebase -i -p 'HEAD^' > GIT_DIR='/tmp/git-alban/trash directory.t3404-rebase-interactive/.git'; > state_dir='.git/rebase-merge'; upstream_name='HEAD^'; > upstream='8f99a4f1fbbd214b25a070ad34ec5a8f833522cc'; > head_name='refs/heads/branch1'; > orig_head='d2d5ba71c6d0266f26238e804f77f026984ae0d9'; > onto='8f99a4f1fbbd214b25a070ad34ec5a8f833522cc'; onto_name='HEAD^'; unset > revisions; unset restrict_revision; GIT_QUIET=''; git_am_opt=''; verbose=''; > diffstat=''; force_rebase=''; action=''; signoff=''; > allow_rerere_autoupdate=''; keep_empty=''; autosquash=''; unset gpg_sign_opt; > unset cmd; allow_empty_message='--allow-empty-message'; rebase_merges=''; > rebase_cousins=''; unset strategy; unset strategy_opts; rebase_root=''; > squash_onto=''; git_format_patch_opt=''; . git-sh-setup && . > git-rebase--common && . git-rebase--preserve-merges && > git_rebase__preserve_merges: line 0: .: git-rebase--common: file not found > error: last command exited with $?=1 > not ok 25 - -p handles "no changes" gracefully > # > # HEAD=$(git rev-parse HEAD) && > # set_fake_editor && > # git rebase -i -p HEAD^ && > # git update-index --refresh && > # git diff-files --quiet && > # git diff-index --quiet --cached HEAD -- && > # test $HEAD = $(git rev-parse HEAD) > # This comes from 't3404-rebase-interactive.sh', with --valgrind-only set to '63'. Cheers, Alban
Re: unused parameters in merge-recursive.c
Hi Peff,
On Fri, Oct 19, 2018 at 10:18 AM Jeff King wrote:
>
> Hi Elijah,
>
> Playing with -Wunused-parameters, I notice there are several functions
> with unused parameters in merge-recursive.c. The patch below drops them
> all. We know they're not being used, so it can't make anything _worse_,
> but this is a good opportunity to confirm that they don't represent some
> latent bug.
>
> In most cases I've been trying to determine the "bug versus cruft" thing
> myself, but I fear that merge-recursive exceeds my abilities here. ;)
These ones all look like cruft to me. I dug through them and tried
looking through history and old submissions for my guesses and how
they ended up here; details below.
> ---
> merge-recursive.c | 19 +++
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index c0fb83d285..f6d634c3a2 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1369,8 +1369,7 @@ static int merge_mode_and_contents(struct merge_options
> *o,
>
> static int handle_rename_via_dir(struct merge_options *o,
> struct diff_filepair *pair,
> -const char *rename_branch,
> -const char *other_branch)
> +const char *rename_branch)
Given the similarity in function signature to handle_rename_delete(),
it's possible I copied the function and then started editing. Whether
I was lazily doing that, or if I really added that parameter because I
thought I was going to add an informational message to the user that
used it, or something else, I don't know. But I agree, it's just not
needed and could be added back later if someone did find a use for it.
> @@ -2071,8 +2070,7 @@ static void handle_directory_level_conflicts(struct
> merge_options *o,
> -static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
> -struct tree *tree)
> +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)
Yeah, nuke it. I don't remember full details here (it's been over a
year), but my best guess is that I started with (at least part of the)
code for handle_directory_level_conflicts() inside of
get_directory_renames() and then broke it out into a separate function
when it got bigger, but forgot to remove the extra argument.
> {
> struct hashmap *dir_renames;
> struct hashmap_iter iter;
> @@ -2318,8 +2316,7 @@ static void apply_directory_rename_modifications(struct
> merge_options *o,
> struct tree *o_tree,
> struct tree *a_tree,
> struct tree *b_tree,
> -struct string_list *entries,
> -int *clean)
> +struct string_list *entries)
Yeah, there's several other functions with such a flag; I probably
added it thinking this function would need it too and then just forgot
to remove it when it turned out to not be necessary.
The remaining chunks in the patch you emailed are just modifying
callers to not pass the extra non-unnecessary args, so they're all
good.
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
Am 19.10.18 um 08:02 schrieb Junio C Hamano: * js/diff-notice-has-drive-prefix (2018-10-19) 1 commit - diff: don't attempt to strip prefix from absolute Windows paths "git diff /a/b/c /a/d/f" noticed these are full paths with shared leading prefix "/a", but failed to notice a similar fact about "git diff D:/a/b/c D:/a/d/f", which has been corrected. This patch isn't about a misdetected leading prefix, but about incorrectly truncated absolute paths. How about: Under certain circumstances, "git diff D:/a/b/c D:/a/b/d" on Windows would strip initial parts from the paths because they were not recognized as absolute, which has been corrected. Want tests. I've sent v2 with a test. -- Hannes
Re: [PATCH] submodule.c: remove some of the_repository references
On Fri, Oct 19, 2018 at 10:34 AM Nguyễn Thái Ngọc Duy wrote:
>
>
> struct collect_changed_submodules_cb_data {
> + struct repository *repo;
This slightly overlaps with sb/submodule-recursive-fetch-gets-the-tip,
but we can have this patch on its own as I have to rebuild that
series, will build on top of this.
The patch looks good,
Stefan
Re: [PATCH/WIP 00/19] Kill the_index, final part
On Fri, Oct 19, 2018 at 7:52 AM Nguyễn Thái Ngọc Duy wrote: > > Stefan I see you start removing more references of the_repository in > another series (haven't really looked at it yet) so this is just heads > up so we could coordinate if needed. Still WIP, not really ready for > review yet. I'll take a brief look anyway, otherwise you wouldn't have put in on a mailing list :-P Yes, coordination would be good; stolee brought up a good point regarding testing these changes. Killing of the index in itself can be just tested by checking if behavior stays the same, but killing of the_repository is a slightly different beast, as it relates to submodules, which means any global state that is still around, need to go into the repository struct (we might have missed some there, whereas the index struct has been around for a long time). > This series removes use of the_index outside builtin/ and t/helper/. > The only the_index reference left is in repository.c. The macro > NO_THE_REPOSITORY_COMPATIBILITY_MACROS is now flipped to > USE_THE_INDEX_COMPATIBILITY_MACROS. "the_index" is forbidden by > default. Wow, that is really cool. Looking forward for the patches. :-) > After this I think we can start pushing the_repository outside library > code. Then perhaps clean them up in builtin code too and you can > finally get rid of it. But I don't think that'll happen in a year's > time. That sounds realistic. Stefan
[PATCH] submodule.c: remove some of the_repository references
Commit 174d131fc9 (submodule.c: remove implicit dependency on
the_index - 2018-09-21) makes collect_changed_submodules() take a
"struct index_state *" as argument even if it's not really used. My
bad.
Instead of deleting this argument and fixing up all call sites. Let's
take this opportunity to remove some the_repository instead because
there's one or two in this function (and two more in its callback).
The callers can also get rid of some the_repository.
Noticed-by: Jeff King
Signed-off-by: Nguyễn Thái Ngọc Duy
---
On Fri, Oct 19, 2018 at 6:55 PM Jeff King wrote:
> > > I think I'd lean toward the former, if
> > > it's not likely to be used (even if we add "struct repository" later,
> > > the separate index parameter would just go away then).
> >
> > Or we could fix it now. Something like this? It adds three the_repository
> > (two technically are already there) but deletes eight, which makes me
> > happy. It's up to you to decide.
>
> Yeah, that's fine by me, if it's not getting in the way of other related
> work. The patch you sent looks reasonable.
OK so here's the cleaned up patch and is actually tested. On top of master.
builtin/pull.c | 2 +-
submodule.c| 57 --
submodule.h| 6 +++---
transport.c| 4 ++--
4 files changed, 38 insertions(+), 31 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..c21aa276f1 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -945,7 +945,7 @@ int cmd_pull(int argc, const char **argv, const char
*prefix)
int ret = 0;
if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
- submodule_touches_in_range(_index, _fork_point,
_head))
+ submodule_touches_in_range(the_repository,
_fork_point, _head))
die(_("cannot rebase with locally recorded submodule
modifications"));
if (!autostash) {
struct commit_list *list = NULL;
diff --git a/submodule.c b/submodule.c
index d9d3046689..a2701ede4a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -694,6 +694,7 @@ static struct oid_array *submodule_commits(struct
string_list *submodules,
}
struct collect_changed_submodules_cb_data {
+ struct repository *repo;
struct string_list *changed;
const struct object_id *commit_oid;
};
@@ -733,7 +734,7 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
if (!S_ISGITLINK(p->two->mode))
continue;
- submodule = submodule_from_path(the_repository,
+ submodule = submodule_from_path(me->repo,
commit_oid, p->two->path);
if (submodule)
name = submodule->name;
@@ -741,7 +742,7 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
name = default_name_or_path(p->two->path);
/* make sure name does not collide with existing one */
if (name)
- submodule = submodule_from_name(the_repository,
+ submodule = submodule_from_name(me->repo,
commit_oid,
name);
if (submodule) {
warning("Submodule in commit %s at path: "
@@ -766,14 +767,14 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
* have a corresponding 'struct oid_array' (in the 'util' field) which lists
* what the submodule pointers were updated to during the change.
*/
-static void collect_changed_submodules(struct index_state *istate,
+static void collect_changed_submodules(struct repository *r,
struct string_list *changed,
struct argv_array *argv)
{
struct rev_info rev;
const struct commit *commit;
- repo_init_revisions(the_repository, , NULL);
+ repo_init_revisions(r, , NULL);
setup_revisions(argv->argc, argv->argv, , NULL);
if (prepare_revision_walk())
die("revision walk setup failed");
@@ -781,10 +782,11 @@ static void collect_changed_submodules(struct index_state
*istate,
while ((commit = get_revision())) {
struct rev_info diff_rev;
struct collect_changed_submodules_cb_data data;
+ data.repo = r;
data.changed = changed;
data.commit_oid = >object.oid;
- repo_init_revisions(the_repository, _rev, NULL);
+ repo_init_revisions(r, _rev, NULL);
diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
diff_rev.diffopt.format_callback =
collect_changed_submodules_cb;
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On 10/19/2018 1:11 PM, Jeff King wrote: On Fri, Oct 19, 2018 at 01:10:34PM -0400, Eric Sunshine wrote: On Fri, Oct 19, 2018 at 12:46 PM Jeff King wrote: On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: How does the user reverse this for a particular git-reset invocation? There is no --no-quiet or --verbose option. Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in builtin/reset.c and document that --verbose overrides --quiet and reset.quiet (or something like that). I think OPT__QUIET() provides --no-quiet, since it's really an OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back to 0. Okay. In any case, --no-quiet probably ought to be mentioned alongside the "reset.quiet" option (and perhaps in git-reset.txt to as a way to reverse "reset.quiet"). Yes, I'd agree with that. -Peff Makes sense. I'll update the docs to say: -q:: --quiet:: --no-quiet:: Be quiet, only report errors. + With --no-quiet report errors and unstaged changes after reset.
unused parameters in merge-recursive.c
Hi Elijah,
Playing with -Wunused-parameters, I notice there are several functions
with unused parameters in merge-recursive.c. The patch below drops them
all. We know they're not being used, so it can't make anything _worse_,
but this is a good opportunity to confirm that they don't represent some
latent bug.
In most cases I've been trying to determine the "bug versus cruft" thing
myself, but I fear that merge-recursive exceeds my abilities here. ;)
---
merge-recursive.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c0fb83d285..f6d634c3a2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1369,8 +1369,7 @@ static int merge_mode_and_contents(struct merge_options
*o,
static int handle_rename_via_dir(struct merge_options *o,
struct diff_filepair *pair,
-const char *rename_branch,
-const char *other_branch)
+const char *rename_branch)
{
/*
* Handle file adds that need to be renamed due to directory rename
@@ -2071,8 +2070,7 @@ static void handle_directory_level_conflicts(struct
merge_options *o,
remove_hashmap_entries(dir_re_merge, _from_merge);
}
-static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
-struct tree *tree)
+static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)
{
struct hashmap *dir_renames;
struct hashmap_iter iter;
@@ -2318,8 +2316,7 @@ static void apply_directory_rename_modifications(struct
merge_options *o,
struct tree *o_tree,
struct tree *a_tree,
struct tree *b_tree,
-struct string_list *entries,
-int *clean)
+struct string_list *entries)
{
struct string_list_item *item;
int stage = (tree == a_tree ? 2 : 3);
@@ -2490,8 +2487,7 @@ static struct string_list *get_renames(struct
merge_options *o,
apply_directory_rename_modifications(o, pair, new_path,
re, tree, o_tree,
a_tree, b_tree,
-entries,
-clean_merge);
+entries);
}
hashmap_iter_init(, );
@@ -2826,8 +2822,8 @@ static int detect_and_process_renames(struct
merge_options *o,
merge_pairs = get_diffpairs(o, common, merge);
if (o->detect_directory_renames) {
- dir_re_head = get_directory_renames(head_pairs, head);
- dir_re_merge = get_directory_renames(merge_pairs, merge);
+ dir_re_head = get_directory_renames(head_pairs);
+ dir_re_merge = get_directory_renames(merge_pairs);
handle_directory_level_conflicts(o,
dir_re_head, head,
@@ -3149,8 +3145,7 @@ static int process_entry(struct merge_options *o,
clean_merge = 1;
if (handle_rename_via_dir(o,
conflict_info->pair1,
- conflict_info->branch1,
- conflict_info->branch2))
+ conflict_info->branch1))
clean_merge = -1;
break;
case RENAME_DELETE:
--
2.19.1.1089.g69f65ee796
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On 10/19/2018 12:46 PM, Jeff King wrote: On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: On Fri, Oct 19, 2018 at 12:12 PM Ben Peart wrote: Add a reset.quiet config setting that sets the default value of the --quiet flag when running the reset command. This enables users to change the default behavior to take advantage of the performance advantages of avoiding the scan for unstaged changes after reset. Defaults to false. Signed-off-by: Ben Peart --- diff --git a/Documentation/config.txt b/Documentation/config.txt @@ -2728,6 +2728,9 @@ rerere.enabled:: +reset.quiet:: + When set to true, 'git reset' will default to the '--quiet' option. How does the user reverse this for a particular git-reset invocation? There is no --no-quiet or --verbose option. Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in builtin/reset.c and document that --verbose overrides --quiet and reset.quiet (or something like that). I think OPT__QUIET() provides --no-quiet, since it's really an OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back to 0. Thanks Peff. That is correct as confirmed by: C:\Repos\VSO\src>git reset --no-quiet Unstaged changes after reset: M init.ps1 It took 6.74 seconds to enumerate unstaged changes after reset. You can use '--quiet' to avoid this. Set the config setting reset.quiet to true to make this the default. -Peff
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 01:10:34PM -0400, Eric Sunshine wrote: > On Fri, Oct 19, 2018 at 12:46 PM Jeff King wrote: > > On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: > > > How does the user reverse this for a particular git-reset invocation? > > > There is no --no-quiet or --verbose option. > > > > > > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in > > > builtin/reset.c and document that --verbose overrides --quiet and > > > reset.quiet (or something like that). > > > > I think OPT__QUIET() provides --no-quiet, since it's really an > > OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back > > to 0. > > Okay. In any case, --no-quiet probably ought to be mentioned alongside > the "reset.quiet" option (and perhaps in git-reset.txt to as a way to > reverse "reset.quiet"). Yes, I'd agree with that. -Peff
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 12:46 PM Jeff King wrote: > On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: > > How does the user reverse this for a particular git-reset invocation? > > There is no --no-quiet or --verbose option. > > > > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in > > builtin/reset.c and document that --verbose overrides --quiet and > > reset.quiet (or something like that). > > I think OPT__QUIET() provides --no-quiet, since it's really an > OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back > to 0. Okay. In any case, --no-quiet probably ought to be mentioned alongside the "reset.quiet" option (and perhaps in git-reset.txt to as a way to reverse "reset.quiet").
Re: [PATCH v2] diff: don't attempt to strip prefix from absolute Windows paths
> +test_expect_success 'diff --no-index from repo subdir with absolute paths' ' I was late looking at the test, and was about to propose to guard it to run only on Windows (as this test seems of little use in other OS), but after thinking about it I think we should keep it as-is, as there may be other OS that have interesting absolute path which I may be unaware of. Reviewed-by: Stefan Beller
[PATCH v2] diff: don't attempt to strip prefix from absolute Windows paths
git diff can be invoked with absolute paths. Typically, this triggers
the --no-index case. Then the absolute paths remain in the file names
that are printed in the output.
There is one peculiarity, though: When the command is invoked from a
a sub-directory in a repository, then it is attempted to strip the
sub-directory from the beginning of relative paths. Yet, to detect a
relative path the code just checks for an initial forward slash.
This mistakes a Windows style path like "D:/base" as a relative path
and the output looks like this, for example:
D:\dir\test\one>git -P diff --numstat D:\dir\base D:\dir\diff
1 1 ir/{base => diff}/1.txt
where the correct output should be
D:\dir\test\one>git -P diff --numstat D:\dir\base D:\dir\diff
1 1 D:/dir/{base => diff}/1.txt
If the sub-directory where 'git diff' is invoked is sufficiently deep
that the prefix becomes longer than the path to be printed, then the
subsequent code accesses the path out of bounds.
Use is_absolute_path() to detect Windows style absolute paths.
One might wonder whether the check for a directory separator that
is visible in the patch context should be changed from == '/' to
is_dir_sep() or not. It turns out not to be necessary. That code
only ever investigates paths that have undergone pathspec
normalization, after which there are only forward slashes even on
Windows.
Signed-off-by: Johannes Sixt
---
v2:
- added a test that demonstrates the problem on Windows
- changed the example in the commit message to clarify that this is
about truncated paths, not about failure to detect common prefix
diff.c | 4 ++--
t/t4053-diff-no-index.sh | 10 ++
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/diff.c b/diff.c
index f0c7557b40..d18eb198f2 100644
--- a/diff.c
+++ b/diff.c
@@ -4267,12 +4267,12 @@ static void diff_fill_oid_info(struct diff_filespec
*one)
static void strip_prefix(int prefix_length, const char **namep, const char
**otherp)
{
/* Strip the prefix but do not molest /dev/null and absolute paths */
- if (*namep && **namep != '/') {
+ if (*namep && !is_absolute_path(*namep)) {
*namep += prefix_length;
if (**namep == '/')
++*namep;
}
- if (*otherp && **otherp != '/') {
+ if (*otherp && !is_absolute_path(*otherp)) {
*otherp += prefix_length;
if (**otherp == '/')
++*otherp;
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 453e6c35eb..6e0dd6f9e5 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -127,4 +127,14 @@ test_expect_success 'diff --no-index from repo subdir
respects config (implicit)
test_cmp expect actual.head
'
+test_expect_success 'diff --no-index from repo subdir with absolute paths' '
+ cat <<-EOF >expect &&
+ 1 1 $(pwd)/non/git/{a => b}
+ EOF
+ test_expect_code 1 \
+ git -C repo/sub diff --numstat \
+ "$(pwd)/non/git/a" "$(pwd)/non/git/b" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.19.1.406.g1aa3f475f3
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
> > > > Yeah. Killing the_index is just the first small step (didn't look that > > small when I started). Now it's all about the_repository ;-) and we > > have like 400 references of it just in library code. > > I suspect it is much worse than that, even. There are many spots that > likely are relying on global data that _should_ be in the repository > struct but aren't yet. I don't think there's even an easy way to count > those. ;) This is a very interesting part of the discussion, please note the series "RFC Bring the_repository into cmd_foo"[1] as that proposes one way how to deal with this, exposing the repository/index changes into the existing test suite. [1] https://public-inbox.org/git/20181018183758.81186-1-sbel...@google.com/ I'll read on.
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
On Fri, Oct 19, 2018 at 6:42 PM Jeff King wrote: > > > I don't see anything in the "Kill the_index, final part" series that > > > would need it. > > > > Yeah. Killing the_index is just the first small step (didn't look that > > small when I started). Now it's all about the_repository ;-) and we > > have like 400 references of it just in library code. > > I suspect it is much worse than that, even. There are many spots that > likely are relying on global data that _should_ be in the repository > struct but aren't yet. I don't think there's even an easy way to count > those. ;) One step at a time. I'm already in the process of killing some global vars related to setup code and was feeling very risky. Don't scare me away with all other potential global data! -- Duy
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
On Fri, Oct 19, 2018 at 06:53:01PM +0200, Duy Nguyen wrote: > > OK, that all makes sense. I need to either drop it or mark it unused to > > satisfy -Wunused-parameters. > > Aha! I thought you looked deeply into this and was very confused. How > could you have found the time :D To be fair, dealing with -Wunused-parameters is its own special time sink. :) > > I think I'd lean toward the former, if > > it's not likely to be used (even if we add "struct repository" later, > > the separate index parameter would just go away then). > > Or we could fix it now. Something like this? It adds three the_repository > (two technically are already there) but deletes eight, which makes me > happy. It's up to you to decide. Yeah, that's fine by me, if it's not getting in the way of other related work. The patch you sent looks reasonable. -Peff
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
On Fri, Oct 19, 2018 at 12:42:38PM -0400, Jeff King wrote:
> On Fri, Oct 19, 2018 at 06:33:30PM +0200, Duy Nguyen wrote:
>
> > > This function doesn't actually make use of the newly added "istate" (nor
> > > does it use the_index, so there's not a spot that forgot to get
> > > converted). Is this in preparation for more refactoring, or is it just a
> > > mistake and can be dropped?
> >
> > It's possible that it was needed at some point when I converted diff
> > api to pass index_state around. But in later iterations I think I
> > passed "struct repository *" instead because diff code needed much
> > more than the index, but did not clean this up. Sorry.
> >
> > In this function, we still have the_repository for
> > repo_init_revisions() and it should be gone eventually. "struct
> > repository *r" could take the place of "struct index_state *istate"
> > instead and we would need to reopen the path again.
> >
> > So I'm not sure, if it's really bad, we could remove it now. Otherwise
> > we could just leave it there, I don't think this "istate" will survive
> > very long. I already started deleting some of the_repository in "kill
> > the_index" series.
>
> OK, that all makes sense. I need to either drop it or mark it unused to
> satisfy -Wunused-parameters.
Aha! I thought you looked deeply into this and was very confused. How
could you have found the time :D
> I think I'd lean toward the former, if
> it's not likely to be used (even if we add "struct repository" later,
> the separate index parameter would just go away then).
Or we could fix it now. Something like this? It adds three the_repository
(two technically are already there) but deletes eight, which makes me
happy. It's up to you to decide.
-- 8< --
diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..c21aa276f1 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -945,7 +945,7 @@ int cmd_pull(int argc, const char **argv, const char
*prefix)
int ret = 0;
if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
- submodule_touches_in_range(_index, _fork_point,
_head))
+ submodule_touches_in_range(the_repository,
_fork_point, _head))
die(_("cannot rebase with locally recorded submodule
modifications"));
if (!autostash) {
struct commit_list *list = NULL;
diff --git a/submodule.c b/submodule.c
index d9d3046689..977c050668 100644
--- a/submodule.c
+++ b/submodule.c
@@ -694,6 +694,7 @@ static struct oid_array *submodule_commits(struct
string_list *submodules,
}
struct collect_changed_submodules_cb_data {
+ struct repository *repo;
struct string_list *changed;
const struct object_id *commit_oid;
};
@@ -733,7 +734,7 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
if (!S_ISGITLINK(p->two->mode))
continue;
- submodule = submodule_from_path(the_repository,
+ submodule = submodule_from_path(me->repo,
commit_oid, p->two->path);
if (submodule)
name = submodule->name;
@@ -741,7 +742,7 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
name = default_name_or_path(p->two->path);
/* make sure name does not collide with existing one */
if (name)
- submodule = submodule_from_name(the_repository,
+ submodule = submodule_from_name(me->repo,
commit_oid,
name);
if (submodule) {
warning("Submodule in commit %s at path: "
@@ -766,14 +767,14 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
* have a corresponding 'struct oid_array' (in the 'util' field) which lists
* what the submodule pointers were updated to during the change.
*/
-static void collect_changed_submodules(struct index_state *istate,
+static void collect_changed_submodules(struct repository *r,
struct string_list *changed,
struct argv_array *argv)
{
struct rev_info rev;
const struct commit *commit;
- repo_init_revisions(the_repository, , NULL);
+ repo_init_revisions(r, , NULL);
setup_revisions(argv->argc, argv->argv, , NULL);
if (prepare_revision_walk())
die("revision walk setup failed");
@@ -781,10 +782,11 @@ static void collect_changed_submodules(struct index_state
*istate,
while ((commit = get_revision())) {
struct rev_info diff_rev;
struct collect_changed_submodules_cb_data data;
+
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 12:36:44PM -0400, Eric Sunshine wrote: > On Fri, Oct 19, 2018 at 12:12 PM Ben Peart wrote: > > Add a reset.quiet config setting that sets the default value of the --quiet > > flag when running the reset command. This enables users to change the > > default behavior to take advantage of the performance advantages of > > avoiding the scan for unstaged changes after reset. Defaults to false. > > > > Signed-off-by: Ben Peart > > --- > > diff --git a/Documentation/config.txt b/Documentation/config.txt > > @@ -2728,6 +2728,9 @@ rerere.enabled:: > > +reset.quiet:: > > + When set to true, 'git reset' will default to the '--quiet' option. > > How does the user reverse this for a particular git-reset invocation? > There is no --no-quiet or --verbose option. > > Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in > builtin/reset.c and document that --verbose overrides --quiet and > reset.quiet (or something like that). I think OPT__QUIET() provides --no-quiet, since it's really an OPT_COUNTUP() under the hood. Saying "--no-quiet" should reset it back to 0. -Peff
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
On Fri, Oct 19, 2018 at 06:33:30PM +0200, Duy Nguyen wrote: > > This function doesn't actually make use of the newly added "istate" (nor > > does it use the_index, so there's not a spot that forgot to get > > converted). Is this in preparation for more refactoring, or is it just a > > mistake and can be dropped? > > It's possible that it was needed at some point when I converted diff > api to pass index_state around. But in later iterations I think I > passed "struct repository *" instead because diff code needed much > more than the index, but did not clean this up. Sorry. > > In this function, we still have the_repository for > repo_init_revisions() and it should be gone eventually. "struct > repository *r" could take the place of "struct index_state *istate" > instead and we would need to reopen the path again. > > So I'm not sure, if it's really bad, we could remove it now. Otherwise > we could just leave it there, I don't think this "istate" will survive > very long. I already started deleting some of the_repository in "kill > the_index" series. OK, that all makes sense. I need to either drop it or mark it unused to satisfy -Wunused-parameters. I think I'd lean toward the former, if it's not likely to be used (even if we add "struct repository" later, the separate index parameter would just go away then). > > I don't see anything in the "Kill the_index, final part" series that > > would need it. > > Yeah. Killing the_index is just the first small step (didn't look that > small when I started). Now it's all about the_repository ;-) and we > have like 400 references of it just in library code. I suspect it is much worse than that, even. There are many spots that likely are relying on global data that _should_ be in the repository struct but aren't yet. I don't think there's even an easy way to count those. ;) -Peff
Re: [PATCH v2 2/3] reset: add new reset.quiet config setting
On Fri, Oct 19, 2018 at 12:12 PM Ben Peart wrote: > Add a reset.quiet config setting that sets the default value of the --quiet > flag when running the reset command. This enables users to change the > default behavior to take advantage of the performance advantages of > avoiding the scan for unstaged changes after reset. Defaults to false. > > Signed-off-by: Ben Peart > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > @@ -2728,6 +2728,9 @@ rerere.enabled:: > +reset.quiet:: > + When set to true, 'git reset' will default to the '--quiet' option. How does the user reverse this for a particular git-reset invocation? There is no --no-quiet or --verbose option. Perhaps you want to use OPT__VERBOSITY() instead of OPT__QUIET() in builtin/reset.c and document that --verbose overrides --quiet and reset.quiet (or something like that).
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
On Fri, Oct 19, 2018 at 6:20 PM Jeff King wrote:
>
> On Mon, Sep 03, 2018 at 08:09:27PM +0200, Nguyễn Thái Ngọc Duy wrote:
>
> > diff --git a/submodule.c b/submodule.c
> > index 50cbf5f13e..c0c1224760 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -766,7 +766,8 @@ static void collect_changed_submodules_cb(struct
> > diff_queue_struct *q,
> > * have a corresponding 'struct oid_array' (in the 'util' field) which
> > lists
> > * what the submodule pointers were updated to during the change.
> > */
> > -static void collect_changed_submodules(struct string_list *changed,
> > +static void collect_changed_submodules(struct index_state *istate,
> > +struct string_list *changed,
> > struct argv_array *argv)
> > {
> > struct rev_info rev;
>
> This function doesn't actually make use of the newly added "istate" (nor
> does it use the_index, so there's not a spot that forgot to get
> converted). Is this in preparation for more refactoring, or is it just a
> mistake and can be dropped?
It's possible that it was needed at some point when I converted diff
api to pass index_state around. But in later iterations I think I
passed "struct repository *" instead because diff code needed much
more than the index, but did not clean this up. Sorry.
In this function, we still have the_repository for
repo_init_revisions() and it should be gone eventually. "struct
repository *r" could take the place of "struct index_state *istate"
instead and we would need to reopen the path again.
So I'm not sure, if it's really bad, we could remove it now. Otherwise
we could just leave it there, I don't think this "istate" will survive
very long. I already started deleting some of the_repository in "kill
the_index" series.
> I don't see anything in the "Kill the_index, final part" series that
> would need it.
Yeah. Killing the_index is just the first small step (didn't look that
small when I started). Now it's all about the_repository ;-) and we
have like 400 references of it just in library code.
> If this can be dropped, then most of the other changes here can, too,
> because they're just pushing the unused parameter down the stack. I.e.:
--
Duy
[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them
At a checkpoint, fast-import resets all branches and tags on disk to the
OID that we have in memory. If --force is not given, only branch resets
that result in fast forwards with respect to the state on disk are
allowed.
With this approach, even for refs that fast-import has not been
commanded to change for a long time (or ever), at each checkpoint, we
will systematically reset them to the last state this process knows
about, a state which may have been set before the previous checkpoint.
Any changes made to these refs by a different process since the last
checkpoint will be overwritten.
1> is one process, 2> is another:
1> $ git fast-import --force
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master $B
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point master points again to $A.
This problem is mitigated somewhat for branches when --force is not
specified (the default), as requiring a fast forward means in practice
that concurrent external changes are likely to be preserved. But it's
not foolproof either:
1> $ git fast-import
1> reset refs/heads/master
1> from $A
1> checkpoint
2> $ git branch -f refs/heads/master refs/heads/master~1
1> reset refs/heads/tip
1> from $C
1> checkpoint
At this point, master points again to $A, not $A~1 as requested by the
second process.
We now keep track of whether branches and tags have been changed by this
fast-import process since our last checkpoint (or startup). At the next
checkpoint, only refs and tags that new commands have changed are
written to disk.
---
fast-import.c | 14 +++
t/t9300-fast-import.sh | 53 ++
2 files changed, 67 insertions(+)
Please let me know what you think of the change itself, before I spend
some quality time bashing out a way to avoid using sleep(1) in the
tests.
Thanks.
diff --git a/fast-import.c b/fast-import.c
index 95600c78e048..d25be5c83110 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -250,6 +250,7 @@ struct branch {
uintmax_t num_notes;
unsigned active : 1;
unsigned delete : 1;
+ unsigned changed : 1;
unsigned pack_id : PACK_ID_BITS;
struct object_id oid;
};
@@ -257,6 +258,7 @@ struct branch {
struct tag {
struct tag *next_tag;
const char *name;
+ unsigned changed : 1;
unsigned int pack_id;
struct object_id oid;
};
@@ -728,6 +730,7 @@ static struct branch *new_branch(const char *name)
b->branch_tree.versions[1].mode = S_IFDIR;
b->num_notes = 0;
b->active = 0;
+ b->changed = 0;
b->pack_id = MAX_PACK_ID;
branch_table[hc] = b;
branch_count++;
@@ -1715,6 +1718,10 @@ static int update_branch(struct branch *b)
struct object_id old_oid;
struct strbuf err = STRBUF_INIT;
+ if (!b->changed)
+ return 0;
+ b->changed = 0;
+
if (is_null_oid(>oid)) {
if (b->delete)
delete_ref(NULL, b->name, NULL, 0);
@@ -1780,6 +1787,10 @@ static void dump_tags(void)
goto cleanup;
}
for (t = first_tag; t; t = t->next_tag) {
+ if (!t->changed)
+ continue;
+ t->changed = 0;
+
strbuf_reset(_name);
strbuf_addf(_name, "refs/tags/%s", t->name);
@@ -2813,6 +2824,7 @@ static void parse_new_commit(const char *arg)
if (!store_object(OBJ_COMMIT, _data, NULL, >oid, next_mark))
b->pack_id = pack_id;
+ b->changed = 1;
b->last_commit = object_count_by_type[OBJ_COMMIT];
}
@@ -2894,6 +2906,7 @@ static void parse_new_tag(const char *arg)
t->pack_id = MAX_PACK_ID;
else
t->pack_id = pack_id;
+ t->changed = 1;
}
static void parse_reset_branch(const char *arg)
@@ -2914,6 +2927,7 @@ static void parse_reset_branch(const char *arg)
b = new_branch(arg);
read_next_command();
parse_from(b);
+ b->changed = 1;
if (command_buf.len > 0)
unread_command_buf = 1;
}
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 40fe7e49767a..548994dfbeb3 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3123,6 +3123,9 @@ test_expect_success 'U: validate root delete result' '
# The commands in input_file should not produce any output on the file
# descriptor set with --cat-blob-fd (or stdout if unspecified).
+#
+# If input_file2 is specified, sleep 2 seconds before writing it to fast-import
+# stdin.
#
# To make sure you're observing the side effects of checkpoint *before*
# fast-import terminates (and thus writes out its state), check that the
@@ -3131,6 +3134,7 @@ test_expect_success 'U: validate root delete result' '
Re: [PATCH v2 19/24] submodule.c: remove implicit dependency on the_index
On Mon, Sep 03, 2018 at 08:09:27PM +0200, Nguyễn Thái Ngọc Duy wrote:
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13e..c0c1224760 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -766,7 +766,8 @@ static void collect_changed_submodules_cb(struct
> diff_queue_struct *q,
> * have a corresponding 'struct oid_array' (in the 'util' field) which lists
> * what the submodule pointers were updated to during the change.
> */
> -static void collect_changed_submodules(struct string_list *changed,
> +static void collect_changed_submodules(struct index_state *istate,
> +struct string_list *changed,
> struct argv_array *argv)
> {
> struct rev_info rev;
This function doesn't actually make use of the newly added "istate" (nor
does it use the_index, so there's not a spot that forgot to get
converted). Is this in preparation for more refactoring, or is it just a
mistake and can be dropped?
I don't see anything in the "Kill the_index, final part" series that
would need it.
If this can be dropped, then most of the other changes here can, too,
because they're just pushing the unused parameter down the stack. I.e.:
diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..49e6f87b31 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -945,7 +945,7 @@ int cmd_pull(int argc, const char **argv, const char
*prefix)
int ret = 0;
if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
- submodule_touches_in_range(_index, _fork_point,
_head))
+ submodule_touches_in_range(_fork_point, _head))
die(_("cannot rebase with locally recorded submodule
modifications"));
if (!autostash) {
struct commit_list *list = NULL;
diff --git a/submodule.c b/submodule.c
index 5635dbda13..55298a4dab 100644
--- a/submodule.c
+++ b/submodule.c
@@ -767,8 +767,7 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
* have a corresponding 'struct oid_array' (in the 'util' field) which lists
* what the submodule pointers were updated to during the change.
*/
-static void collect_changed_submodules(struct index_state *UNUSED(istate),
- struct string_list *changed,
+static void collect_changed_submodules(struct string_list *changed,
struct argv_array *argv)
{
struct rev_info rev;
@@ -933,8 +932,7 @@ static int submodule_needs_pushing(const char *path, struct
oid_array *commits)
return 0;
}
-int find_unpushed_submodules(struct index_state *istate,
-struct oid_array *commits,
+int find_unpushed_submodules(struct oid_array *commits,
const char *remotes_name,
struct string_list *needs_pushing)
{
@@ -948,7 +946,7 @@ int find_unpushed_submodules(struct index_state *istate,
argv_array_push(, "--not");
argv_array_pushf(, "--remotes=%s", remotes_name);
- collect_changed_submodules(istate, , );
+ collect_changed_submodules(, );
for_each_string_list_item(name, ) {
struct oid_array *commits = name->util;
@@ -1049,8 +1047,7 @@ static void submodule_push_check(const char *path, const
char *head,
die("process for submodule '%s' failed", path);
}
-int push_unpushed_submodules(struct index_state *istate,
-struct oid_array *commits,
+int push_unpushed_submodules(struct oid_array *commits,
const struct remote *remote,
const struct refspec *rs,
const struct string_list *push_options,
@@ -1059,7 +1056,7 @@ int push_unpushed_submodules(struct index_state *istate,
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
- if (!find_unpushed_submodules(istate, commits,
+ if (!find_unpushed_submodules(commits,
remote->name, _pushing))
return 1;
@@ -1118,7 +1115,7 @@ void check_for_new_submodule_commits(struct object_id
*oid)
oid_array_append(_tips_after_fetch, oid);
}
-static void calculate_changed_submodule_paths(struct index_state *istate)
+static void calculate_changed_submodule_paths(void)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1139,7 +1136,7 @@ static void calculate_changed_submodule_paths(struct
index_state *istate)
* Collect all submodules (whether checked out or not) for which new
* commits have been recorded upstream in "changed_submodule_names".
*/
- collect_changed_submodules(istate, _submodules, );
+
[PATCH v2 1/3] reset: don't compute unstaged changes after reset when --quiet
From: Ben Peart
When git reset is run with the --quiet flag, don't bother finding any
additional unstaged changes as they won't be output anyway. This speeds up
the git reset command by avoiding having to lstat() every file looking for
changes that aren't going to be reported anyway.
The savings can be significant. In a repo with 200K files "git reset"
drops from 7.16 seconds to 0.32 seconds for a savings of 96%.
Signed-off-by: Ben Peart
---
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..04f0d9b4f5 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -375,7 +375,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
int flags = quiet ? REFRESH_QUIET :
REFRESH_IN_PORCELAIN;
if (read_from_tree(, , intent_to_add))
return 1;
- if (get_git_work_tree())
+ if (!quiet && get_git_work_tree())
refresh_index(_index, flags, NULL, NULL,
_("Unstaged changes after
reset:"));
} else {
--
2.18.0.windows.1
[PATCH v2 3/3] reset: warn when refresh_index() takes more than 2 seconds
From: Ben Peart
refresh_index() is done after a reset command as an optimization. Because
it can be an expensive call, warn the user if it takes more than 2 seconds
and tell them how to avoid it using the --quiet command line option or
reset.quiet config setting.
Signed-off-by: Ben Peart
---
builtin/reset.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/builtin/reset.c b/builtin/reset.c
index 3b43aee544..d95a27d52e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -25,6 +25,8 @@
#include "submodule.h"
#include "submodule-config.h"
+#define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
+
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q]
[]"),
N_("git reset [-q] [] [--] ..."),
@@ -376,9 +378,19 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
int flags = quiet ? REFRESH_QUIET :
REFRESH_IN_PORCELAIN;
if (read_from_tree(, , intent_to_add))
return 1;
- if (!quiet && get_git_work_tree())
+ if (!quiet && get_git_work_tree()) {
+ uint64_t t_begin, t_delta_in_ms;
+
+ t_begin = getnanotime();
refresh_index(_index, flags, NULL, NULL,
_("Unstaged changes after
reset:"));
+ t_delta_in_ms = (getnanotime() - t_begin) /
100;
+ if (t_delta_in_ms >
REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+ printf(_("\nIt took %.2f seconds to
enumerate unstaged changes after reset. You can\n"
+ "use '--quiet' to avoid this.
Set the config setting reset.quiet to true\n"
+ "to make this the default."),
t_delta_in_ms / 1000.0);
+ }
+ }
} else {
int err = reset_index(, reset_type, quiet);
if (reset_type == KEEP && !err)
--
2.18.0.windows.1
[PATCH v2 2/3] reset: add new reset.quiet config setting
From: Ben Peart
Add a reset.quiet config setting that sets the default value of the --quiet
flag when running the reset command. This enables users to change the
default behavior to take advantage of the performance advantages of
avoiding the scan for unstaged changes after reset. Defaults to false.
Signed-off-by: Ben Peart
---
Documentation/config.txt | 3 +++
builtin/reset.c | 1 +
2 files changed, 4 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f6f4c21a54..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,6 +2728,9 @@ rerere.enabled::
`$GIT_DIR`, e.g. if "rerere" was previously used in the
repository.
+reset.quiet::
+ When set to true, 'git reset' will default to the '--quiet' option.
+
include::sendemail-config.txt[]
sequence.editor::
diff --git a/builtin/reset.c b/builtin/reset.c
index 04f0d9b4f5..3b43aee544 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
};
git_config(git_reset_config, NULL);
+ git_config_get_bool("reset.quiet", );
argc = parse_options(argc, argv, prefix, options, git_reset_usage,
PARSE_OPT_KEEP_DASHDASH);
--
2.18.0.windows.1
[PATCH v2 0/3] speed up git reset
From: Ben Peart
This itteration avoids the refresh_index() call completely if 'quiet'.
The advantage of this is that "git refresh" without any pathspec is also
significantly sped up.
Also added a notification if finding unstaged changes after reset takes
longer than 2 seconds to make users aware of the option to speed it up if
they don't need the unstaged changes after reset to be output.
It also renames the new config setting reset.quietDefault to reset.quiet.
Base Ref:
Web-Diff: https://github.com/benpeart/git/commit/50d3415ef1
Checkout: git fetch https://github.com/benpeart/git reset-refresh-index-v2 &&
git checkout 50d3415ef1
### Interdiff (v1..v2):
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a5cf4c019b..a2d1b8b116 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2728,11 +2728,8 @@ rerere.enabled::
`$GIT_DIR`, e.g. if "rerere" was previously used in the
repository.
-reset.quietDefault::
- Sets the default value of the "quiet" option for the reset command.
- Choosing "quiet" can optimize the performance of the reset command
- by avoiding the scan of all files in the repo looking for additional
- unstaged changes. Defaults to false.
+reset.quiet::
+ When set to true, 'git reset' will default to the '--quiet' option.
include::sendemail-config.txt[]
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 8610309b55..1d697d9962 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -95,9 +95,7 @@ OPTIONS
-q::
--quiet::
- Be quiet, only report errors. Can optimize the performance of reset
- by avoiding scaning all files in the repo looking for additional
- unstaged changes.
+ Be quiet, only report errors.
EXAMPLES
diff --git a/builtin/reset.c b/builtin/reset.c
index 7d151d48a0..d95a27d52e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -25,6 +25,8 @@
#include "submodule.h"
#include "submodule-config.h"
+#define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
+
static const char * const git_reset_usage[] = {
N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q]
[]"),
N_("git reset [-q] [] [--] ..."),
@@ -306,7 +308,7 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
};
git_config(git_reset_config, NULL);
- git_config_get_bool("reset.quietDefault", );
+ git_config_get_bool("reset.quiet", );
argc = parse_options(argc, argv, prefix, options, git_reset_usage,
PARSE_OPT_KEEP_DASHDASH);
@@ -376,9 +378,19 @@ int cmd_reset(int argc, const char **argv, const char
*prefix)
int flags = quiet ? REFRESH_QUIET :
REFRESH_IN_PORCELAIN;
if (read_from_tree(, , intent_to_add))
return 1;
- if (get_git_work_tree())
- refresh_index(_index, flags, quiet ?
: NULL, NULL,
+ if (!quiet && get_git_work_tree()) {
+ uint64_t t_begin, t_delta_in_ms;
+
+ t_begin = getnanotime();
+ refresh_index(_index, flags, NULL, NULL,
_("Unstaged changes after
reset:"));
+ t_delta_in_ms = (getnanotime() - t_begin) /
100;
+ if (t_delta_in_ms >
REFRESH_INDEX_DELAY_WARNING_IN_MS) {
+ printf(_("\nIt took %.2f seconds to
enumerate unstaged changes after reset. You can\n"
+ "use '--quiet' to avoid this.
Set the config setting reset.quiet to true\n"
+ "to make this the default."),
t_delta_in_ms / 1000.0);
+ }
+ }
} else {
int err = reset_index(, reset_type, quiet);
if (reset_type == KEEP && !err)
### Patches
Ben Peart (3):
reset: don't compute unstaged changes after reset when --quiet
reset: add new reset.quiet config setting
reset: warn when refresh_index() takes more than 2 seconds
Documentation/config.txt | 3 +++
builtin/reset.c | 15 ++-
2 files changed, 17 insertions(+), 1 deletion(-)
base-commit: ca63497355222acefcca02b9cbb540a4768f3286
--
2.18.0.windows.1
Re: [PATCH v8 7/7] read-cache: load cache entries on worker threads
On Wed, Oct 10, 2018 at 11:59:38AM -0400, Ben Peart wrote:
> +static unsigned long load_cache_entries_threaded(struct index_state *istate,
> const char *mmap, size_t mmap_size,
> + unsigned long src_offset, int nr_threads, struct
> index_entry_offset_table *ieot)
The src_offset parameter isn't used in this function.
In early versions of the series, it was used to feed the p->start_offset
field of each load_cache_entries_thread_data. But after the switch to
ieot, we don't, and instead feed p->ieot_start. But we always begin that
at 0.
Is that right (and we can drop the parameter), or should this logic:
> + offset = ieot_start = 0;
> + ieot_blocks = DIV_ROUND_UP(ieot->nr, nr_threads);
> + for (i = 0; i < nr_threads; i++) {
> [...]
be starting at src_offset instead of 0?
-Peff
Re: No --no-gpg-sign option with "git rebase"
Hi Luca,
On Thu, 18 Oct 2018, Luca Weiss wrote:
> See subject, would be quite useful to have this. ("Countermand commit.gpgSign
> configuration variable that is set to force each and every commit to be
> signed.")
Have you tried the built-in rebase? It's already available in `next`, and
it is available in Git for Windows as an experimental feature.
Ciao,
Johannes
[PATCH 10/19] checkout: avoid the_index when possible
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/checkout.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..38b28c20e2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -284,7 +284,7 @@ static int checkout_paths(const struct checkout_opts *opts,
return run_add_interactive(revision, "--patch=checkout",
>pathspec);
- hold_locked_index(_file, LOCK_DIE_ON_ERROR);
+ repo_hold_locked_index(the_repository, _file, LOCK_DIE_ON_ERROR);
if (read_cache_preload(>pathspec) < 0)
return error(_("index file corrupt"));
--
2.19.1.647.g708186aaf9
[PATCH 16/19] merge-recursive.c: remove implicit dependency on the_repository
Signed-off-by: Nguyễn Thái Ngọc Duy
---
merge-recursive.c | 45 -
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 184945ae67..ad0a5b0202 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -146,7 +146,8 @@ static int err(struct merge_options *o, const char *err,
...)
return -1;
}
-static struct tree *shift_tree_object(struct tree *one, struct tree *two,
+static struct tree *shift_tree_object(struct repository *repo,
+ struct tree *one, struct tree *two,
const char *subtree_shift)
{
struct object_id shifted;
@@ -159,12 +160,14 @@ static struct tree *shift_tree_object(struct tree *one,
struct tree *two,
}
if (oideq(>object.oid, ))
return two;
- return lookup_tree(the_repository, );
+ return lookup_tree(repo, );
}
-static struct commit *make_virtual_commit(struct tree *tree, const char
*comment)
+static struct commit *make_virtual_commit(struct repository *repo,
+ struct tree *tree,
+ const char *comment)
{
- struct commit *commit = alloc_commit_node(the_repository);
+ struct commit *commit = alloc_commit_node(repo);
set_merge_remote_desc(commit, comment, (struct object *)commit);
commit->maybe_tree = tree;
@@ -420,7 +423,7 @@ struct tree *write_tree_from_memory(struct merge_options *o)
return NULL;
}
- result = lookup_tree(the_repository, >cache_tree->oid);
+ result = lookup_tree(o->repo, >cache_tree->oid);
return result;
}
@@ -1200,9 +1203,9 @@ static int merge_submodule(struct merge_options *o,
return 0;
}
- if (!(commit_base = lookup_commit_reference(the_repository, base)) ||
- !(commit_a = lookup_commit_reference(the_repository, a)) ||
- !(commit_b = lookup_commit_reference(the_repository, b))) {
+ if (!(commit_base = lookup_commit_reference(o->repo, base)) ||
+ !(commit_a = lookup_commit_reference(o->repo, a)) ||
+ !(commit_b = lookup_commit_reference(o->repo, b))) {
output(o, 1, _("Failed to merge submodule %s (commits not
present)"), path);
return 0;
}
@@ -3286,8 +3289,8 @@ int merge_trees(struct merge_options *o,
}
if (o->subtree_shift) {
- merge = shift_tree_object(head, merge, o->subtree_shift);
- common = shift_tree_object(head, common, o->subtree_shift);
+ merge = shift_tree_object(o->repo, head, merge,
o->subtree_shift);
+ common = shift_tree_object(o->repo, head, common,
o->subtree_shift);
}
if (oid_eq(>object.oid, >object.oid)) {
@@ -3423,8 +3426,8 @@ int merge_recursive(struct merge_options *o,
/* if there is no common ancestor, use an empty tree */
struct tree *tree;
- tree = lookup_tree(the_repository,
the_repository->hash_algo->empty_tree);
- merged_common_ancestors = make_virtual_commit(tree, "ancestor");
+ tree = lookup_tree(o->repo, o->repo->hash_algo->empty_tree);
+ merged_common_ancestors = make_virtual_commit(o->repo, tree,
"ancestor");
}
for (iter = ca; iter; iter = iter->next) {
@@ -3468,7 +3471,7 @@ int merge_recursive(struct merge_options *o,
}
if (o->call_depth) {
- *result = make_virtual_commit(mrtree, "merged tree");
+ *result = make_virtual_commit(o->repo, mrtree, "merged tree");
commit_list_insert(h1, &(*result)->parents);
commit_list_insert(h2, &(*result)->parents->next);
}
@@ -3481,17 +3484,17 @@ int merge_recursive(struct merge_options *o,
return clean;
}
-static struct commit *get_ref(const struct object_id *oid, const char *name)
+static struct commit *get_ref(struct repository *repo, const struct object_id
*oid,
+ const char *name)
{
struct object *object;
- object = deref_tag(the_repository, parse_object(the_repository, oid),
- name,
- strlen(name));
+ object = deref_tag(repo, parse_object(repo, oid),
+ name, strlen(name));
if (!object)
return NULL;
if (object->type == OBJ_TREE)
- return make_virtual_commit((struct tree*)object, name);
+ return make_virtual_commit(repo, (struct tree*)object, name);
if (object->type != OBJ_COMMIT)
return NULL;
if (parse_commit((struct commit *)object))
@@ -3508,15 +3511,15 @@ int merge_recursive_generic(struct merge_options *o,
{
int clean;
struct lock_file lock = LOCK_INIT;
-
[PATCH 13/19] transport.c: remove implicit dependency on the_index
note, there's still another hidden dependency related to this: even
though we pass a repo to transport_push() we still use
is_bare_repository() which pretty much assumes the_repository (and
some other global state).
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/push.c | 3 ++-
transport.c| 7 ---
transport.h| 3 ++-
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/builtin/push.c b/builtin/push.c
index d09a42062c..efb3e38a8d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -355,7 +355,8 @@ static int push_with_options(struct transport *transport,
struct refspec *rs,
if (verbosity > 0)
fprintf(stderr, _("Pushing to %s\n"), transport->url);
- err = transport_push(transport, rs, flags, _reasons);
+ err = transport_push(the_repository, transport,
+rs, flags, _reasons);
if (err != 0) {
fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
error(_("failed to push some refs to '%s'"), transport->url);
diff --git a/transport.c b/transport.c
index f4ffbd96cb..b86b2b12c6 100644
--- a/transport.c
+++ b/transport.c
@@ -1105,7 +1105,8 @@ static int run_pre_push_hook(struct transport *transport,
return ret;
}
-int transport_push(struct transport *transport,
+int transport_push(struct repository *repo,
+ struct transport *transport,
struct refspec *rs, int flags,
unsigned int *reject_reasons)
{
@@ -1172,7 +1173,7 @@ int transport_push(struct transport *transport,
oid_array_append(,
>new_oid);
- if (!push_unpushed_submodules(_index,
+ if (!push_unpushed_submodules(repo->index,
,
transport->remote,
rs,
@@ -1197,7 +1198,7 @@ int transport_push(struct transport *transport,
oid_array_append(,
>new_oid);
- if (find_unpushed_submodules(_index,
+ if (find_unpushed_submodules(repo->index,
,
transport->remote->name,
_pushing)) {
diff --git a/transport.h b/transport.h
index 9baeca2d7a..f2ee7c4f49 100644
--- a/transport.h
+++ b/transport.h
@@ -223,7 +223,8 @@ void transport_set_verbosity(struct transport *transport,
int verbosity,
#define REJECT_FETCH_FIRST 0x08
#define REJECT_NEEDS_FORCE 0x10
-int transport_push(struct transport *connection,
+int transport_push(struct repository *repo,
+ struct transport *connection,
struct refspec *rs, int flags,
unsigned int * reject_reasons);
--
2.19.1.647.g708186aaf9
[PATCH 11/19] read-cache.c: kill read_index()
read_index() shares the same problem as hold_locked_index(): it
assumes $GIT_DIR/index. Move all call sites to repo_read_index()
instead. read_index_preload() and read_index_unmerged() are also
killed as a consequence.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
apply.c | 2 +-
blame.c | 4 ++--
builtin/am.c| 2 +-
builtin/commit.c| 2 +-
builtin/diff-tree.c | 2 +-
cache.h | 11 +++
merge-recursive.c | 2 +-
merge.c | 2 +-
preload-index.c | 11 ++-
read-cache.c| 11 ---
repository.h| 6 ++
rerere.c| 6 +++---
revision.c | 4 ++--
sequencer.c | 22 +++---
sha1-name.c | 6 +++---
15 files changed, 46 insertions(+), 47 deletions(-)
diff --git a/apply.c b/apply.c
index 122e6ddf92..f5d507a64f 100644
--- a/apply.c
+++ b/apply.c
@@ -4017,7 +4017,7 @@ static int read_apply_cache(struct apply_state *state)
return read_index_from(state->repo->index, state->index_file,
get_git_dir());
else
- return read_index(state->repo->index);
+ return repo_read_index(state->repo);
}
/* This function tries to read the object name from the current index */
diff --git a/blame.c b/blame.c
index d84c937780..6e3de62379 100644
--- a/blame.c
+++ b/blame.c
@@ -183,7 +183,7 @@ static struct commit *fake_working_tree_commit(struct
repository *r,
unsigned mode;
struct strbuf msg = STRBUF_INIT;
- read_index(r->index);
+ repo_read_index(r);
time();
commit = alloc_commit_node(the_repository);
commit->object.parsed = 1;
@@ -265,7 +265,7 @@ static struct commit *fake_working_tree_commit(struct
repository *r,
* want to run "diff-index --cached".
*/
discard_index(r->index);
- read_index(r->index);
+ repo_read_index(r);
len = strlen(path);
if (!mode) {
diff --git a/builtin/am.c b/builtin/am.c
index 3ee9a9d2a9..d2af94500c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2328,7 +2328,7 @@ int cmd_am(int argc, const char **argv, const char
*prefix)
/* Ensure a valid committer ident can be constructed */
git_committer_info(IDENT_STRICT);
- if (read_index_preload(_index, NULL, 0) < 0)
+ if (repo_read_index_preload(the_repository, NULL, 0) < 0)
die(_("failed to read the index"));
if (in_progress) {
diff --git a/builtin/commit.c b/builtin/commit.c
index f7bbba944d..7eda5e4b7e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1363,7 +1363,7 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
if (status_format != STATUS_FORMAT_PORCELAIN &&
status_format != STATUS_FORMAT_PORCELAIN_V2)
progress_flag = REFRESH_PROGRESS;
- read_index_preload(_index, , progress_flag);
+ repo_read_index_preload(the_repository, , progress_flag);
refresh_index(_index,
REFRESH_QUIET|REFRESH_UNMERGED|progress_flag,
, NULL, NULL);
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index ef996126d7..42bc1eb41d 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -165,7 +165,7 @@ int cmd_diff_tree(int argc, const char **argv, const char
*prefix)
if (opt->diffopt.detect_rename) {
if (!the_index.cache)
- read_index(_index);
+ repo_read_index(the_repository);
opt->diffopt.setup |= DIFF_SETUP_USE_SIZE_CACHE;
}
while (fgets(line, sizeof(line), stdin)) {
diff --git a/cache.h b/cache.h
index ef2483ce45..d9303ae25f 100644
--- a/cache.h
+++ b/cache.h
@@ -408,11 +408,11 @@ void validate_cache_entries(const struct index_state
*istate);
#define active_cache_changed (the_index.cache_changed)
#define active_cache_tree (the_index.cache_tree)
-#define read_cache() read_index(_index)
+#define read_cache() repo_read_index(the_repository)
#define read_cache_from(path) read_index_from(_index, (path),
(get_git_dir()))
-#define read_cache_preload(pathspec) read_index_preload(_index,
(pathspec), 0)
+#define read_cache_preload(pathspec) repo_read_index_preload(the_repository,
(pathspec), 0)
#define is_cache_unborn() is_index_unborn(_index)
-#define read_cache_unmerged() read_index_unmerged(_index)
+#define read_cache_unmerged() repo_read_index_unmerged(the_repository)
#define discard_cache() discard_index(_index)
#define unmerged_cache() unmerged_index(_index)
#define cache_name_pos(name, namelen)
index_name_pos(_index,(name),(namelen))
@@ -659,16 +659,11 @@ extern int daemonize(void);
/* Initialize and use the cache information */
struct lock_file;
-extern int read_index(struct index_state *);
-extern int read_index_preload(struct index_state *,
-
[PATCH 15/19] merge-recursive.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/am.c | 2 +-
builtin/checkout.c| 2 +-
builtin/merge-recursive.c | 2 +-
builtin/merge.c | 2 +-
merge-recursive.c | 151 +-
merge-recursive.h | 6 +-
sequencer.c | 4 +-
7 files changed, 94 insertions(+), 75 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index d2af94500c..65e10a5eb3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1597,7 +1597,7 @@ static int fall_back_threeway(const struct am_state
*state, const char *index_pa
* changes.
*/
- init_merge_options();
+ init_merge_options(, the_repository);
o.branch1 = "HEAD";
their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 38b28c20e2..8ecb4c43c7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -670,7 +670,7 @@ static int merge_working_tree(const struct checkout_opts
*opts,
* a pain; plumb in an option to set
* o.renormalize?
*/
- init_merge_options();
+ init_merge_options(, the_repository);
o.verbosity = 0;
work = write_tree_from_memory();
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 9b2f707c29..4864f7b22f 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -28,7 +28,7 @@ int cmd_merge_recursive(int argc, const char **argv, const
char *prefix)
struct merge_options o;
struct commit *result;
- init_merge_options();
+ init_merge_options(, the_repository);
if (argv[0] && ends_with(argv[0], "-subtree"))
o.subtree_shift = "";
diff --git a/builtin/merge.c b/builtin/merge.c
index db22119c93..d2d636d979 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -701,7 +701,7 @@ static int try_merge_strategy(const char *strategy, struct
commit_list *common,
return 2;
}
- init_merge_options();
+ init_merge_options(, the_repository);
if (!strcmp(strategy, "subtree"))
o.subtree_shift = "";
diff --git a/merge-recursive.c b/merge-recursive.c
index b025c10e31..184945ae67 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -318,22 +318,24 @@ static int add_cacheinfo(struct merge_options *o,
unsigned int mode, const struct object_id *oid,
const char *path, int stage, int refresh, int options)
{
+ struct index_state *istate = o->repo->index;
struct cache_entry *ce;
int ret;
- ce = make_cache_entry(_index, mode, oid ? oid : _oid, path,
stage, 0);
+ ce = make_cache_entry(istate, mode, oid ? oid : _oid, path, stage,
0);
if (!ce)
return err(o, _("add_cacheinfo failed for path '%s'; merge
aborting."), path);
- ret = add_cache_entry(ce, options);
+ ret = add_index_entry(istate, ce, options);
if (refresh) {
struct cache_entry *nce;
- nce = refresh_cache_entry(_index, ce, CE_MATCH_REFRESH |
CE_MATCH_IGNORE_MISSING);
+ nce = refresh_cache_entry(istate, ce,
+ CE_MATCH_REFRESH |
CE_MATCH_IGNORE_MISSING);
if (!nce)
return err(o, _("add_cacheinfo failed to refresh for
path '%s'; merge aborting."), path);
if (nce != ce)
- ret = add_cache_entry(nce, options);
+ ret = add_index_entry(istate, nce, options);
}
return ret;
}
@@ -361,7 +363,7 @@ static int unpack_trees_start(struct merge_options *o,
o->unpack_opts.merge = 1;
o->unpack_opts.head_idx = 2;
o->unpack_opts.fn = threeway_merge;
- o->unpack_opts.src_index = _index;
+ o->unpack_opts.src_index = o->repo->index;
o->unpack_opts.dst_index = _index;
o->unpack_opts.aggressive = !merge_detect_rename(o);
setup_unpack_trees_porcelain(>unpack_opts, "merge");
@@ -371,16 +373,16 @@ static int unpack_trees_start(struct merge_options *o,
init_tree_desc_from_tree(t+2, merge);
rc = unpack_trees(3, t, >unpack_opts);
- cache_tree_free(_cache_tree);
+ cache_tree_free(>repo->index->cache_tree);
/*
-* Update the_index to match the new results, AFTER saving a copy
+* Update o->repo->index to match the new results, AFTER saving a copy
* in o->orig_index. Update src_index to point to the saved copy.
* (verify_uptodate() checks src_index, and the original index is
* the one that had the necessary modification timestamps.)
*/
- o->orig_index = the_index;
- the_index = tmp_index;
[PATCH 19/19] Flip NO_THE_REPOSITORY_COMPATIBILITY_MACROS
Similar to NO_THE_INDEX_COMPATIBILITY_MACROS, this switch is now off
by default. It remains here just to make it easier to merge in-flight
topics where the old functions are still used. Then you can just stick
#define USE_THE_REPOSITORY_COMPATIBILITY_MACROS
at the beginning of those files.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
diff.h | 2 +-
rerere.h | 2 +-
revision.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff.h b/diff.h
index ce5e8a8183..de7cb7018f 100644
--- a/diff.h
+++ b/diff.h
@@ -341,7 +341,7 @@ int git_diff_basic_config(const char *var, const char
*value, void *cb);
int git_diff_heuristic_config(const char *var, const char *value, void *cb);
void init_diff_ui_defaults(void);
int git_diff_ui_config(const char *var, const char *value, void *cb);
-#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#ifdef USE_THE_REPOSITORY_COMPATIBILITY_MACROS
#define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts)
#endif
void repo_diff_setup(struct repository *, struct diff_options *);
diff --git a/rerere.h b/rerere.h
index 5ad8864b3c..8ddd0b20da 100644
--- a/rerere.h
+++ b/rerere.h
@@ -24,7 +24,7 @@ struct rerere_id {
};
int setup_rerere(struct string_list *, int);
-#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#ifdef USE_THE_REPOSITORY_COMPATIBILITY_MACROS
#define rerere(flags) repo_rerere(the_repository, flags)
#endif
int repo_rerere(struct repository *, int);
diff --git a/revision.h b/revision.h
index bc30a3023e..5b6ab10143 100644
--- a/revision.h
+++ b/revision.h
@@ -271,7 +271,7 @@ struct setup_revision_opt {
unsigned revarg_opt;
};
-#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
+#ifdef USE_THE_REPOSITORY_COMPATIBILITY_MACROS
#define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs,
prefix)
#endif
void repo_init_revisions(struct repository *r,
--
2.19.1.647.g708186aaf9
[PATCH/WIP 00/19] Kill the_index, final part
Stefan I see you start removing more references of the_repository in another series (haven't really looked at it yet) so this is just heads up so we could coordinate if needed. Still WIP, not really ready for review yet. This series removes use of the_index outside builtin/ and t/helper/. The only the_index reference left is in repository.c. The macro NO_THE_REPOSITORY_COMPATIBILITY_MACROS is now flipped to USE_THE_INDEX_COMPATIBILITY_MACROS. "the_index" is forbidden by default. After this I think we can start pushing the_repository outside library code. Then perhaps clean them up in builtin code too and you can finally get rid of it. But I don't think that'll happen in a year's time. Nguyễn Thái Ngọc Duy (19): wt-status.c: remove implicit dependency on the_index wt-status.c: remove implicit dependency the_repository list-objects-filter.c: remove implicit dependency on the_index list-objects.c: remove implicit dependency on the_index sequencer.c: remove implicit dependency on the_index sequencer.c: remove implicit dependency on the_repository notes-merge.c: remove implicit dependency on the_index notes-merge.c: remove implicit dependency the_repository repository.c: replace hold_locked_index() with repo_hold_locked_index() checkout: avoid the_index when possible read-cache.c: kill read_index() read-cache.c: replace update_index_if_able with repo_& transport.c: remove implicit dependency on the_index sha1-name.c: remove implicit dependency on the_index merge-recursive.c: remove implicit dependency on the_index merge-recursive.c: remove implicit dependency on the_repository read-cache.c: remove the_* from index_has_changes() cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch Flip NO_THE_REPOSITORY_COMPATIBILITY_MACROS apply.c | 5 +- attr.c | 1 - blame.c | 4 +- builtin/add.c| 1 + builtin/am.c | 11 +- builtin/blame.c | 3 +- builtin/cat-file.c | 7 +- builtin/check-attr.c | 1 + builtin/check-ignore.c | 1 + builtin/checkout-index.c | 1 + builtin/checkout.c | 5 +- builtin/clean.c | 1 + builtin/clone.c | 1 + builtin/commit.c | 10 +- builtin/describe.c | 3 +- builtin/diff-files.c | 1 + builtin/diff-index.c | 1 + builtin/diff-tree.c | 3 +- builtin/diff.c | 3 +- builtin/difftool.c | 1 + builtin/fsck.c | 1 + builtin/grep.c | 4 +- builtin/hash-object.c| 3 +- builtin/log.c| 4 +- builtin/ls-files.c | 1 - builtin/merge-index.c| 1 + builtin/merge-ours.c | 1 + builtin/merge-recursive.c| 2 +- builtin/merge-tree.c | 3 +- builtin/merge.c | 5 +- builtin/mv.c | 1 + builtin/notes.c | 2 +- builtin/pack-objects.c | 2 +- builtin/pull.c | 4 +- builtin/push.c | 3 +- builtin/read-tree.c | 1 + builtin/rebase--helper.c | 15 +- builtin/replace.c| 2 +- builtin/reset.c | 1 + builtin/rev-parse.c | 4 +- builtin/revert.c | 6 +- builtin/rm.c | 1 + builtin/submodule--helper.c | 1 + builtin/update-index.c | 1 + builtin/write-tree.c | 1 + cache-tree.h | 2 +- cache.h | 35 +-- convert.c| 1 - diff.h | 2 +- dir.c| 1 - git.c| 4 +- list-objects-filter-options.c| 2 +- list-objects-filter.c| 7 +- list-objects-filter.h| 1 + list-objects.c | 3 + merge-recursive.c| 194 ++-- merge-recursive.h| 6 +- merge.c | 4 +- name-hash.c | 1 - notes-merge.c| 16 +- notes-merge.h| 6 +- pathspec.c | 1 - preload-index.c | 11 +- read-cache.c | 44 ++- ref-filter.c | 2 +- repository.c | 9 + repository.h | 16 + rerere.c | 8 +- rerere.h
[PATCH 12/19] read-cache.c: replace update_index_if_able with repo_
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/commit.c | 2 +-
builtin/describe.c | 2 +-
builtin/diff.c | 2 +-
cache.h| 6 --
read-cache.c | 14 --
repository.h | 6 ++
wt-status.c| 2 +-
7 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7eda5e4b7e..d7afaa9ed4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1392,7 +1392,7 @@ int cmd_status(int argc, const char **argv, const char
*prefix)
wt_status_collect();
if (0 <= fd)
- update_index_if_able(_index, _lock);
+ repo_update_index_if_able(the_repository, _lock);
if (s.relative_paths)
s.prefix = prefix;
diff --git a/builtin/describe.c b/builtin/describe.c
index c48c34e866..cff8ec3d65 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -634,7 +634,7 @@ int cmd_describe(int argc, const char **argv, const char
*prefix)
NULL, NULL, NULL);
fd = hold_locked_index(_lock, 0);
if (0 <= fd)
- update_index_if_able(_index, _lock);
+ repo_update_index_if_able(the_repository,
_lock);
repo_init_revisions(the_repository, , prefix);
argv_array_pushv(, diff_index_args);
diff --git a/builtin/diff.c b/builtin/diff.c
index f0393bba23..ec78920ee2 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -212,7 +212,7 @@ static void refresh_index_quietly(void)
discard_cache();
read_cache();
refresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);
- update_index_if_able(_index, _file);
+ repo_update_index_if_able(the_repository, _file);
}
static int builtin_diff_files(struct rev_info *revs, int argc, const char
**argv)
diff --git a/cache.h b/cache.h
index d9303ae25f..73bf68d8a5 100644
--- a/cache.h
+++ b/cache.h
@@ -816,12 +816,6 @@ extern void fill_stat_cache_info(struct cache_entry *ce,
struct stat *st);
extern int refresh_index(struct index_state *, unsigned int flags, const
struct pathspec *pathspec, char *seen, const char *header_msg);
extern struct cache_entry *refresh_cache_entry(struct index_state *, struct
cache_entry *, unsigned int);
-/*
- * Opportunistically update the index but do not complain if we can't.
- * The lockfile is always committed or rolled back.
- */
-extern void update_index_if_able(struct index_state *, struct lock_file *);
-
extern void set_alternate_index_output(const char *);
extern int verify_index_checksum;
diff --git a/read-cache.c b/read-cache.c
index 8844f723e9..46e5e4000a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2661,9 +2661,9 @@ static int verify_index_from(const struct index_state
*istate, const char *path)
return 0;
}
-static int verify_index(const struct index_state *istate)
+static int repo_verify_index(struct repository *repo)
{
- return verify_index_from(istate, get_index_file());
+ return verify_index_from(repo->index, repo->index_file);
}
static int has_racy_timestamp(struct index_state *istate)
@@ -2679,11 +2679,13 @@ static int has_racy_timestamp(struct index_state
*istate)
return 0;
}
-void update_index_if_able(struct index_state *istate, struct lock_file
*lockfile)
+void repo_update_index_if_able(struct repository *repo,
+ struct lock_file *lockfile)
{
- if ((istate->cache_changed || has_racy_timestamp(istate)) &&
- verify_index(istate))
- write_locked_index(istate, lockfile, COMMIT_LOCK);
+ if ((repo->index->cache_changed ||
+has_racy_timestamp(repo->index)) &&
+ repo_verify_index(repo))
+ write_locked_index(repo->index, lockfile, COMMIT_LOCK);
else
rollback_lock_file(lockfile);
}
diff --git a/repository.h b/repository.h
index cc3879add4..6fe1c089db 100644
--- a/repository.h
+++ b/repository.h
@@ -140,5 +140,11 @@ int repo_read_index_preload(struct repository *,
const struct pathspec *pathspec,
unsigned refresh_flags);
int repo_read_index_unmerged(struct repository *);
+/*
+ * Opportunistically update the index but do not complain if we can't.
+ * The lockfile is always committed or rolled back.
+ */
+void repo_update_index_if_able(struct repository *, struct lock_file *);
+
#endif /* REPOSITORY_H */
diff --git a/wt-status.c b/wt-status.c
index da28921772..bdc4c36f93 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2374,7 +2374,7 @@ int require_clean_work_tree(struct repository *r,
fd = repo_hold_locked_index(r, _file, 0);
refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
if (0 <= fd)
- update_index_if_able(r->index, _file);
+ repo_update_index_if_able(r, _file);
rollback_lock_file(_file);
[PATCH 17/19] read-cache.c: remove the_* from index_has_changes()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/am.c | 6 +++---
cache.h | 6 +++---
merge-recursive.c | 2 +-
read-cache.c | 12 +---
4 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 65e10a5eb3..37b52be7db 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1771,7 +1771,7 @@ static void am_run(struct am_state *state, int resume)
refresh_and_write_cache();
- if (index_has_changes(_index, NULL, )) {
+ if (repo_index_has_changes(the_repository, NULL, )) {
write_state_bool(state, "dirtyindex", 1);
die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
}
@@ -1829,7 +1829,7 @@ static void am_run(struct am_state *state, int resume)
* the result may have produced the same tree as ours.
*/
if (!apply_status &&
- !index_has_changes(_index, NULL, NULL)) {
+ !repo_index_has_changes(the_repository, NULL,
NULL)) {
say(state, stdout, _("No changes -- Patch
already applied."));
goto next;
}
@@ -1883,7 +1883,7 @@ static void am_resolve(struct am_state *state)
say(state, stdout, _("Applying: %.*s"), linelen(state->msg),
state->msg);
- if (!index_has_changes(_index, NULL, NULL)) {
+ if (!repo_index_has_changes(the_repository, NULL, NULL)) {
printf_ln(_("No changes - did you forget to use 'git add'?\n"
"If there is nothing left to stage, chances are that
something else\n"
"already introduced the same changes; you might want to
skip this patch."));
diff --git a/cache.h b/cache.h
index 91c092cf76..4b3ec4ff82 100644
--- a/cache.h
+++ b/cache.h
@@ -701,9 +701,9 @@ extern int unmerged_index(const struct index_state *);
* provided, the space-separated list of files that differ will be appended
* to it.
*/
-extern int index_has_changes(struct index_state *istate,
-struct tree *tree,
-struct strbuf *sb);
+extern int repo_index_has_changes(struct repository *repo,
+ struct tree *tree,
+ struct strbuf *sb);
extern int verify_path(const char *path, unsigned mode);
extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);
diff --git a/merge-recursive.c b/merge-recursive.c
index ad0a5b0202..22b382e85f 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3282,7 +3282,7 @@ int merge_trees(struct merge_options *o,
int code, clean;
struct strbuf sb = STRBUF_INIT;
- if (!o->call_depth && index_has_changes(istate, head, )) {
+ if (!o->call_depth && repo_index_has_changes(o->repo, head, )) {
err(o, _("Your local changes to the following files would be
overwritten by merge:\n %s"),
sb.buf);
return -1;
diff --git a/read-cache.c b/read-cache.c
index 46e5e4000a..19389c6f30 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2372,22 +2372,20 @@ int unmerged_index(const struct index_state *istate)
return 0;
}
-int index_has_changes(struct index_state *istate,
- struct tree *tree,
- struct strbuf *sb)
+int repo_index_has_changes(struct repository *repo,
+ struct tree *tree,
+ struct strbuf *sb)
{
+ struct index_state *istate = repo->index;
struct object_id cmp;
int i;
- if (istate != _index) {
- BUG("index_has_changes cannot yet accept istate != _index;
do_diff_cache needs updating first.");
- }
if (tree)
cmp = tree->object.oid;
if (tree || !get_oid_tree("HEAD", )) {
struct diff_options opt;
- repo_diff_setup(the_repository, );
+ repo_diff_setup(repo, );
opt.flags.exit_with_status = 1;
if (!sb)
opt.flags.quick = 1;
--
2.19.1.647.g708186aaf9
[PATCH 14/19] sha1-name.c: remove implicit dependency on the_index
This kills the_index dependency in get_oid_with_context() but for
get_oid() and friends, they still assume the_repository (which also
means the_index).
Unfortunately the widespread use of get_oid() will make it hard to
make the conversion now. We probably will add repo_get_oid() at some
point and limit the use of get_oid() in builtin/ instead of forcing
all get_oid() call sites to carry struct repository.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/cat-file.c| 6 ++--
builtin/grep.c| 3 +-
builtin/log.c | 3 +-
builtin/rev-parse.c | 3 +-
cache.h | 4 ++-
list-objects-filter-options.c | 2 +-
revision.c| 8 +++---
sha1-name.c | 54 +--
8 files changed, 50 insertions(+), 33 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 8d97c84725..17faea8846 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -66,7 +66,8 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name,
if (unknown_type)
flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
- if (get_oid_with_context(obj_name, GET_OID_RECORD_PATH,
+ if (get_oid_with_context(the_repository, obj_name,
+GET_OID_RECORD_PATH,
, _context))
die("Not a valid object name %s", obj_name);
@@ -374,7 +375,8 @@ static void batch_one_object(const char *obj_name,
int flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;
enum follow_symlinks_result result;
- result = get_oid_with_context(obj_name, flags, >oid, );
+ result = get_oid_with_context(the_repository, obj_name,
+ flags, >oid, );
if (result != FOUND) {
switch (result) {
case MISSING_OBJECT:
diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..9d40b5c073 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1006,7 +1006,8 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
break;
}
- if (get_oid_with_context(arg, GET_OID_RECORD_PATH,
+ if (get_oid_with_context(the_repository, arg,
+GET_OID_RECORD_PATH,
, )) {
if (seen_dashdash)
die(_("unable to resolve revision: %s"), arg);
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..ed28e81a22 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -506,7 +506,8 @@ static int show_blob_object(const struct object_id *oid,
struct rev_info *rev, c
!rev->diffopt.flags.allow_textconv)
return stream_blob_to_fd(1, oid, NULL, 0);
- if (get_oid_with_context(obj_name, GET_OID_RECORD_PATH,
+ if (get_oid_with_context(the_repository, obj_name,
+GET_OID_RECORD_PATH,
, _context))
die(_("Not a valid object name %s"), obj_name);
if (!obj_context.path ||
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 455f62246d..38d7d8fd74 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -932,7 +932,8 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
name++;
type = REVERSED;
}
- if (!get_oid_with_context(name, flags, , )) {
+ if (!get_oid_with_context(the_repository, name,
+ flags, , )) {
if (verify)
revs_count++;
else
diff --git a/cache.h b/cache.h
index 73bf68d8a5..91c092cf76 100644
--- a/cache.h
+++ b/cache.h
@@ -1327,7 +1327,9 @@ extern int get_oid_tree(const char *str, struct object_id
*oid);
extern int get_oid_treeish(const char *str, struct object_id *oid);
extern int get_oid_blob(const char *str, struct object_id *oid);
extern void maybe_die_on_misspelt_object_name(const char *name, const char
*prefix);
-extern int get_oid_with_context(const char *str, unsigned flags, struct
object_id *oid, struct object_context *oc);
+extern int get_oid_with_context(struct repository *repo, const char *str,
+ unsigned flags, struct object_id *oid,
+ struct object_context *oc);
typedef int each_abbrev_fn(const struct object_id *oid, void *);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index c0e2bd6a06..a683a75a35 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -59,7 +59,7 @@ static int gently_parse_list_objects_filter(
* command, but DO NOT complain if we don't have the blob or
* ref locally.
*/
-
[PATCH 18/19] cache.h: flip NO_THE_INDEX_COMPATIBILITY_MACROS switch
>From now on, by default index compat macros are off because they could
hide the_index dependency. Only those in builtin can use it (and even
so should be avoided if possible).
Signed-off-by: Nguyễn Thái Ngọc Duy
---
attr.c | 1 -
builtin/add.c| 1 +
builtin/am.c | 1 +
builtin/blame.c | 3 ++-
builtin/cat-file.c | 1 +
builtin/check-attr.c | 1 +
builtin/check-ignore.c | 1 +
builtin/checkout-index.c | 1 +
builtin/checkout.c | 1 +
builtin/clean.c | 1 +
builtin/commit.c | 1 +
builtin/describe.c | 1 +
builtin/diff-files.c | 1 +
builtin/diff-index.c | 1 +
builtin/diff-tree.c | 1 +
builtin/diff.c | 1 +
builtin/difftool.c | 1 +
builtin/fsck.c | 1 +
builtin/grep.c | 1 +
builtin/hash-object.c| 3 ++-
builtin/log.c| 1 +
builtin/ls-files.c | 1 -
builtin/merge-index.c| 1 +
builtin/merge-ours.c | 1 +
builtin/merge-tree.c | 3 ++-
builtin/merge.c | 1 +
builtin/mv.c | 1 +
builtin/pack-objects.c | 2 +-
builtin/pull.c | 1 +
builtin/read-tree.c | 1 +
builtin/replace.c| 2 +-
builtin/reset.c | 1 +
builtin/rev-parse.c | 1 +
builtin/rm.c | 1 +
builtin/submodule--helper.c | 1 +
builtin/update-index.c | 1 +
builtin/write-tree.c | 1 +
cache-tree.h | 2 +-
cache.h | 6 +++---
convert.c| 1 -
dir.c| 1 -
git.c| 4 ++--
name-hash.c | 1 -
pathspec.c | 1 -
read-cache.c | 2 --
repository.c | 1 +
submodule.c | 1 -
t/helper/test-dump-fsmonitor.c | 4 ++--
t/helper/test-dump-untracked-cache.c | 1 +
t/helper/test-tool.h | 1 +
tree.c | 1 -
unpack-trees.c | 1 -
52 files changed, 49 insertions(+), 24 deletions(-)
diff --git a/attr.c b/attr.c
index 60d284796d..e56ffa9112 100644
--- a/attr.c
+++ b/attr.c
@@ -7,7 +7,6 @@
* an insanely large number of attributes.
*/
-#define NO_THE_INDEX_COMPATIBILITY_MACROS
#include "cache.h"
#include "config.h"
#include "exec-cmd.h"
diff --git a/builtin/add.c b/builtin/add.c
index ad49806ebf..72e9765edf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -3,6 +3,7 @@
*
* Copyright (C) 2006 Linus Torvalds
*/
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
#include "cache.h"
#include "config.h"
#include "builtin.h"
diff --git a/builtin/am.c b/builtin/am.c
index 37b52be7db..4e36c186fc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -3,6 +3,7 @@
*
* Based on git-am.sh by Junio C Hamano.
*/
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
#include "cache.h"
#include "config.h"
#include "builtin.h"
diff --git a/builtin/blame.c b/builtin/blame.c
index a443af9ee9..d004343211 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1001,7 +1001,8 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
long bottom, top;
if (parse_range_arg(range_list.items[range_i].string,
nth_line_cb, , lno, anchor,
- , , sb.path, _index))
+ , , sb.path,
+ the_repository->index))
usage(blame_usage);
if ((!lno && (top || bottom)) || lno < bottom)
die(Q_("file %s has only %lu line",
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 17faea8846..72be3e8065 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -3,6 +3,7 @@
*
* Copyright (C) Linus Torvalds, 2005
*/
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
#include "cache.h"
#include "config.h"
#include "builtin.h"
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 30a2f84274..dd83397786 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -1,3 +1,4 @@
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
#include "builtin.h"
#include "cache.h"
#include "config.h"
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index ec9a959e08..599097304b 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -1,3 +1,4 @@
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
#include "builtin.h"
#include "cache.h"
#include "config.h"
[PATCH 03/19] list-objects-filter.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
---
list-objects-filter.c | 7 +--
list-objects-filter.h | 1 +
list-objects.c| 3 +++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/list-objects-filter.c b/list-objects-filter.c
index a0ba78b20c..8825e2a5ab 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -38,6 +38,7 @@ static enum list_objects_filter_result filter_blobs_none(
struct object *obj,
const char *pathname,
const char *filename,
+ struct index_state *istate,
void *filter_data_)
{
struct filter_blobs_none_data *filter_data = filter_data_;
@@ -94,6 +95,7 @@ static enum list_objects_filter_result filter_blobs_limit(
struct object *obj,
const char *pathname,
const char *filename,
+ struct index_state *istate,
void *filter_data_)
{
struct filter_blobs_limit_data *filter_data = filter_data_;
@@ -200,6 +202,7 @@ static enum list_objects_filter_result filter_sparse(
struct object *obj,
const char *pathname,
const char *filename,
+ struct index_state *istate,
void *filter_data_)
{
struct filter_sparse_data *filter_data = filter_data_;
@@ -216,7 +219,7 @@ static enum list_objects_filter_result filter_sparse(
dtype = DT_DIR;
val = is_excluded_from_list(pathname, strlen(pathname),
filename, , _data->el,
- _index);
+ istate);
if (val < 0)
val = filter_data->array_frame[filter_data->nr].defval;
@@ -279,7 +282,7 @@ static enum list_objects_filter_result filter_sparse(
dtype = DT_REG;
val = is_excluded_from_list(pathname, strlen(pathname),
filename, , _data->el,
- _index);
+ istate);
if (val < 0)
val = frame->defval;
if (val > 0) {
diff --git a/list-objects-filter.h b/list-objects-filter.h
index a6f6b4990b..dfa29595d1 100644
--- a/list-objects-filter.h
+++ b/list-objects-filter.h
@@ -58,6 +58,7 @@ typedef enum list_objects_filter_result (*filter_object_fn)(
struct object *obj,
const char *pathname,
const char *filename,
+ struct index_state *istate,
void *filter_data);
typedef void (*filter_free_fn)(void *filter_data);
diff --git a/list-objects.c b/list-objects.c
index 0c2989d5ca..f9d36dabf2 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -51,6 +51,7 @@ static void process_blob(struct rev_info *revs,
if (!(obj->flags & USER_GIVEN) && filter_fn)
r = filter_fn(LOFS_BLOB, obj,
path->buf, >buf[pathlen],
+ _index,
filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
@@ -136,6 +137,7 @@ static void process_tree(struct rev_info *revs,
if (!(obj->flags & USER_GIVEN) && filter_fn)
r = filter_fn(LOFS_BEGIN_TREE, obj,
base->buf, >buf[baselen],
+ _index,
filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
@@ -175,6 +177,7 @@ static void process_tree(struct rev_info *revs,
if (!(obj->flags & USER_GIVEN) && filter_fn) {
r = filter_fn(LOFS_END_TREE, obj,
base->buf, >buf[baselen],
+ _index,
filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
--
2.19.1.647.g708186aaf9
[PATCH 07/19] notes-merge.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/notes.c | 2 +-
notes-merge.c | 12 +++-
notes-merge.h | 6 +-
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/builtin/notes.c b/builtin/notes.c
index c05cd004ab..d8bd01656a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -808,7 +808,7 @@ static int merge(int argc, const char **argv, const char
*prefix)
usage_with_options(git_notes_merge_usage, options);
}
- init_notes_merge_options();
+ init_notes_merge_options(, the_repository);
o.verbosity = verbosity + NOTES_MERGE_VERBOSITY_DEFAULT;
if (do_abort)
diff --git a/notes-merge.c b/notes-merge.c
index bd05d50b05..c3bee1a87d 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -18,11 +18,13 @@ struct notes_merge_pair {
struct object_id obj, base, local, remote;
};
-void init_notes_merge_options(struct notes_merge_options *o)
+void init_notes_merge_options(struct notes_merge_options *o,
+ struct repository *repo)
{
memset(o, 0, sizeof(struct notes_merge_options));
strbuf_init(&(o->commit_msg), 0);
o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
+ o->repo = repo;
}
static int path_to_oid(const char *path, struct object_id *oid)
@@ -127,7 +129,7 @@ static struct notes_merge_pair *diff_tree_remote(struct
notes_merge_options *o,
trace_printf("\tdiff_tree_remote(base = %.7s, remote = %.7s)\n",
oid_to_hex(base), oid_to_hex(remote));
- repo_diff_setup(the_repository, );
+ repo_diff_setup(o->repo, );
opt.flags.recursive = 1;
opt.output_format = DIFF_FORMAT_NO_OUTPUT;
diff_setup_done();
@@ -190,7 +192,7 @@ static void diff_tree_local(struct notes_merge_options *o,
trace_printf("\tdiff_tree_local(len = %i, base = %.7s, local = %.7s)\n",
len, oid_to_hex(base), oid_to_hex(local));
- repo_diff_setup(the_repository, );
+ repo_diff_setup(o->repo, );
opt.flags.recursive = 1;
opt.output_format = DIFF_FORMAT_NO_OUTPUT;
diff_setup_done();
@@ -350,7 +352,7 @@ static int ll_merge_in_worktree(struct notes_merge_options
*o,
status = ll_merge(_buf, oid_to_hex(>obj), , NULL,
, o->local_ref, , o->remote_ref,
- _index, NULL);
+ o->repo->index, NULL);
free(base.ptr);
free(local.ptr);
@@ -711,7 +713,7 @@ int notes_merge_commit(struct notes_merge_options *o,
/* write file as blob, and add to partial_tree */
if (stat(path.buf, ))
die_errno("Failed to stat '%s'", path.buf);
- if (index_path(_index, _oid, path.buf, ,
HASH_WRITE_OBJECT))
+ if (index_path(o->repo->index, _oid, path.buf, ,
HASH_WRITE_OBJECT))
die("Failed to write blob object from '%s'", path.buf);
if (add_note(partial_tree, _oid, _oid, NULL))
die("Failed to add resolved note '%s' to notes tree",
diff --git a/notes-merge.h b/notes-merge.h
index 6c74e9385b..fe70cbd5e8 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -7,6 +7,8 @@
struct commit;
struct object_id;
+struct repository;
+
#define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
enum notes_merge_verbosity {
@@ -15,6 +17,7 @@ enum notes_merge_verbosity {
};
struct notes_merge_options {
+ struct repository *repo;
const char *local_ref;
const char *remote_ref;
struct strbuf commit_msg;
@@ -23,7 +26,8 @@ struct notes_merge_options {
unsigned has_worktree:1;
};
-void init_notes_merge_options(struct notes_merge_options *o);
+void init_notes_merge_options(struct notes_merge_options *o,
+ struct repository *repo);
/*
* Merge notes from o->remote_ref into o->local_ref
--
2.19.1.647.g708186aaf9
[PATCH 05/19] sequencer.c: remove implicit dependency on the_index
Since we're going to pass 'struct repository *' around most of the
time instead of 'struct index_state *' because most sequencer.c
operations need more than just the index, the_repository is replaced
as well in the functions that now take 'struct repository
*'. the_repository is still present in this file, but total clean up
will be done later. It's not the main focus of this patch.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/commit.c | 3 +-
builtin/merge.c | 2 +-
builtin/rebase--helper.c | 5 +-
builtin/revert.c | 4 +-
sequencer.c | 319 ++-
sequencer.h | 15 +-
6 files changed, 197 insertions(+), 151 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 379d8c5cdf..f7bbba944d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1679,7 +1679,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
flags |= SUMMARY_INITIAL_COMMIT;
if (author_date_is_interesting())
flags |= SUMMARY_SHOW_AUTHOR_DATE;
- print_commit_summary(prefix, , flags);
+ print_commit_summary(the_repository, prefix,
+, flags);
}
UNLEAK(err);
diff --git a/builtin/merge.c b/builtin/merge.c
index 4aa6071598..db22119c93 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -896,7 +896,7 @@ static int suggest_conflicts(void)
filename = git_path_merge_msg(the_repository);
fp = xfopen(filename, "a");
- append_conflicts_hint();
+ append_conflicts_hint(_index, );
fputs(msgbuf.buf, fp);
strbuf_release();
fclose(fp);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc8..b66cd8cd41 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -69,11 +69,12 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
"--rebase-merges"));
if (command == CONTINUE && argc == 1)
- return !!sequencer_continue();
+ return !!sequencer_continue(the_repository, );
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
- return !!sequencer_make_script(stdout, argc, argv, flags);
+ return !!sequencer_make_script(the_repository, stdout,
+ argc, argv, flags);
if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
diff --git a/builtin/revert.c b/builtin/revert.c
index c93393c89b..cd9f068195 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -199,10 +199,10 @@ static int run_sequencer(int argc, const char **argv,
struct replay_opts *opts)
return ret;
}
if (cmd == 'c')
- return sequencer_continue(opts);
+ return sequencer_continue(the_repository, opts);
if (cmd == 'a')
return sequencer_rollback(opts);
- return sequencer_pick_revisions(opts);
+ return sequencer_pick_revisions(the_repository, opts);
}
int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 90435593a4..6b79359114 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -440,9 +440,9 @@ static struct tree *empty_tree(void)
return lookup_tree(the_repository,
the_repository->hash_algo->empty_tree);
}
-static int error_dirty_index(struct replay_opts *opts)
+static int error_dirty_index(struct index_state *istate, struct replay_opts
*opts)
{
- if (read_cache_unmerged())
+ if (read_index_unmerged(istate))
return error_resolve_conflict(_(action_name(opts)));
error(_("your local changes would be overwritten by %s."),
@@ -467,15 +467,18 @@ static void update_abort_safety_file(void)
write_file(git_path_abort_safety_file(), "%s", "");
}
-static int fast_forward_to(const struct object_id *to, const struct object_id
*from,
- int unborn, struct replay_opts *opts)
+static int fast_forward_to(struct repository *r,
+ const struct object_id *to,
+ const struct object_id *from,
+ int unborn,
+ struct replay_opts *opts)
{
struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
- read_index(_index);
- if (checkout_fast_forward(the_repository, from, to, 1))
+ read_index(r->index);
+ if (checkout_fast_forward(r, from, to, 1))
return -1; /* the callee should have complained already */
strbuf_addf(, _("%s: fast-forward"),
[PATCH 08/19] notes-merge.c: remove implicit dependency the_repository
Signed-off-by: Nguyễn Thái Ngọc Duy
---
notes-merge.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/notes-merge.c b/notes-merge.c
index c3bee1a87d..b6a998bfd4 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -558,7 +558,7 @@ int notes_merge(struct notes_merge_options *o,
else if (!check_refname_format(o->local_ref, 0) &&
is_null_oid(_oid))
local = NULL; /* local_oid == null_oid indicates unborn ref */
- else if (!(local = lookup_commit_reference(the_repository, _oid)))
+ else if (!(local = lookup_commit_reference(o->repo, _oid)))
die("Could not parse local commit %s (%s)",
oid_to_hex(_oid), o->local_ref);
trace_printf("\tlocal commit: %.7s\n", oid_to_hex(_oid));
@@ -576,7 +576,7 @@ int notes_merge(struct notes_merge_options *o,
die("Failed to resolve remote notes ref '%s'",
o->remote_ref);
}
- } else if (!(remote = lookup_commit_reference(the_repository,
_oid))) {
+ } else if (!(remote = lookup_commit_reference(o->repo, _oid))) {
die("Could not parse remote commit %s (%s)",
oid_to_hex(_oid), o->remote_ref);
}
--
2.19.1.647.g708186aaf9
[PATCH 06/19] sequencer.c: remove implicit dependency on the_repository
Note that the_hash_algo stays, even if we can easily replace it with
repo->hash_algo. My reason is I still believe tying hash_algo to a
struct repository is a wrong move. But if I'm wrong, we can always go
for another round of conversion.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/rebase--helper.c | 10 ++--
builtin/revert.c | 2 +-
sequencer.c | 108 +--
sequencer.h | 12 ++---
4 files changed, 70 insertions(+), 62 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index b66cd8cd41..605fcaa89c 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -76,14 +76,14 @@ int cmd_rebase__helper(int argc, const char **argv, const
char *prefix)
return !!sequencer_make_script(the_repository, stdout,
argc, argv, flags);
if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
- return !!transform_todos(flags);
+ return !!transform_todos(the_repository, flags);
if (command == CHECK_TODO_LIST && argc == 1)
- return !!check_todo_list();
+ return !!check_todo_list(the_repository);
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
- return !!skip_unnecessary_picks();
+ return !!skip_unnecessary_picks(the_repository);
if (command == REARRANGE_SQUASH && argc == 1)
- return !!rearrange_squash();
+ return !!rearrange_squash(the_repository);
if (command == ADD_EXEC && argc == 2)
- return !!sequencer_add_exec_commands(argv[1]);
+ return !!sequencer_add_exec_commands(the_repository, argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/builtin/revert.c b/builtin/revert.c
index cd9f068195..1fa75b2773 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -201,7 +201,7 @@ static int run_sequencer(int argc, const char **argv,
struct replay_opts *opts)
if (cmd == 'c')
return sequencer_continue(the_repository, opts);
if (cmd == 'a')
- return sequencer_rollback(opts);
+ return sequencer_rollback(the_repository, opts);
return sequencer_pick_revisions(the_repository, opts);
}
diff --git a/sequencer.c b/sequencer.c
index 6b79359114..2c2ea2b45d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,7 +351,8 @@ static void free_message(struct commit *commit, struct
commit_message *msg)
unuse_commit_buffer(commit, msg->message);
}
-static void print_advice(int show_hint, struct replay_opts *opts)
+static void print_advice(struct repository *r, int show_hint,
+struct replay_opts *opts)
{
char *msg = getenv("GIT_CHERRY_PICK_HELP");
@@ -362,7 +363,7 @@ static void print_advice(int show_hint, struct replay_opts
*opts)
* (typically rebase --interactive) wants to take care
* of the commit itself so remove CHERRY_PICK_HEAD
*/
- unlink(git_path_cherry_pick_head(the_repository));
+ unlink(git_path_cherry_pick_head(r));
return;
}
@@ -435,9 +436,9 @@ static int read_oneliner(struct strbuf *buf,
return 1;
}
-static struct tree *empty_tree(void)
+static struct tree *empty_tree(struct repository *r)
{
- return lookup_tree(the_repository,
the_repository->hash_algo->empty_tree);
+ return lookup_tree(r, the_hash_algo->empty_tree);
}
static int error_dirty_index(struct index_state *istate, struct replay_opts
*opts)
@@ -548,8 +549,8 @@ static int do_recursive_merge(struct repository *r,
o.show_rename_progress = 1;
head_tree = parse_tree_indirect(head);
- next_tree = next ? get_commit_tree(next) : empty_tree();
- base_tree = base ? get_commit_tree(base) : empty_tree();
+ next_tree = next ? get_commit_tree(next) : empty_tree(r);
+ base_tree = base ? get_commit_tree(base) : empty_tree(r);
for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
parse_merge_opt(, *xopt);
@@ -595,15 +596,16 @@ static struct object_id *get_cache_tree_oid(struct
index_state *istate)
return >cache_tree->oid;
}
-static int is_index_unchanged(struct index_state *istate)
+static int is_index_unchanged(struct repository *r)
{
struct object_id head_oid, *cache_tree_oid;
struct commit *head_commit;
+ struct index_state *istate = r->index;
if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, _oid, NULL))
return error(_("could not resolve HEAD commit"));
- head_commit = lookup_commit(the_repository, _oid);
+ head_commit = lookup_commit(r, _oid);
/*
* If head_commit is NULL, check_commit, called from
@@ -1221,7 +1223,7 @@ void print_commit_summary(struct
[PATCH 09/19] repository.c: replace hold_locked_index() with repo_hold_locked_index()
hold_locked_index() assumes the index path at $GIT_DIR/index. This is
not good for places that take an arbitrary index_state instead of
the_index, which is basically everywhere except builtin/.
Replace it with repo_hold_locked_index(). hold_locked_index() remains
as a wrapper around repo_hold_locked_index() to reduce changes in builtin/
Signed-off-by: Nguyễn Thái Ngọc Duy
---
apply.c | 3 ++-
builtin/clone.c | 1 +
cache.h | 2 +-
merge-recursive.c | 2 +-
merge.c | 2 +-
read-cache.c | 5 -
repository.c | 8
repository.h | 4
rerere.c | 2 +-
sequencer.c | 8
wt-status.c | 2 +-
11 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/apply.c b/apply.c
index fdae1d423b..122e6ddf92 100644
--- a/apply.c
+++ b/apply.c
@@ -4710,7 +4710,8 @@ static int apply_patch(struct apply_state *state,
state->index_file,
LOCK_DIE_ON_ERROR);
else
- hold_locked_index(>lock_file, LOCK_DIE_ON_ERROR);
+ repo_hold_locked_index(state->repo, >lock_file,
+ LOCK_DIE_ON_ERROR);
}
if (state->check_index && read_apply_cache(state) < 0) {
diff --git a/builtin/clone.c b/builtin/clone.c
index 15b142d646..dfefe85770 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -8,6 +8,7 @@
* Clone a repository into a different directory that does not yet exist.
*/
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
#include "builtin.h"
#include "config.h"
#include "lockfile.h"
diff --git a/cache.h b/cache.h
index 59c8a93046..ef2483ce45 100644
--- a/cache.h
+++ b/cache.h
@@ -433,6 +433,7 @@ void validate_cache_entries(const struct index_state
*istate);
#define unmerge_cache_entry_at(at) unmerge_index_entry_at(_index, at)
#define unmerge_cache(pathspec) unmerge_index(_index, pathspec)
#define read_blob_data_from_cache(path, sz)
read_blob_data_from_index(_index, (path), (sz))
+#define hold_locked_index(lock_file, flags)
repo_hold_locked_index(the_repository, (lock_file), (flags))
#endif
#define TYPE_BITS 3
@@ -826,7 +827,6 @@ extern struct cache_entry *refresh_cache_entry(struct
index_state *, struct cach
*/
extern void update_index_if_able(struct index_state *, struct lock_file *);
-extern int hold_locked_index(struct lock_file *, int);
extern void set_alternate_index_output(const char *);
extern int verify_index_checksum;
diff --git a/merge-recursive.c b/merge-recursive.c
index c0fb83d285..c186812ff2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3510,7 +3510,7 @@ int merge_recursive_generic(struct merge_options *o,
}
}
- hold_locked_index(, LOCK_DIE_ON_ERROR);
+ repo_hold_locked_index(the_repository, , LOCK_DIE_ON_ERROR);
clean = merge_recursive(o, head_commit, next_commit, ca,
result);
if (clean < 0) {
diff --git a/merge.c b/merge.c
index 91008f7602..dbbc9d9f80 100644
--- a/merge.c
+++ b/merge.c
@@ -58,7 +58,7 @@ int checkout_fast_forward(struct repository *r,
refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
- if (hold_locked_index(_file, LOCK_REPORT_ON_ERROR) < 0)
+ if (repo_hold_locked_index(r, _file, LOCK_REPORT_ON_ERROR) < 0)
return -1;
memset(, 0, sizeof(trees));
diff --git a/read-cache.c b/read-cache.c
index 1df5c16dbc..7ad3330942 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1726,11 +1726,6 @@ static int read_index_extension(struct index_state
*istate,
return 0;
}
-int hold_locked_index(struct lock_file *lk, int lock_flags)
-{
- return hold_lock_file_for_update(lk, get_index_file(), lock_flags);
-}
-
int read_index(struct index_state *istate)
{
return read_index_from(istate, get_index_file(), get_git_dir());
diff --git a/repository.c b/repository.c
index 5dd1486718..39b5a80d43 100644
--- a/repository.c
+++ b/repository.c
@@ -3,6 +3,7 @@
#include "object-store.h"
#include "config.h"
#include "object.h"
+#include "lockfile.h"
#include "submodule-config.h"
/* The main repository */
@@ -257,3 +258,10 @@ int repo_read_index(struct repository *repo)
return read_index_from(repo->index, repo->index_file, repo->gitdir);
}
+
+int repo_hold_locked_index(struct repository *repo,
+ struct lock_file *lf,
+ int flags)
+{
+ return hold_lock_file_for_update(lf, repo->index_file, flags);
+}
diff --git a/repository.h b/repository.h
index 9f16c42c1e..968330218f 100644
--- a/repository.h
+++ b/repository.h
@@ -6,6 +6,7 @@
struct config_set;
struct git_hash_algo;
struct index_state;
+struct lock_file;
struct raw_object_store;
struct submodule_cache;
@@ -130,5 +131,8 @@ void repo_clear(struct repository *repo);
* populated then the number of entries
[PATCH 04/19] list-objects.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
---
list-objects.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index f9d36dabf2..c3a1a9b004 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -51,7 +51,7 @@ static void process_blob(struct rev_info *revs,
if (!(obj->flags & USER_GIVEN) && filter_fn)
r = filter_fn(LOFS_BLOB, obj,
path->buf, >buf[pathlen],
- _index,
+ revs->repo->index,
filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
@@ -137,7 +137,7 @@ static void process_tree(struct rev_info *revs,
if (!(obj->flags & USER_GIVEN) && filter_fn)
r = filter_fn(LOFS_BEGIN_TREE, obj,
base->buf, >buf[baselen],
- _index,
+ revs->repo->index,
filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
@@ -177,7 +177,7 @@ static void process_tree(struct rev_info *revs,
if (!(obj->flags & USER_GIVEN) && filter_fn) {
r = filter_fn(LOFS_END_TREE, obj,
base->buf, >buf[baselen],
- _index,
+ revs->repo->index,
filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
--
2.19.1.647.g708186aaf9
[PATCH 02/19] wt-status.c: remove implicit dependency the_repository
Signed-off-by: Nguyễn Thái Ngọc Duy
---
ref-filter.c | 2 +-
wt-status.c | 18 ++
wt-status.h | 4 +++-
3 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 2a05619211..d6d3923eb2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1412,7 +1412,7 @@ char *get_head_description(void)
struct strbuf desc = STRBUF_INIT;
struct wt_status_state state;
memset(, 0, sizeof(state));
- wt_status_get_state(, 1);
+ wt_status_get_state(the_repository, , 1);
if (state.rebase_in_progress ||
state.rebase_interactive_in_progress) {
if (state.branch)
diff --git a/wt-status.c b/wt-status.c
index 0378bc2a48..3632276236 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -756,7 +756,7 @@ void wt_status_collect(struct wt_status *s)
wt_status_collect_changes_index(s);
wt_status_collect_untracked(s);
- wt_status_get_state(>state, s->branch && !strcmp(s->branch, "HEAD"));
+ wt_status_get_state(s->repo, >state, s->branch && !strcmp(s->branch,
"HEAD"));
if (s->state.merge_in_progress && !has_unmerged(s))
s->committable = 1;
}
@@ -1483,7 +1483,8 @@ static int grab_1st_switch(struct object_id *ooid, struct
object_id *noid,
return 1;
}
-static void wt_status_get_detached_from(struct wt_status_state *state)
+static void wt_status_get_detached_from(struct repository *r,
+ struct wt_status_state *state)
{
struct grab_1st_switch_cbdata cb;
struct commit *commit;
@@ -1500,7 +1501,7 @@ static void wt_status_get_detached_from(struct
wt_status_state *state)
/* sha1 is a commit? match without further lookup */
(oideq(, ) ||
/* perhaps sha1 is a tag, try to dereference to a commit */
-((commit = lookup_commit_reference_gently(the_repository, ,
1)) != NULL &&
+((commit = lookup_commit_reference_gently(r, , 1)) != NULL &&
oideq(, >object.oid {
const char *from = ref;
if (!skip_prefix(from, "refs/tags/", ))
@@ -1557,30 +1558,31 @@ int wt_status_check_bisect(const struct worktree *wt,
return 0;
}
-void wt_status_get_state(struct wt_status_state *state,
+void wt_status_get_state(struct repository *r,
+struct wt_status_state *state,
int get_detached_from)
{
struct stat st;
struct object_id oid;
- if (!stat(git_path_merge_head(the_repository), )) {
+ if (!stat(git_path_merge_head(r), )) {
state->merge_in_progress = 1;
} else if (wt_status_check_rebase(NULL, state)) {
; /* all set */
- } else if (!stat(git_path_cherry_pick_head(the_repository), ) &&
+ } else if (!stat(git_path_cherry_pick_head(r), ) &&
!get_oid("CHERRY_PICK_HEAD", )) {
state->cherry_pick_in_progress = 1;
oidcpy(>cherry_pick_head_oid, );
}
wt_status_check_bisect(NULL, state);
- if (!stat(git_path_revert_head(the_repository), ) &&
+ if (!stat(git_path_revert_head(r), ) &&
!get_oid("REVERT_HEAD", )) {
state->revert_in_progress = 1;
oidcpy(>revert_head_oid, );
}
if (get_detached_from)
- wt_status_get_detached_from(state);
+ wt_status_get_detached_from(r, state);
}
static void wt_longstatus_print_state(struct wt_status *s)
diff --git a/wt-status.h b/wt-status.h
index 84653595e3..9dde7091f6 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -134,7 +134,9 @@ void wt_status_prepare(struct wt_status *s, struct
repository *repo);
void wt_status_print(struct wt_status *s);
void wt_status_collect(struct wt_status *s);
void wt_status_collect_free_buffers(struct wt_status *s);
-void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
+void wt_status_get_state(struct repository *repo,
+struct wt_status_state *state,
+int get_detached_from);
int wt_status_check_rebase(const struct worktree *wt,
struct wt_status_state *state);
int wt_status_check_bisect(const struct worktree *wt,
--
2.19.1.647.g708186aaf9
[PATCH 01/19] wt-status.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/commit.c | 2 +-
builtin/pull.c | 3 +-
sequencer.c | 6 ++--
wt-status.c | 77 +++-
wt-status.h | 17 +++
5 files changed, 62 insertions(+), 43 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 074bd9a551..379d8c5cdf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -185,7 +185,7 @@ static void determine_whence(struct wt_status *s)
static void status_init_config(struct wt_status *s, config_fn_t fn)
{
- wt_status_prepare(s);
+ wt_status_prepare(s, the_repository);
init_diff_ui_defaults();
git_config(fn, s);
determine_whence(s);
diff --git a/builtin/pull.c b/builtin/pull.c
index 798ecf7faf..9b0fce07ca 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -888,7 +888,8 @@ int cmd_pull(int argc, const char **argv, const char
*prefix)
die(_("Updating an unborn branch with changes added to
the index."));
if (!autostash)
- require_clean_work_tree(N_("pull with rebase"),
+ require_clean_work_tree(the_repository,
+ N_("pull with rebase"),
_("please commit or stash them."), 1, 0);
if (get_rebase_fork_point(_fork_point, repo, *refspecs))
diff --git a/sequencer.c b/sequencer.c
index 0c164d5f98..90435593a4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2712,7 +2712,7 @@ static int do_exec(const char *command_line)
if (discard_cache() < 0 || read_cache() < 0)
return error(_("could not read index"));
- dirty = require_clean_work_tree("rebase", NULL, 1, 1);
+ dirty = require_clean_work_tree(the_repository, "rebase", NULL, 1, 1);
if (status) {
warning(_("execution failed: %s\n%s"
@@ -3583,10 +3583,10 @@ static int commit_staged_changes(struct replay_opts
*opts,
unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
unsigned int final_fixup = 0, is_clean;
- if (has_unstaged_changes(1))
+ if (has_unstaged_changes(the_repository, 1))
return error(_("cannot rebase: You have unstaged changes."));
- is_clean = !has_uncommitted_changes(0);
+ is_clean = !has_uncommitted_changes(the_repository, 0);
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
diff --git a/wt-status.c b/wt-status.c
index 187568a112..0378bc2a48 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -119,9 +119,11 @@ static void status_printf_more(struct wt_status *s, const
char *color,
va_end(ap);
}
-void wt_status_prepare(struct wt_status *s)
+void wt_status_prepare(struct wt_status *s,
+ struct repository *repo)
{
memset(s, 0, sizeof(*s));
+ s->repo = repo;
memcpy(s->color_palette, default_wt_status_colors,
sizeof(default_wt_status_colors));
s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
@@ -494,19 +496,19 @@ static void wt_status_collect_changed_cb(struct
diff_queue_struct *q,
}
}
-static int unmerged_mask(const char *path)
+static int unmerged_mask(struct index_state *istate, const char *path)
{
int pos, mask;
const struct cache_entry *ce;
- pos = cache_name_pos(path, strlen(path));
+ pos = index_name_pos(istate, path, strlen(path));
if (0 <= pos)
return 0;
mask = 0;
pos = -pos-1;
- while (pos < active_nr) {
- ce = active_cache[pos++];
+ while (pos < istate->cache_nr) {
+ ce = istate->cache[pos++];
if (strcmp(ce->name, path) || !ce_stage(ce))
break;
mask |= (1 << (ce_stage(ce) - 1));
@@ -566,7 +568,8 @@ static void wt_status_collect_updated_cb(struct
diff_queue_struct *q,
s->committable = 1;
break;
case DIFF_STATUS_UNMERGED:
- d->stagemask = unmerged_mask(p->two->path);
+ d->stagemask = unmerged_mask(s->repo->index,
+p->two->path);
/*
* Don't bother setting {mode,oid}_{head,index} since
the print
* code will output the stage values directly and not
use the
@@ -585,7 +588,7 @@ static void wt_status_collect_changes_worktree(struct
wt_status *s)
{
struct rev_info rev;
- repo_init_revisions(the_repository, , NULL);
+ repo_init_revisions(s->repo, , NULL);
setup_revisions(0, NULL, , NULL);
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
rev.diffopt.flags.dirty_submodules = 1;
@@ -610,7 +613,7 @@ static void wt_status_collect_changes_index(struct
wt_status *s)
struct rev_info rev;
struct
Re: [PATCH] send-email: explicitly disable authentication
On Thu, Oct 18, 2018 at 6:02 PM Joshua Watt wrote: > On Thu, 2018-10-18 at 17:53 -0400, Eric Sunshine wrote: > > On Thu, Oct 18, 2018 at 5:16 PM Joshua Watt > > wrote: > > Implementation complexity aside, spelling the option --no-smtp-auth > > might be more intuitive and consistent than --smtp-auth=none. > > One advantage of --smtp-auth=none is that it can also be done with a > config variable sendemail.smtpauth="none". Would be also add a config > variable like sendemail.nosmtpauth (the negative seems strange to me)? I would not expect to see a "negating" config variable like that. I was just suggesting that a "--no-*" command-line option might be more intuitive. > Or maybe --no-smtp-auth is just a shorthand alias for --smtp-auth=none? That's one possibility.
Re: Git wire protocol v2 fails fetching shallow changes with `pack has XXX unresolved deltas` on large repos with lots of tags Inbox x
Hello, Unfortunately I do not have for such step. On one of our internal repositories it is failing but unreliably when fetching from remote. -- Arturas Moskvinas On Wed, Oct 17, 2018 at 11:53 PM Jonathan Tan wrote: > > > No changes are needed in mirrored repository. Crash happens both with > > 2.18.0 and 2.19.1 git versions. Having repository locally is not > > required but reduces test runtime, you can quite reliably reproduce > > issue when fetching over net directly from chromium.orgbypassing > > mirroring step. > > Do you have the reproduction steps for this? If I run > > git -c protocol.version=2 fetch --tags \ > https://chromium.googlesource.com/chromium/src \ > +refs/heads/*:refs/remotes/origin/* --depth=1 > > repeatedly in the same repository, it succeeds. (And I've checked with > GIT_TRACE_PACKET that it uses protocol v2.)
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
On 10/19/2018 2:02 AM, Junio C Hamano wrote: * ds/ci-commit-graph-and-midx (2018-10-19) 1 commit - ci: add optional test variables One of our CI tests to run with "unusual/experimental/random" settings now also uses commti-graph and midx. Will merge to 'next'. s/commti-graph/commit-graph/ * ds/test-multi-pack-index (2018-10-09) 3 commits - multi-pack-index: define GIT_TEST_MULTI_PACK_INDEX - midx: close multi-pack-index on repack - midx: fix broken free() in close_midx() Tests for the recently introduced multi-pack index machinery. Expecting a reroll. cf. <8b5dbe3d-b382-bf48-b524-d9e8a074a...@gmail.com> A reroll exists: https://public-inbox.org/git/pull.27.v2.git.gitgitgad...@gmail.com/ Thanks, -Stolee
Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)
On Fri, Oct 19, 2018 at 03:02:22PM +0900, Junio C Hamano wrote:
> Two large set of topics on "rebase in C" and "rebase -i in C" are
> now in 'next'.
I see occasional failures in 't5520-pull.sh':
expecting success:
test_config rebase.autostash true &&
test_pull_autostash --rebase
+ test_config rebase.autostash true
+ config_dir=
+ test rebase.autostash = -C
+ test_when_finished test_unconfig 'rebase.autostash'
+ test 0 = 0
+ test_cleanup={ test_unconfig 'rebase.autostash'
} && (exit "$eval_ret"); eval_ret=$?; :
+ git config rebase.autostash true
+ test_pull_autostash --rebase
+ git reset --hard before-rebase
HEAD is now at 12212b3 new file
+ echo dirty
+ git add new_file
+ git pull --rebase . copy
>From .
* branchcopy -> FETCH_HEAD
Created autostash: 5417697
HEAD is now at 12212b3 new file
First, rewinding head to replay your work on top of it...
Applying: new file
Applying autostash resulted in conflicts.
Your changes are safe in the stash.
You can run "git stash pop" or "git stash drop" at any time.
+ test_cmp_rev HEAD^ copy
+ git rev-parse --verify HEAD^
+ git rev-parse --verify copy
+ test_cmp expect.rev actual.rev
+ diff -u expect.rev actual.rev
+ cat new_file
cat: new_file: No such file or directory
+ test = dirty
error: last command exited with $?=1
not ok 25 - pull --rebase succeeds with dirty working directory and
rebase.autostash set
When running t5520 in a loop, it tends to fail between 10-40
iterations, even when the machine is not under heavy load.
It appears that these failures started with commit 5541bd5b8f (rebase:
default to using the builtin rebase, 2018-08-08), i.e. tip of
'pk/rebase-in-c-6-final', but it's a "flip the big switch" commit, so
not very useful...
Bug with "git mv" and submodules, and with "git submodule add something --force"
Hi there, tested on both, git 2.18 and git 2.19.1: moving a file with `git mv` from a project to a submodule results in an **undefined state** of the local repository. It breaks up the submodule (it's still in .gitmodules, but not accessable via `git submodule`), and is not reversible on local repository. Either `git mv submodule/file .` nor deleting the folder works. For the locale repo the submodule is gone. But: trying to add it with `git submodule add` also do not work and results in an error message (with and without `--force` flag): $ git submodule add g...@github.com:---/wiki-public.git public --force A git directory for 'public' is found locally with remote(s): origin g...@github.com:---/wiki-public.git If you want to reuse this local git directory instead of cloning again from g...@github.com:---/wiki-public.git use the '--force' option. If the local git directory is not the correct repo or you are unsure what this means choose another name with the '--name' option. Therefore, it's in a undefined, broken state. Another bug I've got by testing upper line: * --force will be used as folder name * when used in `git submodule add g...@github.com:someone/some.git --force`: $ git submodule add g...@github.com:---/wiki-public.git --force Cloning into '/home/---/---/---/---/wiki-internal.wiki/--force'... remote: Enumerating objects: 29, done. remote: Counting objects: 100% (29/29), done. remote: Compressing objects: 100% (25/25), done. remote: Total 29 (delta 5), reused 20 (delta 2), pack-reused 0 Receiving objects: 100% (29/29), 37.03 KiB | 421.00 KiB/s, done. Resolving deltas: 100% (5/5), done. /usr/libexec/git-core/git-submodule: line 273: cd: --: invalid option cd: usage: cd [-L|-P] [dir] Unable to checkout submodule '--force' but it creates the `--force` folder: $ tree . ├── --force Best, Jürgen Vogl
Re: [PATCH 1/1] commit-reach: fix first-parent heuristic
On 10/19/2018 1:24 AM, Junio C Hamano wrote: "Derrick Stolee via GitGitGadget" writes: We can also re-run the performance tests from commit 4fbcca4e "commit-reach: make can_all_from_reach... linear". Performance was measured on the Linux repository using 'test-tool reach can_all_from_reach'. The input included rows seeded by tag values. The "small" case included X-rows as v4.[0-9]* and Y-rows as v3.[0-9]*. This mimics a (very large) fetch that says "I have all major v3 releases and want all major v4 releases." The "large" case included X-rows as "v4.*" and Y-rows as "v3.*". This adds all release-candidate tags to the set, which does not greatly increase the number of objects that are considered, but does increase the number of 'from' commits, demonstrating the quadratic nature of the previous code. These micro-benchmarks are interesting, but we should also remember to describe the impact of the bug being fixed in the larger picture. What end-user visible operations are affected? Computing merge-base? Finding if a push fast-forwards? Something else? Sorry about that. Here is a description that could be inserted into the commit message: The can_all_from_reach() method synthesizes the logic for one iteration of can_all_from_reach_with_flags() which is used by the server during fetch negotiation to determine if more haves/wants are needed. The logic is also used by is_descendant_of() which is used to check if a force-push is required and in 'git branch --contains'. Thanks, -Stolee
Mirror of git.git on gitlab.com
On Thu, Oct 18 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > >>> sites could do the same polling and mirroring. I am just too lazy >>> to open a new account at yet another hosting site to add that for >>> loop, but I may choose to when I am absolutely bored and nothing >>> else to do ;-). >> >> Do you mind if I squat gitlab.com/git/git in the meantime (i.e. create >> an org etc.) and have it mirror github.com/git/git?, I'll hand the > > Obviously somebody who is not even interested in obtaining an > account would appreciate, not just "would not mind", if a trusted > member in the community did that for the community ;-) I've set this up at https://gitlab.com/git-vcs The /git namespace was taken (and I asked GitLab support if it was stale, they said no). Also tried /git-scm and /gitscm, ditto. So I settled on /git-vcs (version control system). I mirrored all the repos that were on github.com/git (except for cabal.git of course). This is being kept up-to-date with GitLab's own mirroring feature, so it should always be max ~15m out of date v.s. the GitHub version. As an aside, I noticed that https://github.com/git/sha1collisiondetection/ has never worked in combination with git.git, i.e. it's cloned at a version that pre-dates the initial introduction of the sha1collisiondetection submodule. Our other mirrors don't seem to have it at all relative to ../sha1collisiondetection.git from their git.git. Junio: What was the plan with that? It's never been used in combination with git.git, so maybe we should just drop it? If we'd like to keep it and if it was kept up-to-date some scripts of yours we could use a relative URL in the .gitmodules, so e.g. if you cloned from kernel.org you'd also get the submodule from there. Also since we're fixing it, it would be good to delete the existing repo and re-make it by clicking "fork" on https://github.com/cr-marcstevens/sha1collisiondetection so GitHub shows "forked from...", i.e. it's associated with the parent project in GitHub's UI.
Re: [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
On Fri, Oct 19, 2018 at 05:16:46PM +0900, Junio C Hamano wrote:
> SZEDER Gábor writes:
>
> >>if (entry)
> >> - fprintf(out, "\n%c Branch %s\n", comment_line_char,
> >> entry->string);
> >> + strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
> >> entry->string);
> >>else
> >> - fprintf(out, "\n");
> >> + strbuf_addf(out, "\n");
> >
> > Please use plain strbuf_add() here.
>
> FWIW, contrib/coccinelle/strbuf.cocci.patch gave us this:
>
> diff -u -p a/sequencer.c b/sequencer.c
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4311,7 +4311,7 @@ static int make_script_with_merges(struc
> if (entry)
> strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
> entry->string);
> else
> - strbuf_addf(out, "\n");
> + strbuf_addstr(out, "\n");
>
> while (oidset_contains(, >object.oid) &&
> !oidset_contains(, >object.oid)) {
Uh, right. I didn't want to copy-paste a patch with too long lines
into my mailer, as it usually doesn't end too good, so I just typed
the function name. Evidently I couldn't quite manage.
Come meet Japan’s big buyers! - GIFTEX TOKYO 2019 [January]
Dear International Sales & Marketing Director, Zhejiang Wuchuan Industrial Co., Ltd Hello, this is Eiichi Hasegawa from GIFTEX TOKYO Show Management. Are you interested in meeting more buyers in Japan? If yes, GIFTEX TOKYO 2019 [January], Japan's leading trade fair for gifts, lifestyle goods & homeware is the best platform for you! https://www.lifestyle-expo-spring.jp/giftex_en/ The show has expanded its scale every year as a must-attend business event for buyers from all over Japan. Meet our high quality visitors! The following is an excerpt of major buyers that visited the previous show: --- Major Japanese Buyers -- - Importers/Distributors ANA TRADING, ITOCHU, MARUBENI, MITSUBISHI, MITSUI & CO., etc. - Gift Shops AFTERNOON TEA, BLEU BLEUET, INOBUN, CUISINE HABITS, etc. - Lifestyle Shops LOFT, MUJI, PLAZA, TOKYU HANDS, FRANCFRANC, etc. - Department Stores DAIMARU MATSUZAKAYA, ISETAN MITSUKOSHI, ODAKYU, TAKASHIMAYA, etc. - GIFTEX TOKYO gathers key buyers, of which 83.1 %(*) of visitors were decision makers with purchasing authorities. If you are interested in expanding your business in Japan, why not start by exhibiting at GIFTEX TOKYO 2019 [January]? To receive information on exhibiting, please reply by filling in the following REPLY FORM. Show Management will assist you with the latest booth availability, cost estimation and any other inquiries. == Reply Form mailto:lifestyle-...@reedexpo.co.jp Company Name: Contact Person: Email: TEL: Main Products: Your Request ( ) Cost Information ( ) Available Booth Locations ( ) Information on Packaged Booths ( ) Other ( ) === We are looking forward to hearing from you soon. Sincerely, Eiichi Hasegawa (Mr.), Chisato Miyawaki (Ms.), Mikako Shimada (Ms.), Qu Jun (Mr.), Choi Ilyong (Mr.) GIFTEX TOKYO Show Management Reed Exhibitions Japan Ltd. TEL: +81-3-3349-8505 mailto:lifestyle-...@reedexpo.co.jp -- Show Information --- GIFTEX TOKYO 2019 [January] - 2nd Variety-Gifts Expo Held Inside: LIFESTYLE EXPO TOKYO 2019 [January] Dates: Jan. 30 (Wed.) - Feb. 1 (Fri.), 2019 Venue: Makuhari Messe, Japan https://www.lifestyle-expo-spring.jp/giftex_en/ --- GIFTEX TOKYO 2019 [June] - 14th Variety-Gifts Expo Held Inside: LIFESTYLE EXPO TOKYO 2019 [June] Dates: June 26 (Wed.) - 28 (Fri.), 2019 Venue: Tokyo Big Sight, Japan https://www.lifestyle-expo.jp/giftex_en/ --- ID: E36-G1402-0075 (* Figures from 2018 January show with total visitor number of 50,287, including concurrent shows) This message is delivered to you to provide details of exhibitions and conferences organized, co-organized, or managed by Reed Exhibitions Japan Ltd. If you would like to change your contact information, or prefer not to receive further information on this exhibition/conference, please follow the directions below. Please click the URL below and follow the directions on the website to update your e-mail and other information. https://contact.reedexpo.co.jp/expo/REED/?lg=en=ch=CHANGE Please reply to this mail changing the subject to "REMOVE FROM LIST". You will not receive any further information on this exhibition/conference.
Re: [PATCH v2 02/13] ci/lib.sh: encapsulate Travis-specific things
SZEDER Gábor writes: >> >> +if test true = "$TRAVIS" >> >> +then >> >> +... >> >> + export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" >> >> + export GIT_TEST_OPTS="--verbose-log -x --immediate" >> >> +fi >> > ... > > $GIT_PROVE_OPTS and $GIT_TEST_OPTS, however, are only used in > 't/Makefile' but not in the build scripts, thus their values don't > show up in the build logs. > > I run some Travis CI builds with custom $GIT_TEST_OPTS, which, of > course, conflicted with this patch, and I messed up the conflict > resolution. I think I would have noticed sooner what went wrong, if > the value of $GIT_TEST_OPTS were visible in the build logs. > > I've found the output with 'set -x' sufficient so far, and don't think > that an explicit logging facility is worth it. That's an interesting perspective. I would have thoguht that the values of these two variables we can see above, that are constants without any substitution or customization, are the least interesting things to see in the log.
Re: [PATCH] alias: detect loops in mixed execution mode
On Thu, Oct 18 2018, Ævar Arnfjörð Bjarmason wrote:
> +static void init_cmd_history(struct strbuf *env, struct string_list
> *cmd_list)
> +{
> + const char *old = getenv(COMMAND_HISTORY_ENVIRONMENT);
> + struct strbuf **cmd_history, **ptr;
> +
> + if (!old || !*old)
> + return;
> +
> + strbuf_addstr(env, old);
> + strbuf_rtrim(env);
> +
> + cmd_history = strbuf_split_buf(old, strlen(old), ' ', 0);
> + for (ptr = cmd_history; *ptr; ptr++) {
> + strbuf_rtrim(*ptr);
> + string_list_append(cmd_list, (*ptr)->buf);
> + }
> + strbuf_list_free(cmd_history);
> +}
> +
> +static void add_cmd_history(struct strbuf *env, struct string_list *cmd_list,
> + const char *cmd)
> +{
> + string_list_append(cmd_list, cmd);
> + if (env->len)
> + strbuf_addch(env, ' ');
> + strbuf_addstr(env, cmd);
> + setenv(COMMAND_HISTORY_ENVIRONMENT, env->buf, 1);
> +}
> +
> static int run_argv(int *argcp, const char ***argv)
> {
> int done_alias = 0;
> - struct string_list cmd_list = STRING_LIST_INIT_NODUP;
> + struct string_list cmd_list = STRING_LIST_INIT_DUP;
> struct string_list_item *seen;
> + struct strbuf env = STRBUF_INIT;
>
> + init_cmd_history(, _list);
> while (1) {
> /*
>* If we tried alias and futzed with our environment,
> @@ -711,7 +742,7 @@ static int run_argv(int *argcp, const char ***argv)
> " not terminate:%s"), cmd_list.items[0].string,
> sb.buf);
> }
>
> - string_list_append(_list, *argv[0]);
> + add_cmd_history(, _list, *argv[0]);
>
> /*
>* It could be an alias -- this works around the insanity
Just to sanity check an assumption of mine: One thing I didn't do is use
sq_quote_buf() and sq_dequote_to_argv() like we do for
CONFIG_DATA_ENVIRONMENT. This is because in the case of config we need
to deal with:
$ git config alias.cfgdump
!env
$ git -c x.y=z -c "foo.bar='baz'" cfgdump|grep baz
GIT_CONFIG_PARAMETERS='x.y=z' 'foo.bar='\''baz'\'''
But in this case I don't see how a command-name would ever contain
whitespace. So we skip quoting and just delimit by space.
There's also nothing stopping you from doing e.g.:
$ GIT_COMMAND_HISTORY='foo bar' ~/g/git/git --exec-path=$PWD one
fatal: alias loop detected: expansion of 'foo' does not terminate:
foo
bar
one
two <==
three
four
five ==>
Or even confuse the code by adding a whitespace at the beginning:
$ GIT_COMMAND_HISTORY=' foo bar' ~/g/git/git --exec-path=$PWD one
fatal: alias loop detected: expansion of '' does not terminate:
foo
bar
one
two <==
three
four
five ==>
I thought none of this was worth dealing with. Worst case someone's
screwing with this, but I don't see how it would happen accidentally,
and even then we detect the infinite loop and just degrade to confusing
error messages because you decided to screw with git's GIT_* env vars.
Re: [PATCH v2 02/13] ci/lib.sh: encapsulate Travis-specific things
On Fri, Oct 19, 2018 at 11:06:25AM +0900, Junio C Hamano wrote: > SZEDER Gábor writes: > > > On Mon, Oct 15, 2018 at 03:12:00AM -0700, Johannes Schindelin via > > GitGitGadget wrote: > >> diff --git a/ci/lib.sh b/ci/lib.sh > >> index 06970f7213..8532555b4e 100755 > >> --- a/ci/lib.sh > >> +++ b/ci/lib.sh > >> @@ -1,5 +1,26 @@ > >> # Library of functions shared by all CI scripts > >> > >> +if test true = "$TRAVIS" > >> +then > >> +... > >> + export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save" > >> + export GIT_TEST_OPTS="--verbose-log -x --immediate" > >> +fi > > > > Please set all these variables ... > > Do you mean "VAR=VAL; export VAR" is kosher, "export VAR=VAL" is > not? > > >> @@ -81,7 +102,6 @@ check_unignored_build_artifacts () > >> # and installing dependencies. > >> set -ex > > > > ... after we turn on 'set -x', so the variables' values will be > > visible in the logs. > > Ah, no, you didn't. Although I think both are valid points, I think > ci/lib.sh is expected to be used only inside a more predictable > environment (e.g. we know the shell used is not a random POSIX shell > but one that is happy with "export VAR=VAL"), so it should be OK. Yes. Travis CI runs an Ubuntu LTS, where /bin/sh is dash, which understands 'export VAR=val' just fine. I don't know what Linux distro runs on Azure Pipelines, but the build definition in patch 6 explicitly asks for Bash, so that should be fine as well. > Showing the values of these variables in the log may still be good > idea. > > > (Or move this 'set -ex' to the beginning of the script? Then we > > could perhaps avoid similar issues in the future.) > > Sure (provided that it is an issue to begin with---if we are > interested in the value of TRAVIS_BRANCH, for example, being able to > see it only because "CI_BRANCH=$TRAVIS_BRANCH" assignment is made > feels a bit sideways---we'd be better off explicitly logging > anything we are interested in in the longer term, no?). Well, all the $TRAVIS_* and $CI_* variables are not that interessant, because they are only used in the build scripts, and then we can see their substituted values in the build logs. The same applies to the variables $cache_dir and $BREW_INSTALL_PACKAGES as well. $GIT_PROVE_OPTS and $GIT_TEST_OPTS, however, are only used in 't/Makefile' but not in the build scripts, thus their values don't show up in the build logs. I run some Travis CI builds with custom $GIT_TEST_OPTS, which, of course, conflicted with this patch, and I messed up the conflict resolution. I think I would have noticed sooner what went wrong, if the value of $GIT_TEST_OPTS were visible in the build logs. I've found the output with 'set -x' sufficient so far, and don't think that an explicit logging facility is worth it.
Re: [PATCH 07/15] sequencer: make sequencer_make_script() write its script to a strbuf
SZEDER Gábor writes:
>> if (entry)
>> -fprintf(out, "\n%c Branch %s\n", comment_line_char,
>> entry->string);
>> +strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
>> entry->string);
>> else
>> -fprintf(out, "\n");
>> +strbuf_addf(out, "\n");
>
> Please use plain strbuf_add() here.
FWIW, contrib/coccinelle/strbuf.cocci.patch gave us this:
diff -u -p a/sequencer.c b/sequencer.c
--- a/sequencer.c
+++ b/sequencer.c
@@ -4311,7 +4311,7 @@ static int make_script_with_merges(struc
if (entry)
strbuf_addf(out, "\n%c Branch %s\n", comment_line_char,
entry->string);
else
- strbuf_addf(out, "\n");
+ strbuf_addstr(out, "\n");
while (oidset_contains(, >object.oid) &&
!oidset_contains(, >object.oid)) {
Re: [PATCH 00/19] Bring more repository handles into our code base
Stefan Beller writes: > This rerolls sb/more-repo-in-api. > It applies on nd/the-index merged with ds/reachable and is available via > git fetch https://github.com/stefanbeller/git object-store-final-3 Thanks. Luckily we have both of these prerequisites in 'master' now, o hopefully this can be applied to 'master' directly and would play well when merged to 'next' and to 'pu'. Will queue and play with it a bit before sending comments.
What's cooking in git.git (Oct 2018, #04; Fri, 19)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Two large set of topics on "rebase in C" and "rebase -i in C" are now in 'next'. A handful of improvements around "git help", "git grep", and "git status" are in 'master', as well as other internal changes. Note that "cf. " are not meant as "clear all of these and you are home free". They are primarily to prevent me from merging topics that are not ready to 'next' by mistake and remind me where to go back to read the last state of the discussion in the archive. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bp/read-cache-parallel (2018-10-11) 7 commits (merged to 'next' on 2018-10-12 at ed6edde799) + read-cache: load cache entries on worker threads + ieot: add Index Entry Offset Table (IEOT) extension + read-cache: load cache extensions on a worker thread + config: add new index.threads config setting + eoie: add End of Index Entry (EOIE) extension + read-cache: clean up casting and byte decoding + read-cache.c: optimize reading index format v4 A new extension to the index file has been introduced, which allows the file to be read in parallel. * bp/rename-test-env-var (2018-09-28) 6 commits (merged to 'next' on 2018-10-12 at 201e451d20) + t: do not get self-test disrupted by environment warnings + preload-index: update GIT_FORCE_PRELOAD_TEST support + read-cache: update TEST_GIT_INDEX_VERSION support + fsmonitor: update GIT_TEST_FSMONITOR support + preload-index: use git_env_bool() not getenv() for customization + t/README: correct spelling of "uncommon" Some environment variables that control the runtime options of Git used during tests are getting renamed for consistency. * ds/commit-graph-leakfix (2018-10-07) 3 commits (merged to 'next' on 2018-10-12 at 8cc7f2f1e9) + commit-graph: reduce initial oid allocation + builtin/commit-graph.c: UNLEAK variables + commit-graph: clean up leaked memory during write Code clean-up. * jc/how-to-document-api (2018-09-29) 1 commit (merged to 'next' on 2018-10-12 at 7c9bd82285) + CodingGuidelines: document the API in *.h files Doc update. * jt/avoid-ls-refs (2018-10-07) 4 commits (merged to 'next' on 2018-10-12 at 5775aabbc1) + fetch: do not list refs if fetching only hashes + transport: list refs before fetch if necessary + transport: do not list refs if possible + transport: allow skipping of ref listing Over some transports, fetching objects with an exact commit object name can be done without first seeing the ref advertisements. The code has been optimized to exploit this. * jt/cache-tree-allow-missing-object-in-partial-clone (2018-10-10) 1 commit (merged to 'next' on 2018-10-12 at 152ad8e336) + cache-tree: skip some blob checks in partial clone In a partial clone that will lazily be hydrated from the originating repository, we generally want to avoid "does this object exist (locally)?" on objects that we deliberately omitted when we created the clone. The cache-tree codepath (which is used to write a tree object out of the index) however insisted that the object exists, even for paths that are outside of the partial checkout area. The code has been updated to avoid such a check. * jt/fetch-tips-in-partial-clone (2018-09-21) 2 commits (merged to 'next' on 2018-10-12 at 521b3fb44d) + fetch: in partial clone, check presence of targets + connected: document connectivity in partial clones "git fetch $repo $object" in a partial clone did not correctly fetch the asked-for object that is referenced by an object in promisor packfile, which has been fixed. * jt/non-blob-lazy-fetch (2018-10-04) 2 commits (merged to 'next' on 2018-10-12 at 7466c6bd7d) + fetch-pack: exclude blobs when lazy-fetching trees + fetch-pack: avoid object flags if no_dependents A partial clone that is configured to lazily fetch missing objects will on-demand issue a "git fetch" request to the originating repository to fill not-yet-obtained objects. The request has been optimized for requesting a tree object (and not the leaf blob objects contained in it) by telling the originating repository that no blobs are needed. * nd/complete-fetch-multiple-args (2018-09-21) 1 commit (merged to 'next' on 2018-10-10 at f78e14123c) + completion: support "git fetch --multiple" Teach bash completion that "git fetch --multiple" only takes remote names as arguments and no refspecs. * nd/help-commands-verbose-by-default (2018-10-03) 1 commit (merged to 'next' on 2018-10-12 at 32de8f53e0) + help -a: improve and make --verbose default "git help -a" and "git
