* Re: git fast-import crashing on big imports
From: Ulrich Spoerlein @ 2017-01-18 14:01 UTC (permalink / raw)
To: git, Jeff King; +Cc: Ed Maste, Junio C Hamano
In-Reply-To: <20170112082138.GJ4426@acme.spoerlein.net>
Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
22nd, that changes delta_base_cache to use hashmap.h is the culprit for
git fast-import crashing on large imports.
Please read below, you can find a 55G SVN dump that should show the
problem after a couple of minutes to less than an hour. Please also see
similar issues from 2009 and 2011. This seems to be a rather fragile
part of the code, could you add unit tests that make sure this
regression is not re-introduce again once you fix it? Thanks!
I'm happy to test any patches that you can provide.
Cheers,
Uli
On Do., 2017-01-12 at 09:21:38 +0100, Ulrich Spörlein wrote:
> Hey,
>
> the FreeBSD svn2git conversion is crashing somewhat
> non-deterministically during its long conversion process. From memory,
> this was not as bad is it is with more recent versions of git (but I
> can't be sure, really).
>
> I have a dump file that you can grab at
> http://scan.spoerlein.net/pub/freebsd-base.dump.xz (19G, 55G uncompressed)
> that shows this problem after a couple of minutes of runtime. The caveat is
> that for another member of the team on a different machine the crashes are on
> different revisions.
>
> Googling around I found two previous threads that were discussing
> problems just like this (memory corruption, bad caching, etc)
>
> https://www.spinics.net/lists/git/msg93598.html from 2009
> and
> http://git.661346.n2.nabble.com/long-fast-import-errors-out-quot-failed-to-apply-delta-quot-td6557884.html
> from 2011
>
> % git fast-import --stats < ../freebsd-base.dump
> ...
> progress SVN r49318 branch master = :49869
> progress SVN r49319 branch stable/3 = :49870
> progress SVN r49320 branch master = :49871
> error: failed to apply delta
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
> fast-import: dumping crash report to fast_import_crash_29613
>
>
> fast-import crash report:
> fast-import process: 29613
> parent process : 29612
> at 2017-01-11 19:33:37 +0000
>
> fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
>
>
> git fsck shows a somewhat incomplete pack file (I guess that's expected if the
> process dies mid-stream?)
>
> % git fsck
> Checking object directories: 100% (256/256), done.
> error: failed to apply delta6/614500)
> error: cannot unpack d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 122654805
> error: failed to apply delta
> error: failed to read delta base object d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
> error: cannot unpack 8523bde63ef34bef725347994fdaec996d756510 from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 122671596
> error: failed to apply delta0/614500)
> error: failed to read delta base object d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
> ...
>
>
> Any comments on whether the original problems from 2009 and 2011 were ever
> fixed and committed?
>
> Some more facts:
> - git version 2.11.0
> - I don't recall these sorts of crashes with a git from 2-3 years ago
> - adding more checkpoints does help, but not fix the problem, it merely shifts
> the crashes around to different revisions
> - incremental runs of the conversion *will* complete most of the time, but
> depending on how often checkpoints are used, I've seen it croak on specific
> commits and not being able to progress further :(
>
> Thanks for any pointers or things to try!
> Cheers
> Uli
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Johannes Schindelin @ 2017-01-18 13:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbmv5fp22.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 17 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Sun, 15 Jan 2017, Junio C Hamano wrote:
> >
> >> * js/prepare-sequencer-more (2017-01-09) 38 commits
>
> > The only outstanding review comments I know about are your objection
> > to the name of the read_env_script() function (which I shot down as
> > bogus),
>
> Not the "name", but the implementation not sharing the same code
> with "am" codepath even though they originate from the same shell
> function and serve the same purpose.
They do not originate from the same shell function at all. The scripted
git-am used a function called get_author_ident_from_commit to read the
author-script file. It also used "eval $(cat .../author-script)" to
evaluate the script later on.
And while git-rebase--interactive.sh's ". .../author-script" is very
similar to that latter invocation, I grant you that, the claim that those
codepaths originate from the same shell function is false.
Worse.
git-am.sh not only used a different method to read out the author-script,
and not only left no way for the user to modify that author-script, there
are many more differences in the exact code paths when comparing git-am.sh
to git-rebase--interactive.sh.
git-am.sh goes out of its way to not only apply the patch manually but
also to use the low-level `write-tree` and `commit-tree` commands. The
builtin am reproduces this as faithfully as possible (the author-script is
not actually evaluated, of course, but the builtin am knows that it has
to validate the author-script's contents for the first code path reading
the author-script anyway, so it reuses that function for the second code
path, knowing fully well the exact format of that file because it knows it
wrote it without any chance of the user messing it up).
The builtin am also avoids spawning the low-level commands, opting instead
to call the functions in libgit.a directly. This is all done properly, and
as a consequence, the environment variables GIT_AUTHOR_* never become,
ahem, environment variables in the builtin am, but an "author" parameter
(which is a const char * pointing to a ready-to-use ident line) that gets
passed to the commit_tree() function.
Now, let's have a brief look at git-rebase--interactive.sh.
It was rightfully nicknamed "cherry-pick on drugs" in its early days,
because that is what it does: it calls `git cherry-pick` *most* of the
time.
So what does the (sequencer via being called from the) rebase--helper do?
Yep, exactly: it calls the internal do_pick() function that is the working
horse of the cherry-pick. And does that working horse call write_tree()
and commit_tree()? No, it does not. It *spawns a git-commit process*! And
the way to specify the author there is to pass the GIT_AUTHOR_*
environment variables.
Let's recapitulate.
Not only did the scripted am use a totally different code path to *read*
the author-script than rebase -i, the code path after that also differed
substantially from rebase -i to the point that very, very different Git
commands were called.
It also so happens that the proper way to convert those code paths into
builtin code uses very, very different functions from libgit.a that
require very, very different handling.
The builtin am never sets the environment variables GIT_AUTHOR_* but
instead constructs a complete ident line from it, and therefore *must*
validate the contents of author-script.
The sequencer *has to* set the GIT_AUTHOR_* environment variables because
it calls `git commit` in a different process *that already validates the
GIT_AUTHOR_* variables*.
Even worse (and this is not the first, or second, or third time I pointed
this out): if you mistook the identical file name, and identical content,
to mean that the different code paths *must* use a *single* function to
read the author-script (nevermind that the am code path reads it into a
structure that is specifically designed to support the "am" operation, and
nothing else), you would not only force the sequencer call to provide a
data structure it does not want, not only to validate the contents
unnecessarily, but it would have to lego the parsed contents *back into
almost the original shape as the original contents of the author-script*.
So it would have to add tons of code relative to the current version, for
no benefit at all, *increasing your maintenance burden for no good
reason*.
This repeated suggestion to reuse the code path to read the author-script
repeatedly ignores the fact that we have to do very, very different things
with the contents in those two code paths.
And if that was not enough: we are talking about code that is rock solid,
has been tested for almost a *year*, half of that year with a painful
cross-validation by running old and new rebase -i and comparing their
output. And this rock solid code is what you want to force me to change to
code that neither makes sense nor is tested well.
And I refuse to be forced to do that: we just proved collectively how good
a job we do when reviewing patches: we just flimsically introduced a
regression at the core of the operation of rebase -i. Not despite patch
review. Because of it.
In short: the objections against keeping the read_env_script() function
as-is (now fixed, of course) make no sense from several points of view:
logically, time-wise, and robustness.
> > and the rather real bug fix I sent out as a fixup! which you may want
> > to squash in (in the alternative, I can mailbomb v4 of the entire
> > sequencer-i patch series, that is your choice).
>
> I'd rather see the "make elengant" thing redone in a more sensible
> way,
No, no, no, NO!
This is starting to become really, really annoying. The reasons you keep
repeating for rewriting this to use the same functions as builtin/am.c *do
not make sense*. Even when you repeat them a hundred times (I know, in the
US you have this really public example where somebody is really successful
repeating incorrect things over and over until some people start to
believe them, but this is not an example you should try to follow).
Just because you keep repeating that "am" and "rebase -i" both read
"author-script" files with the same syntax does not mean that it is
sensible to force them to use the same code path when the data needs to be
processed very differently afterwards!
And you can repeat another thousand times that it would make maintaining
the code easier, and it *still* would not make that statement less false.
The "am" command needs to validate the contents and process them into an
ident line.
The sequencer needs to pass the contents as environment variables to the
"git commit" command that validates the contents itself.
> I'll squash the fixup! thing in, and as I already said a few days ago, I
> do not think we'd want v4 (rather we'd want to go incremental).
Now, those are three statements that all make sense.
Thank you,
Johannes
^ permalink raw reply
* Re: [BUG/RFC] Git Gui: GIT_DIR leaks to spawned Git Bash
From: Johannes Schindelin @ 2017-01-18 13:04 UTC (permalink / raw)
To: Max Kirillov; +Cc: Pat Thoyts, git, Junio C Hamano
In-Reply-To: <20170117184224.GA23841@jessie.local>
Hi Max,
On Tue, 17 Jan 2017, Max Kirillov wrote:
> On Tue, Jan 17, 2017 at 11:52:29AM +0100, Johannes Schindelin wrote:
>
> > And having said *that*, I have to admit that I was unable to reproduce
>
> I have found exact way to reproduce it and actually the reason. Turns
> out to be obvious mistake in code. PR is in github:
> https://github.com/git-for-windows/git/pull/1032
As I pointed out in my review, I think that GIT_DIR is actually only set
for `gitk` in the non-submodule case. So I guess if I run Git GUI, spawn
gitk, and then Git Bash, it will have GIT_DIR set.
*clicketyclick*
Bingo. That reproduces the problem here.
Thanks,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Jeff King @ 2017-01-18 12:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <alpine.DEB.2.20.1701181334040.3469@virtualbox>
On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:
> > > Let's fix this by telling Git that a remote is not configured unless any
> > > fetch/push URL or refspect is configured explicitly.
> >
> > Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
> > the system gitconfig, for a similar purpose to what you want to do with
> > remote.origin.prune.
> >
> > I notice here that setting a refspec _does_ define a remote. Is there a
> > reason you drew the line there, and not at, say, whether it has a URL?
>
> I want to err on the side of caution. That's why.
I guess I just don't see why changing the behavior with respect to
"prune" or "proxy" is any less conservative than changing the one for
"refspec". Both can make some real-world cases work, and both can cause
breakage in some possible real-world cases. If we are going to change
anything, it seems like we should at least aim for a simple and
consistent rule (since users have to know which keys can be put in
~/.gitconfig and which cannot).
I can think of one alternative approach that might be easier for users
to understand, and that we already use elsewhere (e.g., with "http.*"
config): have a set of "default" remote keys (e.g., just "remote.key")
that git falls back to when the remote.*.key isn't set. Then your use
case becomes something like:
[remote]
prune = true
That's not _exactly_ the same, as it applies to all remotes, not just
"origin" (other remotes can override it, but would need to do so
explicitly). But I have a suspicion that may actually be what users
want.
-Peff
^ permalink raw reply
* Re: [PATCH 5/5] describe: teach describe negative pattern matches
From: Johannes Schindelin @ 2017-01-18 12:44 UTC (permalink / raw)
To: Jacob Keller; +Cc: Johannes Sixt, Jacob Keller, Git mailing list
In-Reply-To: <CA+P7+xqFHG52Xo8ncUwa3owDn3OOz+rr3ZaGwfcUDCiXJmh80Q@mail.gmail.com>
Hi Jake,
On Tue, 17 Jan 2017, Jacob Keller wrote:
> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
> >>
> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> >>>
> >>> When you write
> >>>
> >>> git log --branches --exclude=origin/* --remotes
> >>>
> >>> --exclude=origin/* applies only to --remotes, but not to --branches.
> >>
> >>
> >> Well for describe I don't think the order matters.
> >
> >
> > That is certainly true today. But I would value consistency more. We would
> > lose it if some time in the future 'describe' accepts --branches and
> > --remotes in addition to --tags and --all.
> >
> > -- Hannes
> >
>
> I am not sure that the interface for git-log and git-describe are
> similar enough to make this distinction work. --match already seems to
> imply that it only works on refs in refs/tags, as it says it considers
> globs matching excluding the "refs/tags" prefix.
>
> In git-describe, we already have "--tags" and "--all" but they are
> mutually exclusive. We don't support using more than one at once, and
> I'm not really convinced that describe will ever support more than one
> at a time. Additionally, match already doesn't respect order.
I agree that it would keep the code much simpler if you kept the order
"exclude before include".
However, should you decide to look into making the logic dependent on the
order in which the flags were specified in the command-line, we do have a
data structure for such a beast: we use it in gitignore and in
sparse-checkout, it is called struct exclude_list.
Just some food for thought,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Johannes Schindelin @ 2017-01-18 12:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Thomas Gummerer, Andrew Arnott
In-Reply-To: <xmqqd1flcosv.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 17 Jan 2017, Junio C Hamano wrote:
> Perhaps instead of adding "is it configured?" flag that is too
> broadly named and has too narrow meaning, would it make more sense
> to introduce "int can_prune(struct remote *remote)" that looks at
> the remote refspecs?
That ("can we prune?") is not the question we need to ask when determining
whether we can rename a remote to a new name.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Johannes Schindelin @ 2017-01-18 12:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Thomas Gummerer, Andrew Arnott
In-Reply-To: <20170117214723.p5rni6wwggei366j@sigill.intra.peff.net>
Hi Peff,
On Tue, 17 Jan 2017, Jeff King wrote:
> On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:
>
> > One of the really nice features of the ~/.gitconfig file is that users
> > can override defaults by their own preferred settings for all of their
> > repositories.
> >
> > One such default that some users like to override is whether the
> > "origin" remote gets auto-pruned or not. The user would simply call
> >
> > git config --global remote.origin.prune true
> >
> > and from now on all "origin" remotes would be pruned automatically when
> > fetching into the local repository.
> >
> > There is just one catch: now Git thinks that the "origin" remote is
> > configured, as it does not discern between having a remote whose
> > fetch (and/or push) URL and refspec is set, and merely having
> > preemptively-configured, global flags for specific remotes.
> >
> > Let's fix this by telling Git that a remote is not configured unless any
> > fetch/push URL or refspect is configured explicitly.
>
> Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
> the system gitconfig, for a similar purpose to what you want to do with
> remote.origin.prune.
>
> I notice here that setting a refspec _does_ define a remote. Is there a
> reason you drew the line there, and not at, say, whether it has a URL?
I want to err on the side of caution. That's why.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 3/3] Retire the scripted difftool
From: Johannes Schindelin @ 2017-01-18 12:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqk29tcqb8.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 17 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > It served its purpose, but now we have a builtin difftool. Time for the
> > Perl script to enjoy Florida.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> The endgame makes a lot of sense. Both in the cover letter and in
> the previous patch you talk about having both in the released
> version, so do you want this step to proceed slower than the other
> two?
I did proceed that slowly. Already three Git for Windows versions have
been released with both.
But I submitted this iteration with this patch, so my intent is clearly to
retire the Perl script.
Ciao,
Johannes
^ permalink raw reply
* [PATCH] mingw: follow-up to "replace isatty() hack"
From: Johannes Schindelin @ 2017-01-18 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt, Beat Bolli
The version of the "replace isatty() hack" that got applied to the
`maint` branch did not actually reflect the latest iteration of the
patch series: v3 was sent out with these changes, as requested by
the reviewer Johannes Sixt:
- reworded the comment about "recycling handles"
- moved the reassignment of the `console` variable before the dup2()
call so that it is valid at all times
- removed the "handle = INVALID_HANDLE_VALUE" assignment, as the local
variable `handle` is not used afterwards anyway
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1
compat/winansi.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/compat/winansi.c b/compat/winansi.c
index 3c9ed3cfe0..82b89ab137 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -494,19 +494,16 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
* It is because of this implicit close() that we created the
* copy of the original.
*
- * Note that the OS can recycle HANDLE (numbers) just like it
- * recycles fd (numbers), so we must update the cached value
- * of "console". You can use GetFileType() to see that
- * handle and _get_osfhandle(fd) may have the same number
- * value, but they refer to different actual files now.
+ * Note that we need to update the cached console handle to the
+ * duplicated one because the dup2() call will implicitly close
+ * the original one.
*
* Note that dup2() when given target := {0,1,2} will also
* call SetStdHandle(), so we don't need to worry about that.
*/
- dup2(new_fd, fd);
if (console == handle)
console = duplicate;
- handle = INVALID_HANDLE_VALUE;
+ dup2(new_fd, fd);
/* Close the temp fd. This explicitly closes "new_handle"
* (because it has been associated with it).
base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a
--
2.11.0.windows.3
^ permalink raw reply related
* Re: [PATCH v3 3/3] mingw: replace isatty() hack
From: Johannes Schindelin @ 2017-01-18 12:13 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Junio C Hamano, Pranit Bauva, Johannes Sixt,
Beat Bolli
In-Reply-To: <2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de>
Hi,
On Fri, 23 Dec 2016, Johannes Schindelin wrote:
> + /*
> + * Use stock dup2() to re-bind fd to the new handle. Note that
> + * this will implicitly close(1) and close both fd=1 and the
> + * originally associated handle. It will open a new fd=1 and
> + * call DuplicateHandle() on the handle associated with new_fd.
> + * It is because of this implicit close() that we created the
> + * copy of the original.
> + *
> + * Note that we need to update the cached console handle to the
> + * duplicated one because the dup2() call will implicitly close
> + * the original one.
> + *
> + * Note that dup2() when given target := {0,1,2} will also
> + * call SetStdHandle(), so we don't need to worry about that.
> + */
> + if (console == handle)
> + console = duplicate;
> + dup2(new_fd, fd);
Sadly, v2 was applied to `maint` instead of this revision, so Hannes'
suggestions that I addressed in v3 were dropped silently.
I will send out a follow-up patch in a moment.
Ciao,
Johannes
^ permalink raw reply
* Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"
From: Jeff King @ 2017-01-18 11:17 UTC (permalink / raw)
To: Matt McCutchen; +Cc: git
In-Reply-To: <1484704915.2096.16.camel@mattmccutchen.net>
On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:
> A bug report: I noticed that "git diff --ignore-space-change --stat"
> lists files with only whitespace differences as having changed with 0
> differing lines. This is inconsistent with the behavior without --
> stat, which doesn't list such files at all. (Same behavior with all
> the --ignore*space* flags.) I can reproduce this with the current
> "next", af746e4. Quick test case:
Hmm. This is pretty easy to do naively, but the special-casing for
addition/deletion (which I think we _do_ need, and which certainly we
fail t4205 without) makes me feel dirty. I'd worry there are other
cases, too (perhaps renames?). And I also notice that the
binary-diffstat code path just above my changes explicitly creates 0/0
diffstats, but I'm not even sure how one would trigger that.
So I dunno. A sensible rule to me is "iff -p would show a diff header,
then --stat should mention it". I think we'd want to somehow extract the
logic from builtin_diff() and reuse it.
---
diff --git a/diff.c b/diff.c
index e2eb6d66a..57ff5c1dc 100644
--- a/diff.c
+++ b/diff.c
@@ -2105,17 +2105,20 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o
gather_dirstat(options, &dir, changed, "", 0);
}
+static void free_diffstat_file(struct diffstat_file *f)
+{
+ if (f->name != f->print_name)
+ free(f->print_name);
+ free(f->name);
+ free(f->from_name);
+ free(f);
+}
+
static void free_diffstat_info(struct diffstat_t *diffstat)
{
int i;
- for (i = 0; i < diffstat->nr; i++) {
- struct diffstat_file *f = diffstat->files[i];
- if (f->name != f->print_name)
- free(f->print_name);
- free(f->name);
- free(f->from_name);
- free(f);
- }
+ for (i = 0; i < diffstat->nr; i++)
+ free_diffstat_file(diffstat->files[i]);
free(diffstat->files);
}
@@ -2603,6 +2606,23 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
if (xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
&xpp, &xecfg))
die("unable to generate diffstat for %s", one->path);
+
+ /*
+ * Omit diffstats where nothing changed. Even if
+ * !same_contents, this might be the case due to ignoring
+ * whitespace changes, etc.
+ *
+ * But note that we special-case additions and deletions,
+ * as adding an empty file, for example, is still of interest.
+ */
+ if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
+ struct diffstat_file *file =
+ diffstat->files[diffstat->nr - 1];
+ if (!file->added && !file->deleted) {
+ free_diffstat_file(file);
+ diffstat->nr--;
+ }
+ }
}
diff_free_filespec_data(one);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 289806d0c..2805db411 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -736,7 +736,7 @@ test_expect_success 'checkdiff allows new blank lines' '
cat <<EOF >expect
EOF
-test_expect_success 'whitespace-only changes not reported' '
+test_expect_success 'whitespace-only changes not reported (diff)' '
git reset --hard &&
echo >x "hello world" &&
git add x &&
@@ -746,6 +746,12 @@ test_expect_success 'whitespace-only changes not reported' '
test_cmp expect actual
'
+test_expect_success 'whitespace-only changes not reported (diffstat)' '
+ # reuse state from previous test
+ git diff --stat -b >actual &&
+ test_cmp expect actual
+'
+
cat <<EOF >expect
diff --git a/x b/z
similarity index NUM%
^ permalink raw reply related
* Git: new feature suggestion
From: Joao Pinto @ 2017-01-18 10:40 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds, CARLOS.PALMINHA@synopsys.com
Hello,
My name is Joao Pinto, I work at Synopsys and I am a frequent Linux Kernel
contributor.
Let me start by congratulate you for the fantastic work you have been doing with
Git which is an excellent tool.
The Linux Kernel as all systems needs to be improved and re-organized to be
better prepared for future development and sometimes we need to change
folder/files names or even move things around.
I have seen a lot of Linux developers avoid this re-organization operations
because they would lose the renamed file history, because a new log is created
for the new file, even if it is a renamed version of itself.
I am sending you this e-mail to suggest the creation of a new feature in Git:
when renamed, a file or folder should inherit his parent’s log and a “rename: …”
would be automatically created or have some kind of pointer to its “old” form to
make history analysis easier.
I volunteer to help in the new feature if you find it useful. I think it would
improve log history analysis and would enable developers to better organize old
code.
Thank you for your attention.
Best Regards,
Joao Pinto
^ permalink raw reply
* [ANNOUNCE] git-cinnabar 0.4.0
From: Mike Hommey @ 2017-01-18 9:22 UTC (permalink / raw)
To: git
Hi,
Git-cinnabar is a git remote helper to interact with mercurial
repositories. It allows to clone, pull and push from/to mercurial remote
repositories, using git.
Code on https://github.com/glandium/git-cinnabar
This release on
https://github.com/glandium/git-cinnabar/releases/tag/0.4.0
What's new since 0.3.2?
- Various bug fixes.
- Updated git to 2.11.0 for cinnabar-helper.
- Now supports bundle2 for both fetch/clone and push
(https://www.mercurial-scm.org/wiki/BundleFormat2).
- Now supports `git credential` for HTTP authentication.
- Now supports `git push --dry-run`.
- Added a new `git cinnabar fetch` command to fetch a specific revision
that is not necessarily a head.
- Added a new `git cinnabar download` command to download a helper on
platforms where one is available.
- Removed upgrade path from repositories used with version < 0.3.0.
- Experimental (and partial) support for using git-cinnabar without
having mercurial installed.
- Use a mercurial subprocess to access local mercurial repositories.
- Cinnabar-helper now handles fast-import, with workarounds for
performance issues on macOS.
- Fixed some corner cases involving empty files. This prevented cloning
Mozilla's stylo incubator repository.
- Fixed some correctness issues in file parenting when pushing
changesets pulled from one mercurial repository to another.
- Various improvements to the rules to build the helper.
- Experimental (and slow) support for pushing merges, with caveats. See
https://github.com/glandium/git-cinnabar/issues/20 for details about
the current status.
- Fail graft earlier when no commit was found to graft
- Allow graft to work with git version < 1.9
- Allow `git cinnabar bundle` to do the same grafting as git push
Mike
^ permalink raw reply
* Re: [PATCH] gitk: remove translated message from comments
From: Paul Mackerras @ 2017-01-18 10:15 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, git
In-Reply-To: <20170118035245.1757-1-davvid@gmail.com>
On Tue, Jan 17, 2017 at 07:52:45PM -0800, David Aguilar wrote:
> "make update-po" fails because a previously untranslated string
> has now been translated:
>
> Updating po/sv.po
> po/sv.po:1388: duplicate message definition...
> po/sv.po:380: ...this is the location of the first definition
>
> Remove the duplicate message definition.
>
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
Thanks, applied.
Junio, please do a pull from my repository to get this fix.
The new head is 7f03c6e32891.
Paul.
^ permalink raw reply
* Re: [RFH - Tcl/Tk] use of procedure before declaration?
From: Konstantin Khomoutov @ 2017-01-18 6:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Philip Oakley, Git List, Pat Thoyts
In-Reply-To: <alpine.DEB.2.20.1701171218260.3469@virtualbox>
On Tue, 17 Jan 2017 12:29:23 +0100 (CET)
Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > In
> > https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L242
> > the procedure `_unset_recentrepo` is called, however the procedure
> > isn't declared until line 248. My reading of the various Tcl
> > tutorials suggest (but not explictly) that this isn't the right way.
>
> Indeed, calling a procedure before it is declared sounds incorrect.
[...]
> And it is perfectly legitimate to use not-yet-declared procedures in
> other procedures, otherwise recursion would not work.
[...]
Sorry for chiming in too late, but I'd throw a bit of theory in.
Since Tcl is an interpreter (though it automatically compiles certain
stuff to bytecode as it goes through the script, and caches this
representation), everything is interpreted in the normal script order --
top to bottom as we usually see it in a text editor.
That is, there are simply no declaration vs definition: the main script
passed to tclsh / wish is read and interpreted from top to bottom;
as soon as it calls the [source] command, the specified script is read
and interpreted from top to bottom etc; after that the control is back
to the original script and its interpretation continues.
Hence when Tcl sees a command (everything it executes is a command; this
includes stuff like [proc], [foreach] and others, which are syntax in
other languages) it looks up this command in the current list of
commands it knows and this either succeeds or fails. The built-in
command [proc] defines a new Tcl procedure with the given name, and
registers it in that list of known commands.
So the general rule for user-defined procedures is relatively
straightforward: to call a procedure, the interpreter should have read
and executed its definition before the attempted call.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Johannes Sixt @ 2017-01-18 6:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git
In-Reply-To: <xmqqo9z5fqdj.fsf@gitster.mtv.corp.google.com>
Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
> So... can we move this forward?
I have no objects anymore.
-- Hannes
^ permalink raw reply
* difflame
From: Edmundo Carmona Antoranz @ 2017-01-18 5:24 UTC (permalink / raw)
To: Git List
Hi!
For a very long time I had wanted to get the output of diff to include
blame information as well (to see when something was added/removed).
I just created a very small (and rough) tool for that purpose. It's
written in python and if it gets to something better than a small
tool, I think it could be worth to be added into git main (perhaps
into contrib?).
If you want to give ir a try:
https://github.com/eantoranz/difflame
Just provide the two treeishs you would like to diff (no more
parameters are accepted at the time) and you will get the diff along
with blame. Running it right now on the project itself:
✔ ~/difflame [master L|⚑ 1]
23:21 $ ./difflame.py HEAD~3 HEAD~
diff --git a/README.txt b/README.txt
new file mode 100644
index 0000000..a82aa27
--- /dev/null
+++ b/README.txt
@@ -0,0 +1,11 @@
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 1) difflame
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 2)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 3)
Copyright 2017 Edmundo Carmona Antoranz
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 4)
Released under the terms of GPLv2
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 5)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 6) Show
the output of diff with the additional information of blame.
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 7)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 8)
Lines that remain the same or that were added will indicate when those
lines were 'added' to the file
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 9)
Lines that were removed will display the last revision where those
lines were _present_ on the file (as provided by blame --re
verse)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 10)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 11) At
the moment, only two parameters need to be provided: two treeishs to
get the diff from
diff --git a/difflame.py b/difflame.py
index f6e879b..06bfc03 100755
--- a/difflame.py
+++ b/difflame.py
@@ -112,16 +112,20 @@ def process_file_from_diff_output(output_lines,
starting_line):
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 112)
diff_line = output_lines[i].split()
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 113)
if diff_line[0] != "diff":
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 114)
raise Exception("Doesn't seem to exist a 'diff' line at line " +
str(i + 1) + ": " + output_lines[i])
-3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 115)
original_name = diff_line[1]
-3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 116)
final_name = diff_line[2]
+f135bf04 (Edmundo Carmona Antoranz 2017-01-17 22:50:50 -0600 115)
original_name = diff_line[2]
+f135bf04 (Edmundo Carmona Antoranz 2017-01-17 22:50:50 -0600 116)
final_name = diff_line[3]
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 117)
print output_lines[i]; i+=1
.
.
.
Hope you find it useful
Best regards!
^ permalink raw reply related
* [PATCH] gitk: remove translated message from comments
From: David Aguilar @ 2017-01-18 3:52 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Junio C Hamano, git
In-Reply-To: <xmqq4m0xpmbz.fsf@gitster.mtv.corp.google.com>
"make update-po" fails because a previously untranslated string
has now been translated:
Updating po/sv.po
po/sv.po:1388: duplicate message definition...
po/sv.po:380: ...this is the location of the first definition
Remove the duplicate message definition.
Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
po/sv.po | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/po/sv.po b/po/sv.po
index 32fc752..2a06fe5 100644
--- a/po/sv.po
+++ b/po/sv.po
@@ -1385,21 +1385,6 @@ msgstr "Felaktiga argument till gitk:"
#~ msgid "mc"
#~ msgstr "mc"
-#~ msgid ""
-#~ "\n"
-#~ "Gitk - a commit viewer for git\n"
-#~ "\n"
-#~ "Copyright © 2005-2016 Paul Mackerras\n"
-#~ "\n"
-#~ "Use and redistribute under the terms of the GNU General Public License"
-#~ msgstr ""
-#~ "\n"
-#~ "Gitk - en incheckningsvisare för git\n"
-#~ "\n"
-#~ "Copyright © 2005-2016 Paul Mackerras\n"
-#~ "\n"
-#~ "Använd och vidareförmedla enligt villkoren i GNU General Public License"
-
#~ msgid "next"
#~ msgstr "nästa"
--
2.11.0.536.gaf746e49c2
^ permalink raw reply related
* "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"
From: Matt McCutchen @ 2017-01-18 2:01 UTC (permalink / raw)
To: git
A bug report: I noticed that "git diff --ignore-space-change --stat"
lists files with only whitespace differences as having changed with 0
differing lines. This is inconsistent with the behavior without --
stat, which doesn't list such files at all. (Same behavior with all
the --ignore*space* flags.) I can reproduce this with the current
"next", af746e4. Quick test case:
echo ' ' >test1 && echo ' ' >test2 &&
git diff --stat --no-index --ignore-space-change test1 test2
This caused me some inconvenience in the following scenario: I was
reading a commit diff that had a bulk license change in all files
combined with code changes. I attempted to revert the bulk license
change locally using "sed" to more easily read the code diff, but my
reversion left some whitespace diffs where the original files had
inconsistent whitespace. So the diffstat after my reversion was
cluttered with these "0" entries.
Regards,
Matt
^ permalink raw reply
* [PATCHv3 4/4] unpack-trees: support super-prefix option
From: Stefan Beller @ 2017-01-18 1:05 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, novalis, Stefan Beller
In-Reply-To: <xmqqh94zbwlu.fsf@gitster.mtv.corp.google.com>
In the future we want to support working tree operations within submodules,
e.g. "git checkout --recurse-submodules", which will update the submodule
to the commit as recorded in its superproject. In the submodule the
unpack-tree operation is carried out as usual, but the reporting to the
user needs to prefix any path with the superproject. The mechanism for
this is the super-prefix. (see 74866d757, git: make super-prefix option)
Add support for the super-prefix option for commands that unpack trees
by wrapping any path output in unpacking trees in the newly introduced
super_prefixed function. This new function prefixes any path with the
super-prefix if there is one. Assuming the submodule case doesn't happen
in the majority of the cases, we'd want to have a fast behavior for no
super prefix, i.e. no reallocation/copying, but just returning path.
Another aspect of introducing the `super_prefixed` function is to consider
who owns the memory and if this is the right place where the path gets
modified. As the super prefix ought to change the output behavior only and
not the actual unpack tree part, it is fine to be that late in the line.
As we get passed in 'const char *path', we cannot change the path itself,
which means in case of a super prefix we have to copy over the path.
We need two static buffers in that function as the error messages
contain at most two paths.
For testing purposes enable it in read-tree, which has no output
of paths other than an unpack-trees.c. These are all converted in
this patch.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
This replaces the last two commits (squash and unpack-trees:
support super-prefix option) of sb/unpack-trees-super-prefix.
> * sb/unpack-trees-super-prefix (2017-01-12) 5 commits
> - SQUASH
> - unpack-trees: support super-prefix option
> - t1001: modernize style
> - t1000: modernize style
> - read-tree: use OPT_BOOL instead of OPT_SET_INT
>
> "git read-tree" and its underlying unpack_trees() machinery learned
> to report problematic paths prefixed with the --super-prefix option.
>
> Expecting a reroll.
> The first three are in good shape. The last one needs a better
> explanation and possibly an update to its test.
> cf. <CAGZ79kaHDnVLQr8WLoaD5KKs7EqeW=KbkptF=iHpU5t054Xcdw@mail.gmail.com>
>
This paragraph is for the first version of the last patch?
I reviewed what you queued and both the test is in modern
style as well as the commit message is adequate now.
I agree the SQUASH commit is a good idea, so this is a reroll
with the SQUASH squashed. No further changes applied.
Thanks,
Stefan
git.c | 2 +-
t/t1001-read-tree-m-2way.sh | 8 ++++++++
unpack-trees.c | 43 ++++++++++++++++++++++++++++++++++++++++---
3 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/git.c b/git.c
index bbaa949e9c..50e559c2a8 100644
--- a/git.c
+++ b/git.c
@@ -471,7 +471,7 @@ static struct cmd_struct commands[] = {
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
{ "push", cmd_push, RUN_SETUP },
- { "read-tree", cmd_read_tree, RUN_SETUP },
+ { "read-tree", cmd_read_tree, RUN_SETUP | SUPPORT_SUPER_PREFIX},
{ "receive-pack", cmd_receive_pack },
{ "reflog", cmd_reflog, RUN_SETUP },
{ "remote", cmd_remote, RUN_SETUP },
diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 7b70089705..5ededd8e40 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -363,6 +363,14 @@ test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '
test -f a/b
'
+test_expect_success 'read-tree supports the super-prefix' '
+ cat <<-EOF >expect &&
+ error: Updating '\''fictional/a'\'' would lose untracked files in it
+ EOF
+ test_must_fail git --super-prefix fictional/ read-tree -u -m "$treeH" "$treeM" 2>actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'a/b vs a, plus c/d case setup.' '
rm -f .git/index &&
rm -fr a &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 7a6df99d10..eee4865804 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -52,6 +52,41 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
? ((o)->msgs[(type)]) \
: (unpack_plumbing_errors[(type)]) )
+static const char *super_prefixed(const char *path)
+{
+ /*
+ * It is necessary and sufficient to have two static buffers
+ * here, as the return value of this function is fed to
+ * error() using the unpack_*_errors[] templates we see above.
+ */
+ static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
+ static int super_prefix_len = -1;
+ static unsigned idx = ARRAY_SIZE(buf) - 1;
+
+ if (super_prefix_len < 0) {
+ const char *super_prefix = get_super_prefix();
+ if (!super_prefix) {
+ super_prefix_len = 0;
+ } else {
+ int i;
+ for (i = 0; i < ARRAY_SIZE(buf); i++)
+ strbuf_addstr(&buf[i], super_prefix);
+ super_prefix_len = buf[0].len;
+ }
+ }
+
+ if (!super_prefix_len)
+ return path;
+
+ if (++idx >= ARRAY_SIZE(buf))
+ idx = 0;
+
+ strbuf_setlen(&buf[idx], super_prefix_len);
+ strbuf_addstr(&buf[idx], path);
+
+ return buf[idx].buf;
+}
+
void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
const char *cmd)
{
@@ -172,7 +207,7 @@ static int add_rejected_path(struct unpack_trees_options *o,
const char *path)
{
if (!o->show_all_errors)
- return error(ERRORMSG(o, e), path);
+ return error(ERRORMSG(o, e), super_prefixed(path));
/*
* Otherwise, insert in a list for future display by
@@ -196,7 +231,7 @@ static void display_error_msgs(struct unpack_trees_options *o)
something_displayed = 1;
for (i = 0; i < rejects->nr; i++)
strbuf_addf(&path, "\t%s\n", rejects->items[i].string);
- error(ERRORMSG(o, e), path.buf);
+ error(ERRORMSG(o, e), super_prefixed(path.buf));
strbuf_release(&path);
}
string_list_clear(rejects, 0);
@@ -1918,7 +1953,9 @@ int bind_merge(const struct cache_entry * const *src,
o->merge_size);
if (a && old)
return o->gently ? -1 :
- error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, old->name);
+ error(ERRORMSG(o, ERROR_BIND_OVERLAP),
+ super_prefixed(a->name),
+ super_prefixed(old->name));
if (!a)
return keep_entry(old, o);
else
--
2.11.0.299.g762782ba8a
^ permalink raw reply related
* [PATCH 4/4] documentation: retire unfinished documentation
From: Stefan Beller @ 2017-01-17 23:35 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20170117233503.27137-1-sbeller@google.com>
When looking for documentation for a specific function, you may be tempted
to run
git -C Documentation grep index_name_pos
only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.
In the previous patches we have documented
* index_name_pos()
* remove_index_entry_at()
* add_[file_]to_index()
in cache.h
We already have documentation for:
* add_index_entry()
* read_index()
Which leaves us with a TODO for:
* cache -> the_index macros
* refresh_index()
* discard_index()
* ie_match_stat() and ie_modified(); how they are different and when to
use which.
* write_index() that was renamed to write_locked_index
* cache_tree_invalidate_path()
* cache_tree_update()
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/technical/api-in-core-index.txt | 21 ---------------------
1 file changed, 21 deletions(-)
delete mode 100644 Documentation/technical/api-in-core-index.txt
diff --git a/Documentation/technical/api-in-core-index.txt b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..0000000000
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=================
-
-Talk about <read-cache.c> and <cache-tree.c>, things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
- use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
--
2.11.0.299.g762782ba8a
^ permalink raw reply related
* Re: [PATCH] transport submodules: correct error message
From: Junio C Hamano @ 2017-01-18 0:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Heiko Voigt, Dave Borowitz
In-Reply-To: <CAGZ79kYu1Y2pwk9+kbSrMxwP3S0n8FMW6f4wdEE0mrACqrOPNA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Trying to push with --recurse-submodules=on-demand would run into
>>> the same problem. To fix this issue
>>> 1) specifically mention that we looked for branches on the remote.
>>
>> That makes an incorrect statement ("not found on any remote"---we
>> did not inspect all of the said remote, only heads and tags) into an
>> irrelevant statement ("not found on any remote branch"---the end
>> user would say "so what? I know it exists there, it's just that not
>> all remote refs have corresponding tracking ref locally on our side").
>
> eh. So to be correct we need to tell the user we did not find any match on
> a "remote-tracking branch" as the gitglossary puts it.
I think the updated text is already "correct". I am pointing out
that it may be correct but not very helpful to the users.
>> where remote tracking information is
>> incomplete if you only look at heads and refs, in the sense that we
>> no longer suggest ineffective workaround.
>
> s/ineffective/an effective/ ?
Even though I make many typoes, I meant ineffective in this case.
"The old message suggested workaround that would not help. You no
longer give that workaround that does not work."
>> If that is the case, perhaps configuring push.recurseSubmodules to
>> turn this off (especially because you plan to turn the defaul to
>> "check") and not giving the command line option would give a more
>> pleasant end-user experience, I suspect.
>
> I though about going another way and adding another new value
> to the enum, such that
>
> git push --recurse-submodules=sameRefSpecButNoCheck \
> origin HEAD:refs/for/master
>
> works for Gerrit users.
It is unclear what that enum tells Git to do. Care to explain? How
is it different from "no"?
^ permalink raw reply
* Re: [PATCH] transport submodules: correct error message
From: Stefan Beller @ 2017-01-18 0:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Heiko Voigt, Dave Borowitz
In-Reply-To: <xmqqr341b4vm.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 4:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> Trying to push with --recurse-submodules=on-demand would run into
>>>> the same problem. To fix this issue
>>>> 1) specifically mention that we looked for branches on the remote.
>>>
>>> That makes an incorrect statement ("not found on any remote"---we
>>> did not inspect all of the said remote, only heads and tags) into an
>>> irrelevant statement ("not found on any remote branch"---the end
>>> user would say "so what? I know it exists there, it's just that not
>>> all remote refs have corresponding tracking ref locally on our side").
>>
>> eh. So to be correct we need to tell the user we did not find any match on
>> a "remote-tracking branch" as the gitglossary puts it.
>
> I think the updated text is already "correct". I am pointing out
> that it may be correct but not very helpful to the users.
>
>>> where remote tracking information is
>>> incomplete if you only look at heads and refs, in the sense that we
>>> no longer suggest ineffective workaround.
>>
>> s/ineffective/an effective/ ?
>
> Even though I make many typoes, I meant ineffective in this case.
> "The old message suggested workaround that would not help. You no
> longer give that workaround that does not work."
Oh, I misunderstood the original as I lumped on-demand and check
together mentally, because in the Gerrit world they behave similar.
(Both error out; in the on-demand case the server produces the failure
message, though)
>
>>> If that is the case, perhaps configuring push.recurseSubmodules to
>>> turn this off (especially because you plan to turn the defaul to
>>> "check") and not giving the command line option would give a more
>>> pleasant end-user experience, I suspect.
>>
>> I though about going another way and adding another new value
>> to the enum, such that
>>
>> git push --recurse-submodules=sameRefSpecButNoCheck \
>> origin HEAD:refs/for/master
>>
>> works for Gerrit users.
>
> It is unclear what that enum tells Git to do. Care to explain? How
> is it different from "no"?
In those submodules, that are checked positively, blindly run
git -C ${sub} push ${ref_spec} and do not double check again,
which we currently do in the on-demand case.
^ permalink raw reply
* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
From: Junio C Hamano @ 2017-01-18 0:19 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117233723.23897-5-santiago@nyu.edu>
santiago@nyu.edu writes:
> @@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> if (filter.merge_commit)
> die(_("--merged and --no-merged option are only allowed with -l"));
> if (cmdmode == 'd')
> - return for_each_tag_name(argv, delete_tag);
> - if (cmdmode == 'v')
> - return for_each_tag_name(argv, verify_tag);
> + return for_each_tag_name(argv, delete_tag, NULL);
> + if (cmdmode == 'v') {
> + if (format)
> + verify_ref_format(format);
> + return for_each_tag_name(argv, verify_tag, format);
> + }
This triggers:
builtin/tag.c: In function 'cmd_tag':
builtin/tag.c:451:3: error: passing argument 3 of
'for_each_tag_name' discards 'const' qualifier from pointer target type [-Werror]
return for_each_tag_name(argv, verify_tag, format);
Either for-each-tag-name's new parameter needs to be typed
correctly, or the type of the "format" variable needs to be updated.
^ permalink raw reply
* [PATCH v2 0/5] extend git-describe pattern matching
From: Jacob Keller @ 2017-01-18 0:03 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
Teach git describe and git name-rev the ability to match multiple
patterns inclusively. Additionally, teach these commands to also accept
negative patterns to exclude any refs which match.
The pattern lists for positive and negative patterns are inclusive. This
means that for the positive patterns, a reference will be considered as
long as it matches at least one of the match patterns. It need not match
all given patterns. Additionally for negative patterns, we will not
consider any ref which matches any negative pattern, even if it matches
one of the positive patterns.
Together this allows the ability to express far more sets of tags than a
single match pattern alone. It does not provide quite the same depth as
would teaching full regexp but it is simpler and easy enough to
understand.
- v2
* use --exclude instead of --discard
* use modern style in tests
I chose *not* to implement the suggestion of ordered values for exclude
and match, since this is not how the current implementation of
git-describe worked, and it didn't really make sense to me what was
being requested. I looked at the interface for git-log, and it appears
that the command accepts multiple invocations of --branches, --remotes,
and similar. I do not believe these need to be identical interfaces. I
welcome feedback on this decision, but I am not convinced yet that the
ordered arguments are worth the trouble.
diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt
index a89bbde207b2..21a43b78924a 100644
--- c/Documentation/git-describe.txt
+++ w/Documentation/git-describe.txt
@@ -88,12 +88,12 @@ OPTIONS
patterns will be considered. Use `--no-match` to clear and reset the
list of patterns.
---discard <pattern>::
+--exclude <pattern>::
Do not consider tags matching the given `glob(7)` pattern, excluding
the "refs/tags/" prefix. This can be used to narrow the tag space and
find only tags matching some meaningful criteria. If given multiple
times, a list of patterns will be accumulated and tags matching any
- of the patterns will be discarded. Use `--no-discard` to clear and
+ of the patterns will be excluded. Use `--no-exclude` to clear and
reset the list of patterns.
--always::
diff --git c/Documentation/git-name-rev.txt w/Documentation/git-name-rev.txt
index 9b46e5ea9aae..301b4a8d55e6 100644
--- c/Documentation/git-name-rev.txt
+++ w/Documentation/git-name-rev.txt
@@ -30,11 +30,11 @@ OPTIONS
given multiple times, use refs whose names match any of the given shell
patterns. Use `--no-refs` to clear any previous ref patterns given.
---discard=<pattern>::
+--exclude=<pattern>::
Do not use any ref whose name matches a given shell pattern. The
pattern can be one of branch name, tag name or fully qualified ref
- name. If given multiple times, discard refs that match any of the given
- shell patterns. Use `--no-discards` to clear the list of discard
+ name. If given multiple times, exclude refs that match any of the given
+ shell patterns. Use `--no-exclude` to clear the list of exclude
patterns.
--all::
diff --git c/Documentation/technical/api-parse-options.txt w/Documentation/technical/api-parse-options.txt
index 15e876e4c804..6914f54f5f44 100644
--- c/Documentation/technical/api-parse-options.txt
+++ w/Documentation/technical/api-parse-options.txt
@@ -169,9 +169,9 @@ There are some macros to easily define options:
The string argument is put into `str_var`.
`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
- Introduce an option with a string argument. Repeated invocations
- accumulate into a list of strings. Reset and clear the list with
- `--no-option`.
+ Introduce an option with string argument.
+ The string argument is stored as an element in `&list` which must be a
+ struct string_list. Reset the list using `--no-option`.
`OPT_INTEGER(short, long, &int_var, description)`::
Introduce an option with integer argument.
diff --git c/builtin/describe.c w/builtin/describe.c
index c09288ee6321..6769446e1f57 100644
--- c/builtin/describe.c
+++ w/builtin/describe.c
@@ -29,7 +29,7 @@ static int max_candidates = 10;
static struct hashmap names;
static int have_util;
static struct string_list patterns = STRING_LIST_INIT_NODUP;
-static struct string_list discard_patterns = STRING_LIST_INIT_NODUP;
+static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
static int always;
static const char *dirty;
@@ -131,16 +131,16 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
return 0;
/*
- * If we're given discard patterns, first discard any tag which match
- * any of the discard pattern.
+ * If we're given exclude patterns, first exclude any tag which match
+ * any of the exclude pattern.
*/
- if (discard_patterns.nr) {
+ if (exclude_patterns.nr) {
struct string_list_item *item;
if (!is_tag)
return 0;
- for_each_string_list_item(item, &discard_patterns) {
+ for_each_string_list_item(item, &exclude_patterns) {
if (!wildmatch(item->string, path + 10, 0, NULL))
return 0;
}
@@ -438,7 +438,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
N_("consider <n> most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
N_("only consider tags matching <pattern>")),
- OPT_STRING_LIST(0, "discard", &discard_patterns, N_("pattern"),
+ OPT_STRING_LIST(0, "exclude", &exclude_patterns, N_("pattern"),
N_("do not consider tags matching <pattern>")),
OPT_BOOL(0, "always", &always,
N_("show abbreviated commit object as fallback")),
@@ -477,8 +477,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
argv_array_push(&args, "--tags");
for_each_string_list_item(item, &patterns)
argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
- for_each_string_list_item(item, &discard_patterns)
- argv_array_pushf(&args, "--discard=refs/tags/%s", item->string);
+ for_each_string_list_item(item, &exclude_patterns)
+ argv_array_pushf(&args, "--exclude=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(&args, argv);
diff --git c/builtin/name-rev.c w/builtin/name-rev.c
index 86479c17a7c9..da4a0d7c0fdf 100644
--- c/builtin/name-rev.c
+++ w/builtin/name-rev.c
@@ -109,7 +109,7 @@ struct name_ref_data {
int tags_only;
int name_only;
struct string_list ref_filters;
- struct string_list discard_filters;
+ struct string_list exclude_filters;
};
static struct tip_table {
@@ -151,10 +151,10 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
- if (data->discard_filters.nr) {
+ if (data->exclude_filters.nr) {
struct string_list_item *item;
- for_each_string_list_item(item, &data->discard_filters) {
+ for_each_string_list_item(item, &data->exclude_filters) {
if (subpath_matches(path, item->string) >= 0)
return 0;
}
@@ -339,7 +339,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
N_("only use refs matching <pattern>")),
- OPT_STRING_LIST(0, "discard", &data.discard_filters, N_("pattern"),
+ OPT_STRING_LIST(0, "exclude", &data.exclude_filters, N_("pattern"),
N_("ignore refs matching <pattern>")),
OPT_GROUP(""),
OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
diff --git c/t/t6007-rev-list-cherry-pick-file.sh w/t/t6007-rev-list-cherry-pick-file.sh
index 8a4c35f6ffee..83838d0da208 100755
--- c/t/t6007-rev-list-cherry-pick-file.sh
+++ w/t/t6007-rev-list-cherry-pick-file.sh
@@ -100,42 +100,43 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
'
test_expect_success 'name-rev multiple --refs combine inclusive' '
- git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+ git rev-list --left-right --cherry-pick F...E -- bar >actual &&
git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
- < actual > actual.named &&
+ <actual >actual.named &&
test_cmp actual.named expect
'
cat >expect <<EOF
<tags/F
-$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
EOF
test_expect_success 'name-rev --refs excludes non-matched patterns' '
- git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+ git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+ git rev-list --left-right --cherry-pick F...E -- bar >actual &&
git name-rev --stdin --name-only --refs="*tags/F" \
- < actual > actual.named &&
- test_cmp actual.named expect
-'
-
-test_expect_success 'name-rev --discard excludes matched patterns' '
- git rev-list --left-right --cherry-pick F...E -- bar > actual &&
- git name-rev --stdin --name-only --refs="*tags/*" --discard="*E" \
- < actual > actual.named &&
+ <actual >actual.named &&
test_cmp actual.named expect
'
cat >expect <<EOF
-$(git rev-list --left-right --cherry-pick F...E -- bar)
+<tags/F
EOF
-test_expect_success 'name-rev --no-refs clears the refs list' '
- git rev-list --left-right --cherry-pick F...E -- bar > actual &&
- git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
- < actual > actual.named &&
+test_expect_success 'name-rev --exclude excludes matched patterns' '
+ git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+ git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+ git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+ <actual >actual.named &&
test_cmp actual.named expect
'
+test_expect_success 'name-rev --no-refs clears the refs list' '
+ git rev-list --left-right --cherry-pick F...E -- bar >expect &&
+ git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+ <actual >actual &&
+ test_cmp actual expect
+'
+
cat >expect <<EOF
+tags/F
=tags/D
diff --git c/t/t6120-describe.sh w/t/t6120-describe.sh
index 4e4a9f2e5305..167491fd5b0d 100755
--- c/t/t6120-describe.sh
+++ w/t/t6120-describe.sh
@@ -218,11 +218,11 @@ test_expect_success 'describe --contains and --match' '
test_cmp expect actual
'
-test_expect_success 'describe --discard' '
+test_expect_success 'describe --exclude' '
echo "c~1" >expect &&
tagged_commit=$(git rev-parse "refs/tags/A^0") &&
test_must_fail git describe --contains --match="B" $tagged_commit &&
- git describe --contains --match="?" --discard="A" $tagged_commit >actual &&
+ git describe --contains --match="?" --exclude="A" $tagged_commit >actual &&
test_cmp expect actual
'
Jacob Keller (5):
doc: add documentation for OPT_STRING_LIST
name-rev: extend --refs to accept multiple patterns
name-rev: add support to exclude refs by pattern match
describe: teach --match to accept multiple patterns
describe: teach describe negative pattern matches
Documentation/git-describe.txt | 13 ++++++-
Documentation/git-name-rev.txt | 11 +++++-
Documentation/technical/api-parse-options.txt | 5 +++
builtin/describe.c | 51 ++++++++++++++++++++++----
builtin/name-rev.c | 53 +++++++++++++++++++++------
t/t6007-rev-list-cherry-pick-file.sh | 38 +++++++++++++++++++
t/t6120-describe.sh | 27 ++++++++++++++
7 files changed, 177 insertions(+), 21 deletions(-)
--
2.11.0.403.g196674b8396b
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox