* Re: [PATCH] blame: add the ability to ignore commits
From: Barret Rhoden @ 2019-01-08 16:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Stefan Beller, Jeff Smith, Jeff King
In-Reply-To: <xmqqbm4s6ozl.fsf@gitster-ct.c.googlers.com>
On 2019-01-07 at 15:13 Junio C Hamano <gitster@pobox.com> wrote:
> If I read it correctly, this gives a very limited form of -S, in the
> sense that anything this can do can be expressed by using -S but the
> reverse is not true, but is designed to be easier to use, in the
> sense that unlike -S, this does not have to describe the part of the
> history you do not have to lie about. The documentation should say
> something about these pros-and-cons to help readers decide which
> feature to use.
Yeah, -S lists the revs to use, this lists the revs to *not* use. I'll
add a note.
> I somehow feel that this is rare enough that it should not squat on
> short-and-sweet '-i'. We would want to reserve it to something like
> "--ignore-case", for example.
Can do. I'll change the interface to your suggestion from down below.
> > The file .git-blame-ignore-revs is checked by default.
>
> Giving the projects a way to easily help participants to share the
> same distorted view of the history is a good idea, but I do not
> think we should allow projects to do so to those who merely clone it
> without their consent. IOW, "by default" is a terrible idea.
>
> Perhaps paying attention to blame.skipRevsFile configuration
> variable that is set by the end user would be sufficient----the
> project can ship .blame-skip-revs (or any random filename of their
> choice) in-tree as tracked contents and tell its users that they can
> optionally use it.
A blame config option works for me. I'd like the users/cloners of a
repo to not need to do anything extravagant, but a one-time config
would be fine.
> > It's useful to be alerted to the presence of an ignored commit in the
> > history of a line. Those lines will be marked with '*' in the
> > non-porcelain output. The '*' is attached to the line number to keep
> > from breaking tools that rely on the whitespace between columns.
>
> A policy decision like the above two shouldn't be hardcoded in the
> feature like this, but should be done as a separate option. By
> default, these shouldn't be marked with '*', as the same tools you
> said you are afraid of breaking would be expecting a word with only
> digits and no asterisk in the column where line numbers appear and
> will get broken by this change if done unconditionally.
Since users are already opting-in to the blame-ignore, do you also want
them to opt-in to the annotation? I can make a separate config option
to turn on the annotation. Any preference for how it is marked?
> In general, I find this patch trying to change too many things at
> the same time, without sufficient justification. Perhaps do these
> different things as separate steps in a single series?
>
> > A blame_entry attributed to an ignored commit will get passed to its
> > parent.
>
> Obviously, an interesting consideration is what happens when a merge
> commit is skipped. Is it sufficient to change this description to
> "...will get passed to its parentS", or would the code do completely
> nonsensical things without further tweaks (a possible simple tweak
> could be to refuse skipping merges)?
If we skip a merge commit, it might pick the wrong parent. For
example, this line was brought in via a merge:
$ ~/src/git/git-blame include/linux/mm.h | grep VM_SYNC
b6fb293f2497a (Jan Kara 2017-11-01 16:36:41 +0100 204) #define VM_SYNC
It's from merge: a3841f94c7ec ("Merge tag 'libnvdimm-for-4.15', and if
we ignore it:
$ ~/src/git/git-blame -i a3841f94c7ecb include/linux/mm.h | grep VM_SYNC
cc2383ec06be0 (Konstantin Khlebnikov 2012-10-08 16:28:37 -0700 204*) #define VM_SYNC
The wrong commit is blamed.
I can put a note in the doc about it and die if we're given a merge
commit. Is there a convenient helper for detecting a merge, or can I
just check for multiple parents? (something like commit->parents &&
commit->parents->next)
> > If an ignored commit changed a line, an ancestor that changed
> > the line will get blamed. However, if an ignored commit added lines, a
> > commit changing a nearby line may get blamed. If no commit is found,
> > the original commit for the file will get blamed.
>
> The above somehow does not read as describing a carefully designed
> behaviour; rather, it sounds as if it is saying "the code does
> whatever random things it happens to do". For example, when there
> is a newly added line how is "A" commit changing a nearby line
> chosen (a line has lines before it and after it---they may be
> touched by different commits, and before and after that skipped
> commit, so there are multiple commits to choose from)?
This was more of a commentary about its behavior. If you ignore a
commit that added lines, it'd be nice to get a hint of what might have
caused it, and picking a commit that affected an adjacent line seemed
fine. But yeah, it's not doing anything crazy.
> > diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
> > index 16323eb80e31..e41375374892 100644
> > --- a/Documentation/git-blame.txt
> > +++ b/Documentation/git-blame.txt
> > @@ -10,6 +10,7 @@ SYNOPSIS
> > [verse]
> > 'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] [--incremental]
> > [-L <range>] [-S <revs-file>] [-M] [-C] [-C] [-C] [--since=<date>]
> > + [-i <rev>] [--no-default-ignores] [--ignore-file=<file>]
> > [--progress] [--abbrev=<n>] [<rev> | --contents <file> | --reverse <rev>..<rev>]
> > [--] <file>
> >
> > @@ -84,6 +85,20 @@ include::blame-options.txt[]
> > Ignore whitespace when comparing the parent's version and
> > the child's to find where the lines came from.
> >
> > +-i <rev>::
> > + Ignore revision when assigning blame. Lines that were changed by an
> > + ignored commit will be marked with a `*` in the blame output. Lines
> > + that were added by an ignored commit may be attributed commits making
> > + nearby changes or to the first commit touching the file.
>
> It probably deserves to be told that this option can be given
> multiple times and used cumulatively (unlike usual "last one wins"
> rule).
>
> > +--no-default-ignores::
> > + Do not automatically ignore revisions in the file
> > + `.git-blame-ignore-revs`.
>
> This should not be "opt-out" like this.
>
> > +--ignore-file=<file>::
> > + Ignore revisions listed in `file`, one revision per line. Whitespace
> > + and comments beginning with `#` are ignored.
>
> Should it be capable to take two or more ignore-files? Or should we
> use the usual "the last one wins" rule?
>
> I think we should support blame.skipRevFile configuration variable
> so that the users do not have to constantly give the option from the
> command line. And with that, there is no need to have a hardcoded
> filename .git-blame-ignore-revs or anything like that.
>
> If we are to use configuration variable, however, we'd need a way to
> override its use from the command line, too. Perhaps a sane
> arrangement would be
>
> - if one or more --skip-revs-file=<file> are given from the
> command line, use all of them and ignore blame.skipRevsFile
> configuration variable.
>
> - if no --skip-revs-file=<file> is given from the command line,
> use blame.skipRevsFile configuration variable.
>
> - regardless of the above two, pay attention to --skip-rev=<rev>
> command line option(s).
Sounds fine to me.
> Somehow the damage to blame.c codebase looks way too noisy for what
> the code wants to do. If all we want is to pretend in a history,
> e.g.
>
> ---O---A---B---C---D
>
> that commit B does not exist, i.e. use a distorted view of the
> history
>
> ---O---A-------C---D
>
> wouldn't it be sufficient to modify pass_blame(), i.e. the only the
> caller of the pass_blame_to_parent(), where we find the parent
> commits of "C" and dig the history to pass blame to "B", and have it
> pretend as if "B" does not exist and pass blame directly to "A"?
I originally tried to skip 'B' in pass_blame() when B popped up as a
scapegoat. That broke the offsets of the blame_entries in the
parent. By running diff_hunks(), we get the opportunity to adjust the
offsets. Also, when it comes to marking the blame_entries for marking
the output, we want to know the specific diffs and to break them up at
the boundaries of [tlno,same) in blame_chunk().
> Thanks. I am personally not all that interested in this yet.
Thanks for taking a look.
Barret
^ permalink raw reply
* Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment
From: Phillip Wood @ 2019-01-08 16:22 UTC (permalink / raw)
To: Git Mailing List, Stefan Beller, Junio C Hamano; +Cc: Phillip Wood
In-Reply-To: <20181123111658.30342-1-phillip.wood@talktalk.net>
Hi Junio
I just wanted to check that these patches are on your radar as they
haven't made it into pu yet.
Best Wishes for the New Year
Phillip
On 23/11/2018 11:16, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Thanks to Stefan for his feedback on v1. I've updated patches 2 & 8 in
> response to those comments - see the range-diff below for details (the
> patch numbers are off by one in the range diff, I think because the
> first patch is unchanged and so it was used as the merge base by
> --range-diff=<old-head>. For some reason the range-diff also includes
> the notes even though I did not give --notes to format-patch)
>
> When trying out the new --color-moved-ws=allow-indentation-change I
> was disappointed to discover it did not work if the indentation
> contains a mix of spaces and tabs. This series reworks it so that it
> does.
>
>
> Phillip Wood (9):
> diff: document --no-color-moved
> Use "whitespace" consistently
> diff: allow --no-color-moved-ws
> diff --color-moved-ws: demonstrate false positives
> diff --color-moved-ws: fix false positives
> diff --color-moved=zebra: be stricter with color alternation
> diff --color-moved-ws: optimize allow-indentation-change
> diff --color-moved-ws: modify allow-indentation-change
> diff --color-moved-ws: handle blank lines
>
> Documentation/diff-options.txt | 15 ++-
> Documentation/git-cat-file.txt | 8 +-
> diff.c | 219 +++++++++++++++++++++------------
> t/t4015-diff-whitespace.sh | 99 ++++++++++++++-
> 4 files changed, 255 insertions(+), 86 deletions(-)
>
> Range-diff against v1:
> 1: ae58ae4f29 ! 1: 4939ee371d diff: use whitespace consistently
> @@ -1,9 +1,10 @@
> Author: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> - diff: use whitespace consistently
> + Use "whitespace" consistently
>
> - Most of the documentation uses 'whitespace' rather than 'white space'
> - or 'white spaces' convert to latter two to the former for consistency.
> + Most of the messages and documentation use 'whitespace' rather than
> + 'white space' or 'white spaces' convert to latter two to the former for
> + consistency.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> @@ -29,6 +30,39 @@
> whitespace is the same per line. This is incompatible with the
> other modes.
>
> + diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> + --- a/Documentation/git-cat-file.txt
> + +++ b/Documentation/git-cat-file.txt
> +@@
> + stdin, and the SHA-1, type, and size of each object is printed on stdout. The
> + output format can be overridden using the optional `<format>` argument. If
> + either `--textconv` or `--filters` was specified, the input is expected to
> +-list the object names followed by the path name, separated by a single white
> +-space, so that the appropriate drivers can be determined.
> ++list the object names followed by the path name, separated by a single
> ++whitespace, so that the appropriate drivers can be determined.
> +
> + OPTIONS
> + -------
> +@@
> + Print object information and contents for each object provided
> + on stdin. May not be combined with any other options or arguments
> + except `--textconv` or `--filters`, in which case the input lines
> +- also need to specify the path, separated by white space. See the
> ++ also need to specify the path, separated by whitespace. See the
> + section `BATCH OUTPUT` below for details.
> +
> + --batch-check::
> + --batch-check=<format>::
> + Print object information for each object provided on stdin. May
> + not be combined with any other options or arguments except
> + `--textconv` or `--filters`, in which case the input lines also
> +- need to specify the path, separated by white space. See the
> ++ need to specify the path, separated by whitespace. See the
> + section `BATCH OUTPUT` below for details.
> +
> + --batch-all-objects::
> +
> diff --git a/diff.c b/diff.c
> --- a/diff.c
> +++ b/diff.c
> 2: 7072bc6211 = 2: 204c7fea9d diff: allow --no-color-moved-ws
> 3: ce3ad19eea = 3: 542b79b215 diff --color-moved-ws: demonstrate false positives
> 4: 700e0b61e7 = 4: 4ffb5c4122 diff --color-moved-ws: fix false positives
> 5: 9ecd8159a7 = 5: a3a84f90c5 diff --color-moved=zebra: be stricter with color alternation
> 6: 1b1158b1ca = 6: f94f2e0bae diff --color-moved-ws: optimize allow-indentation-change
> 7: d8a362be6a ! 7: fe8eb9cdbc diff --color-moved-ws: modify allow-indentation-change
> @@ -17,7 +17,7 @@
>
> This commit changes the way the indentation is handled to track the
> visual size of the indentation rather than the characters in the
> - indentation. This has they benefit that any whitespace errors do not
> + indentation. This has the benefit that any whitespace errors do not
> interfer with the move detection (the whitespace errors will still be
> highlighted according to --ws-error-highlight). During the discussion
> of this feature there were concerns about the correct detection of
> @@ -30,7 +30,7 @@
> they are uncolored.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> - Changes since rfc:
> + changes since rfc:
> - It now replaces the existing implementation rather than adding a new
> mode.
> - The indentation deltas are now calculated once for each line and
> @@ -49,8 +49,8 @@
> const char *line;
> int len;
> int flags;
> -+ int indent_off;
> -+ int indent_width;
> ++ int indent_off; /* Offset to first non-whitespace character */
> ++ int indent_width; /* The visual width of the indentation */
> enum diff_symbol s;
> };
> #define EMITTED_DIFF_SYMBOL_INIT {NULL}
> 8: 1f7e99d45c = 8: e600f8247c diff --color-moved-ws: handle blank lines
>
>
^ permalink raw reply
* Re: git-lfs integration?
From: Ævar Arnfjörð Bjarmason @ 2019-01-08 15:45 UTC (permalink / raw)
To: Harald Dunkel; +Cc: git
In-Reply-To: <79fd2b4e-243c-a9f5-3485-2954fb0f50ef@aixigo.de>
On Tue, Jan 08 2019, Harald Dunkel wrote:
> I wonder why git-lfs is needed to efficiently handle large files
> in git. Would it be reasonable to integrate this functionality
> into the native git?
>
> Please excuse me asking. I read some pretty scary articles about
> rewriting history, asking everybody to clone existing repositories
> again, and strange errors if git-lfs is *not* installed. Apparently
> this is a one-way street, so I didn't dare to install git-lfs yet.
It depends on what "integrate this" means.
I think it's unlikely that git-lfs would be integrated as-is. There's
various clean/smudge filters like it that do remote downloads (also
git-annex). Everyone's probably better off if git itself maintains the
infra needed for that ecosystem, and users can vote by their usage which
one(s) they like.
But in more general terms the problem of making git natively friendlier
to "large files" is being worked on on multiple fronts.
For one, Microsoft has been upstreaming parts of their GVFS fork, if you
search for "partial clone" in release notes since 2.16.0 (including in
2.20.0) you'll see some of that work relevant to that. I.e. one part of
this is the general ability to have partially available local history,
whether it's skipping (big) blobs, some trees etc.
Another effort has been the introduction of the v2 protocol to Git,
which has happened recently, and is only now starting to get rolled out
at various hosting providers. That in and of itself hasn't helped with
this, but allows for future extensions to the protocol, such as "this is
not the full data, you can find the rest at xyz".
Then there's the "odb" effort, see e.g. here:
https://public-inbox.org/git/20180802061505.2983-1-chriscool@tuxfamily.org/
I think that long-term (5-20yrs) those effors will probably completely
supplant the approach git-lfs is taking. It's a very useful tool, but
ultimately a bit of a hacky workaround in lieu of addressing fixable
issues in git itself, i.e. native support for partially downloaded
history. But getting to that point will take time & effort.
^ permalink raw reply
* git-lfs integration?
From: Harald Dunkel @ 2019-01-08 15:16 UTC (permalink / raw)
To: git
Hi folks,
I wonder why git-lfs is needed to efficiently handle large files
in git. Would it be reasonable to integrate this functionality
into the native git?
Please excuse me asking. I read some pretty scary articles about
rewriting history, asking everybody to clone existing repositories
again, and strange errors if git-lfs is *not* installed. Apparently
this is a one-way street, so I didn't dare to install git-lfs yet.
Regards
Harri
^ permalink raw reply
* git rebase: retain original head?
From: Markus Wiederkehr @ 2019-01-08 15:06 UTC (permalink / raw)
To: git
Hello,
I frequently run an interactive rebase to change the order of recent
commits, apply fixups, etc. When merge conflicts occur I often want to
compare the result with the original head prior to starting the
rebase.
In the past I used "git diff ORIG_HEAD". This used to work as long as
I did not manually invoke "git reset" during the operation.
Since git version 2.20.1 (not sure, maybe 2.20.0?) this no longer
works because ORIG_HEAD seems to get modified by fixup commits during
rebase. Now I have to search through reflog to find the commit I want
to compare the current head to.
During the rebase operation the original head seems to get stored in
'rebase-merge/orig-head'. Unfortunately this references gets removed
after the rebase operation completes.
Would it be possible to retain this information? Could you set
ORIG_HEAD back to rebase-merge/orig-head after rebase completes?
Alternatively, could something like REBASE_HEAD be added for this
purpose?
Undoing a rebase would be related to this and is a very popular
question on SO [1]. Users recommend using "git reset --hard ORIG_HEAD"
which now no longer works.
Regards,
Markus
[1] https://stackoverflow.com/questions/134882
^ permalink raw reply
* Re: Recovering from a "detached from" HEAD
From: Junio C Hamano @ 2019-01-08 14:44 UTC (permalink / raw)
To: Alyssa Ross; +Cc: git
In-Reply-To: <20190108141927.a4q5fqug3ymkh5v3@x220>
Alyssa Ross <hi@alyssa.is> writes:
>> The commit the message shows is meant to indicate where your unnamed
>> branch diverged at named branches. Immediately after moving to the
>> unnamed branch by detaching the HEAD, the message says "at"; the
>> HEAD is pointing directly at the tip of the then-current branch and
>> that is where the tip of the unnamed branch is. You can tell from
>> that message that you will not lose any commit if you were to check
>> out a named branch from that state.
>>
>> After you make a commit on the unnamed branch, you have something to
>> lose if you were to check out a named branch from that state, as the
>> detached HEAD is the _only_ thing these new commits you built on top
>> of the fork point. Upon seeing "HEAD detached from 9745ede235", you
>> could do "git log 9745ede235.." and see what you would end up losing
>> if you were to switch to another branch without saving them first to
>> a named branch.
>
> Thank you for the detailed explanation. I was for some reason under the
> impression that "git status" would show the current HEAD, rather than
> where it was detached. Why I thought that, I don't know.
I wrote not from the documentation but from what I remember about
the discussion that led to final phrasing of the message, and I
strongly suspect that your impression is the sign that our docs are
insufficient. If you can spot where we can improve on, that would
be great.
Thanks.
^ permalink raw reply
* Re: Recovering from a "detached from" HEAD
From: Alyssa Ross @ 2019-01-08 14:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8szw8d8o.fsf@gitster-ct.c.googlers.com>
[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]
> The commit the message shows is meant to indicate where your unnamed
> branch diverged at named branches. Immediately after moving to the
> unnamed branch by detaching the HEAD, the message says "at"; the
> HEAD is pointing directly at the tip of the then-current branch and
> that is where the tip of the unnamed branch is. You can tell from
> that message that you will not lose any commit if you were to check
> out a named branch from that state.
>
> After you make a commit on the unnamed branch, you have something to
> lose if you were to check out a named branch from that state, as the
> detached HEAD is the _only_ thing these new commits you built on top
> of the fork point. Upon seeing "HEAD detached from 9745ede235", you
> could do "git log 9745ede235.." and see what you would end up losing
> if you were to switch to another branch without saving them first to
> a named branch.
Thank you for the detailed explanation. I was for some reason under the
impression that "git status" would show the current HEAD, rather than
where it was detached. Why I thought that, I don't know.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Translation of git manpages
From: Ævar Arnfjörð Bjarmason @ 2019-01-08 13:54 UTC (permalink / raw)
To: Jean-Noël AVILA
Cc: Alexander Shopov, Jordi Mas, Ralf Thielow, Christopher Díaz,
Marco Paolone, Gwan-gyeong Mun, Vasco Almeida,
Dimitriy Ryazantcev, Peter Krefting,
Trần Ngọc Quân, Alessandro Menti, Jiang Xin, git,
Jeff King
In-Reply-To: <1992944.NOdEsaAZKb@cayenne>
On Mon, Jan 07 2019, Jean-Noël AVILA wrote:
> Dear fellow translators,
>
> I'm trying to put up a longstanding project of providing translated manual
> pages for Git. After several experiments, the best choice seemed to be the use
> of po4a[1] to convert the asciidoc[2] sources of git manpages into po files
> that could be processed the same way we are already doing for the translation
> of Git itself.
>
> The text is segmented into paragraphs and structural units (titles, list
> items...) and when translated, they are reinjected into the original text
> structure. Only inline asciidoc formatting marks are passed in segments.
>
> The translation takes place in a dedicated repository[3] . It simpler to not
> meddle in git main workflow while adjusting the translation workflow. If
> everybody is satisfied with it, we can maybe migrate the repo under git
> organization. Now, this repo is standalone with respect to translation content
> source, but a patch has been submitted so that the translated manpages can be
> generated and installed from the main git project[4]. Symmetrically, there's a
> script in the project to pull the manpages source files from the main git repo.
>
> The repository is connected to Weblate[5] if you have collaborators who don't
> know how to process po files and prefer translating in the browser.
>
> The repository is also open to pull-request on the tooling. Let me know if you
> have issues. In any case, the translation work can be reused for any other
> arrangements.
>
> There is already a kernel of translation in French, from my experiments and a
> previous effort of German translation[6] was gettextized. If you have such
> archives for other languages, I'd be happy to integrate them, even if they are
> not up to date.
Thanks. This has come up on list many times before and it's great to
have some movement on it.
I think a way to have early exposure of these to a lot more people would
be to have these on the git-scm.com site. Jeff knows more about the
build process there.
I see the general completion of French & German is at ~10%, but maybe
there's some pages that are fully translated already? We could then have
some UI or git-scm.com that allows you to switch between languages along
with some general overview page per-language.
Are the translations contributed through
https://hosted.weblate.org/projects/git-manpages/translations/ something
that license-wise (Signed-off-by etc.) we'd eventually be able to
upstream in git.git?
It would be great to eventually have something we can build as part of
our build process, so e.g. Debian can have git-man-de similar to
manpages-de now.
^ permalink raw reply
* [PATCH] log: add %S option (like --source) to log --format
From: issac.trotts @ 2019-01-08 13:20 UTC (permalink / raw)
To: git; +Cc: noemi, Issac Trotts, Issac Trotts
From: Issac Trotts <issac.trotts@gmail.com>
Make it possible to write for example
git log --format="%H,%S"
where the %S at the end is a new placeholder that prints out the ref
(tag/branch) for each commit.
Using %d might seem like an alternative but it only shows the ref for the last
commit in the branch.
Signed-off-by: Issac Trotts <issactrotts@google.com>
---
This change is based on a question from Stack Overflow:
https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
---
Documentation/pretty-formats.txt | 2 ++
builtin/log.c | 2 +-
log-tree.c | 1 +
pretty.c | 14 +++++++++
pretty.h | 1 +
t/t4205-log-pretty-formats.sh | 50 ++++++++++++++++++++++++++++++++
t/t6006-rev-list-format.sh | 4 +++
7 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..de6953108 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -134,6 +134,8 @@ The placeholders are:
- '%cI': committer date, strict ISO 8601 format
- '%d': ref names, like the --decorate option of linkgit:git-log[1]
- '%D': ref names without the " (", ")" wrapping.
+- '%S': ref name given on the command line by which the commit was reached
+ (like `git log --source`), only works with `git log`
- '%e': encoding
- '%s': subject
- '%f': sanitized subject line, suitable for a filename
diff --git a/builtin/log.c b/builtin/log.c
index e8e51068b..9deff32b8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
rev->diffopt.filter || rev->diffopt.flags.follow_renames)
rev->always_show_header = 0;
- if (source) {
+ if (source || w.source) {
init_revision_sources(&revision_sources);
rev->sources = &revision_sources;
}
diff --git a/log-tree.c b/log-tree.c
index 10680c139..3cb14256e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -700,6 +700,7 @@ void show_log(struct rev_info *opt)
ctx.color = opt->diffopt.use_color;
ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
ctx.output_encoding = get_log_output_encoding();
+ ctx.rev = opt;
if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
ctx.from_ident = &opt->from_ident;
if (opt->graph)
diff --git a/pretty.c b/pretty.c
index b83a3ecd2..06075d625 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1084,6 +1084,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
struct commit_list *p;
const char *arg;
int ch;
+ char **slot;
/* these are independent of the commit */
switch (placeholder[0]) {
@@ -1194,6 +1195,16 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
load_ref_decorations(NULL, DECORATE_SHORT_REFS);
format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
return 1;
+ case 'S': /* tag/branch like --source */
+ if (c->pretty_ctx->rev == NULL || c->pretty_ctx->rev->sources == NULL) {
+ return 0;
+ }
+ slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
+ if (!(slot && *slot)) {
+ return 0;
+ }
+ strbuf_addstr(sb, *slot);
+ return 1;
case 'g': /* reflog info */
switch(placeholder[1]) {
case 'd': /* reflog selector */
@@ -1498,6 +1509,9 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
case 'N':
w->notes = 1;
break;
+ case 'S':
+ w->source = 1;
+ break;
}
return 0;
}
diff --git a/pretty.h b/pretty.h
index 7359d318a..87ca5dfcb 100644
--- a/pretty.h
+++ b/pretty.h
@@ -60,6 +60,7 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
struct userformat_want {
unsigned notes:1;
+ unsigned source:1;
};
/* Set the flag "w->notes" if there is placeholder %N in "fmt". */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..7df8c3d4e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -621,4 +621,54 @@ test_expect_success 'trailer parsing not fooled by --- line' '
test_cmp expect actual
'
+test_expect_success 'set up %S tests' '
+ git checkout --orphan source-a &&
+ test_commit one &&
+ test_commit two &&
+ git checkout -b source-b HEAD^ &&
+ test_commit three
+'
+
+test_expect_success 'log --format=%S paints branch names' '
+ cat >expect <<-\EOF &&
+ source-b
+ source-a
+ source-b
+ EOF
+ git log --format=%S source-a source-b >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --format=%S paints tag names' '
+ git tag -m tagged source-tag &&
+ cat >expect <<-\EOF &&
+ source-tag
+ source-a
+ source-tag
+ EOF
+ git log --format=%S source-tag source-a >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'log --format=%S paints symmetric ranges' '
+ cat >expect <<-\EOF &&
+ source-b
+ source-a
+ EOF
+ git log --format=%S source-a...source-b >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '%S in git log --format works with other placeholders (part 1)' '
+ git log --format="source-b %h" source-b >expect &&
+ git log --format="%S %h" source-b >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '%S in git log --format works with other placeholders (part 2)' '
+ git log --format="%h source-b" source-b >expect &&
+ git log --format="%h %S" source-b >actual &&
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index ec42c2f77..da113d975 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -185,6 +185,10 @@ test_expect_success 'basic colors' '
test_cmp expect actual
'
+test_expect_success '%S is not a placeholder for rev-list yet' '
+ git rev-list --format="%S" -1 master | grep "%S"
+'
+
test_expect_success 'advanced colors' '
cat >expect <<-EOF &&
commit $head2
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] log: add %S option (like --source) to log --format
From: Issac Trotts @ 2019-01-08 13:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Noemi Mercado, Issac Trotts
In-Reply-To: <xmqqmuoikg6x.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 3, 2019 at 6:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> issac.trotts@gmail.com writes:
>
> > From: Issac Trotts <issac.trotts@gmail.com>
>
> I think you want to have
>
> From: Issac Trotts <issactrotts@google.com>
>
> instead, so that the authorship actually matches your sign-off.
Makes sense. I'll do that.
>
> > - if (source) {
> > + if (source || (rev->pretty_given && (rev->commit_format == CMIT_FMT_USERFORMAT) && w.source)) {
>
> I am not sure why the above is not simply
>
> if (source || w.source) {
>
> but needs to check pretty-given etc. Care to explain why?
Good idea. Your simplified version passes all the tests.
^ permalink raw reply
* Re: [PATCH] blame: add the ability to ignore commits
From: Ævar Arnfjörð Bjarmason @ 2019-01-08 13:12 UTC (permalink / raw)
To: Barret Rhoden; +Cc: git, Stefan Beller, Jeff Smith, Junio C Hamano, Jeff King
In-Reply-To: <20190107213013.231514-1-brho@google.com>
On Mon, Jan 07 2019, Barret Rhoden wrote:
> +static int handle_ignore_file(const char *path, struct string_list *ignores)
> +{
> + FILE *fp = fopen(path, "r");
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (!fp)
> + return -1;
> + while (!strbuf_getline(&sb, fp)) {
> + const char *hash;
> +
> + hash = strchr(sb.buf, '#');
> + if (hash)
> + strbuf_setlen(&sb, hash - sb.buf);
> + strbuf_trim(&sb);
> + if (!sb.len)
> + continue;
> + string_list_append(ignores, sb.buf);
> + }
> + fclose(fp);
> + strbuf_release(&sb);
> + return 0;
> +}
Aside from other comments on this patch that Junio had either you mostly
copy-pasted this from init_skiplist() or you've come up with almost the
same code on your own.
In any case, if we're going to integrate something like this patch let's
split this "parse file with SHA-1s or comments/whitespace" into a
utility function that both this and init_skiplist() can call.
Then we could split up the description for the fsck.skipList config
variable to reference that format, and say that both it and this new
thing should consult those docs for how it's parsed.
^ permalink raw reply
* Re: `git reset` for delete + intent-to-add doesn't reset
From: Duy Nguyen @ 2019-01-08 10:06 UTC (permalink / raw)
To: Anthony Sottile; +Cc: Git Mailing List
In-Reply-To: <CACsJy8Atwp1nQbDaxYSNiDFmVmMG2h88w=dAZWU1SF6JQ18EEw@mail.gmail.com>
On Tue, Jan 8, 2019 at 4:44 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Tue, Jan 8, 2019 at 2:28 PM Anthony Sottile <asottile@umich.edu> wrote:
> >
> > ```
> > git --version
> > rm -rf t
> > git init t
> > cd t
> > touch a
> > git add a
> > git commit -m "add a"
> > git rm a
> > touch a
> > git add --intent-to-add a
> > git status --short
> > git reset -- a
>
> "git reset" without "-- a" does remove intent-to-add status.
No I'm wrong. But it was because I didn't follow your exact steps.
This is quite unique corner case. What happens is "git reset"
internally does "git diff --cached" basically to see what paths need
to update, then reset those paths and as a side effect, intent-to-add
status will be removed. But in this case, "a" in HEAD has empty
content, exactly the same content represented by an intent-to-add
entry. So "git diff --cached" decides there's no changes in "a", no
need to update it (so i-t-a status remains).
This can only happen if a file in HEAD is empty, which is quite
unlikely. This fix could be go through the index list the second time
just for resetting i-t-a status, but I'm not sure if it's worth doing.
--
Duy
^ permalink raw reply
* tg/checkout-no-overlay, was Re: What's cooking in git.git (Jan 2019, #01; Mon, 7)
From: Thomas Gummerer @ 2019-01-08 9:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqq7efg6o0d.fsf@gitster-ct.c.googlers.com>
On Mon, Jan 7, 2019 at 11:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> * tg/checkout-no-overlay (2019-01-02) 8 commits
> - checkout: introduce checkout.overlayMode config
> - checkout: introduce --{,no-}overlay option
> - checkout: factor out mark_cache_entry_for_checkout function
> - checkout: clarify comment
> - read-cache: add invalidate parameter to remove_marked_cache_entries
> - entry: support CE_WT_REMOVE flag in checkout_entry
> - entry: factor out unlink_entry function
> - move worktree tests to t24*
>
> "git checkout --no-overlay" can be used to trigger a new mode of
> checking out paths out of the tree-ish, that allows paths that
> match the pathspec that are in the current index and working tree
> and are not in the tree-ish.
>
> Will merge to 'next'.
Please hold off on merging this to 'next'. There's still an
outstanding comment from Duy and Eric [*1*], that should be
addressed before this goes to next. (Their comments also apply
to the documentation in 8/8). I'll send v3 of the series with
this fixed today.
*1*: <CAPig+cSOyCQZXiG7sJWb12WzzujM-nsqqpt+cFZTFvXB1+-SVQ@mail.gmail.com>
^ permalink raw reply
* Re: `git reset` for delete + intent-to-add doesn't reset
From: Duy Nguyen @ 2019-01-08 9:44 UTC (permalink / raw)
To: Anthony Sottile; +Cc: Git Mailing List
In-Reply-To: <CA+dzEBmVQ8H78uHSPSBy+HqLXZ3xE5=jVdYDfPOVZ_53U8YA-A@mail.gmail.com>
On Tue, Jan 8, 2019 at 2:28 PM Anthony Sottile <asottile@umich.edu> wrote:
>
> ```
> git --version
> rm -rf t
> git init t
> cd t
> touch a
> git add a
> git commit -m "add a"
> git rm a
> touch a
> git add --intent-to-add a
> git status --short
> git reset -- a
"git reset" without "-- a" does remove intent-to-add status. I'll look
into whether "reset -- a" should do the same.
> git status --short
> ```
>
> (the git version below is compiled from
> ecbdaf0899161c067986e9d9d564586d4b045d62)
>
> ```
> $ bash -x t.sh
> + git --version
> git version 2.20.GIT
> + rm -rf t
> + git init t
> Initialized empty Git repository in /tmp/t/t/.git/
> + cd t
> + touch a
> + git add a
> + git commit -m 'add a'
> [master (root-commit) 95a1815] add a
> 1 file changed, 0 insertions(+), 0 deletions(-)
> create mode 100644 a
> + git rm a
> rm 'a'
> + touch a
> + git add --intent-to-add a
> + git status --short
> DA a
> + git reset -- a
> Unstaged changes after reset:
> A a
> + git status --short
> DA a
> ```
>
> Even `git reset --intent-to-add -- a` or `git checkout -- a` don't
> seem to clear the `intent-to-add` state
>
> How do I reset the intent-to-add status in this case?
>
> Anthony
--
Duy
^ permalink raw reply
* Re: `git reset` for delete + intent-to-add doesn't reset
From: Anthony Sottile @ 2019-01-08 7:29 UTC (permalink / raw)
To: Git Mailing List
In-Reply-To: <CA+dzEBmVQ8H78uHSPSBy+HqLXZ3xE5=jVdYDfPOVZ_53U8YA-A@mail.gmail.com>
On Mon, Jan 7, 2019 at 11:25 PM Anthony Sottile <asottile@umich.edu> wrote:
> Even `git reset --intent-to-add -- a` or `git checkout -- a` don't
> seem to clear the `intent-to-add` state
>
> How do I reset the intent-to-add status in this case?
>
> Anthony
Pressed send too quickly, it appears I can use `git rm --cached -- a`
to undo that.
^ permalink raw reply
* Re: Suspicious fetch-pack behaviour
From: Guilhem Bonnefille @ 2019-01-08 7:26 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20190108063456.GA17588@sigill.intra.peff.net>
Le mar. 8 janv. 2019 à 07:34, Jeff King <peff@peff.net> a écrit :
>
> On Thu, Jan 03, 2019 at 10:52:48AM +0100, Guilhem Bonnefille wrote:
>
> > One of my users reported a strange problem: a simple HTTPS clone did
> > not work with Git 1.8.3.1 on RedHat 7.
> > I did many tests and I was not able to understand why his clone don't
> > work while I'm able to do it on other similar host.
> >
> > Nevertheless, we did more investigations. One of them: a raw strace.
> > I discovered two strange behaviours:
> > - fetch-pack closes its standard input and standard output and then
> > tries to print the references on standard input and finaly dies.
> > - git-remote-https does not react to fetch-pack death and continue
> > polling an empty set of FD.
> >
> > [...]
> >
> > 2769 poll(NULL, 0, 1000) = 0 (Timeout)
>
> We actually don't use poll() very much in Git. And poking around the
> v1.8.3.1 source, I do not see any places where remote-https would call
> poll(), and none outside of "git help" and "git credential-cache-daemon"
> that would ever provide a timeout like "1000".
>
> I wonder if this poll is actually being run by libcurl. Is it possible
> to get a backtrace of the looping process with gdb?
Quite hard to debug as it occured on a computer provided by a
customer, far from me...
> I'd also point out that v1.8.3.1 is over 5 years old, and there have
> been quite a few http-related fixes over the years.
Yes, it is an old version, but it is the version provided with RedHat 7.
> There is a good
> chance that if this is a Git bug, it has long since been fixed. Is it
> possible to reproduce with a more modern version of Git?
>
What is surprisingly is that I was unable to reproduce with the same
version on an other computer.
During this time, my user discovered the IT team of the customer
prvide a much more recent version of Git (2.X). With this new version,
the problem was not reproduced.
Thanks for all your investigations.
--
Guilhem BONNEFILLE
-=- JID: guyou@im.apinc.org MSN: guilhem_bonnefille@hotmail.com
-=- mailto:guilhem.bonnefille@gmail.com
-=- http://nathguil.free.fr/
^ permalink raw reply
* `git reset` for delete + intent-to-add doesn't reset
From: Anthony Sottile @ 2019-01-08 7:25 UTC (permalink / raw)
To: Git Mailing List
```
git --version
rm -rf t
git init t
cd t
touch a
git add a
git commit -m "add a"
git rm a
touch a
git add --intent-to-add a
git status --short
git reset -- a
git status --short
```
(the git version below is compiled from
ecbdaf0899161c067986e9d9d564586d4b045d62)
```
$ bash -x t.sh
+ git --version
git version 2.20.GIT
+ rm -rf t
+ git init t
Initialized empty Git repository in /tmp/t/t/.git/
+ cd t
+ touch a
+ git add a
+ git commit -m 'add a'
[master (root-commit) 95a1815] add a
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 a
+ git rm a
rm 'a'
+ touch a
+ git add --intent-to-add a
+ git status --short
DA a
+ git reset -- a
Unstaged changes after reset:
A a
+ git status --short
DA a
```
Even `git reset --intent-to-add -- a` or `git checkout -- a` don't
seem to clear the `intent-to-add` state
How do I reset the intent-to-add status in this case?
Anthony
^ permalink raw reply
* git v2.20.1 hangs on git svn init/fetch (https only)
From: Andry @ 2019-01-08 6:48 UTC (permalink / raw)
To: git
Hi git folks, i've hitted the issues around "git svn" command.
I've installed git from git-scm.com and found that the git hangs on
almost any "git svn" command. In mine case it is "git svn init" and "git svn
fetch".
Example: git svn init "https://svn.code.sf.net/p/blabla/blabla" --stdlayout
What i've found:
* it does stable reproduce under svn https connection, http does work fine
* version 2.20.0 works fine
* git consumes 1 core and does nothing, even has no network traffic
My specs:
* Windows 7 x64
* git x64 v2.20.1 from git-scm.com
Difference from previous working version (see release notes from
git-scm.com):
* updated git up to v2.20.1
* updated curl up to v7.63.0
^ permalink raw reply
* Re: gitk shows local uncommit changes after touch file + reload
From: Jacob Kroon @ 2019-01-08 6:41 UTC (permalink / raw)
To: Philip Oakley, git
In-Reply-To: <2666a093-f9a5-782e-40b3-0cfbce8fe2a3@iee.org>
On 1/7/19 6:41 PM, Philip Oakley wrote:
> On 06/01/2019 22:51, Jacob Kroon wrote:
>> Hi,
>>
>> Not sure if this has already been reported, but I observe this odd
>> behaviour in gitk from master:
>>
>> git status
>> gitk # everything looks good
>> touch <file-under-version-control>
>> gitk # gitk shows "local uncomitted changes" on the file I touched
>> git status
>> gitk # gitk is back to normal again, showing no local uncommitted changes
>>
>> The issue has been discussed on stackoverflow here:
>> https://stackoverflow.com/questions/49990403/after-tar-untar-of-git-repo-gitk-shows-local-uncommitted-changes-not-checked
>>
>>
>> Any chance gitk could be changed to so that it doesn't display the
>> "local uncommitted changes" blob in this case ?
>>
>> Regards Jacob
>
> I believe this is doing the right thing (TM) at the level of
> investigation that gitk uses to determine the status of the files. In
> particular, Git uses the modified time stamp as a surrogate indication
> for detecting that the user has probably edited the file (it's been
> modified at time hhmmss, right?).
>
> Now as I understand it, the full (without limiting options) git status
> command does go and check the content of anything that's potentially
> changed (but it can be costly), and at that point the status command
> simply updates its 'Index' record with the new mtime after noticing that
> nothing had really changed. Meanwhile, gitk, being a continuously
> running GUI avoids the overhead of the git status (though you can force
> it) and does report the mtime change as being a potential file
> modification.
Although gitk it is a continuously running application, I don't expect
it is continuously monitoring the files. If I explicitly tell gitk to
"reload" I'd be perfectly fine with it taking some more time to discard
any false positives in the same way as "git status".
What do you mean by "you can force it", is there some option in gitk I
can set which forces it to do the equivalent of "git status" on reload ?
Thanks,
Jacob
> There is a separate discussion on the git users forum regarding the
> compatibility with other tools that has a similar root cause in the use
> and abuse of mtime as a canary for modification, given that the Git repo
> storage does not record any file times, so will get a (moderately)
> arbitrary mtime & ctime when checked out.
>
^ permalink raw reply
* Re: Suspicious fetch-pack behaviour
From: Jeff King @ 2019-01-08 6:34 UTC (permalink / raw)
To: Guilhem Bonnefille; +Cc: Git List
In-Reply-To: <CA+BUw6jXTt6QGXvdFjRDNqJcij+1hNP5xybUUuGqo3bY0=ueuA@mail.gmail.com>
On Thu, Jan 03, 2019 at 10:52:48AM +0100, Guilhem Bonnefille wrote:
> One of my users reported a strange problem: a simple HTTPS clone did
> not work with Git 1.8.3.1 on RedHat 7.
> I did many tests and I was not able to understand why his clone don't
> work while I'm able to do it on other similar host.
>
> Nevertheless, we did more investigations. One of them: a raw strace.
> I discovered two strange behaviours:
> - fetch-pack closes its standard input and standard output and then
> tries to print the references on standard input and finaly dies.
> - git-remote-https does not react to fetch-pack death and continue
> polling an empty set of FD.
>
> [...]
>
> 2769 poll(NULL, 0, 1000) = 0 (Timeout)
We actually don't use poll() very much in Git. And poking around the
v1.8.3.1 source, I do not see any places where remote-https would call
poll(), and none outside of "git help" and "git credential-cache-daemon"
that would ever provide a timeout like "1000".
I wonder if this poll is actually being run by libcurl. Is it possible
to get a backtrace of the looping process with gdb?
I'd also point out that v1.8.3.1 is over 5 years old, and there have
been quite a few http-related fixes over the years. There is a good
chance that if this is a Git bug, it has long since been fixed. Is it
possible to reproduce with a more modern version of Git?
-Peff
^ permalink raw reply
* [PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error
From: Masaya Suzuki @ 2019-01-08 2:47 UTC (permalink / raw)
To: masayasuzuki; +Cc: git, jrnieder, peff, sunshine
In-Reply-To: <20190108024741.62176-1-masayasuzuki@google.com>
This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This
exercises the code in remote_curl.
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
t/lib-httpd/apache.conf | 1 +
t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 29 insertions(+)
create mode 100755 t/t5581-http-curl-verbose.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..cc4b87507e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
</LocationMatch>
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
ScriptAlias /broken_smart/ broken-smart-http.sh/
ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 0000000000..cd9283eeec
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+ mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+ git config push.default matching &&
+ echo content >file &&
+ git add file &&
+ git commit -m one &&
+ git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+ test_might_fail env GIT_CURL_VERBOSE=1 \
+ git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+ 2>curl_log &&
+ grep "< HTTP/1.1 500 Intentional Breakage" curl_log
+'
+
+stop_httpd
+
+test_done
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related
* [PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR
From: Masaya Suzuki @ 2019-01-08 2:47 UTC (permalink / raw)
To: masayasuzuki; +Cc: git, jrnieder, peff, sunshine
In-Reply-To: <20190108024741.62176-1-masayasuzuki@google.com>
By not setting CURLOPT_FAILONERROR, curl parses the HTTP response
headers even if the response is an error. This makes GIT_CURL_VERBOSE to
show the HTTP headers, which is useful for debugging.
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
remote-curl.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c
index d4673b6e8c..91b39ca098 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
struct rpc_in_data {
struct rpc_state *rpc;
+ struct active_request_slot *slot;
};
/*
@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,
{
size_t size = eltsize * nmemb;
struct rpc_in_data *data = buffer_;
+ long response_code;
+
+ if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+ &response_code) != CURLE_OK)
+ return size;
+ if (response_code >= 300)
+ return size;
if (size)
data->rpc->any_written = 1;
write_or_die(data->rpc->in, ptr, size);
@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
rpc_in_data.rpc = rpc;
+ rpc_in_data.slot = slot;
curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
rpc->any_written = 0;
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related
* [PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION
From: Masaya Suzuki @ 2019-01-08 2:47 UTC (permalink / raw)
To: masayasuzuki; +Cc: git, jrnieder, peff, sunshine
In-Reply-To: <20190108024741.62176-1-masayasuzuki@google.com>
In order to pass more values for rpc_in, define a struct and pass it as
an additional value.
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
remote-curl.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index d8eda2380a..d4673b6e8c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
}
#endif
+struct rpc_in_data {
+ struct rpc_state *rpc;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
static size_t rpc_in(char *ptr, size_t eltsize,
size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
- struct rpc_state *rpc = buffer_;
+ struct rpc_in_data *data = buffer_;
if (size)
- rpc->any_written = 1;
- write_or_die(rpc->in, ptr, size);
+ data->rpc->any_written = 1;
+ write_or_die(data->rpc->in, ptr, size);
return size;
}
@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)
size_t gzip_size = 0;
int err, large_request = 0;
int needs_100_continue = 0;
+ struct rpc_in_data rpc_in_data;
/* Try to load the entire request, if we can fit it into the
* allocated buffer space we can use HTTP/1.0 and avoid the
@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
- curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+ rpc_in_data.rpc = rpc;
+ curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
rpc->any_written = 0;
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related
* [PATCH v3 2/5] http: enable keep_error for HTTP requests
From: Masaya Suzuki @ 2019-01-08 2:47 UTC (permalink / raw)
To: masayasuzuki; +Cc: git, jrnieder, peff, sunshine
In-Reply-To: <20190108024741.62176-1-masayasuzuki@google.com>
curl stops parsing a response when it sees a bad HTTP status code and it
has CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP
headers on error.
keep_error is an option to receive the HTTP response body for those
error responses. By enabling this option, curl will process the HTTP
response headers, and they're shown if GIT_CURL_VERBOSE is set.
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
http.c | 30 +++++++++++++-----------------
http.h | 1 -
remote-curl.c | 1 -
3 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/http.c b/http.c
index 06450da96e..a72db87723 100644
--- a/http.c
+++ b/http.c
@@ -1876,8 +1876,6 @@ static int http_request(const char *url,
strbuf_addstr(&buf, "Pragma:");
if (options && options->no_cache)
strbuf_addstr(&buf, " no-cache");
- if (options && options->keep_error)
- curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
if (options && options->initial_request &&
http_follow_config == HTTP_FOLLOW_INITIAL)
curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -1895,6 +1893,7 @@ static int http_request(const char *url,
curl_easy_setopt(slot->curl, CURLOPT_URL, url);
curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
+ curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
ret = run_one_slot(slot, &results);
@@ -1989,22 +1988,19 @@ static int http_request_reauth(const char *url,
return ret;
/*
- * If we are using KEEP_ERROR, the previous request may have
- * put cruft into our output stream; we should clear it out before
- * making our next request.
+ * The previous request may have put cruft into our output stream; we
+ * should clear it out before making our next request.
*/
- if (options && options->keep_error) {
- switch (target) {
- case HTTP_REQUEST_STRBUF:
- strbuf_reset(result);
- break;
- case HTTP_REQUEST_FILE:
- fflush(result);
- ftruncate(fileno(result), 0);
- break;
- default:
- BUG("Unknown http_request target");
- }
+ switch (target) {
+ case HTTP_REQUEST_STRBUF:
+ strbuf_reset(result);
+ break;
+ case HTTP_REQUEST_FILE:
+ fflush(result);
+ ftruncate(fileno(result), 0);
+ break;
+ default:
+ BUG("Unknown http_request target");
}
credential_fill(&http_auth);
diff --git a/http.h b/http.h
index d305ca1dc7..eebf40688c 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
- keep_error:1,
initial_request:1;
/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcdc..d8eda2380a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
http_options.extra_headers = &extra_headers;
http_options.initial_request = 1;
http_options.no_cache = 1;
- http_options.keep_error = 1;
http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related
* [PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR
From: Masaya Suzuki @ 2019-01-08 2:47 UTC (permalink / raw)
To: masayasuzuki; +Cc: git, jrnieder, peff, sunshine
In-Reply-To: <20190108024741.62176-1-masayasuzuki@google.com>
HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order
to make HTTP_KEEP_ERROR enabled for all requests, file handles need to
be supported.
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
http.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/http.c b/http.c
index 0b6807cef9..06450da96e 100644
--- a/http.c
+++ b/http.c
@@ -1991,16 +1991,19 @@ static int http_request_reauth(const char *url,
/*
* If we are using KEEP_ERROR, the previous request may have
* put cruft into our output stream; we should clear it out before
- * making our next request. We only know how to do this for
- * the strbuf case, but that is enough to satisfy current callers.
+ * making our next request.
*/
if (options && options->keep_error) {
switch (target) {
case HTTP_REQUEST_STRBUF:
strbuf_reset(result);
break;
+ case HTTP_REQUEST_FILE:
+ fflush(result);
+ ftruncate(fileno(result), 0);
+ break;
default:
- BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+ BUG("Unknown http_request target");
}
}
--
2.20.1.97.g81188d93c3-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox