Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration
On Thu, Jun 16, 2016 at 4:32 PM, Joey Hess wrote: > This adds new smudge-to-file and clean-from-file filter commands, > which are similar to smudge and clean but allow direct access to files on > disk. > [...] > Signed-off-by: Joey Hess > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > @@ -1299,14 +1299,29 @@ format.useAutoBase:: > +filter..clean-from-file:: > + Optional command which is used on checkin to convert the content > + of a worktree file, which can be read from disk, to a blob > + (written to stdout). > + Only used when filter..clean is also configured. > + See linkgit:gitattributes[5] for details. > + > +filter..smudge-to-file:: > + Optional command which is used to convert the content of a blob > + object (provided on stdin) to a worktree file, writing directly > + to the file. > + Only used when filter..clean is also configured. s/clean/smudge/ > + See linkgit:gitattributes[5] for details. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] clarify %f documentation
Joey Hess writes: > Junio C Hamano wrote: >> I agree that "the name of the file" can be interpreted in many ways, >> and I agree that it would be a good idea to find a better phrase to >> name the path that is being worked on, but I do not think "the file >> in the git repository" is that phrase. > >> I think using the word "path" somewhere in the updated description >> is more likely to have the effect you desire. > > "path" is also very ambiguous. I see that "tracked" is often used to > describe what %f is, so how about: > > + Note that "%f" is the name of a file tracked by Git. Depending on the > + version that is being filtered, the corresponding file on disk may not > + exist, or may have different contents. So, smudge and clean commands should > + not try to access the file on disk. I think that places stress on a wrong point. I do have a preference between "file" or "path", merely because, as I showed already (go back and read what you are responding to), the preceding paragraphs all talk in terms of "paths". But that is not the important part. "tracked by Git" is not all that interesting, compared to the fact that your filter needs to give contents relevant to that path because that is what the command line argument Git gives you with '%f' means. It is not a random filename "tracked by Git". Among 47 other files tracked by Git, the single one being given is the one the code that drives the filter is WORKING ON, and I think that needs to be written in the description, hence "the path that is being worked on" was my suggestion. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Jun 2016, #05; Thu, 16)
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. Let's start a new cycle by rewinding the tip of 'next' soonish. I expect I'd eject a few premature topics out of 'next' while doing so. There are many topics that need input from the list (look for '?' in this document) to decide either to drop them or to move them forward. 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 -- [New Topics] * ap/git-svn-propset-doc (2016-06-15) 1 commit - git-svn: document the 'git svn propset' command "git svn propset" subcommand that was added in 2.3 days is documented now. Will merge to 'next'. * jk/add-i-diff-compact-heuristics (2016-06-16) 1 commit - add--interactive: respect diff.compactionHeuristic "git add -i/-p" learned to honor diff.compactionHeuristic experimental knob, so that the user can work on the same hunk split as "git diff" output. Will merge to 'next'. * jk/big-and-old-archive-tar (2016-06-16) 2 commits - archive-tar: write extended headers for far-future mtime - archive-tar: write extended headers for file sizes >= 8GB "git archive" learned to handle files that are larger than 8GB and commits far in the future than expressible by the traditional US-TAR format. Will merge to 'next'. * jk/gpg-interface-cleanup (2016-06-16) 7 commits - gpg-interface: check gpg signature creation status - sign_buffer: use pipe_command - verify_signed_buffer: use pipe_command - run-command: add pipe_command helper - verify_signed_buffer: use tempfile object - verify_signed_buffer: drop pbuf variable - gpg-interface: use child_process.args A new run-command API function pipe_command() is introduced to sanely feed data to the standard input while capturing data from the standard output and the standard error of an external process, which is cumbersome to hand-roll correctly without deadlocking. The codepath to sign data in a prepared buffer with GPG has been updated to use this API to read from the status-fd to check for errors (instead of relying on GPG's exit status). Will merge to 'next'. * lf/sideband-returns-void (2016-06-16) 2 commits - upload-pack.c: make send_client_data() return void - sideband.c: make send_sideband() return void A small internal API cleanup. Will merge to 'next'. * nd/graph-width-padded (2016-06-16) 2 commits - pretty.c: support |() forms - pretty: pass graph width to pretty formatting for use in '%>|(N)' "log --graph --format=" learned that "%>|(N)" specifies the width relative to the terminal's left edge, not relative to the area to draw text that is to the right of the ancestry-graph section. It also now accepts negative N that means the column limit is relative to the right border. Will merge to 'next'. * dn/gpg-doc (2016-06-16) 1 commit - Documentation: GPG capitalization The documentation tries to consistently spell "GPG"; when referring to the specific program name, "gpg" is used. Will merge to 'next'. * jk/bisect-show-tree (2016-06-16) 1 commit - bisect: always call setup_revisions after init_revisions "git bisect" makes an internal call to "git diff-tree" when bisection finds the culprit, but this call did not initialize the data structure to pass to the diff-tree API correctly. Will merge to 'next'. -- [Stalled] * mj/log-show-signature-conf (2016-06-06) 2 commits - log: "--no-show-signature" commmand-line option - log: add "log.showsignature" configuration variable "git log" learns log.showSignature configuration variable, and a command line option "--no-show-signature" to countermand it. The order of the commits in the topic need to be reversed. Expecting a reroll. * sb/bisect (2016-04-15) 22 commits - SQUASH??? - bisect: get back halfway shortcut - bisect: compute best bisection in compute_relevant_weights() - bisect: use a bottom-up traversal to find relevant weights - bisect: prepare for different algorithms based on find_all - bisect: rename count_distance() to compute_weight() - bisect: make total number of commits global - bisect: introduce distance_direction() - bisect: extract get_distance() function from code duplication - bisect: use commit instead of commit list as arguments when appropriate - bisect: replace clear_distance() by unique markers - bisect: use struct node_data array instead of int array - bisect: get rid of recursion in count_distance() - bisect: make algorithm behavior independent of DEBUG_BISECT - bisect: make bisect compile if DEBUG_BISECT is set - bisect: plug the biggest memory leak - bisect: add test for the bisect algorithm - t
Re: [PATCH 1/4] clarify %f documentation
Junio C Hamano wrote: > I agree that "the name of the file" can be interpreted in many ways, > and I agree that it would be a good idea to find a better phrase to > name the path that is being worked on, but I do not think "the file > in the git repository" is that phrase. > I think using the word "path" somewhere in the updated description > is more likely to have the effect you desire. "path" is also very ambiguous. I see that "tracked" is often used to describe what %f is, so how about: + Note that "%f" is the name of a file tracked by Git. Depending on the + version that is being filtered, the corresponding file on disk may not + exist, or may have different contents. So, smudge and clean commands should + not try to access the file on disk. -- see shy jo signature.asc Description: PGP signature
Re: [bug] assertion in 2.8.4 triggering on old-ish worktree
On Fri, Jun 17, 2016 at 7:48 AM, Stefan Beller wrote:
> On Thu, Jun 16, 2016 at 10:59 AM, Junio C Hamano wrote:
>> Chris Packham writes:
>>
>>> On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham
>>> wrote:
Hi All,
I have the git-sh-prompt configured in my .bashrc today I visited an
old worktree that I haven't really touched in a few years (sorry can't
remember the git version I was using back then). I received the
following output when changing to the directory
git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len
<= item->len && item->prefix <= item->len' failed.
I assume it's one of the git invocations in git-sh-prompt that's
hitting the assertion. Any thoughts on what might be triggering it?
Any debug I can gather?
>>>
>>> A bit more info. The directory in question is a uninitialised
>>> submodule. It doesn't trigger in the root of the parent project.
The command that fails appears to be 'git check-ignore -q .'
>>
>>
>> Sounds like
>> http://article.gmane.org/gmane.comp.version-control.git/283549
>>
>
> I looked into this. In pathspec.c#prefix_pathspec (the function
> that has this assertion at the end), the assertion can only
> trigger if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
> or PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP was given
> as these are the only places that reduce item->len.
>
> Regarding the assert:
> We are sure it's a submodule related thing, so we can
> have a quite narrow warning there, roughly:
>
> diff --git a/pathspec.c b/pathspec.c
> index c9e9b6c..d0ea87a 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,11 @@ static unsigned prefix_pathspec(struct pathspec_item
> *item,
> }
>
> /* sanity checks, pathspec matchers assume these are sane */
> - assert(item->nowildcard_len <= item->len &&
> - item->prefix <= item->len);
> + if (item->nowildcard_len <= item->len &&
> + item->prefix <= item->len)
> + die (_("Path leads inside submodule '%s', but the submodule "
> + "was not recognized, i.e. not initialized or deleted"),
> + ce->name);
This certainly would have pointed out the uninitialised state of my
setup in this case.
> return magic;
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bisect: always call setup_revisions after init_revisions
On Thu, Jun 16, 2016 at 05:03:53PM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > The former initializes the rev_info struct to default
> > values, and the latter parsers any command-line arguments
> > and finalizes the struct.
>
> The former refers to init and the latter setup?
Yeah, sorry, I guess I was reaching back to the subject line.
Maybe (also fixing a typo):
init_revisions() initializes the rev_info struct to default values,
and setup_revisions() parses any command-line arguments and finalizes
the struct.
> I wonder if we can make it even harder to make the same mistake
> again somehow. I notice that run_diff_files() and run_diff_index()
> in diff-lib.c share the ideal name for such an easy-to-use helper
> and run_diff_tree(), which does not exist yet, could sit alongside
> with them, but the actual implementation of the former two do not
> address this issue either. I guess that the diversity of the set of
> pre-packaged options that various callers want to use are so graet
> that we need a rather unpleasntly large API refactoring before we
> could even contemplate doing so?
>
> In any case, this is a strict improvement. Let's queue it for the
> first maintenance release.
I wondered about something like the patch below, to detect such problems
consistently (and not just blow up on some corner case that isn't hit in
the test suite).
But it doesn't cover every way somebody might use a "struct rev_info",
so we'd have to sprinkle more "check" functions around. And a bunch of
stuff fails in the test suite (though it looks like it's mostly rebase
stuff, so it's probably all one or two plumbing call-sites).
I do notice some sites, like builtin/pull, use init_revisions() coupled
with diff_setup_done(). That's OK if you're just doing a diff, though
I'd argue they should use setup_revisions() to be on the safe side.
---
diff --git a/log-tree.c b/log-tree.c
index 78a5381..8303e64 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -538,6 +538,12 @@ static void show_mergetag(struct rev_info *opt, struct
commit *commit)
for_each_mergetag(show_one_mergetag, commit, opt);
}
+static void check_rev_info(struct rev_info *opt)
+{
+ if (!opt->setup_finished)
+ die("BUG: init_revisions called without setup_revisions");
+}
+
void show_log(struct rev_info *opt)
{
struct strbuf msgbuf = STRBUF_INIT;
@@ -547,6 +553,8 @@ void show_log(struct rev_info *opt)
const char *extra_headers = opt->extra_headers;
struct pretty_print_context ctx = {0};
+ check_rev_info(opt);
+
opt->loginfo = NULL;
if (!opt->verbose_header) {
graph_show_commit(opt->graph);
@@ -799,6 +807,8 @@ static int log_tree_diff(struct rev_info *opt, struct
commit *commit, struct log
struct commit_list *parents;
struct object_id *oid;
+ check_rev_info(opt);
+
if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))
return 0;
diff --git a/revision.c b/revision.c
index d30d1c4..2677b2e 100644
--- a/revision.c
+++ b/revision.c
@@ -2341,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+ revs->setup_finished = 1;
+
return left;
}
diff --git a/revision.h b/revision.h
index 9fac1a6..2dc6ecb 100644
--- a/revision.h
+++ b/revision.h
@@ -213,6 +213,8 @@ struct rev_info {
struct commit_list *previous_parents;
const char *break_bar;
+
+ unsigned setup_finished;
};
extern int ref_excluded(struct string_list *, const char *path);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] bisect: always call setup_revisions after init_revisions
Jeff King writes: > The former initializes the rev_info struct to default > values, and the latter parsers any command-line arguments > and finalizes the struct. The former refers to init and the latter setup? > In e22278c (bisect: display first bad commit without forking > a new process, 2009-05-28), a show_diff_tree() was added > that calls the former but not the latter. It doesn't have > any arguments to parse, but it still should do the > finalizing step. > > This may have caused other minor bugs over the years, but it > became much more prominent after fe37a9c (pretty: allow > tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves > the expected tab width as "-1", rather than the true default > of "8". When we see a commit with tabs to be expanded, we > end up trying to add (size_t)-1 spaces to a strbuf, which > complains about the integer overflow. > > The fix is easy: just call setup_revisions() with no > arguments. Thanks. I wonder if we can make it even harder to make the same mistake again somehow. I notice that run_diff_files() and run_diff_index() in diff-lib.c share the ideal name for such an easy-to-use helper and run_diff_tree(), which does not exist yet, could sit alongside with them, but the actual implementation of the former two do not address this issue either. I guess that the diversity of the set of pre-packaged options that various callers want to use are so graet that we need a rather unpleasntly large API refactoring before we could even contemplate doing so? In any case, this is a strict improvement. Let's queue it for the first maintenance release. > Signed-off-by: Jeff King > --- > Same patch as earlier, now with 100% more commit message. > > I didn't add a test, as it seemed weirdly specific to be checking "can > bisect show a commit with tabs in it". I.e., it's not likely to actually > regress in this specific way again. > > bisect.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/bisect.c b/bisect.c > index 6d93edb..dc13319 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct > commit *commit) > if (!opt.diffopt.output_format) > opt.diffopt.output_format = DIFF_FORMAT_RAW; > > + setup_revisions(0, NULL, &opt, NULL); > log_tree_commit(&opt, commit); > } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add--interactive: respect diff.compactionHeuristic
On Thu, Jun 16, 2016 at 09:50:45AM -0700, Stefan Beller wrote: > > -- >8 -- > > Subject: add--interactive: respect diff.compactionHeuristic > > > > We use plumbing to generate the diff, so it doesn't > > automatically pick up UI config like compactionHeuristic. > > Let's forward it on, since interactive adding is porcelain. > > > > Note that we only need to handle the "true" case. There's no > > point in passing --no-compaction-heuristic when the variable > > is false, since nothing else could have turned it on. > > because we don't want to implement --[no-]compaction-heuristic > as a command line switch to git-add? > Fine with me. We could, but I don't think it is worth the effort (and anyway, it would override the config :) ). > Stepping back and looking how the compaction heuristic turned out, > I think this is what we did not want to see, i.e. the need to bring it in > every command, but rather enable and release it. But we backed off > of the default-on, and now people may ask for the --no-compaction-heuristic > in interactive add eventually, when they run into a corner case. Yeah, I'm not excited to be plumbing it through, especially if we just end up flipping it on by default. But perhaps people would still want to be able to do the opposite (turning it off for a specific case via the config). I dunno. > For now: > Reviewed-by: Stefan Beller Thanks. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] bisect: always call setup_revisions after init_revisions
The former initializes the rev_info struct to default values, and the latter parsers any command-line arguments and finalizes the struct. In e22278c (bisect: display first bad commit without forking a new process, 2009-05-28), a show_diff_tree() was added that calls the former but not the latter. It doesn't have any arguments to parse, but it still should do the finalizing step. This may have caused other minor bugs over the years, but it became much more prominent after fe37a9c (pretty: allow tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves the expected tab width as "-1", rather than the true default of "8". When we see a commit with tabs to be expanded, we end up trying to add (size_t)-1 spaces to a strbuf, which complains about the integer overflow. The fix is easy: just call setup_revisions() with no arguments. Signed-off-by: Jeff King --- Same patch as earlier, now with 100% more commit message. I didn't add a test, as it seemed weirdly specific to be checking "can bisect show a commit with tabs in it". I.e., it's not likely to actually regress in this specific way again. bisect.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bisect.c b/bisect.c index 6d93edb..dc13319 100644 --- a/bisect.c +++ b/bisect.c @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) if (!opt.diffopt.output_format) opt.diffopt.output_format = DIFF_FORMAT_RAW; + setup_revisions(0, NULL, &opt, NULL); log_tree_commit(&opt, commit); } -- 2.9.0.165.g4aacdc3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Documentation: GPG capitalization
Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Documentation: GPG capitalization
When "GPG" is used in a sentence it is now consistently capitalized. When referring to the binary it is left as "gpg". Signed-off-by: David Nicolson --- Documentation/git-tag.txt | 2 +- Documentation/git-verify-commit.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index abab481..32bc4aa 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt @@ -78,7 +78,7 @@ OPTIONS -v:: --verify:: - Verify the gpg signature of the given tag names. + Verify the GPG signature of the given tag names. -n:: specifies how many lines from the annotation, if any, diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt index ecf4da1..92097f6 100644 --- a/Documentation/git-verify-commit.txt +++ b/Documentation/git-verify-commit.txt @@ -12,7 +12,7 @@ SYNOPSIS DESCRIPTION --- -Validates the gpg signature created by 'git commit -S'. +Validates the GPG signature created by 'git commit -S'. OPTIONS --- -- https://github.com/git/git/pull/246 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] add smudge-to-file and clean-from-file filter configuration
Joey Hess writes: > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 2e1b2e4..bbb9296 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1299,14 +1299,29 @@ format.useAutoBase:: > format-patch by default. > > filter..clean:: > - The command which is used to convert the content of a worktree > - file to a blob upon checkin. See linkgit:gitattributes[5] for > - details. > + The command which is used on checkin to convert the content of > + a worktree file (provided on stdin) to a blob (written to stdout). > + See linkgit:gitattributes[5] for details. > > filter..smudge:: > - The command which is used to convert the content of a blob > - object to a worktree file upon checkout. See > - linkgit:gitattributes[5] for details. > + The command which is used on checkout to convert the content of a > + blob object (provided on stdin) to a worktree file (written to > + stdout). > + See linkgit:gitattributes[5] for details. A "filter" by definition reads from its standard input and writes the result to its standard output, so I do not know if this change is necessary. If you are going to do this, in documentation (i.e. not code), please spell standard input/output out, and avoid stdin/stdout. There is what we would want to fix, though. "worktree file" should be spelled "working tree file". This used not to matter before "git worktree" was invented (before that we used these two terms interchangeably), but these days the distinction matters. > +filter..clean-from-file:: Documentation/config.txt hopefully lists all the configuration, but I do not see anything that uses 'words-joined-with-dash' format. Please do not invent new out-of-convention names. > + Optional command which is used on checkin to convert the content > + of a worktree file, which can be read from disk, to a blob > + (written to stdout). I am not sure why we want to say "which can be read from disk". I think what a reader needs to be told (but this paragraph is not telling her) in order to understand what you meant to say is: cleanFromFile is asked to produce the "cleaned" content to its standard output (to be stored in the object database), but unlike clean, it does not work as a filter and is not given a file descriptor to read the working tree contents from. Instead, it is told the path for which the contents need to be generated as the first parameter on its command line. (the above is deliberately made verbose and is not meant as a suggestion to be literally used in your updated documentation). With that understanding, the reader may be able to guess that "can be read from disk" is a permission for her cleanFromFile filter (i.e. it does not necessarily have to read from it and produced its output by some other magic); otherwise it can be misread as if the content of a working tree file is deposited on the disk to enable the filter to read it, but the location of that on-disk file is somewhere unspecified by this paragraph. > + Only used when filter..clean is also configured. > + See linkgit:gitattributes[5] for details. > + > +filter..smudge-to-file:: > + Optional command which is used to convert the content of a blob > + object (provided on stdin) to a worktree file, writing directly > + to the file. A similar comment applies. With ", writing directly to the file." replaced with something like ". The command is expected to write to the working tree file at path given as the first parameter on the command line." it would be sufficient (i.e. it is clear enough that it does not give the smudged contents via its standard output, and no need to say "unlike smudge filter, ..." like we needed to for the "clean" side). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff compaction heuristic: favor shortest neighboring blank lines
>
> The heuristic I proposed was
>
> * Prefer to start and end hunks *following* lines with the least
> indentation.
>
> * Define the "indentation" of a blank line to be the indentation of
> the previous non-blank line minus epsilon.
>
> * In the case of a tie, prefer to slide the hunk down as far as
> possible.
>
> If that's what you are trying to implement, then notice that the first
> rule says that the hunk should start *following* the line with the least
> indentation. So the "score" for
I did not try to implement that heuristic.
> * Prefer to start and end hunks *following* lines with the least
> indentation.
We had this alone as a heuristic and it turned out badly. (More
false positives than the current "empty line only" thing)
It may however be a good starting block when combined with the other
conditions, as blank lines are assigned a longer virtual length than
what they have.
> * In the case of a tie, prefer to slide the hunk down as far as
> possible.
>
This is what the current implementation does as well. (i.e. if there is
more than one blank line, we have a tie, so choose the last one as the
end of the hunk)
So what I am proposing in this patch:
* Prefer to start and end hunks *following* lines with an empty line.
(as implemented currently)
* In the case of a tie, make use of a second tier heuristic.
* Define the "length" of a blank line to be the
minimum of the amount of white space in the line before and after
* Choose that empty line (first tier) that has the shortest length
(second tier)
* In the case of a tie, prefer to slide the hunk down as far as
possible.
I would be really interested how these two heuristics compare in practice.
>
>> +end
>> +
>> +def bal
>
> would be the indentation of the line preceding "end", which is larger
> than the indentation of "end". And the score for
>
>> +do_bal_stuff()
>> +
>> +common_ending()
>
> would come from the line preceding "do_bal_stuff()", namely "def bal()",
> which is indented the same as "end". So the latter would be preferred.
>
> But actually, I don't understand how your example is meaningful. I think
> this heuristic is only used to slide hunks around; i.e., when the line
> following the hunk is the same as the first lines of the hunk, or when
> the line preceding the hunk is the same as the last line of the hunk.
Right.
> Right? So your snippets would never compete against each other.
right, I was using that to show why some parts are more favorable to
put a break in (i.e. the transisiton from "+ lines" to surrounding lines.
I was not clear enough there. :(
> Let's
> take the full example. The five possible placements for the `+`
> characters are marked with the digits 1 through 5 here:
>
>> def bar
>> do_bar_stuff()
>> 1
>> 12 common_ending()
>> 123 end
>> 1234
>> 12345def bal
>> 12345do_bal_stuff()
>> 2345
>> 345common_ending()
>>45end
>> 5
>> def baz
>> do_baz_stuff()
>>
>> common_ending()
>> end
>
> Of the lines preceding the candidate hunks, the blank line preceding the
> hunk marked "5" has the smallest indent, namely "epsilon" less than the
> indentation of the "end" line preceding it. So placement "5" would be
> chosen.
I agree 5 is the best here, but for other reasons.
Imagine /s/end/long final boilerplate code/, and it may be more clear.
What I am trying to say is, that you "indentation" of the blank line is not
reaching until the end of the "end" line, but rather to the front of "end".
(If you don't care of white spaces in the git history and have an editor, that
makes fancy white space stuff, i.e. your text editor cursor is in the "end" line
and you press "enter" to get to the next line, it is most likely indented to
strlen ("") - strlen("end"), as that is how much indentation we had
in the preceding line?
>
> I don't know enough about this area of the code to review your patch in
> detail, but I did note below a typo that jumped out at me.
I did not know the code either before writing the patch. :)
>> + if (min_bl_neigh_indent >
>> bl_neigh_indent)
>> + min_bl_neigh_indent =
>> min_bl_neigh_indent;
>
> The line above has no effect (same variable on both sides of the =).
Thanks!
I had the feeling something was off judging from behavior,
but could not tell what exactly. This is it.
It should be
min_bl_neigh_indent = bl_neigh_indent;
i.e. the minimum of the existing and newly computed surrounding line
starting blanks.
Junio writes:
> Now, on what column does 'd' sit on the example below?
>
>def foo
>
> I typed SP SP HT 'd' 'e' 'f'; it still is indented by 8 places.
I see, so for the TAB case we would need to do a
ret += (8 - ret%8);
Thanks!
S
Re: [PATCH 1/4] clarify %f documentation
Joey Hess writes: > It's natural to expect %f to be an actual file on disk; help avoid that > mistake. I agree that "the name of the file" can be interpreted in many ways, and I agree that it would be a good idea to find a better phrase to name the path that is being worked on, but I do not think "the file in the git repository" is that phrase. When I first read the updated text without the above two lines in the log message, I thought "hmph, so we may use a temporary file somewhere in $GIT_DIR/ and that would be pointed at by %f, not the actual path we are working on???", i.e. the rephrasing had exactly the opposite effect on me. Given that this being part of gitattributes(5) that begins with A `gitattributes` file is a simple text file that gives `attributes` to pathnames. Each line in `gitattributes` file is of form: pattern attr1 attr2 ... That is, a pattern followed by an attributes list, separated by whitespaces. When the pattern matches the path in question, the attributes listed on the line are given to the path. and that the readers already read something like this in the paragraphs before the mention of '%f': For example, in .gitattributes, you would assign the `filter` attribute for paths. *.c filter=indent Then you would define ... I think using the word "path" somewhere in the updated description is more likely to have the effect you desire. > Signed-off-by: Joey Hess > --- > Documentation/gitattributes.txt | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt > index e3b1de8..e077cc9 100644 > --- a/Documentation/gitattributes.txt > +++ b/Documentation/gitattributes.txt > @@ -365,8 +365,8 @@ you can declare that the filter is `required`, in the > configuration: > > > Sequence "%f" on the filter command line is replaced with the name of > -the file the filter is working on. A filter might use this in keyword > -substitution. For example: > +the file in the git repository the filter is working on. > +A filter might use this in keyword substitution. For example: > > > [filter "p4"] > @@ -374,6 +374,9 @@ substitution. For example: > smudge = git-p4-filter --smudge %f > > > +Note that the "%f" is the name of a file in the git repository; the > +corresponding file on disk may not exist, or may have unrelated contents to > +what git is filtering. > > Interaction between checkin/checkout attributes > ^^^ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)
Jeff King writes: > On Wed, Jun 15, 2016 at 09:35:20AM +0200, Elia Pinto wrote: > >> > * ep/http-curl-trace (2016-05-24) 2 commits >> > - imap-send.c: introduce the GIT_TRACE_CURL enviroment variable >> > - http.c: implement the GIT_TRACE_CURL environment variable >> > >> > HTTP transport gained an option to produce more detailed debugging >> > trace. >> > >> > Rerolled. Is everybody happy with this version? >> > >> The refs is there >> http://git.661346.n2.nabble.com/PATCH-v7-0-2-Implement-the-GIT-TRACE-CURL-environment-variable-td7657079.html >> >> If kindly someone who has reviewed and helped me to do the patch >> could give an ack (or a nack eventually). Thanks in advance > > I gave another look at what is queued in pu, and I think it is OK. I > still find the output a little verbose, but I think we should ship it > and see how it fares in practice while debugging. I don't think its > exact format needs to be set in stone, so we can tweak it later if we > choose. Thanks, let's push it forwared then. > > -Peff > > PS Please trim your quotes to just the relevant bits. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff compaction heuristic: favor shortest neighboring blank lines
On 06/16/2016 07:46 PM, Stefan Beller wrote:
> The compaction heuristic for diffs was deemed quite good, but in 5580b27
> we have an example demonstrates a common case, that fails with the existing
> heuristic. That is why we disabled the heuristic in the v2.9.0 release.
>
> With this patch a diff looks like:
>
> def bar
> do_bar_stuff()
>
> common_ending()
> end
>
> +def bal
> +do_bal_stuff()
> +
> +common_ending()
> +end
> +
> def baz
> do_baz_stuff()
>
> common_ending()
> end
>
> whereas before we had:
>
> WIP (TODO: ask peff to provide an example that actually triggers, I seem to
> be
>unable to write one, though I thought the above was one)
>
>
> The way we do it, is by inspecting the neighboring lines and see how
> much indent there is and we choose the line that has the shortest amount
> of blanks in the neighboring lines.
>
> (TODO: update comments in the file)
>
> Signed-off-by: Stefan Beller
> ---
>
> Hi Jeff, hi Michael,
>
> thanks for pointing out the flaw in this ruby code base before the 2.9
> release.
> I think this patch would fix your finding, though I cannot reproduce it.
> Can you provide an example repo/patch that looks ugly on current
> origin/master,
> so I can be sure this fixes the issue?
>
> This can also be a start of a discussion if this is a too short-sighted
> enhancement
> of the heuristic. Essentially we'd want to prefer
>
> +end
> +
> +def bal
>
> over:
>
> +do_bal_stuff()
> +
> +common_ending()
>
> because there is less space between line start and {end, def bal}
> than for {do_bal_stuff, common_ending}.
It's really cool that you are trying to implement the indentation
heuristic. I'm curious how it works in practice (something we can
probably only determine by applying it to a corpus of diffs to see how
often it outperforms/underperforms the other possible approaches).
The heuristic I proposed was
* Prefer to start and end hunks *following* lines with the least
indentation.
* Define the "indentation" of a blank line to be the indentation of
the previous non-blank line minus epsilon.
* In the case of a tie, prefer to slide the hunk down as far as
possible.
If that's what you are trying to implement, then notice that the first
rule says that the hunk should start *following* the line with the least
indentation. So the "score" for
> +end
> +
> +def bal
would be the indentation of the line preceding "end", which is larger
than the indentation of "end". And the score for
> +do_bal_stuff()
> +
> +common_ending()
would come from the line preceding "do_bal_stuff()", namely "def bal()",
which is indented the same as "end". So the latter would be preferred.
But actually, I don't understand how your example is meaningful. I think
this heuristic is only used to slide hunks around; i.e., when the line
following the hunk is the same as the first lines of the hunk, or when
the line preceding the hunk is the same as the last line of the hunk.
Right? So your snippets would never compete against each other. Let's
take the full example. The five possible placements for the `+`
characters are marked with the digits 1 through 5 here:
> def bar
> do_bar_stuff()
> 1
> 12 common_ending()
> 123 end
> 1234
> 12345def bal
> 12345do_bal_stuff()
> 2345
> 345common_ending()
>45end
> 5
> def baz
> do_baz_stuff()
>
> common_ending()
> end
Of the lines preceding the candidate hunks, the blank line preceding the
hunk marked "5" has the smallest indent, namely "epsilon" less than the
indentation of the "end" line preceding it. So placement "5" would be
chosen.
I don't know enough about this area of the code to review your patch in
detail, but I did note below a typo that jumped out at me.
Michael
> xdiff/xdiffi.c | 41 +++--
> 1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
> index b3c6848..58adc74 100644
> --- a/xdiff/xdiffi.c
> +++ b/xdiff/xdiffi.c
> [...]
> @@ -485,7 +515,13 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo,
> long flags) {
>* the group.
>*/
> while (ix < nrec && recs_match(recs, ixs, ix, flags)) {
> - blank_lines += is_blank_line(recs, ix, flags);
> + if (is_blank_line(recs, ix, flags)) {
> + unsigned int bl_neigh_indent =
> + surrounding_leading_blank(recs,
> ix, flags, nrec);
> +
Re: [PATCH] diff compaction heuristic: favor shortest neighboring blank lines
On Thu, Jun 16, 2016 at 1:27 PM, Junio C Hamano wrote:
>> ...
>> because there is less space between line start and {end, def bal}
>> than for {do_bal_stuff, common_ending}.
>
> I haven't thought this carefully yet, but would this equally work
> well for Python, where it does not have the "end" or does the lack
> of "end" pose a problem? You'll still find "def bal" is a good
> boundary (but you cannot tell if it is the beginning or the end of a
> block, unless you understand the language), though.
Good point. I found a flaw in my implementation
(as it doesn't match my mental model, not necessarily a bad thing)
We take the minimum of the two neighbors, i.e.
+do_bal_stuff()
+
+common_ending()
is preferrable to
+do_bal_stuff()
+
+common_ending()
and in python the example would look like:
def foo():
do_foo()
common_thing()
+def baz():
+do_baz()
+
+common_thing()
+
def bar():
do_bar()
common_thing()
and breaking between
common_thing()
def bar():
is more favorable than between
do_baz()
common_thing()
because in the first former the count of
white space in front of "def bar():" is smaller
than for any of "do_baz()" and "common_thing()"
>
>> +static unsigned int leading_blank(const char *line)
>> +{
>> + unsigned int ret = 0;
>> + while (*line) {
>> + if (*line == '\t')
>> + ret += 8;
>
> This will be broken with a line with space-before-tab whitespace
> breakage, I suspect...
How so? We inspect each character on its own and then move on later
by line++. (I am not seeing how this could cause trouble, so please
help me?)
Going back to python, this may become a problem when you have a code like:
def baz():
do_baz()
common_thing()
def bar():
+ do_bal()
+
+ common_thing()
+
+def bar():
+
do_bar()
common_thing()
but this was fabricated with a typo (the first definition of bar
should have been bal),
(Also it doesn't worsen the diff, as it is same without the heuristic)
once that typo is fixed we get:
(both with and without the heuristic)
do_foo()
common_thing()
def baz():
do_baz()
common_thing()
+def bal():
+
+ do_bal()
+
+ common_thing()
+
def bar():
do_bar()
common_thing()
Clearly it can also be intentional to have 2 methods with the same
code for historical reasons, (even without the blank line after the
function definition this produces the same result)
When playing around with various diffs I could not find a thing that
this patch makes worse, it only fixes the actual issue.
(I realized Peff actually attached a script to produce a bad diff, which
is gone with this patch)
Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] use smudge-to-file in git checkout etc
Joey Hess wrote:
> + int smudge_to_file = can_smudge_to_file(ce->name);
> if (ce_mode_s_ifmt == S_IFREG &&
> + ! smudge_to_file &&
> convert_to_working_tree(ce->name, new, size, &buf)) {
> free(new);
> new = strbuf_detach(&buf, &newsize);
> @@ -189,13 +193,29 @@ static int write_entry(struct cache_entry *ce,
> + if (! can_smudge_to_file(ce->name)) {
> + }
> + else {
> + close(fd);
> + convert_to_working_tree_filter_to_file(ce->name, path,
> new, size);
Oops, I had meant to avoid using smudge-to-file when
e_mode_s_ifmt != S_IFREG, and forgot it in this patch, so it does the
wrong thing for symlinks.
I'll send an updated patch set fixing this after any other review.
--
see shy jo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
On Thu, Jun 16, 2016 at 9:25 PM, Pranit Bauva wrote:
> Hey Eric,
>
> On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine
> wrote:
>> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva wrote:
>>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine
>>> wrote:
On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva
wrote:
> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
> call it from git-bisect.sh .
> [...]
> Signed-off-by: Pranit Bauva
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
> +static int is_expected_rev(const char *expected_hex)
> +{
> + struct strbuf actual_hex = STRBUF_INIT;
> + int res;
> +
> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(),
> 0) < 0) {
> + strbuf_release(&actual_hex);
> + return 0;
> + }
> +
> + strbuf_trim(&actual_hex);
> + res = !strcmp(actual_hex.buf, expected_hex);
> + strbuf_release(&actual_hex);
> + return res;
> +}
Not worth a re-roll, but this could be re-structured to avoid having
to remember to release the strbuf at all exits:
struct strbuf actual_hex = ...;
int res = 0;
if (strbuf_read_file(...) >= 0) {
strbuf_trim(...);
res = !strcmp(...);
}
strbuf_release(...);
return res;
Alternately:
if (strbuf_read_file(...) < 0)
goto done;
strbuf_trim(...);
res = !strcmp(...);
done:
strbuf_release(...);
return res;
which is a bit less compact.
>>>
>>> I will avoid this for the reason that I will have to create a label
>>> for a lot of functions. If I choose to do this for one function, I
>>> think it would be more appropriate to do the same for other functions.
>>> There would be a lot of functions in future which would be in the same
>>> scenario and creating a separate label for each of them would be quite
>>> tedious. What do you think?
>>
>> Not sure what you're talking about. Label names are not shared across
>> functions. Anyhow, the first suggestion I presented above is more
>> concise than the 'goto' version.
>
> Yes I am aware of the fact that labels aren't shared across functions.
> What I meant by "separate label" was that I will have to make a label
> "fail" in each function. But I recently noticed that its used quite a
> lot so I think it would be okay to use it. Will re-roll with using
> labels and goto.
My opinion is that if there is a more concise version without labels
and gotos, it's better to use it, so I would suggest Eric's first
suggestion which is:
> struct strbuf actual_hex = ...;
> int res = 0;
>
> if (strbuf_read_file(...) >= 0) {
> strbuf_trim(...);
> res = !strcmp(...);
> }
> strbuf_release(...);
> return res;
Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C
On Thu, Jun 16, 2016 at 9:01 PM, Pranit Bauva wrote: > Hey Eric, > > On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine > wrote: >> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva >> wrote: >>> >>> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD >>> from the global shell script thus we need to pass it to the subcommand >>> using the arguments. After the whole conversion, we can remove the extra >>> arguments and make the method use the two variables from the global scope >>> within the C code. >> >> You could do this now rather than waiting for later. Instead of >> passing these arguments to bisect_write(), create global variables in >> this patch and assign them in the BISECT_WRITE case of >> cmd_bisect__helper() before calling bisect_write(). >> >> Not necessarily worth a re-roll, but would save you the effort of >> having to explain it here and then change it in some later patch. > > I have actually done it in my next conversion which is converting > check_and_set_terms()[1] which also sets those variables to some value > so its more appropriate there. My opinion about this is that using global variables would go against a possible future libification of the bisect functionality and might be less safe than just adding 2 parameters to a small number of functions. If we think that 2 parameters are too much or that there could be more parameters to pass like this, we could just pass a pointer to a 'struct bisect_state' or something like that ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] use smudge-to-file in git checkout etc
This makes git checkout, git reset, etc use smudge-to-file.
Includes test cases.
(There's a call to convert_to_working_tree in merge-recursive.c
that could also be made to use smudge-to-file as well.)
Signed-off-by: Joey Hess
---
entry.c | 34 +++---
t/t0021-conversion.sh | 44
2 files changed, 63 insertions(+), 15 deletions(-)
diff --git a/entry.c b/entry.c
index 519e042..6a23159 100644
--- a/entry.c
+++ b/entry.c
@@ -175,8 +175,12 @@ static int write_entry(struct cache_entry *ce,
/*
* Convert from git internal format to working tree format
+* unless the smudge-to-file filter can write to the
+* file directly.
*/
+ int smudge_to_file = can_smudge_to_file(ce->name);
if (ce_mode_s_ifmt == S_IFREG &&
+ ! smudge_to_file &&
convert_to_working_tree(ce->name, new, size, &buf)) {
free(new);
new = strbuf_detach(&buf, &newsize);
@@ -189,13 +193,29 @@ static int write_entry(struct cache_entry *ce,
return error_errno("unable to create file %s", path);
}
- wrote = write_in_full(fd, new, size);
- if (!to_tempfile)
- fstat_done = fstat_output(fd, state, &st);
- close(fd);
- free(new);
- if (wrote != size)
- return error("unable to write file %s", path);
+ if (! can_smudge_to_file(ce->name)) {
+ wrote = write_in_full(fd, new, size);
+ if (!to_tempfile)
+ fstat_done = fstat_output(fd, state, &st);
+ close(fd);
+ free(new);
+ if (wrote != size)
+ return error("unable to write file %s", path);
+ }
+ else {
+ close(fd);
+ convert_to_working_tree_filter_to_file(ce->name, path,
new, size);
+ free(new);
+ if (!to_tempfile) {
+ /* Re-open the file to stat it; the
+* smudge-to-file filter may have replaced
+* the file. */
+ fd = open(path, O_RDONLY);
+ if (fd < 0) {
+ return error_errno("unable to create
file %s", path);
+ }
+ }
+ }
break;
case S_IFGITLINK:
if (to_tempfile)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 1043ea5..b0e2e5e 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -14,12 +14,20 @@ chmod +x rot13.sh
cat "\$destfile"
+EOF
+chmod +x rot13-to-file.sh
+
test_expect_success setup '
git config filter.rot13.smudge ./rot13.sh &&
git config filter.rot13.clean ./rot13.sh &&
@@ -291,17 +299,37 @@ test_expect_success 'clean-from-file filter is used when
adding a file' '
cmp test.t fstest.t
'
+test_expect_success 'smudge-to-file filter is used when checking out a file' '
+ test_config filter.rot13.smudge-to-file "./rot13-to-file.sh %p" &&
+
+ rm -f fstest.t &&
+ git checkout -- fstest.t &&
+ cmp test.t fstest.t &&
+
+ test -e rot13-to-file.ran &&
+ rm -f rot13-to-file.ran
+'
+
test_expect_success 'clean-from-file filter is not used when clean filter is
not configured' '
- test_config filter.no.smudge ./rot13.sh &&
- test_config filter.no.clean-from-file "./rot13-from-file.sh %p" &&
+ test_config filter.noclean.smudge ./rot13.sh &&
+ test_config filter.noclean.clean-from-file "./rot13-from-file.sh %p" &&
- echo "*.no filter=no" >.gitattributes &&
+ echo "*.no filter=noclean" >.gitattributes &&
cat test.t >test.no &&
git add test.no &&
- test ! -e rot13-from-file.ran &&
- git cat-file blob :test.no >actual &&
- cmp test.t actual
+ test ! -e rot13-from-file.ran
+'
+
+test_expect_success 'smudge-to-file filter is not used when smudge filter is
not configured' '
+ test_config filter.nosmudge.clean ./rot13.sh &&
+ test_config filter.nosmudge.smudge-to-file "./rot13-to-file.sh %p" &&
+
+ echo "*.no filter=nosmudge" >.gitattributes &&
+
+ rm -f fstest.t &&
+ git checkout -- fstest.t &&
+ test ! -e rot13-to-file
[PATCH 0/4] extend smudge/clean filters with direct file access
As discussed in this thread: http://thread.gmane.org/gmane.comp.version-control.git/294425 This adds smudge-to-file and clean-from-file commands supplimenting the smudge and clean filters. This interface can be much more efficient when operating on large files, because the whole file content does not need to be streamed through the filter. It even allows for things like clean-from-file commands that avoid reading the whole content of the file, and for smudge-to-file commands that populate a work tree file using an efficient Copy On Write operation. Joey Hess (4): clarify %f documentation add smudge-to-file and clean-from-file filter configuration use clean-from-file in git add use smudge-to-file in git checkout etc Documentation/config.txt| 27 +++--- Documentation/gitattributes.txt | 44 - convert.c | 107 ++-- convert.h | 10 entry.c | 34 ++--- sha1_file.c | 42 +--- t/t0021-conversion.sh | 64 7 files changed, 292 insertions(+), 36 deletions(-) -- 2.9.0.4.g2856e74.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] clarify %f documentation
It's natural to expect %f to be an actual file on disk; help avoid that mistake. Signed-off-by: Joey Hess --- Documentation/gitattributes.txt | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index e3b1de8..e077cc9 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -365,8 +365,8 @@ you can declare that the filter is `required`, in the configuration: Sequence "%f" on the filter command line is replaced with the name of -the file the filter is working on. A filter might use this in keyword -substitution. For example: +the file in the git repository the filter is working on. +A filter might use this in keyword substitution. For example: [filter "p4"] @@ -374,6 +374,9 @@ substitution. For example: smudge = git-p4-filter --smudge %f +Note that the "%f" is the name of a file in the git repository; the +corresponding file on disk may not exist, or may have unrelated contents to +what git is filtering. Interaction between checkin/checkout attributes ^^^ -- 2.9.0.4.g2856e74.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] add smudge-to-file and clean-from-file filter configuration
This adds new smudge-to-file and clean-from-file filter commands, which are similar to smudge and clean but allow direct access to files on disk. In smudge-to-file and clean-from-file, "%p" is expanded to the path to the file that should be cleaned from, or smudged to. This interface can be much more efficient when operating on large files, because the whole file content does not need to be streamed through the filter. It even allows for things like clean-from-file commands that avoid reading the whole content of the file, and for smudge-to-file commands that populate a work tree file using an efficient Copy On Write operation. The new filter commands will not be used for all filtering. They are efficient to use when git add is adding a file, or when the work tree is being updated, but not a good fit when git is internally filtering blob objects in memory for eg, a diff. So, a user who wants to use smudge-to-file should also provide a smudge command to be used in cases where smudge-to-file is not used. And ditto with clean-from-file and clean. To avoid foot-shooting configurations, the new commands are not used unless the old commands are also configured. That also ensures that a filter driver configuration that includes these new commands will work, although less efficiently, when used with an older version of git that does not support them. Signed-off-by: Joey Hess --- Documentation/config.txt| 27 +++--- Documentation/gitattributes.txt | 37 ++ convert.c | 107 ++-- convert.h | 10 4 files changed, 160 insertions(+), 21 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 2e1b2e4..bbb9296 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1299,14 +1299,29 @@ format.useAutoBase:: format-patch by default. filter..clean:: - The command which is used to convert the content of a worktree - file to a blob upon checkin. See linkgit:gitattributes[5] for - details. + The command which is used on checkin to convert the content of + a worktree file (provided on stdin) to a blob (written to stdout). + See linkgit:gitattributes[5] for details. filter..smudge:: - The command which is used to convert the content of a blob - object to a worktree file upon checkout. See - linkgit:gitattributes[5] for details. + The command which is used on checkout to convert the content of a + blob object (provided on stdin) to a worktree file (written to + stdout). + See linkgit:gitattributes[5] for details. + +filter..clean-from-file:: + Optional command which is used on checkin to convert the content + of a worktree file, which can be read from disk, to a blob + (written to stdout). + Only used when filter..clean is also configured. + See linkgit:gitattributes[5] for details. + +filter..smudge-to-file:: + Optional command which is used to convert the content of a blob + object (provided on stdin) to a worktree file, writing directly + to the file. + Only used when filter..clean is also configured. + See linkgit:gitattributes[5] for details. fsck.:: Allows overriding the message type (error, warn or ignore) of a diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index e077cc9..32621e7 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -378,6 +378,43 @@ Note that the "%f" is the name of a file in the git repository; the corresponding file on disk may not exist, or may have unrelated contents to what git is filtering. +There are two extra commands "clean-from-file" and "smudge-to-file", which +can optionally be set in a filter driver. These are similar to the "clean" +and "smudge" commands, but avoid needing to pipe the contents of files +through the filters, and instead reading/writing files in the filesystem. +This can be more efficient when using filters with large files that are not +directly stored in the repository. + +Sequence "%p" on the "clean-from-file" and "smudge-to-file" command line +is replaced with the path to a file that the filter can access. + +In the "clean-from-file" command, "%p" is the path to the file that +it should clean. Like the "clean" command, it should output the cleaned +version to standard output. + +In the "smudge-from-file" command, "%p" is the path to the file that it +should write to. (This file will already exist, as an empty file that can +be written to or replaced.) Like the "smudge" command, "smudge-from-file" +is fed the blob object from its standard input. + +Some git operations that need to apply filters cannot use "clean-from-file" +and "smudge-to-file", since the files are not present to disk. So, to avoid +inconsistent behavior, "clean-from-file" will only be used if "clean" is +also config
[PATCH 3/4] use clean-from-file in git add
Includes test cases.
Signed-off-by: Joey Hess
---
sha1_file.c | 42 --
t/t0021-conversion.sh | 36
2 files changed, 72 insertions(+), 6 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d5e1121..8df86a0 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3329,6 +3329,29 @@ static int index_stream_convert_blob(unsigned char
*sha1, int fd,
return ret;
}
+static int index_from_file_convert_blob(unsigned char *sha1,
+ const char *path, unsigned flags)
+{
+ int ret;
+ const int write_object = flags & HASH_WRITE_OBJECT;
+ struct strbuf sbuf = STRBUF_INIT;
+
+ assert(path);
+ assert(can_clean_from_file(path));
+
+ convert_to_git_filter_from_file(path, &sbuf,
+write_object ? safe_crlf : SAFE_CRLF_FALSE);
+
+ if (write_object)
+ ret = write_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),
+ sha1);
+ else
+ ret = hash_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),
+sha1);
+ strbuf_release(&sbuf);
+ return ret;
+}
+
static int index_pipe(unsigned char *sha1, int fd, enum object_type type,
const char *path, unsigned flags)
{
@@ -3421,12 +3444,19 @@ int index_path(unsigned char *sha1, const char *path,
struct stat *st, unsigned
switch (st->st_mode & S_IFMT) {
case S_IFREG:
- fd = open(path, O_RDONLY);
- if (fd < 0)
- return error_errno("open(\"%s\")", path);
- if (index_fd(sha1, fd, st, OBJ_BLOB, path, flags) < 0)
- return error("%s: failed to insert into database",
-path);
+ if (can_clean_from_file(path)) {
+ if (index_from_file_convert_blob(sha1, path, flags) < 0)
+ return error("%s: failed to insert into
database",
+path);
+ }
+ else {
+ fd = open(path, O_RDONLY);
+ if (fd < 0)
+ return error_errno("open(\"%s\")", path);
+ if (index_fd(sha1, fd, st, OBJ_BLOB, path, flags) < 0)
+ return error("%s: failed to insert into
database",
+path);
+ }
break;
case S_IFLNK:
if (strbuf_readlink(&sb, path, st->st_size))
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 7bac2bc..1043ea5 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -12,6 +12,14 @@ tr \
EOF
chmod +x rot13.sh
+cat .gitattributes &&
+
+ cat test.t >fstest.t &&
+ git add fstest.t &&
+ test -e rot13-from-file.ran &&
+ rm -f rot13-from-file.ran &&
+
+ rm -f fstest.t &&
+ git checkout -- fstest.t &&
+ cmp test.t fstest.t
+'
+
+test_expect_success 'clean-from-file filter is not used when clean filter is
not configured' '
+ test_config filter.no.smudge ./rot13.sh &&
+ test_config filter.no.clean-from-file "./rot13-from-file.sh %p" &&
+
+ echo "*.no filter=no" >.gitattributes &&
+
+ cat test.t >test.no &&
+ git add test.no &&
+ test ! -e rot13-from-file.ran &&
+ git cat-file blob :test.no >actual &&
+ cmp test.t actual
+'
+
test_done
--
2.9.0.4.g2856e74.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] diff compaction heuristic: favor shortest neighboring blank lines
Stefan Beller writes:
> +def bal
> +do_bal_stuff()
> +
> +common_ending()
> +end
> +
> def baz
> do_baz_stuff()
>
> common_ending()
> end
>
> whereas before we had:
>
> WIP (TODO: ask peff to provide an example that actually triggers, I seem to
> be
>unable to write one, though I thought the above was one)
>
>
> The way we do it, is by inspecting the neighboring lines and see how
> much indent there is and we choose the line that has the shortest amount
> of blanks in the neighboring lines.
> ...
> because there is less space between line start and {end, def bal}
> than for {do_bal_stuff, common_ending}.
I haven't thought this carefully yet, but would this equally work
well for Python, where it does not have the "end" or does the lack
of "end" pose a problem? You'll still find "def bal" is a good
boundary (but you cannot tell if it is the beginning or the end of a
block, unless you understand the language), though.
> +static unsigned int leading_blank(const char *line)
> +{
> + unsigned int ret = 0;
> + while (*line) {
> + if (*line == '\t')
> + ret += 8;
This will be broken with a line with space-before-tab whitespace
breakage, I suspect...
> + else if (*line == ' ')
> + ret ++;
> + else
> + break;
> + line++;
> + }
> + return ret;
> +}
> +
> +static unsigned int surrounding_leading_blank(xrecord_t **recs, long ix,
> + long flags, long nrec)
> +{
> + unsigned int i, ret = UINT_MAX;
> + if (ix > 0)
> + ret = leading_blank(recs[ix - 1]->ptr);
> + if (ix < nrec - 1) {
> + i = leading_blank(recs[ix + 1]->ptr);
> + if (i < ret)
> + ret = i;
> + }
> + return ret;
> +}
> +
> static int recs_match(xrecord_t **recs, long ixs, long ix, long flags)
> {
> return (recs[ixs]->ha == recs[ix]->ha &&
> @@ -416,7 +445,7 @@ static int recs_match(xrecord_t **recs, long ixs, long
> ix, long flags)
> int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
> long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;
> char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
> - unsigned int blank_lines;
> + unsigned int blank_lines, min_bl_neigh_indent;
> xrecord_t **recs = xdf->recs;
>
> /*
> @@ -451,6 +480,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo,
> long flags) {
> do {
> grpsiz = ix - ixs;
> blank_lines = 0;
> + min_bl_neigh_indent = UINT_MAX;
>
> /*
>* If the line before the current change group, is
> equal to
> @@ -485,7 +515,13 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo,
> long flags) {
>* the group.
>*/
> while (ix < nrec && recs_match(recs, ixs, ix, flags)) {
> - blank_lines += is_blank_line(recs, ix, flags);
> + if (is_blank_line(recs, ix, flags)) {
> + unsigned int bl_neigh_indent =
> + surrounding_leading_blank(recs,
> ix, flags, nrec);
> + if (min_bl_neigh_indent >
> bl_neigh_indent)
> + min_bl_neigh_indent =
> min_bl_neigh_indent;
> + blank_lines++;
> + }
>
> rchg[ixs++] = 0;
> rchg[ix++] = 1;
> @@ -525,6 +561,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo,
> long flags) {
> if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
> while (ixs > 0 &&
> !is_blank_line(recs, ix - 1, flags) &&
> +surrounding_leading_blank(recs, ix - 1, flags,
> nrec) > min_bl_neigh_indent &&
> recs_match(recs, ixs - 1, ix - 1, flags)) {
> rchg[--ixs] = 1;
> rchg[--ix] = 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] assertion in 2.8.4 triggering on old-ish worktree
On Thu, Jun 16, 2016 at 10:59 AM, Junio C Hamano wrote:
> Chris Packham writes:
>
>> On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham
>> wrote:
>>> Hi All,
>>>
>>> I have the git-sh-prompt configured in my .bashrc today I visited an
>>> old worktree that I haven't really touched in a few years (sorry can't
>>> remember the git version I was using back then). I received the
>>> following output when changing to the directory
>>>
>>> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len
>>> <= item->len && item->prefix <= item->len' failed.
>>>
>>> I assume it's one of the git invocations in git-sh-prompt that's
>>> hitting the assertion. Any thoughts on what might be triggering it?
>>> Any debug I can gather?
>>
>> A bit more info. The directory in question is a uninitialised
>> submodule. It doesn't trigger in the root of the parent project.
>
>
> Sounds like
> http://article.gmane.org/gmane.comp.version-control.git/283549
>
I looked into this. In pathspec.c#prefix_pathspec (the function
that has this assertion at the end), the assertion can only
trigger if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE
or PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP was given
as these are the only places that reduce item->len.
I converted the example test case to follow our test syntax
(module tab/whitespace issues):
test_expect_success 'remove submodule and add its files' '
mkdir test &&
(
cd test &&
git init sub &&
(
cd sub &&
touch foo &&
git add foo &&
git commit -m "foo"
) &&
git init &&
git add sub &&
rm -rf sub/.git &&
(
cd sub &&
git add .
)
)
'
And the issue here is that the submodule $GIT_DIR
was removed, so that the "git add ." was called with
the parents $GIT_DIR, and a prefix of sub/
(the slash will be removed in PATHSPEC_STRIP_SUBMODULE_SLASH...)
such that the length will be 3 ("sub") and the other
(prefix, nowildcard_len) are still 4.
One fix would be to adjust prefix and nowildcard_len
as well (in case they were as long as len, and now are
overlength, if they were shorter, no need to cut off one)
However I think that is missleading.
So let's step back a bit and think about what should happen
in the test case above. I think the users intent may be
"remove the submodule and add the files
directly to the regular tree".
However this would not happen in case we do the quickfix
of cutting one off prefix and nowildcard_len, because
we have similar thing as a D/F (directory/file) conflict,
just that it is a TREE/SUBMODULE conflict.
In the parent project there is still a submodule recorded for
sub/ but the user wants it to be a tree, so we would have
to rewrite that.
Using "rm -rf sub/.git" is a bad UI for saying " I want this to be
a native tree/blobs instead of a submodule", so I would expect
that we need to have another command there eventually
(git submodule convert-to-subtree ?)
Regarding the assert:
We are sure it's a submodule related thing, so we can
have a quite narrow warning there, roughly:
diff --git a/pathspec.c b/pathspec.c
index c9e9b6c..d0ea87a 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
/* sanity checks, pathspec matchers assume these are sane */
- assert(item->nowildcard_len <= item->len &&
- item->prefix <= item->len);
+ if (item->nowildcard_len <= item->len &&
+ item->prefix <= item->len)
+ die (_("Path leads inside submodule '%s', but the submodule "
+ "was not recognized, i.e. not initialized or deleted"),
+ ce->name);
return magic;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hey Eric,
On Fri, Jun 17, 2016 at 12:46 AM, Eric Sunshine wrote:
> On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva wrote:
>> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine
>> wrote:
>>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva
>>> wrote:
Reimplement `is_expected_rev` & `check_expected_revs` shell function in
C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
call it from git-bisect.sh .
[...]
Signed-off-by: Pranit Bauva
---
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
@@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
+static int is_expected_rev(const char *expected_hex)
+{
+ struct strbuf actual_hex = STRBUF_INIT;
+ int res;
+
+ if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(),
0) < 0) {
+ strbuf_release(&actual_hex);
+ return 0;
+ }
+
+ strbuf_trim(&actual_hex);
+ res = !strcmp(actual_hex.buf, expected_hex);
+ strbuf_release(&actual_hex);
+ return res;
+}
>>>
>>> Not worth a re-roll, but this could be re-structured to avoid having
>>> to remember to release the strbuf at all exits:
>>>
>>> struct strbuf actual_hex = ...;
>>> int res = 0;
>>>
>>> if (strbuf_read_file(...) >= 0) {
>>> strbuf_trim(...);
>>> res = !strcmp(...);
>>> }
>>> strbuf_release(...);
>>> return res;
>>>
>>> Alternately:
>>>
>>> if (strbuf_read_file(...) < 0)
>>> goto done;
>>>
>>> strbuf_trim(...);
>>> res = !strcmp(...);
>>>
>>> done:
>>> strbuf_release(...);
>>> return res;
>>>
>>> which is a bit less compact.
>>
>> I will avoid this for the reason that I will have to create a label
>> for a lot of functions. If I choose to do this for one function, I
>> think it would be more appropriate to do the same for other functions.
>> There would be a lot of functions in future which would be in the same
>> scenario and creating a separate label for each of them would be quite
>> tedious. What do you think?
>
> Not sure what you're talking about. Label names are not shared across
> functions. Anyhow, the first suggestion I presented above is more
> concise than the 'goto' version.
Yes I am aware of the fact that labels aren't shared across functions.
What I meant by "separate label" was that I will have to make a label
"fail" in each function. But I recently noticed that its used quite a
lot so I think it would be okay to use it. Will re-roll with using
labels and goto.
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
On Thu, Jun 16, 2016 at 3:05 PM, Pranit Bauva wrote:
> On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine
> wrote:
>> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva
>> wrote:
>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
>>> call it from git-bisect.sh .
>>> [...]
>>> Signed-off-by: Pranit Bauva
>>> ---
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>>> +static int is_expected_rev(const char *expected_hex)
>>> +{
>>> + struct strbuf actual_hex = STRBUF_INIT;
>>> + int res;
>>> +
>>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(),
>>> 0) < 0) {
>>> + strbuf_release(&actual_hex);
>>> + return 0;
>>> + }
>>> +
>>> + strbuf_trim(&actual_hex);
>>> + res = !strcmp(actual_hex.buf, expected_hex);
>>> + strbuf_release(&actual_hex);
>>> + return res;
>>> +}
>>
>> Not worth a re-roll, but this could be re-structured to avoid having
>> to remember to release the strbuf at all exits:
>>
>> struct strbuf actual_hex = ...;
>> int res = 0;
>>
>> if (strbuf_read_file(...) >= 0) {
>> strbuf_trim(...);
>> res = !strcmp(...);
>> }
>> strbuf_release(...);
>> return res;
>>
>> Alternately:
>>
>> if (strbuf_read_file(...) < 0)
>> goto done;
>>
>> strbuf_trim(...);
>> res = !strcmp(...);
>>
>> done:
>> strbuf_release(...);
>> return res;
>>
>> which is a bit less compact.
>
> I will avoid this for the reason that I will have to create a label
> for a lot of functions. If I choose to do this for one function, I
> think it would be more appropriate to do the same for other functions.
> There would be a lot of functions in future which would be in the same
> scenario and creating a separate label for each of them would be quite
> tedious. What do you think?
Not sure what you're talking about. Label names are not shared across
functions. Anyhow, the first suggestion I presented above is more
concise than the 'goto' version.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] bisect--helper: `bisect_reset` shell function in C
Hey Eric,
On Thu, Jun 16, 2016 at 2:35 AM, Eric Sunshine wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva wrote:
>> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh .
>> [...]
>> Signed-off-by: Pranit Bauva
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -123,12 +127,48 @@ static int bisect_clean_state(void)
>> +static int bisect_reset(const char *commit)
>> +{
>> + struct strbuf branch = STRBUF_INIT;
>> +
>> + if (!commit) {
>> + if (strbuf_read_file(&branch, git_path_bisect_start(), 0) <
>> 1) {
>> + printf("We are not bisecting.\n");
>> + return 0;
>> + }
>> + strbuf_rtrim(&branch);
>> +
>
> Style: unnecessary blank line
Will fix.
>> + } else {
>> + struct object_id oid;
>> + if (get_oid(commit, &oid))
>> + return error(_("'%s' is not a valid commit"),
>> commit);
>> + strbuf_addf(&branch, "%s", commit);
>
> strbuf_addstr(&branch, commit);
Seems better. :)
>> + }
>> +
>> + if (!file_exists(git_path_bisect_head())) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
>> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> + error(_("Could not check out original HEAD '%s'. Try"
>> + "'git bisect reset '."),
>> branch.buf);
>> + strbuf_release(&branch);
>> + argv_array_clear(&argv);
>> + return -1;
>> + }
>> + argv_array_clear(&argv);
>> + }
>> +
>> + strbuf_release(&branch);
>> + return bisect_clean_state();
>> +}
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hey Eric,
On Thu, Jun 16, 2016 at 2:44 AM, Eric Sunshine wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva wrote:
>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
>> call it from git-bisect.sh .
>> [...]
>> Signed-off-by: Pranit Bauva
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -162,13 +162,44 @@ static int bisect_reset(const char *commit)
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> + struct strbuf actual_hex = STRBUF_INIT;
>> + int res;
>> +
>> + if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0)
>> < 0) {
>> + strbuf_release(&actual_hex);
>> + return 0;
>> + }
>> +
>> + strbuf_trim(&actual_hex);
>> + res = !strcmp(actual_hex.buf, expected_hex);
>> + strbuf_release(&actual_hex);
>> + return res;
>> +}
>
> Not worth a re-roll, but this could be re-structured to avoid having
> to remember to release the strbuf at all exits:
>
> struct strbuf actual_hex = ...;
> int res = 0;
>
> if (strbuf_read_file(...) >= 0) {
> strbuf_trim(...);
> res = !strcmp(...);
> }
> strbuf_release(...);
> return res;
>
> Alternately:
>
> if (strbuf_read_file(...) < 0)
> goto done;
>
> strbuf_trim(...);
> res = !strcmp(...);
>
> done:
> strbuf_release(...);
> return res;
>
> which is a bit less compact.
I will avoid this for the reason that I will have to create a label
for a lot of functions. If I choose to do this for one function, I
think it would be more appropriate to do the same for other functions.
There would be a lot of functions in future which would be in the same
scenario and creating a separate label for each of them would be quite
tedious. What do you think?
>> +static int check_expected_revs(const char **revs, int rev_nr)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < rev_nr; i++) {
>> + if (!is_expected_rev(revs[i])) {
>> + remove_path(git_path_bisect_ancestors_ok());
>> + remove_path(git_path_bisect_expected_rev());
>> + return 0;
>> + }
>> + }
>> + return 0;
>> +}
>
> Hmm, all execution paths return 0, so it feels a bit pointless to have
> this function return a value at all.
>
> You could also use a 'break' inside the loop rather than 'return'
> since the return value is the same inside or outside the loop and
> nothing else happens after the loop.
Sure!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C
Hey Eric,
On Fri, Jun 17, 2016 at 12:25 AM, Eric Sunshine wrote:
> On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva wrote:
>> Reimplement the `bisect_write` shell function in C and add a
>> `bisect-write` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh
>>
>> Using `--bisect-write` subcommand is a temporary measure to port shell
>> function in C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other methods.
>>
>> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
>> from the global shell script thus we need to pass it to the subcommand
>> using the arguments. After the whole conversion, we can remove the extra
>> arguments and make the method use the two variables from the global scope
>> within the C code.
>
> You could do this now rather than waiting for later. Instead of
> passing these arguments to bisect_write(), create global variables in
> this patch and assign them in the BISECT_WRITE case of
> cmd_bisect__helper() before calling bisect_write().
>
> Not necessarily worth a re-roll, but would save you the effort of
> having to explain it here and then change it in some later patch.
I have actually done it in my next conversion which is converting
check_and_set_terms()[1] which also sets those variables to some value
so its more appropriate there.
>> Signed-off-by: Pranit Bauva
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int
>> rev_nr)
>> +static int bisect_write(const char *state, const char *rev,
>> + const char *term_good, const char *term_bad,
>> + int nolog)
>> +{
>> + struct strbuf tag = STRBUF_INIT;
>> + struct strbuf commit_name = STRBUF_INIT;
>> + struct object_id oid;
>> + struct commit *commit;
>> + struct pretty_print_context pp = {0};
>> + FILE *fp;
>> +
>> + if (!strcmp(state, term_bad))
>> + strbuf_addf(&tag, "refs/bisect/%s", state);
>> + else if(one_of(state, term_good, "skip", NULL))
>> + strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
>> + else
>> + return error(_("Bad bisect_write argument: %s"), state);
>> +
>> + if (get_oid(rev, &oid)) {
>> + strbuf_release(&tag);
>> + return error(_("couldn't get the oid of the rev '%s'"), rev);
>> + }
>
> Minor: If you move the get_oid() conditional before the one above it,
> then you won't have to worry about releasing 'strbuf tag' at this
> point.
Nice. I will do that. :)
>> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
>> + UPDATE_REFS_MSG_ON_ERR)) {
>> + strbuf_release(&tag);
>> + return -1;
>> + }
>
> If you release 'strbuf tag' right here, after it's final use, then you
> won't have to worry about releasing it anywhere below (particularly in
> the error cases).
True.
>> + fp = fopen(git_path_bisect_log(), "a");
>> + if (!fp) {
>> + strbuf_release(&tag);
>> + return error_errno(_("couldn't open the file '%s'"),
>> git_path_bisect_log());
>> + }
>> +
>> + commit = lookup_commit_reference(oid.hash);
>> + format_commit_message(commit, "%s", &commit_name, &pp);
>> + fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
>> + commit_name.buf);
>> +
>> + if (!nolog)
>> + fprintf(fp, "git bisect %s %s\n", state, rev);
>> +
>> + strbuf_release(&commit_name);
>> + strbuf_release(&tag);
>> + fclose(fp);
>> + return 0;
>> +}
>> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv,
>> const char *prefix)
>> + case BISECT_WRITE:
>> + if (argc != 4 && argc != 5)
>> + die(_("--bisect-write requires either 4 or 5
>> arguments"));
>> + nolog = (argc == 5) && !strcmp(argv[4], "nolog");
>
> This is minor and won't matter in the long run when this code goes
> away later in the C conversion, but this differs from the shell code
> which only cared that a (non-empty) fifth argument was provided but
> didn't care about the actual value, whereas this code expects the
> argument to be exactly "nolog".
We currently have tight control over the arguments we are passing as
they are only programmer defined.
>> + return bisect_write(argv[0], argv[1], argv[2], argv[3],
>> nolog);
>> default:
>> die("BUG: unknown subcommand '%d'", cmdmode);
>> }
[1]: https://github.com/pranitbauva1997/git/pull/16
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gnome-keyring: Don't hard-code pkg-config executable
On Thu, Jun 16, 2016 at 2:50 AM, Jeff King wrote: > On Tue, Jun 14, 2016 at 01:27:05PM +0200, Heiko Becker wrote: > >> Helpful if your pkg-config executable has a prefix based on the >> architecture, for example. >> >> Signed-off-by: Heiko Becker > > Sounds like a reasonable thing to want to do... ditto. > ...and the implementation looks obviously correct. ditto. > Thanks. ditto. See I'm still alive, really! -Brandon -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 6/6] bisect--helper: `bisect_write` shell function in C
On Wed, Jun 15, 2016 at 10:00 AM, Pranit Bauva wrote:
> Reimplement the `bisect_write` shell function in C and add a
> `bisect-write` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
>
> Using `--bisect-write` subcommand is a temporary measure to port shell
> function in C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
> from the global shell script thus we need to pass it to the subcommand
> using the arguments. After the whole conversion, we can remove the extra
> arguments and make the method use the two variables from the global scope
> within the C code.
You could do this now rather than waiting for later. Instead of
passing these arguments to bisect_write(), create global variables in
this patch and assign them in the BISECT_WRITE case of
cmd_bisect__helper() before calling bisect_write().
Not necessarily worth a re-roll, but would save you the effort of
having to explain it here and then change it in some later patch.
> Signed-off-by: Pranit Bauva
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -192,6 +193,55 @@ static int check_expected_revs(const char **revs, int
> rev_nr)
> +static int bisect_write(const char *state, const char *rev,
> + const char *term_good, const char *term_bad,
> + int nolog)
> +{
> + struct strbuf tag = STRBUF_INIT;
> + struct strbuf commit_name = STRBUF_INIT;
> + struct object_id oid;
> + struct commit *commit;
> + struct pretty_print_context pp = {0};
> + FILE *fp;
> +
> + if (!strcmp(state, term_bad))
> + strbuf_addf(&tag, "refs/bisect/%s", state);
> + else if(one_of(state, term_good, "skip", NULL))
> + strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
> + else
> + return error(_("Bad bisect_write argument: %s"), state);
> +
> + if (get_oid(rev, &oid)) {
> + strbuf_release(&tag);
> + return error(_("couldn't get the oid of the rev '%s'"), rev);
> + }
Minor: If you move the get_oid() conditional before the one above it,
then you won't have to worry about releasing 'strbuf tag' at this
point.
> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
> + UPDATE_REFS_MSG_ON_ERR)) {
> + strbuf_release(&tag);
> + return -1;
> + }
If you release 'strbuf tag' right here, after it's final use, then you
won't have to worry about releasing it anywhere below (particularly in
the error cases).
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + strbuf_release(&tag);
> + return error_errno(_("couldn't open the file '%s'"),
> git_path_bisect_log());
> + }
> +
> + commit = lookup_commit_reference(oid.hash);
> + format_commit_message(commit, "%s", &commit_name, &pp);
> + fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
> + commit_name.buf);
> +
> + if (!nolog)
> + fprintf(fp, "git bisect %s %s\n", state, rev);
> +
> + strbuf_release(&commit_name);
> + strbuf_release(&tag);
> + fclose(fp);
> + return 0;
> +}
> @@ -241,6 +295,11 @@ int cmd_bisect__helper(int argc, const char **argv,
> const char *prefix)
> + case BISECT_WRITE:
> + if (argc != 4 && argc != 5)
> + die(_("--bisect-write requires either 4 or 5
> arguments"));
> + nolog = (argc == 5) && !strcmp(argv[4], "nolog");
This is minor and won't matter in the long run when this code goes
away later in the C conversion, but this differs from the shell code
which only cared that a (non-empty) fifth argument was provided but
didn't care about the actual value, whereas this code expects the
argument to be exactly "nolog".
> + return bisect_write(argv[0], argv[1], argv[2], argv[3],
> nolog);
> default:
> die("BUG: unknown subcommand '%d'", cmdmode);
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: final git bisect step leads to: "fatal: you want to use way too much memory"
Jeff King writes: > Interesting. But `git show` on the commit in question (f216419e5) does > not have any problems. It looks like bisect's internal "show the commit" > code does not properly call setup_revisions() to finalize the "struct > rev_info". That leaves the expand_tabs_in_log flag as "-1", which then > ends up cast to an unsigned of 2^64 when we use it in a size > computation. Yuck > And who knows what other bugs have been lurking there over the years; > there are other flags that should be finalized by setup_revision(), too. > > This patch should fix it. Looks sensible. > diff --git a/bisect.c b/bisect.c > index 6d93edb..dc13319 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct > commit *commit) > if (!opt.diffopt.output_format) > opt.diffopt.output_format = DIFF_FORMAT_RAW; > > + setup_revisions(0, NULL, &opt, NULL); > log_tree_commit(&opt, commit); > } > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] Log pretty format alignment improvements
Nguyễn Thái Ngọc Duy writes: > The first patch was from a long time ago. The concern was it may be > breaking existing user expectation [1]. I still maintain that it's a good > thing to do and should not break anything. Hence the resubmission. I do not think "it's a good feature to have" was a question from the beginning. Thread [1] stopped with me saying "as long as >(N) can be used as Duy claims as a workaround to get the original behaviour, it is good to allow using >|(N) for this new output format; I didn't check if >(N) does behave that way, though". What was necessary to resurrect the patch was "Yes, >(N) can be used that way and here is a test" or something like that. > The second patch adds negative column specifier to >|() and friends. > A positive number 'n' specifies the n-th column from the left border > of the screen, '-n' specifies the n-th column from the _right_ border. That is quite nice enhancement. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] gpg-interface cleanups
Jeff King writes: > This started off with Michael's patch to sign_buffer, which is at the > tip, and then me trying to address the possible deadlocks there and in > verify_signed_buffer. While I was in the area, I took the opportunity to > do a few cleanups. > > It's unclear to me whether the deadlocks are possible in practice; see > patch 5 for discussion. I do recall thinking about the verification side and coming up with the same conclusion as yours when we queued that code (i.e. they need to read the whole thing before checking). > My guess is probably not, but the amount of code > to support doing it right is not all that much. But if we don't like it, > we can drop 4-6. Let's keep all of them; they all looked reasonable. > Patch 7 is still authored by Michael, but has been massaged greatly by > me. I'll comment more directly on the changes there. > > [1/7]: gpg-interface: use child_process.args > [2/7]: verify_signed_buffer: drop pbuf variable > [3/7]: verify_signed_buffer: use tempfile object > [4/7]: run-command: add pipe_command helper > [5/7]: verify_signed_buffer: use pipe_command > [6/7]: sign_buffer: use pipe_command > [7/7]: gpg-interface: check gpg signature creation status Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
Duy Nguyen writes: >> You don't think the revert is correct or you don't think the revert is >> sufficient? (I wasn't able to find a test case which proved that the >> change to line 399 was necessary, so perhaps I don't understand.) > > OK insufficient. > >> I would have thought that grepping the empty SHA-1 would be correct for >> with or without -v. An "intent to add" file has no content in the index >> so I would expect it to have zero matching and zero non-matching lines >> for any grep --cached query? >> >> Or is this an efficiency and not a correctness concern? > > "git grep --cached" searches file content that will be committed by > "git commit" (no -a). An i-t-a entry will not be committed (you would > need "git add" first, or do "git commit -a"). So if I say "search > among the to-be-committed file content, list files that do not match > abc" (git grep -l -v --cached abc), the i-t-a entry will show up > because its fake content is empty (i.e. not contain "abc"), even > though it's not in the "to-be-committed" list. So yeah, correctness > issue. OK, sounds like a good start for a proper log message for the fix. Thanks for hashing it all out before I got to the end of the thread ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
Charles Bailey writes: > http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358 > > http://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002 > > Unless I've misunderstood the conversation and commit message, the > referenced commit was supposed to be a "code as a comment" commit with > no change in observable behavior Thanks for a pointer; 276358 claims that ce->ce_mode would be zero for path added with "git add -N path", but I do not think it is correct. The updated behaviour is more understandable. With "add -N", the user said "Just keep an eye on this path, I cannot decide what the contents for this path in the index should be at this moment". grep_cache() that checks the contents in the index cannot say what is in the index, because the contents is not yet there. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] assertion in 2.8.4 triggering on old-ish worktree
Chris Packham writes: > On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham > wrote: >> Hi All, >> >> I have the git-sh-prompt configured in my .bashrc today I visited an >> old worktree that I haven't really touched in a few years (sorry can't >> remember the git version I was using back then). I received the >> following output when changing to the directory >> >> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len >> <= item->len && item->prefix <= item->len' failed. >> >> I assume it's one of the git invocations in git-sh-prompt that's >> hitting the assertion. Any thoughts on what might be triggering it? >> Any debug I can gather? > > A bit more info. The directory in question is a uninitialised > submodule. It doesn't trigger in the root of the parent project. Sounds like http://article.gmane.org/gmane.comp.version-control.git/283549 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] friendlier handling of overflows in archive-tar
Both patches looked good to me. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] diff compaction heuristic: favor shortest neighboring blank lines
The compaction heuristic for diffs was deemed quite good, but in 5580b27
we have an example demonstrates a common case, that fails with the existing
heuristic. That is why we disabled the heuristic in the v2.9.0 release.
With this patch a diff looks like:
def bar
do_bar_stuff()
common_ending()
end
+def bal
+do_bal_stuff()
+
+common_ending()
+end
+
def baz
do_baz_stuff()
common_ending()
end
whereas before we had:
WIP (TODO: ask peff to provide an example that actually triggers, I seem to be
unable to write one, though I thought the above was one)
The way we do it, is by inspecting the neighboring lines and see how
much indent there is and we choose the line that has the shortest amount
of blanks in the neighboring lines.
(TODO: update comments in the file)
Signed-off-by: Stefan Beller
---
Hi Jeff, hi Michael,
thanks for pointing out the flaw in this ruby code base before the 2.9 release.
I think this patch would fix your finding, though I cannot reproduce it.
Can you provide an example repo/patch that looks ugly on current origin/master,
so I can be sure this fixes the issue?
This can also be a start of a discussion if this is a too short-sighted
enhancement
of the heuristic. Essentially we'd want to prefer
+end
+
+def bal
over:
+do_bal_stuff()
+
+common_ending()
because there is less space between line start and {end, def bal}
than for {do_bal_stuff, common_ending}.
Thanks,
Stefan
xdiff/xdiffi.c | 41 +++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index b3c6848..58adc74 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -405,6 +405,35 @@ static int is_blank_line(xrecord_t **recs, long ix, long
flags)
return xdl_blankline(recs[ix]->ptr, recs[ix]->size, flags);
}
+static unsigned int leading_blank(const char *line)
+{
+ unsigned int ret = 0;
+ while (*line) {
+ if (*line == '\t')
+ ret += 8;
+ else if (*line == ' ')
+ ret ++;
+ else
+ break;
+ line++;
+ }
+ return ret;
+}
+
+static unsigned int surrounding_leading_blank(xrecord_t **recs, long ix,
+ long flags, long nrec)
+{
+ unsigned int i, ret = UINT_MAX;
+ if (ix > 0)
+ ret = leading_blank(recs[ix - 1]->ptr);
+ if (ix < nrec - 1) {
+ i = leading_blank(recs[ix + 1]->ptr);
+ if (i < ret)
+ ret = i;
+ }
+ return ret;
+}
+
static int recs_match(xrecord_t **recs, long ixs, long ix, long flags)
{
return (recs[ixs]->ha == recs[ix]->ha &&
@@ -416,7 +445,7 @@ static int recs_match(xrecord_t **recs, long ixs, long ix,
long flags)
int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long flags) {
long ix, ixo, ixs, ixref, grpsiz, nrec = xdf->nrec;
char *rchg = xdf->rchg, *rchgo = xdfo->rchg;
- unsigned int blank_lines;
+ unsigned int blank_lines, min_bl_neigh_indent;
xrecord_t **recs = xdf->recs;
/*
@@ -451,6 +480,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long
flags) {
do {
grpsiz = ix - ixs;
blank_lines = 0;
+ min_bl_neigh_indent = UINT_MAX;
/*
* If the line before the current change group, is
equal to
@@ -485,7 +515,13 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long
flags) {
* the group.
*/
while (ix < nrec && recs_match(recs, ixs, ix, flags)) {
- blank_lines += is_blank_line(recs, ix, flags);
+ if (is_blank_line(recs, ix, flags)) {
+ unsigned int bl_neigh_indent =
+ surrounding_leading_blank(recs,
ix, flags, nrec);
+ if (min_bl_neigh_indent >
bl_neigh_indent)
+ min_bl_neigh_indent =
min_bl_neigh_indent;
+ blank_lines++;
+ }
rchg[ixs++] = 0;
rchg[ix++] = 1;
@@ -525,6 +561,7 @@ int xdl_change_compact(xdfile_t *xdf, xdfile_t *xdfo, long
flags) {
if ((flags & XDF_COMPACTION_HEURISTIC) && blank_lines) {
while (ixs > 0 &&
!is_blank_line(recs, ix - 1, flags) &&
+ surrounding_leading_blank(recs, ix - 1, flags,
nrec) > min_bl_neigh_inden
Re: [PATCH] verify-tag: allow to verify signed blob objects
Michael J Gruber writes: > Currently, there is no easy way to verify push certificates. They have > the same structure as signed tags: "attached detached signatures", that > is: the concatenation of the signed material and its detached signature. > > Introduce a `--blob` option to verify-tag so that it allows to verify > tags and blobs. > > Signed-off-by: Michael J Gruber > --- > The first outcome of my long announced project to describe our signature > formats in Documentation/technical (progress underway) > > In fact, that whole area is in need of refactoring: gpg related bits are > all over the place, including tag.c. The proposed patch neither improves > nor worsens the situation in that respect. But, since we make it > unnecessarily hard to verify signatures (git cat-file | gpg --verify fails) > it's only fair to provide a tool for pre-receive hook writers. Another (orthogonal) thing to think about is "is it sensible to add a new feature to verify blob objects that has signature?" That is, if we are adding one new feature, isn't it more sensible for it to accept a stream of bytes from the standard input and run verify-signed-bufter on it? That way, if you already happen to have a blob, you can feed it "cat-file blob" output to get the new feature you added in this patch. But you cannot go in the other direction without incurring downside. If you start from "verify signature in blob" interface, and if all you have is a log of push certificates in a flat text file [1], you would need to do "hash-objects -w" first only to be able to use that interface (which in turn means that you would need write access to the object store of the repository. [Footnote] *1* push certificate is first written as a blob in the object store only so that we can safely run multiple receive-pack instances without them stepping on each others' toes; it is expected that they are collected by the server operator in chronological order and published in a human readable log form, so that outside people can verify honesty of the server operator. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: I lost my commit signature
Jeff King writes: > But why does somebody run "commit -S" for a single commit, but not all > the time? Is it because that commit is special? Or is that particular > moment special? One implies that it's important for the signature to be > retained during a rebase, and one does not. > > So I dunno. I would not be opposed to such a feature, but I'm having > trouble figuring out why it would be useful (though for the most part, I > do not see why anything but per-project commit.gpgSign config is > particularly useful. Maybe I just lack imagination). I am not so imaginative, either. One remotely plausible use case may be a project that has two classes of paths (let's call these classes sensitive and others), and requires its participants to sign commits that touch sensitive paths. The user needs something finter grained than per-project commit.gpgSign there. But even in such a case, the fact that an original commit is with a signature should not be a good indication that the rewritten version of that commit in the new history still touches the sensitive paths that required the original to be signed (i.e. the history the user is rebasing onto may already have the necessary changes to these paths). So, I dunno, either. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] assertion in 2.8.4 triggering on old-ish worktree
On Thu, Jun 16, 2016 at 9:55 AM, Dennis Kaarsemaker wrote: > On do, 2016-06-16 at 17:02 +1200, Chris Packham wrote: >> On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham > om> wrote: >> > >> > Hi All, >> > >> > I have the git-sh-prompt configured in my .bashrc today I visited >> > an >> > old worktree that I haven't really touched in a few years (sorry >> > can't >> > remember the git version I was using back then). I received the >> > following output when changing to the directory >> > >> > git: pathspec.c:317: prefix_pathspec: Assertion `item- >> > >nowildcard_len >> > <= item->len && item->prefix <= item->len' failed. >> > >> > I assume it's one of the git invocations in git-sh-prompt that's >> > hitting the assertion. Any thoughts on what might be triggering it? >> > Any debug I can gather? >> A bit more info. The directory in question is a uninitialised >> submodule. It doesn't trigger in the root of the parent project. > > That very much smells like a class of bugs we've seen before, with git > getting confused around submodules. See also for example > > https://www.mail-archive.com/git@vger.kernel.org/msg68447.html > > I don't think an accepted fix exists yet. Thanks for pointing me at that report. I was not aware of this "class of bugs", I'll see if I can fix it. Thanks, Stefan > > D. > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] assertion in 2.8.4 triggering on old-ish worktree
On do, 2016-06-16 at 17:02 +1200, Chris Packham wrote: > On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham om> wrote: > > > > Hi All, > > > > I have the git-sh-prompt configured in my .bashrc today I visited > > an > > old worktree that I haven't really touched in a few years (sorry > > can't > > remember the git version I was using back then). I received the > > following output when changing to the directory > > > > git: pathspec.c:317: prefix_pathspec: Assertion `item- > > >nowildcard_len > > <= item->len && item->prefix <= item->len' failed. > > > > I assume it's one of the git invocations in git-sh-prompt that's > > hitting the assertion. Any thoughts on what might be triggering it? > > Any debug I can gather? > A bit more info. The directory in question is a uninitialised > submodule. It doesn't trigger in the root of the parent project. That very much smells like a class of bugs we've seen before, with git getting confused around submodules. See also for example https://www.mail-archive.com/git@vger.kernel.org/msg68447.html I don't think an accepted fix exists yet. D. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] add--interactive: respect diff.compactionHeuristic
On Thu, Jun 16, 2016 at 5:27 AM, Jeff King wrote:
> On Wed, Jun 15, 2016 at 08:24:47AM +0200, Alex Prengère wrote:
>
>> I see, it makes sense ;-) Indeed it would seem logical to have all
>> commands showing diffs (diff, add -p, log -p, reset -p, etc..) respect
>> the diff options.
>
> Here's a patch to do so, similar to what we do for diff.algorithm.
>
> -- >8 --
> Subject: add--interactive: respect diff.compactionHeuristic
>
> We use plumbing to generate the diff, so it doesn't
> automatically pick up UI config like compactionHeuristic.
> Let's forward it on, since interactive adding is porcelain.
>
> Note that we only need to handle the "true" case. There's no
> point in passing --no-compaction-heuristic when the variable
> is false, since nothing else could have turned it on.
because we don't want to implement --[no-]compaction-heuristic
as a command line switch to git-add?
Fine with me.
Stepping back and looking how the compaction heuristic turned out,
I think this is what we did not want to see, i.e. the need to bring it in
every command, but rather enable and release it. But we backed off
of the default-on, and now people may ask for the --no-compaction-heuristic
in interactive add eventually, when they run into a corner case.
For now:
Reviewed-by: Stefan Beller
Thanks,
Stefan
>
> Signed-off-by: Jeff King
> ---
> git-add--interactive.perl | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 822f857..642cce1 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -45,6 +45,7 @@ my ($diff_new_color) =
> my $normal_color = $repo->get_color("", "reset");
>
> my $diff_algorithm = $repo->config('diff.algorithm');
> +my $diff_compaction_heuristic =
> $repo->config_bool('diff.compactionheuristic');
> my $diff_filter = $repo->config('interactive.difffilter');
>
> my $use_readkey = 0;
> @@ -749,6 +750,9 @@ sub parse_diff {
> if (defined $diff_algorithm) {
> splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
> }
> + if ($diff_compaction_heuristic) {
> + splice @diff_cmd, 1, 0, "--compaction-heuristic";
> + }
> if (defined $patch_mode_revision) {
> push @diff_cmd, get_diff_reference($patch_mode_revision);
> }
> --
> 2.9.0.160.g4984cba
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [bug] assertion in 2.8.4 triggering on old-ish worktree
On Wed, Jun 15, 2016 at 10:02 PM, Chris Packham wrote: > On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham > wrote: >> Hi All, >> >> I have the git-sh-prompt configured in my .bashrc today I visited an >> old worktree that I haven't really touched in a few years (sorry can't >> remember the git version I was using back then). I received the >> following output when changing to the directory >> >> git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len >> <= item->len && item->prefix <= item->len' failed. >> >> I assume it's one of the git invocations in git-sh-prompt that's >> hitting the assertion. Any thoughts on what might be triggering it? >> Any debug I can gather? The first step would be to identify which git command is actually causing it. Can you take your git-sh-prompt and run it step by step to figure out what command is failing? The next step after that would be to see if we can reproduce it in a reduced test case (with no real data) so we can add a regression test and a fix for it. > > A bit more info. The directory in question is a uninitialised > submodule. It doesn't trigger in the root of the parent project. so it's a cd submodule git that is failing? Thanks, Stefan > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: final git bisect step leads to: "fatal: you want to use way too much memory"
On Thu, Jun 16, 2016 at 03:29:52PM +0200, Markus Trippelsdorf wrote: > On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote: > > markus@x4 gcc % git bisect good > > f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit > > commit f216419e5c4c41df70dbe00a6ea1faea46484dc8 > > fatal: you want to use way too much memory > > markus@x4 gcc % > > The issue started with: > > commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023 > Author: Junio C Hamano > Date: Tue Mar 29 16:05:39 2016 -0700 > > pretty: allow tweaking tabwidth in --expand-tabs Interesting. But `git show` on the commit in question (f216419e5) does not have any problems. It looks like bisect's internal "show the commit" code does not properly call setup_revisions() to finalize the "struct rev_info". That leaves the expand_tabs_in_log flag as "-1", which then ends up cast to an unsigned of 2^64 when we use it in a size computation. And who knows what other bugs have been lurking there over the years; there are other flags that should be finalized by setup_revision(), too. This patch should fix it. diff --git a/bisect.c b/bisect.c index 6d93edb..dc13319 100644 --- a/bisect.c +++ b/bisect.c @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit) if (!opt.diffopt.output_format) opt.diffopt.output_format = DIFF_FORMAT_RAW; + setup_revisions(0, NULL, &opt, NULL); log_tree_commit(&opt, commit); } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: final git bisect step leads to: "fatal: you want to use way too much memory"
On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote: > markus@x4 gcc % git bisect good > f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit > commit f216419e5c4c41df70dbe00a6ea1faea46484dc8 > fatal: you want to use way too much memory > markus@x4 gcc % The issue started with: commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023 Author: Junio C Hamano Date: Tue Mar 29 16:05:39 2016 -0700 pretty: allow tweaking tabwidth in --expand-tabs -- Markus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] pretty.c: support |() forms
%>|(num), %><|(num) and %<|(num), where num is a positive number, sets a
fixed column from the screen's left border. There is no way for us to
specifiy a column relative to the right border, which is useful when you
want to make use of all terminal space (on big screens). Use negative
num for that. Inspired by Go's array syntax (*).
(*) I know Python has this first (or before Go, at least) but the idea
didn't occur to me until I learned Go.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
pretty.c | 8 +++-
t/t4205-log-pretty-formats.sh | 33 +
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/pretty.c b/pretty.c
index 4f33b09..1c67b23 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1022,9 +1022,15 @@ static size_t parse_padding_placeholder(struct strbuf
*sb,
int width;
if (!end || end == start)
return 0;
- width = strtoul(start, &next, 10);
+ width = strtol(start, &next, 10);
if (next == start || width == 0)
return 0;
+ if (width < 0) {
+ if (to_column)
+ width += term_columns();
+ if (width < 0)
+ return 0;
+ }
c->padding = to_column ? -width : width;
c->flush_type = flush_type;
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index d75cdbb..d9f6242 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -176,6 +176,17 @@ EOF
test_cmp expected actual
'
+test_expect_success 'left alignment formatting at the nth column' '
+ COLUMNS=50 git log --pretty="tformat:%h %<|(-10)%s" >actual &&
+ qz_to_tab_space actual &&
qz_to_tab_space actual &&
+ qz_to_tab_space actual &&
qz_to_tab_space actual &&
+ qz_to_tab_space actual &&
qz_to_tab_space
[PATCH 0/2] Log pretty format alignment improvements
The first patch was from a long time ago. The concern was it may be breaking existing user expectation [1]. I still maintain that it's a good thing to do and should not break anything. Hence the resubmission. The second patch adds negative column specifier to >|() and friends. A positive number 'n' specifies the n-th column from the left border of the screen, '-n' specifies the n-th column from the _right_ border. I have been happy with 2/2 improving my "git l1" (fancy --oneline) printout for many months. It's definitely a good thing to do in my opinion. [1] http://thread.gmane.org/gmane.comp.version-control.git/277710/focus=278326 Josef Kufner (1): pretty: pass graph width to pretty formatting for use in '%>|(N)' Nguyễn Thái Ngọc Duy (1): pretty.c: support |() forms commit.h | 1 + graph.c | 7 ++ graph.h | 5 log-tree.c| 2 ++ pretty.c | 9 ++- t/t4205-log-pretty-formats.sh | 57 +++ 6 files changed, 80 insertions(+), 1 deletion(-) -- 2.8.2.524.g6ff3d78 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] pretty: pass graph width to pretty formatting for use in '%>|(N)'
From: Josef Kufner
Pass graph width to pretty formatting, to make N in '%>|(N)'
include columns consumed by graph rendered when --graph option
is in use.
For example, in the output of
git log --all --graph --pretty='format: [%>|(20)%h] %ar%d'
this change will make all commit hashes align at 20th column from
the edge of the terminal, not from the edge of the graph.
Signed-off-by: Josef Kufner
Signed-off-by: Junio C Hamano
Signed-off-by: Nguyễn Thái Ngọc Duy
---
commit.h | 1 +
graph.c | 7 +++
graph.h | 5 +
log-tree.c| 2 ++
pretty.c | 1 +
t/t4205-log-pretty-formats.sh | 24
6 files changed, 40 insertions(+)
diff --git a/commit.h b/commit.h
index b06db4d..0c923f0 100644
--- a/commit.h
+++ b/commit.h
@@ -161,6 +161,7 @@ struct pretty_print_context {
* should not be counted on by callers.
*/
struct string_list in_body_headers;
+ int graph_width;
};
struct userformat_want {
diff --git a/graph.c b/graph.c
index 1350bdd..ad766fa 100644
--- a/graph.c
+++ b/graph.c
@@ -669,6 +669,13 @@ static void graph_output_padding_line(struct git_graph
*graph,
graph_pad_horizontally(graph, sb, graph->num_new_columns * 2);
}
+
+int graph_width(struct git_graph *graph)
+{
+ return graph->width;
+}
+
+
static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)
{
/*
diff --git a/graph.h b/graph.h
index 0be62bd..3f48c19 100644
--- a/graph.h
+++ b/graph.h
@@ -67,6 +67,11 @@ int graph_is_commit_finished(struct git_graph const *graph);
int graph_next_line(struct git_graph *graph, struct strbuf *sb);
+/*
+ * Return current width of the graph in on-screen characters.
+ */
+int graph_width(struct git_graph *graph);
+
/*
* graph_show_*: helper functions for printing to stdout
*/
diff --git a/log-tree.c b/log-tree.c
index 78a5381..8d39315 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -687,6 +687,8 @@ void show_log(struct rev_info *opt)
ctx.output_encoding = get_log_output_encoding();
if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
ctx.from_ident = &opt->from_ident;
+ if (opt->graph)
+ ctx.graph_width = graph_width(opt->graph);
pretty_print_commit(&ctx, commit, &msgbuf);
if (opt->add_signoff)
diff --git a/pretty.c b/pretty.c
index 87c4497..4f33b09 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1299,6 +1299,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /*
in UTF-8 */
if (!start)
start = sb->buf;
occupied = utf8_strnwidth(start, -1, 1);
+ occupied += c->pretty_ctx->graph_width;
padding = (-padding) - occupied;
}
while (1) {
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 7398605..d75cdbb 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -319,6 +319,19 @@ EOF
test_cmp expected actual
'
+# Note: Space between 'message' and 'two' should be in the same column
+# as in previous test.
+test_expect_success 'right alignment formatting at the nth column with
--graph. i18n.logOutputEncoding' '
+ git -c i18n.logOutputEncoding=$test_encoding log --graph
--pretty="tformat:%h %>|(40)%s" >actual &&
+ iconv -f utf-8 -t $test_encoding >expected actual &&
+ cat actual &&
cat
Re: final git bisect step leads to: "fatal: you want to use way too much memory"
On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote: > To reproduce the issue, just run: Forget to mention: markus@x4 ~ % git --version git version 2.9.0 git-2.8.4 is fine. -- Markus -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
final git bisect step leads to: "fatal: you want to use way too much memory"
To reproduce the issue, just run: markus@x4 ~ % git clone git://gcc.gnu.org/git/gcc.git markus@x4 gcc % git checkout -b gcc-6 origin/gcc-6-branch markus@x4 gcc % git bisect bad 23240454adf1 You need to start by "git bisect start" Do you want me to do it for you [Y/n]? y markus@x4 gcc % git bisect good 558525d0cf3 Bisecting: 45 revisions left to test after this (roughly 6 steps) [c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372* cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags to the newly built tree. markus@x4 gcc % git bisect bad Bisecting: 22 revisions left to test after this (roughly 5 steps) [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener markus@x4 gcc % git bisect bad Bisecting: 10 revisions left to test after this (roughly 4 steps) [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const" markus@x4 gcc % git bisect bad Bisecting: 5 revisions left to test after this (roughly 3 steps) [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/ markus@x4 gcc % git bisect bad Bisecting: 2 revisions left to test after this (roughly 1 step) [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path markus@x4 gcc % git bisect good Bisecting: 0 revisions left to test after this (roughly 1 step) [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle markus@x4 gcc % git bisect good f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit commit f216419e5c4c41df70dbe00a6ea1faea46484dc8 fatal: you want to use way too much memory markus@x4 gcc % -- Markus git bisect start # bad: [23240454adf160862453a3a850451a304867c29b] Backported from mainline2016-06-04 Jakub Jelinek git bisect bad 23240454adf160862453a3a850451a304867c29b # good: [558525d0cf372a48ae12b1dc27201dd47ab7878c] Daily bump. git bisect good 558525d0cf372a48ae12b1dc27201dd47ab7878c # bad: [c4727bc59ecad9e8879aed135624781fabae16b5] PR c++/71372* cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression is INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags. For ARRAY_REF and ARRAY_RANGE_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags to the newly built tree. git bisect bad c4727bc59ecad9e8879aed135624781fabae16b5 # bad: [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31 Richard Biener git bisect bad 6935372a7f032acccf7876e7f54ddf494e101e5e # bad: [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const" git bisect bad 909ed6a89604db5baa55b62ad253a9bcd5d89c03 # bad: [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/ git bisect bad f216419e5c4c41df70dbe00a6ea1faea46484dc8 # good: [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path git bisect good 745d4ecd59a3430f7c7b3bf33db1083d529a018b # good: [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26 Jerry DeLisle git bisect good a64301f7ac47b9d8f4f816c5e935cda4c92716b7
Re: What's cooking in git.git (Jun 2016, #04; Tue, 14)
On Wed, Jun 15, 2016 at 09:35:20AM +0200, Elia Pinto wrote: > > * ep/http-curl-trace (2016-05-24) 2 commits > > - imap-send.c: introduce the GIT_TRACE_CURL enviroment variable > > - http.c: implement the GIT_TRACE_CURL environment variable > > > > HTTP transport gained an option to produce more detailed debugging > > trace. > > > > Rerolled. Is everybody happy with this version? > > > The refs is there > http://git.661346.n2.nabble.com/PATCH-v7-0-2-Implement-the-GIT-TRACE-CURL-environment-variable-td7657079.html > > If kindly someone who has reviewed and helped me to do the patch > could give an ack (or a nack eventually). Thanks in advance I gave another look at what is queued in pu, and I think it is OK. I still find the output a little verbose, but I think we should ship it and see how it fares in practice while debugging. I don't think its exact format needs to be set in stone, so we can tweak it later if we choose. -Peff PS Please trim your quotes to just the relevant bits. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] add--interactive: respect diff.compactionHeuristic
On Wed, Jun 15, 2016 at 08:24:47AM +0200, Alex Prengère wrote:
> I see, it makes sense ;-) Indeed it would seem logical to have all
> commands showing diffs (diff, add -p, log -p, reset -p, etc..) respect
> the diff options.
Here's a patch to do so, similar to what we do for diff.algorithm.
-- >8 --
Subject: add--interactive: respect diff.compactionHeuristic
We use plumbing to generate the diff, so it doesn't
automatically pick up UI config like compactionHeuristic.
Let's forward it on, since interactive adding is porcelain.
Note that we only need to handle the "true" case. There's no
point in passing --no-compaction-heuristic when the variable
is false, since nothing else could have turned it on.
Signed-off-by: Jeff King
---
git-add--interactive.perl | 4
1 file changed, 4 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 822f857..642cce1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -45,6 +45,7 @@ my ($diff_new_color) =
my $normal_color = $repo->get_color("", "reset");
my $diff_algorithm = $repo->config('diff.algorithm');
+my $diff_compaction_heuristic = $repo->config_bool('diff.compactionheuristic');
my $diff_filter = $repo->config('interactive.difffilter');
my $use_readkey = 0;
@@ -749,6 +750,9 @@ sub parse_diff {
if (defined $diff_algorithm) {
splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
}
+ if ($diff_compaction_heuristic) {
+ splice @diff_cmd, 1, 0, "--compaction-heuristic";
+ }
if (defined $patch_mode_revision) {
push @diff_cmd, get_diff_reference($patch_mode_revision);
}
--
2.9.0.160.g4984cba
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: URL rewrite in local config unable to override global config rule
On Thu, Jun 16, 2016 at 03:44:17PM +0530, Saksham Saxena wrote: > Summary : Added a new rule to the local config of a git repo by > issuing ' git config --local url."https://".insteadOf git:// ', but it > wasn't observed by git as it kept using "git://". Hmm. This works in a simple example for me... > Details : I had set my global config to use "git://" instead of > "https://";, as I prefer working with SSH. ..even if I have an existing "url.git://.insteadOf=https://";. But I could believe that having other config confuses things. The url-rewriting is not "last one wins", but rather that we try all of them, and the longest match wins. Can you show us the output of "git config --list" on a repository that is having this problem, and then the command that you run and its output? > Recently, I began writing a > 'GitHub Wiki' of one of my GitHub projects, and, apparently, those > Wikis are normal git repositories, and can be cloned and edited > locally. However, the clone url available is served over HTTPS only, > and doesn't support any other protocol. You should be able to clone, fetch, or push wiki repositories using any of the normal protocols. So: g...@github.com:username/repo.wiki.git should work. Likewise, git:// will work if the repository is public, but you cannot push over it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Code review tool recommendations - replacement for crucible?
On Thu, 16 Jun 2016 11:55:56 +, Richard Ipsum wrote: ... > Have you considered Gerrit[1] already? > It would seem to handle the cases you're interested in. Possible, but only after a lot of user education. We don't currently rewrite commits for review comments, and neither can we get (all) people to create feature-oriented commits, so we really need to review multiple commits in one review, and assign reviewers to subtrees. We could do that by repacking the changes into new commits, but that would defeat the purpose. Also, we often do reviews after feature integration, and also do partial reviews long before a new project is integrated. In other words: Lots of impedance mismatch. Andreas -- "Totally trivial. Famous last words." From: Linus Torvalds Date: Fri, 22 Jan 2010 07:29:21 -0800 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 6:44 PM, Charles Bailey wrote: > On Thu, Jun 16, 2016 at 05:57:18PM +0700, Duy Nguyen wrote: >> >> "git grep --cached" searches file content that will be committed by >> "git commit" (no -a). An i-t-a entry will not be committed (you would >> need "git add" first, or do "git commit -a"). So if I say "search >> among the to-be-committed file content, list files that do not match >> abc" (git grep -l -v --cached abc), the i-t-a entry will show up >> because its fake content is empty (i.e. not contain "abc"), even >> though it's not in the "to-be-committed" list. So yeah, correctness >> issue. > > OK, I think there is an issue there but it's not with "-l -v --cached" > but rather with "-L --cached". If my understanding is correct, "-l -v" > means "has a line that doesn't match" whereas "-L" means "has no line > that matches". > > Does this sound correct? I'll try adding a new test. Yeah "-L --cached" should work the same, I think. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 05:57:18PM +0700, Duy Nguyen wrote: > > "git grep --cached" searches file content that will be committed by > "git commit" (no -a). An i-t-a entry will not be committed (you would > need "git add" first, or do "git commit -a"). So if I say "search > among the to-be-committed file content, list files that do not match > abc" (git grep -l -v --cached abc), the i-t-a entry will show up > because its fake content is empty (i.e. not contain "abc"), even > though it's not in the "to-be-committed" list. So yeah, correctness > issue. OK, I think there is an issue there but it's not with "-l -v --cached" but rather with "-L --cached". If my understanding is correct, "-l -v" means "has a line that doesn't match" whereas "-L" means "has no line that matches". Does this sound correct? I'll try adding a new test. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
Jeff King venit, vidit, dixit 16.06.2016 11:25: > On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote: > >> As for the flexibility: >> We do code specifically for gpg, which happens to work for gpg2 also. >> The patch doesn't add any gpg ui requirements that we don't require >> elsewhere already. >> More flexibility requires a completely pluggable approach - gpgsm >> already fails to meet the gpg command line ui. > > Does it? I haven't run it in production, but I did some tests with gpgsm > a few months ago and found it to be a drop-in replacement, at least with > respect to git. Though I don't think that matters one way or the other > for the current discussion; it _does_ do --status-fd. You are right! I solely trusted in the documentation rather than simply trying it out. gpgsm even supports the undocumented short options. Only the resulting header is different (plus the fact that I have to turn of crl checks for whatever reason). >> In any case, "status-fd" is *the* way to interface with gpg reliably >> just like plumbing commands are *the* way to interface with git reliably. > > Fair enough. I've generally found its exit code pretty reliable. It's > unclear to me if the problem you saw was because gpg was exiting 0 but > producing no signature, or if your debugging was masking its exit code. > > Either way, I do not mind using --status-fd; it seems like it should be > more robust in general. It's the tip commit in the series I'm about to > post. > >> As for the read locking: >> I'm sorry I have no idea about that area at all. I thought that I'm >> doing the same that we do for verify, but apparently not. My >> strbuf_read-fu is not that strong either (read: copy&paste). I trust >> your assessment completely, though. > > Yeah, you're right that the deadlock thing is also a possibility on the > verification side. I'm not sure whether it's possible to trigger in > practice or not. See the analysis in the series. With great admiration I've looked at your series which solves this problem at the root. Way above my head (the solution, not the root, fortunately). >> As for the original problem: >> That had a different cause, as we know now (rebase dropping signatures >> without hint). I still think we should check gpg status codes properly. > > Yep, I agree. > > -Peff > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
core.fscache on Windows trashes system
Hello List, We use git in our company and found a problem with the version 2.8.3 (x64 and x86). After installing git, with activated file system caching on Windows 8.1 Enterprise, on the first use of git there a git.exe processes started again and again. Multiple thousands instances are generated and do not stop until I renamed the git.exe and piece by piece the processes vanish. Renaming the executable to git.exe again, open git bash, same procedure. I tried to let it go over night and kept the pc running (the processes used nearly all RAM (8GB) and the system swapped all the time. Next day everything went fine. After a reboot, it happens again. Only workaround till now for us is to uninstall git and install again with “Enable Filesystem cache” disabled. Any other occurrences of this behavior? It does not happen on all pcs but we had 5 pcs until now with this strange behavior. We have Kaspersky Endpoint Security 10 running that quarantines git.exe from time to time, maybe this could cause issues. -- Ciao, Sven -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 4:47 PM, Charles Bailey wrote: > On Thu, Jun 16, 2016 at 02:47:09PM +0700, Duy Nguyen wrote: >> I don't think revert is right. It rather needs a re-fix like below. >> Basically we want grep_file() to run as normal, but grep_sha1() >> (i.e. git grep --cached) should ignore i-t-a entries, because empty >> SHA-1 is not the right content to grep. It does not matter in positive >> matching, sure, but it may in -v cache. > > You don't think the revert is correct or you don't think the revert is > sufficient? (I wasn't able to find a test case which proved that the > change to line 399 was necessary, so perhaps I don't understand.) OK insufficient. > I would have thought that grepping the empty SHA-1 would be correct for > with or without -v. An "intent to add" file has no content in the index > so I would expect it to have zero matching and zero non-matching lines > for any grep --cached query? > > Or is this an efficiency and not a correctness concern? "git grep --cached" searches file content that will be committed by "git commit" (no -a). An i-t-a entry will not be committed (you would need "git add" first, or do "git commit -a"). So if I say "search among the to-be-committed file content, list files that do not match abc" (git grep -l -v --cached abc), the i-t-a entry will show up because its fake content is empty (i.e. not contain "abc"), even though it's not in the "to-be-committed" list. So yeah, correctness issue. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Code review tool recommendations - replacement for crucible?
On Thu, Jun 16, 2016 at 11:41:08AM +0200, Andreas Krey wrote: > Hi all, > > I'm looking for pointers to review tools that work with git (obviously), > and can deal sensibly with bigger reviews. Things we need: > > - Ability to split (set of) commits into multiple reviews, > so parts of changes can be reviewed by the respective owners > (or assign different reviewers to different files/subtrees > in a review). > > - Tracking of files (or changes) already reviewed (due to the large numbers), > and of the handling of issues found so far. > > - Support incremental reviews, not just e.g. over the content of a > pull request (bitbucket) When review comments are processed people > want to be able to only review that change, and not to be forced > to find that change in the entire previous changeset without > the tool's support. > Hey, Have you considered Gerrit[1] already? It would seem to handle the cases you're interested in. [1]: https://gerrit-review.googlesource.com/Documentation/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
URL rewrite in local config unable to override global config rule
Hello everyone I came across an anomaly in git recently, and would like to share it here. Please suggest if this is a bug, or if my approach is wrong. Summary : Added a new rule to the local config of a git repo by issuing ' git config --local url."https://".insteadOf git:// ', but it wasn't observed by git as it kept using "git://". Details : I had set my global config to use "git://" instead of "https://";, as I prefer working with SSH. Recently, I began writing a 'GitHub Wiki' of one of my GitHub projects, and, apparently, those Wikis are normal git repositories, and can be cloned and edited locally. However, the clone url available is served over HTTPS only, and doesn't support any other protocol. Naturally, I had to change my config to change the behavior of git while connecting to the remote. I thus proceeded to update the local config of the repo to use "https://"; instead of "git://". However, my remote urls weren't updated. I manually then removed these remote urls and added new HTTPS ones. Yet, the remote urls showed "git://" protocol in them. Screenshot of the terminal window can be found here (http://i.stack.imgur.com/y7GMP.png). I tried to add the "pushInsteadOf" rule as well, but again, it didn't take effect. This is particularly disappointing, because the local config have the highest priority, and yet it isn't effective at all. Because of the limitation of my remote, I cannot even push my changes. True, that I can dig up some dirty workarounds, but I do not understand why this behavior is seen. I have also asked this on the popular Stack Overflow forum (http://stackoverflow.com/questions/37844460/git-remote-url-is-not-updating). If any of the moderators here do not find this issue as a bug, then they may help me out on the forum itself (if you happen to be on it) to reduce the noise on this thread. I hope I described my problem clearly. Awaiting response. Regards Saksham Saxena -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Un-paged commit messages in git filter-branch's commit-filter?
On Mon, Jun 13, 2016 at 08:28:18AM +0200, Stefan Tauner wrote: > I am trying to do a major cleanup of the repository in one of my > projects (and switch from git-svn to native git). I have developed a > commit-filter script over the last months that massages partially > dreadful commit messages into something acceptable. While I am not 100% > sure I think that upgrading git has broken it partially. AFAICT since > the update the commit-filter does not get the original message anymore > but at least the subject/first paragraph is run through a pager or > something similar: > The first line is broken into multiple lines (i.e. some line breaks are > inserted about every 72 characters where none have been before). There are some output formats that will wrap lines, but by default, filter-branch should not be using them (and I could not reproduce the issue in a simple test). Can you show us what your commit-filter looks like? -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gnome-keyring: Don't hard-code pkg-config executable
On Tue, Jun 14, 2016 at 01:27:05PM +0200, Heiko Becker wrote: > Helpful if your pkg-config executable has a prefix based on the > architecture, for example. > > Signed-off-by: Heiko Becker Sounds like a reasonable thing to want to do... > diff --git a/contrib/credential/gnome-keyring/Makefile > b/contrib/credential/gnome-keyring/Makefile > index c3c7c98..22c19df 100644 > --- a/contrib/credential/gnome-keyring/Makefile > +++ b/contrib/credential/gnome-keyring/Makefile > @@ -4,12 +4,13 @@ all:: $(MAIN) > CC = gcc > RM = rm -f > CFLAGS = -g -O2 -Wall > +PKG_CONFIG = pkg-config > > -include ../../../config.mak.autogen > -include ../../../config.mak > > -INCS:=$(shell pkg-config --cflags gnome-keyring-1 glib-2.0) > -LIBS:=$(shell pkg-config --libs gnome-keyring-1 glib-2.0) > +INCS:=$(shell $(PKG_CONFIG) --cflags gnome-keyring-1 glib-2.0) > +LIBS:=$(shell $(PKG_CONFIG) --libs gnome-keyring-1 glib-2.0) ...and the implementation looks obviously correct. Thanks. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 02:47:09PM +0700, Duy Nguyen wrote: > I don't think revert is right. It rather needs a re-fix like below. > Basically we want grep_file() to run as normal, but grep_sha1() > (i.e. git grep --cached) should ignore i-t-a entries, because empty > SHA-1 is not the right content to grep. It does not matter in positive > matching, sure, but it may in -v cache. You don't think the revert is correct or you don't think the revert is sufficient? (I wasn't able to find a test case which proved that the change to line 399 was necessary, so perhaps I don't understand.) I would have thought that grepping the empty SHA-1 would be correct for with or without -v. An "intent to add" file has no content in the index so I would expect it to have zero matching and zero non-matching lines for any grep --cached query? Or is this an efficiency and not a correctness concern? Charles. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug: compactionheuristic config var case issue
On Wed, Jun 15, 2016 at 07:33:31PM +0700, Duy Nguyen wrote: > On Wed, Jun 15, 2016 at 5:39 PM, Brian Lalor wrote: > > I’m very happy to see the new compaction heuristic option; it’s the way I > > always thought diffs should read! > > > > The config option in the documentation references > > “diff.compactionHeuristic”, but diff.c does a case-sensitive > > comparison on “diff.compactionheuristic” (note the case of the “h” > > in “heuristic”) > > I think this misled you. All configuration variable names are > lower-cased before they reach that strcmp() call, the whole picture is > more like strcmp(tolower(var), "diff.compactionheuristic"), which I > believe is correct. Yep, that is correct. Config keys are case-insensitive (except for the middle portion "Y" of a key like "X.Y.Z"), and the downcasing happens before they even hit the config callbacks. > > and `git diff` does not honor the config. Confusingly, `git config > > diff.compactionheuristic` returns true when diff.compactionHeuristic > > is set in ~/.gitconfig. When diff.compactionheuristic is set to > > true in ~/.gitconfig, the desired behavior is achieved. Brian, do you have a case you can share where it is not working as expected? Config isn't enabled automatically for plumbing commands like diff-tree. So some commands may not respect it, but "git diff" definitely should. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] gpg-interface: check gpg signature creation status
When we create a signature, it may happen that gpg returns with
"success" but not with an actual detached signature on stdout.
Check for the correct signature creation status to catch these cases
better. Really, --status-fd parsing is the only way to check gpg status
reliably. We do the same for verify already.
Signed-off-by: Michael J Gruber
Signed-off-by: Jeff King
---
Obviously the big change from your v3 is that this just passes the
buffer to pipe_command() instead of doing the read itself.
I changed the name of your "err" strbuf to "gpg_status" to be more
descriptive, and to match the similar usage on the verification side.
In the test, I bumped the config-setting inside the test, and used
test_config to get the automatic cleanup (using "git -c" would also have
worked).
I left the test description as-is, though it is a bit misleading. We are
not really checking for bad input anymore, but rather "did gpg send us
the special stats". Your fake "echo" fails on both counts, so the test
is still effective. A more robust test would be to output something that
_looked_ like real gpg output, but not to output the correct status
token.
gpg-interface.c | 8 ++--
t/t7004-tag.sh | 9 -
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 74f08a2..08356f9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,9 +153,11 @@ int sign_buffer(struct strbuf *buffer, struct strbuf
*signature, const char *sig
struct child_process gpg = CHILD_PROCESS_INIT;
int ret;
size_t i, j, bottom;
+ struct strbuf gpg_status = STRBUF_INIT;
argv_array_pushl(&gpg.args,
gpg_program,
+"--status-fd=2",
"-bsau", signing_key,
NULL);
@@ -167,10 +169,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf
*signature, const char *sig
*/
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(&gpg, buffer->buf, buffer->len,
- signature, 1024, NULL, 0);
+ signature, 1024, &gpg_status, 0);
sigchain_pop(SIGPIPE);
- if (ret || signature->len == bottom)
+ ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
+ strbuf_release(&gpg_status);
+ if (ret)
return error(_("gpg failed to sign the data"));
/* Strip CR from the line endings, in case we are on Windows. */
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f9b7d79..8b0f71a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,10 +1202,17 @@ test_expect_success GPG,RFC1991 \
# try to sign with bad user.signingkey
git config user.signingkey BobTheMouse
test_expect_success GPG \
- 'git tag -s fails if gpg is misconfigured' \
+ 'git tag -s fails if gpg is misconfigured (bad key)' \
'test_must_fail git tag -s -m tail tag-gpg-failure'
git config --unset user.signingkey
+# try to produce invalid signature
+test_expect_success GPG \
+ 'git tag -s fails if gpg is misconfigured (bad signature format)' \
+ 'test_config gpg.program echo &&
+test_must_fail git tag -s -m tail tag-gpg-failure'
+
+
# try to verify without gpg:
rm -rf gpghome
--
2.9.0.160.g4984cba
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Code review tool recommendations - replacement for crucible?
Hi all,
I'm looking for pointers to review tools that work with git (obviously),
and can deal sensibly with bigger reviews. Things we need:
- Ability to split (set of) commits into multiple reviews,
so parts of changes can be reviewed by the respective owners
(or assign different reviewers to different files/subtrees
in a review).
- Tracking of files (or changes) already reviewed (due to the large numbers),
and of the handling of issues found so far.
- Support incremental reviews, not just e.g. over the content of a
pull request (bitbucket) When review comments are processed people
want to be able to only review that change, and not to be forced
to find that change in the entire previous changeset without
the tool's support.
We were mostly content with atlassian crucible so far, but
it simply fails to index[1] our large product repo (5+ Gb) so
we switched to just give it diffs to review, and it fails
to properly display renames, and fails in a few minor
but annoying ways in dealing with these big reviews.
Any pointers?
- Andreas
[1] Crucible seems to be svn-centric, and the mapping
from git to svn changesets they use internally
apparently is O(n^{too much}) - just indexing
a new branch identical to an existing one takes
hours here.
--
"Totally trivial. Famous last words."
From: Linus Torvalds
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] sign_buffer: use pipe_command
Similar to the prior commit for verify_signed_buffer, the
motivation here is both to make the code simpler, and to
avoid any possible deadlocks with gpg.
In this case we have the same "write to stdin, then read
from stdout" that the verify case had. This is unlikely to
be a problem in practice, since stdout has the detached
signature, which it cannot compute until it has read all of
stdin (if it were a non-detached signature, that would be a
problem, though).
We don't read from stderr at all currently. However, we will
want to in a future patch, so this also prepares us there
(and in that case gpg _does_ write before reading all of the
input, though again, it is unlikely that a key uid will fill
up a pipe buffer).
Signed-off-by: Jeff King
---
gpg-interface.c | 24 +---
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index c98035d..74f08a2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -151,40 +151,26 @@ const char *get_signing_key(void)
int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char
*signing_key)
{
struct child_process gpg = CHILD_PROCESS_INIT;
- ssize_t len;
+ int ret;
size_t i, j, bottom;
- gpg.in = -1;
- gpg.out = -1;
argv_array_pushl(&gpg.args,
gpg_program,
"-bsau", signing_key,
NULL);
- if (start_command(&gpg))
- return error(_("could not run gpg."));
+ bottom = signature->len;
/*
* When the username signingkey is bad, program could be terminated
* because gpg exits without reading and then write gets SIGPIPE.
*/
sigchain_push(SIGPIPE, SIG_IGN);
-
- if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
- close(gpg.in);
- close(gpg.out);
- finish_command(&gpg);
- return error(_("gpg did not accept the data"));
- }
- close(gpg.in);
-
- bottom = signature->len;
- len = strbuf_read(signature, gpg.out, 1024);
- close(gpg.out);
-
+ ret = pipe_command(&gpg, buffer->buf, buffer->len,
+ signature, 1024, NULL, 0);
sigchain_pop(SIGPIPE);
- if (finish_command(&gpg) || !len || len < 0)
+ if (ret || signature->len == bottom)
return error(_("gpg failed to sign the data"));
/* Strip CR from the line endings, in case we are on Windows. */
--
2.9.0.160.g4984cba
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/7] verify_signed_buffer: use pipe_command
This is shorter and should make the function easier to
follow. But more importantly, it removes the possibility of
any deadlocks based on reading or writing to gpg.
It's not clear if such a deadlock is possible in practice.
We do write the whole payload before reading anything, so we
could deadlock there. However, in practice gpg will need to
read our whole input to verify the signature, so it will
drain our payload first. It could write an error to stderr
before reading, but it's unlikely that such an error
wouldn't be followed by it immediately exiting, or that the
error would actually be larger than a pipe buffer.
On the writing side, we drain stderr (with the
human-readable output) in its entirety before reading stdout
(with the status-fd data). Running strace on "gpg --verify"
does show interleaved output on the two descriptors:
write(2, "gpg: ", 5)= 5
write(2, "Signature made Thu 16 Jun 2016 0"..., 73) = 73
write(1, "[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj"..., 66) = 66
write(1, "[GNUPG:] GOODSIG 69808639F9430ED"..., 60) = 60
write(2, "gpg: ", 5)= 5
write(2, "Good signature from \"Jeff King <"..., 47) = 47
write(2, "\n", 1) = 1
write(2, "gpg: ", 5)= 5
write(2, "aka \"Jeff King <"..., 49) = 49
write(2, "\n", 1) = 1
write(1, "[GNUPG:] VALIDSIG C49CE24156AF08"..., 135) = 135
write(1, "[GNUPG:] TRUST_ULTIMATE\n", 24) = 24
The second line written to stdout there contains the
signer's UID, which can be arbitrarily long. If it fills the
pipe buffer, then gpg would block writing to its stdout,
while we are blocked trying to read its stderr.
In practice, GPG seems to limit UIDs to 2048 bytes, so
unless your pipe buffer size is quite small, or unless gpg
does not enforce the limit under some conditions, this seems
unlikely in practice.
Still, it is not hard for us to be cautious and just use
pipe_command.
Signed-off-by: Jeff King
---
gpg-interface.c | 22 +++---
1 file changed, 3 insertions(+), 19 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 854c1e5..c98035d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -229,29 +229,13 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
"--status-fd=1",
"--verify", temp.filename.buf, "-",
NULL);
- gpg.in = -1;
- gpg.out = -1;
- if (gpg_output)
- gpg.err = -1;
- if (start_command(&gpg)) {
- delete_tempfile(&temp);
- return error(_("could not run gpg."));
- }
- sigchain_push(SIGPIPE, SIG_IGN);
- write_in_full(gpg.in, payload, payload_size);
- close(gpg.in);
-
- if (gpg_output) {
- strbuf_read(gpg_output, gpg.err, 0);
- close(gpg.err);
- }
if (!gpg_status)
gpg_status = &buf;
- strbuf_read(gpg_status, gpg.out, 0);
- close(gpg.out);
- ret = finish_command(&gpg);
+ sigchain_push(SIGPIPE, SIG_IGN);
+ ret = pipe_command(&gpg, payload, payload_size,
+ gpg_status, 0, gpg_output, 0);
sigchain_pop(SIGPIPE);
delete_tempfile(&temp);
--
2.9.0.160.g4984cba
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] run-command: add pipe_command helper
We already have capture_command(), which captures the stdout
of a command in a way that avoids deadlocks. But sometimes
we need to do more I/O, like capturing stderr as well, or
sending data to stdin. It's easy to write code that
deadlocks racily in these situations depending on how fast
the command reads its input, or in which order it writes its
output.
Let's give callers an easy interface for doing this the
right way, similar to what capture_command() did for the
simple case.
The whole thing is backed by a generic poll() loop that can
feed an arbitrary number of buffers to descriptors, and fill
an arbitrary number of strbufs from other descriptors. This
seems like overkill, but the resulting code is actually a
bit cleaner than just handling the three descriptors
(because the output code for stdout/stderr is effectively
duplicated, so being able to loop is a benefit).
Signed-off-by: Jeff King
---
It's possible this could come in handy other places, too. I didn't look,
but I suspect pump_io() could back some of the parallel submodule stuff.
run-command.c | 152 --
run-command.h | 31 +---
2 files changed, 171 insertions(+), 12 deletions(-)
diff --git a/run-command.c b/run-command.c
index af0c8a1..609fa4c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -864,19 +864,161 @@ int run_hook_le(const char *const *env, const char
*name, ...)
return ret;
}
-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)
+struct io_pump {
+ /* initialized by caller */
+ int fd;
+ int type; /* POLLOUT or POLLIN */
+ union {
+ struct {
+ const char *buf;
+ size_t len;
+ } out;
+ struct {
+ struct strbuf *buf;
+ size_t hint;
+ } in;
+ } u;
+
+ /* returned by pump_io */
+ int error; /* 0 for success, otherwise errno */
+
+ /* internal use */
+ struct pollfd *pfd;
+};
+
+int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
+{
+ int pollsize = 0;
+ int i;
+
+ for (i = 0; i < nr; i++) {
+ struct io_pump *io = &slots[i];
+ if (io->fd < 0)
+ continue;
+ pfd[pollsize].fd = io->fd;
+ pfd[pollsize].events = io->type;
+ io->pfd = &pfd[pollsize++];
+ }
+
+ if (!pollsize)
+ return 0;
+
+ if (poll(pfd, pollsize, -1) < 0) {
+ if (errno == EINTR)
+ return 1;
+ die_errno("poll failed");
+ }
+
+ for (i = 0; i < nr; i++) {
+ struct io_pump *io = &slots[i];
+
+ if (io->fd < 0)
+ continue;
+
+ if (!(io->pfd->revents &
(POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))
+ continue;
+
+ if (io->type == POLLOUT) {
+ ssize_t len = xwrite(io->fd,
+io->u.out.buf, io->u.out.len);
+ if (len < 0) {
+ io->error = errno;
+ close(io->fd);
+ io->fd = -1;
+ } else {
+ io->u.out.buf += len;
+ io->u.out.len -= len;
+ if (!io->u.out.len) {
+ close(io->fd);
+ io->fd = -1;
+ }
+ }
+ }
+
+ if (io->type == POLLIN) {
+ ssize_t len = strbuf_read_once(io->u.in.buf,
+ io->fd, io->u.in.hint);
+ if (len < 0)
+ io->error = errno;
+ if (len <= 0) {
+ close(io->fd);
+ io->fd = -1;
+ }
+ }
+ }
+
+ return 1;
+}
+
+int pump_io(struct io_pump *slots, int nr)
+{
+ struct pollfd *pfd;
+ int i;
+
+ for (i = 0; i < nr; i++)
+ slots[i].error = 0;
+
+ ALLOC_ARRAY(pfd, nr);
+ while (pump_io_round(slots, nr, pfd))
+ ; /* nothing */
+ free(pfd);
+
+ /* There may be multiple errno values, so just pick the first. */
+ for (i = 0; i < nr; i++) {
+ if (slots[i].error) {
+ errno = slots[i].error;
+ return -1;
+ }
+ }
+ return 0;
+}
+
+
+int pipe_command(struct child_process *cmd,
+const char *in, size_t in_len,
+struct strbuf *out, size_t out_hint,
+struct strbuf *err, size_t err_hint)
{
-
[PATCH 3/7] verify_signed_buffer: use tempfile object
We use git_mkstemp to create a temporary file, and try to
clean it up in all exit paths from the function. But that
misses any cases where we die by signal, or by calling die()
in a sub-function. In addition, we missed one of the exit
paths.
Let's convert to using a tempfile object, which handles the
hard cases for us, and add the missing cleanup call. Note
that we would not simply want to rely on program exit to
catch our missed cleanup, as this function may be called
many times in a single program (for the same reason, we use
a static tempfile instead of heap-allocating a new one; that
gives an upper bound on our memory usage).
Signed-off-by: Jeff King
---
gpg-interface.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 216cad8..854c1e5 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -3,6 +3,7 @@
#include "strbuf.h"
#include "gpg-interface.h"
#include "sigchain.h"
+#include "tempfile.h"
static char *configured_signing_key;
static const char *gpg_program = "gpg";
@@ -208,28 +209,32 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
struct strbuf *gpg_output, struct strbuf *gpg_status)
{
struct child_process gpg = CHILD_PROCESS_INIT;
- char path[PATH_MAX];
+ static struct tempfile temp;
int fd, ret;
struct strbuf buf = STRBUF_INIT;
- fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXX");
+ fd = mks_tempfile_t(&temp, ".git_vtag_tmpXX");
if (fd < 0)
- return error_errno(_("could not create temporary file '%s'"),
path);
- if (write_in_full(fd, signature, signature_size) < 0)
- return error_errno(_("failed writing detached signature to
'%s'"), path);
+ return error_errno(_("could not create temporary file"));
+ if (write_in_full(fd, signature, signature_size) < 0) {
+ error_errno(_("failed writing detached signature to '%s'"),
+ temp.filename.buf);
+ delete_tempfile(&temp);
+ return -1;
+ }
close(fd);
argv_array_pushl(&gpg.args,
gpg_program,
"--status-fd=1",
-"--verify", path, "-",
+"--verify", temp.filename.buf, "-",
NULL);
gpg.in = -1;
gpg.out = -1;
if (gpg_output)
gpg.err = -1;
if (start_command(&gpg)) {
- unlink(path);
+ delete_tempfile(&temp);
return error(_("could not run gpg."));
}
@@ -249,7 +254,7 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
ret = finish_command(&gpg);
sigchain_pop(SIGPIPE);
- unlink_or_warn(path);
+ delete_tempfile(&temp);
ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
strbuf_release(&buf); /* no matter it was used or not */
--
2.9.0.160.g4984cba
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] verify_signed_buffer: drop pbuf variable
If our caller gave us a non-NULL gpg_status parameter, we write the gpg status into their strbuf. If they didn't, then we write it to a temporary local strbuf (since we still need to look at it). The variable "pbuf" adds an extra layer of indirection so that the rest of the function can just access whichever is appropriate. However, the name "pbuf" isn't very descriptive, and it's easy to get confused about what is supposed to be in it (especially because we are reading both "status" and "output" from gpg). Rather than give it a more descriptive name, we can just use gpg_status as our indirection pointer. Either it points to the caller's input, or we can point it directly to our temporary buffer. Signed-off-by: Jeff King --- gpg-interface.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 0ed9fa7..216cad8 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -211,7 +211,6 @@ int verify_signed_buffer(const char *payload, size_t payload_size, char path[PATH_MAX]; int fd, ret; struct strbuf buf = STRBUF_INIT; - struct strbuf *pbuf = &buf; fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXX"); if (fd < 0) @@ -242,9 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size, strbuf_read(gpg_output, gpg.err, 0); close(gpg.err); } - if (gpg_status) - pbuf = gpg_status; - strbuf_read(pbuf, gpg.out, 0); + if (!gpg_status) + gpg_status = &buf; + strbuf_read(gpg_status, gpg.out, 0); close(gpg.out); ret = finish_command(&gpg); @@ -252,7 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, unlink_or_warn(path); - ret |= !strstr(pbuf->buf, "\n[GNUPG:] GOODSIG "); + ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG "); strbuf_release(&buf); /* no matter it was used or not */ return ret; -- 2.9.0.160.g4984cba -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] gpg-interface: use child_process.args
Our argv allocations are relatively straightforward, but
this avoids us having to manually keep the count up to date
(or create new to-be-replaced slots in the declaration) when
we add new arguments.
Signed-off-by: Jeff King
---
gpg-interface.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index c4b1e8c..0ed9fa7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -150,17 +150,15 @@ const char *get_signing_key(void)
int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char
*signing_key)
{
struct child_process gpg = CHILD_PROCESS_INIT;
- const char *args[4];
ssize_t len;
size_t i, j, bottom;
- gpg.argv = args;
gpg.in = -1;
gpg.out = -1;
- args[0] = gpg_program;
- args[1] = "-bsau";
- args[2] = signing_key;
- args[3] = NULL;
+ argv_array_pushl(&gpg.args,
+gpg_program,
+"-bsau", signing_key,
+NULL);
if (start_command(&gpg))
return error(_("could not run gpg."));
@@ -210,13 +208,11 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
struct strbuf *gpg_output, struct strbuf *gpg_status)
{
struct child_process gpg = CHILD_PROCESS_INIT;
- const char *args_gpg[] = {NULL, "--status-fd=1", "--verify", "FILE",
"-", NULL};
char path[PATH_MAX];
int fd, ret;
struct strbuf buf = STRBUF_INIT;
struct strbuf *pbuf = &buf;
- args_gpg[0] = gpg_program;
fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXX");
if (fd < 0)
return error_errno(_("could not create temporary file '%s'"),
path);
@@ -224,12 +220,15 @@ int verify_signed_buffer(const char *payload, size_t
payload_size,
return error_errno(_("failed writing detached signature to
'%s'"), path);
close(fd);
- gpg.argv = args_gpg;
+ argv_array_pushl(&gpg.args,
+gpg_program,
+"--status-fd=1",
+"--verify", path, "-",
+NULL);
gpg.in = -1;
gpg.out = -1;
if (gpg_output)
gpg.err = -1;
- args_gpg[3] = path;
if (start_command(&gpg)) {
unlink(path);
return error(_("could not run gpg."));
--
2.9.0.160.g4984cba
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] gpg-interface cleanups
This started off with Michael's patch to sign_buffer, which is at the tip, and then me trying to address the possible deadlocks there and in verify_signed_buffer. While I was in the area, I took the opportunity to do a few cleanups. It's unclear to me whether the deadlocks are possible in practice; see patch 5 for discussion. My guess is probably not, but the amount of code to support doing it right is not all that much. But if we don't like it, we can drop 4-6. Patch 7 is still authored by Michael, but has been massaged greatly by me. I'll comment more directly on the changes there. [1/7]: gpg-interface: use child_process.args [2/7]: verify_signed_buffer: drop pbuf variable [3/7]: verify_signed_buffer: use tempfile object [4/7]: run-command: add pipe_command helper [5/7]: verify_signed_buffer: use pipe_command [6/7]: sign_buffer: use pipe_command [7/7]: gpg-interface: check gpg signature creation status -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote: > As for the flexibility: > We do code specifically for gpg, which happens to work for gpg2 also. > The patch doesn't add any gpg ui requirements that we don't require > elsewhere already. > More flexibility requires a completely pluggable approach - gpgsm > already fails to meet the gpg command line ui. Does it? I haven't run it in production, but I did some tests with gpgsm a few months ago and found it to be a drop-in replacement, at least with respect to git. Though I don't think that matters one way or the other for the current discussion; it _does_ do --status-fd. > In any case, "status-fd" is *the* way to interface with gpg reliably > just like plumbing commands are *the* way to interface with git reliably. Fair enough. I've generally found its exit code pretty reliable. It's unclear to me if the problem you saw was because gpg was exiting 0 but producing no signature, or if your debugging was masking its exit code. Either way, I do not mind using --status-fd; it seems like it should be more robust in general. It's the tip commit in the series I'm about to post. > As for the read locking: > I'm sorry I have no idea about that area at all. I thought that I'm > doing the same that we do for verify, but apparently not. My > strbuf_read-fu is not that strong either (read: copy&paste). I trust > your assessment completely, though. Yeah, you're right that the deadlock thing is also a possibility on the verification side. I'm not sure whether it's possible to trigger in practice or not. See the analysis in the series. > As for the original problem: > That had a different cause, as we know now (rebase dropping signatures > without hint). I still think we should check gpg status codes properly. Yep, I agree. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] grep: fix grepping for "intent to add" files
On Thu, Jun 16, 2016 at 07:53:24AM +0100, Charles Bailey wrote:
> From: Charles Bailey
>
> This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e.
>
> This commit caused 'git grep' to no longer find matches in new files in
> the working tree where the corresponding index entry had the "intent to
> add" bit set.
I don't think revert is right. It rather needs a re-fix like below.
Basically we want grep_file() to run as normal, but grep_sha1()
(i.e. git grep --cached) should ignore i-t-a entries, because empty
SHA-1 is not the right content to grep. It does not matter in positive
matching, sure, but it may in -v cache.
-- 8< --
diff --git a/builtin/grep.c b/builtin/grep.c
index 462e607..ae73831 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct
pathspec *pathspec, int
for (nr = 0; nr < active_nr; nr++) {
const struct cache_entry *ce = active_cache[nr];
- if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
+ if (!S_ISREG(ce->ce_mode))
continue;
if (!ce_path_match(ce, pathspec, NULL))
continue;
@@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct
pathspec *pathspec, int
* cache version instead
*/
if (cached || (ce->ce_flags & CE_VALID) ||
ce_skip_worktree(ce)) {
- if (ce_stage(ce))
+ if (ce_stage(ce) || ce_intent_to_add(ce))
continue;
hit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);
}
-- 8< --
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: I lost my commit signature
On Wed, Jun 15, 2016 at 09:07:16AM +0200, Michael J Gruber wrote: > > Ah, so the problem is probably that you had a signature _initially_, but > > that it did not survive the rebase. Which makes sense, as rebase would > > need to re-sign. It does not by default, but you can tell it to do so > > with "-S". Or you can set `commit.gpgsign`, which should sign in both > > cases. > > While it's clear that a rebase invalidates the signature, we could try > to be more helpful here, especially given the fact that (with our model) > you can't sign a commit afterwards any more. > > commit.gpgsign signs everything, but there should be a mode for > re-signing signed commits, or at least a warning that rebase dropped a > signature so that you can --amend -S the last commit. I had a similar thought, though I'm not sure how useful a "re-sign signed commits" mode would be in practice. Mostly because I'm not sure why signing would be more important for one commit versus another. That is, I can see why somebody would set "commit.gpgSign"; their preference (or that of their project) is to sign commits, and they've set up gpg, etc, to make it relatively painless. But why does somebody run "commit -S" for a single commit, but not all the time? Is it because that commit is special? Or is that particular moment special? One implies that it's important for the signature to be retained during a rebase, and one does not. So I dunno. I would not be opposed to such a feature, but I'm having trouble figuring out why it would be useful (though for the most part, I do not see why anything but per-project commit.gpgSign config is particularly useful. Maybe I just lack imagination). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Issue with subtree split in git version post 2.7.X on MAC
Hi, I am trying to extract part of larger git repo to create a new smaller repo maintaining al the history of the extracted portion. For this I used git subtree split command. Following are the steps followed: 1. git clone of full repo in 'full_rep' dir 2. Initialised new empty target repo: mkdir new_repo cd new_repo git init 3. split the desired subtree into a new branch: cd ../full_repo git subtree split --prefix='folder_name_to_be_extracted' --annotate="(split)" -b new-repo 4. pull the new branch into the new empty repo: cd ../new_repo git pull ../full_repo new-repo:master 5. check size of the new repo : du -sh . With git version 2.8.4 on Mac EL Capitan OS, size of repo is coming out to be 117M and also the history of new repo is not specific to the extracted portion only. Its containing commits from different folders too. Whereas with git version 2.5.5, size of repo comes tout to be 9.5M and history of new repo is correct, containing only extracted portion commits history. Please check if there is some issue with git version 2.8.4 on mac while running subtree split. Regards, Shrikant Prasad -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/3] bisect: rewrite `check_term_format` shell function in C
On 24 May 2016, at 09:21, Pranit Bauva wrote: > Reimplement the `check_term_format` shell function in C and add > a `--check-term-format` subcommand to `git bisect--helper` to call it > from git-bisect.sh > > Using `--check-term-format` subcommand is a temporary measure to port > shell function to C so as to use the existing test suite. As more > functions are ported, this subcommand will be retired and will > be called by some other method/subcommand. For eg. In conversion of > write_terms() of git-bisect.sh, the subcommand will be removed and > instead check_term_format() will be called in its C implementation while > a new subcommand will be introduced for write_terms(). > > Helped-by: Johannes Schindelein > Mentored-by: Lars Schneider > Mentored-by: Christian Couder Hi Pranit, please drop my Mentored-by until I contribute something useful. I feel bad being mentioned in the same way as Christian although he does all the work. Thanks, Lars > Signed-off-by: Pranit Bauva > --- -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] grep: fix grepping for "intent to add" files
From: Charles Bailey
This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e.
This commit caused 'git grep' to no longer find matches in new files in
the working tree where the corresponding index entry had the "intent to
add" bit set.
Add tests to cover this case and a few related cases which previously
lacked coverage.
Signed-off-by: Charles Bailey
---
Originally discussed:
http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358
http://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002
Unless I've misunderstood the conversation and commit message, the
referenced commit was supposed to be a "code as a comment" commit with
no change in observable behavior however a user was surprised that 'git
grep' couldn't find something that regular grep could, despite the file
being tracked - albeit new and "intended to add".
builtin/grep.c | 2 +-
t/t7810-grep.sh | 29 +
2 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 462e607..d5aacba 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct
pathspec *pathspec, int
for (nr = 0; nr < active_nr; nr++) {
const struct cache_entry *ce = active_cache[nr];
- if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))
+ if (!S_ISREG(ce->ce_mode))
continue;
if (!ce_path_match(ce, pathspec, NULL))
continue;
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1e72971..eae731a 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1364,4 +1364,33 @@ test_expect_success 'grep --color -e A --and -e B -p
with context' '
test_cmp expected actual
'
+test_expect_success 'grep can find things only in the work tree' '
+ touch work-tree-only &&
+ git add work-tree-only &&
+ echo "find in work tree" >work-tree-only &&
+ git grep --quiet "find in work tree" &&
+ test_must_fail git grep --quiet --cached "find in work tree" &&
+ test_must_fail git grep --quiet "find in work tree" HEAD &&
+ git rm -f work-tree-only
+'
+
+test_expect_success 'grep can find things only in the work tree (i-t-a)' '
+ echo "intend to add this" >intend-to-add &&
+ git add -N intend-to-add &&
+ git grep --quiet "intend to add this" &&
+ test_must_fail git grep --quiet --cached "intend to add this" &&
+ test_must_fail git grep --quiet "intend to add this" HEAD &&
+ git rm -f intend-to-add
+'
+
+test_expect_success 'grep can find things only in the index' '
+ echo "only in the index" >cache-this &&
+ git add cache-this &&
+ rm cache-this &&
+ test_must_fail git grep --quiet "only in the index" &&
+ git grep --quiet --cached "only in the index" &&
+ test_must_fail git grep --quiet "only in the index" HEAD &&
+ git rm --cached cache-this
+'
+
test_done
--
2.8.2.311.gee88674
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
