Re: [PATCH v2 0/7] t5520: tests for --[no-]autostash option
Mehul Jain writes: > -test_rebase_autostash () { > +test_pull_autostash () { > git reset --hard before-rebase && > echo dirty >new_file && > git add new_file && > - git pull --rebase --autostash . copy && > + git pull $@ . copy && Not strictly needed here, but I'd write "$@" (with the double-quotes) which is the robust way to say "transmit all my arguments without whitespace interpretation". I don't mind for this patch since there's no whitespace to interpret, but some people (sysadmins ;-) ) have the bad habit of writting $@, $* or "$*" in wrapper scripts and it breaks when you call them with spaces so it's better to take good habits IHMO. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 is an efficient way to get all blobs / trees that have notes attached?
On Fri, Apr 1, 2016 at 2:16 PM, Johan Herland wrote: >> 3) Recursively list all blobs / trees (git-ls-tree) and look whether an >> object's hash is conatined in our table to get its notes. >> >> In particular 3) could be expensive for repos with a lot of files as we're >> looking at all of them just to see whether they have notes attached. > > In (3), why would you need to search through _all_ blobs/trees? Would > it not be cheaper to simply query the object type of each annotated > object from (2)? I.e. something like: > > for notes_ref in $(git for-each-ref refs/notes | cut -c 49-) > do > echo "--- $notes_ref ---" > for annotated_obj in $(git notes --ref=$notes_ref list | cut -c 41-) > do > type=$(git cat-file -t "$annotated_obj") > if test "$type" != "commit" > then > echo "$annotated_obj: $type" > fi > done > done Thanks for the idea. The problem is that I do want to list the notes by path of the object they belong to. As a blob could potentially belong to more than one path (copies of files in the repo), I do not see another way of getting that information other than iterating over all blobs and checking what path(s) they belong to. -- Sebastian Schuberth -- 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 v3 13/16] ref-filter: allow porcelain to translate messages in the output
Karthik Nayak writes: > cc'ing Matthieu since this patch was initially written by him. > > On Thu, Mar 31, 2016 at 3:28 AM, Junio C Hamano wrote: >> Karthik Nayak writes: >> >>> +static struct ref_msg { >>> + const char *gone; >>> + const char *ahead; >>> + const char *behind; >>> + const char *ahead_behind; >>> +} msgs = { >>> + "gone", >>> + "ahead %d", >>> + "behind %d", >>> + "ahead %d, behind %d" >>> +}; >>> + >>> +void setup_ref_filter_porcelain_msg(void) >>> +{ >>> + msgs.gone = _("gone"); >>> + msgs.ahead = _("ahead %d"); >>> + msgs.behind = _("behind %d"); >>> + msgs.ahead_behind = _("ahead %d, behind %d"); >>> +} >> >> I do not think this patch is wrong, but I wonder if it would be >> sufficient to have a single bit in file-scope static variable and >> flip it in setup_ref_filter_porcelain_msg(). I.e. >> >> static int use_porcelain_msg; /* false by default */ >> >> void setup_ref_filter_porcelain_msg(void) >> { >> use_porcelain_msg = 1; >> } >> >> static const char *P_(const char *msg) >> { >> return use_porcelain_msg ? _(msg) : msg; >> } >> >> and then mark the message up perhaps like so: >> >> - *s = xstrdup("gone"); >> + *s = xstrdup(P_("gone")); >> >> which may make things much simpler. ... but less evolutive. The non-translatable strings also need to be cast in stone, while the translatable ones may be subject to future improvements/tweaks. If they are already duplicated in the code, then updating one won't change the other, but factoring them means that the porcelain message can't easily be changed without modifying the plumbing one. I'm not sure how important it is in this case, but it was in the case of setup_unpack_trees_porcelain which I took inspiration from when we discussed this (actually, in setup_unpack_trees_porcelain, there's isn't any translation even in porcelain). Note that this can be worked around later by adding another function like static const char *get_message(const char *porcelain, const char *plumbing) { return use_porcelain_msg ? porcelain : plumbing; } to be called with get_message(_("this ref was gone"), "gone") or so. >> We'd need to update Makefile to recognize X_() as another keyword; (I guess you meant P_, not X_) >> I'd think something like this should do: >> >> XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \ >> ---keyword=_ --keyword=N_ --keyword="Q_:1,2" >> +--keyword=_ --keyword=N_ --keyword=P_ --keyword="Q_:1,2" I'm a bit reluctant to modifying the Makefile for something not really build-related. > I'm not totally knowledgeable about how porcelain works, although > Matthieu did give me a > brief explanation. I guess it'd better to hear his thoughts on this. In summary: both would work. No strong opinion from me, but I slightly prefer the version in the patch (i.e. the one I suggested IIRC) to Junio's version. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] doc: clarify that notes can be attached to any type of stored object
On Fri, Apr 1, 2016 at 5:31 PM, Junio C Hamano wrote: > Sebastian Schuberth writes: > >> Signed-off-by: Sebastian Schuberth >> --- >> Documentation/git-notes.txt | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt >> index 8de3499..5375d98 100644 >> --- a/Documentation/git-notes.txt >> +++ b/Documentation/git-notes.txt >> @@ -234,8 +234,9 @@ which operation triggered the update, and the commit >> authorship is >> determined according to the usual rules (see linkgit:git-commit[1]). >> These details may change in the future. >> >> -It is also permitted for a notes ref to point directly to a tree >> -object, in which case the history of the notes can be read with >> +It is also permitted for a notes ref to point to any other object in >> +the object store besides commit objects, that is annotated tags, blobs >> +or trees. For the latter, the history of the notes can be read with >> `git log -p -g `. > > I do not think this is correct place to patch. The original is not > talking about what objects can have notes attached at all. What it > explains is this. Thanks for the explanation, I was indeed misreading this. I'll try to clarify this section then, too. In order to do so, I think we should mention how to actually create a that directly points to a tree instead of a commit for the history of notes. Would you have an example how to do that? -- Sebastian Schuberth -- 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
[ANNOUNCE] Git for Windows 2.8.1
Dear Git users, It is my pleasure to announce that Git for Windows 2.8.1 is available from: https://git-for-windows.github.io/ Changes since Git for Windows v2.8.0 (March 29th 2016) New Features ??? Comes with Git v2.8.1. ??? The Git for Windows project updated its contributor guidelines to the Contributor Covenant 1.4. Bug Fixes ??? Git's default editor (vim) is no longer freezing in CMD windows. ??? GIT_SSH (and other executable paths that Git wants to spawn) can now contain spaces. Filename | SHA-256 | --- Git-2.8.1-64-bit.exe | 5e5283990cc91d1e9bd0858f8411e7d0afb70ce26e23680252fb4869288c7cfb Git-2.8.1-32-bit.exe | 17418c2e507243b9c98db161e9e5e8041d958b93ce6078530569b8edaec6b8a4 PortableGit-2.8.1-64-bit.7z.exe | dc9d971156cf3b6853bc0c1ad0ca76f1d2c24478cca80036919f12fe46acd64e PortableGit-2.8.1-32-bit.7z.exe | 0b6efaaeb4b127edb3a534261b2c9175bd86ee8683dff6e12ccb194e6abb990e Git-2.8.1-64-bit.tar.bz2 | 3ebc00b96607174fffb35cf6d96b3b3246aefa504a4bd30375182fea6ab64bde Git-2.8.1-32-bit.tar.bz2 | 9e754d83190ba154f012d8eaa7433d29517f7c85fff229d4fb62e6418acf8d41 Ciao, Johannes
Re: [PATCH] doc: clarify that notes can be attached to any type of stored object
On Fri, Apr 1, 2016 at 6:47 PM, Junio C Hamano wrote: > DESCRIPTION > --- > Adds, removes, or reads notes attached to objects, without touching > the objects themselves. > > This says "notes attached to objects" and "the objects themselves". > They do not limit the target of attaching a note to "commits". > So this may be the place to add " Note that notes can be attached > to any kind of objects, not limited to commits" or something, if > we really wanted to. Done, I'll send a patch shortly. But I wanted to list the supported object types explicitly, in particular as many guide in the net are unclear that only annotated tags are object, and lightweight ones are not. > Notes can also be added to patches prepared with `git format-patch` by > using the `--notes` option. Such notes are added as a patch commentary > after a three dash separator line. > > This paragraph _might_ be confusing to new readers. The "added to" > sounds as if you are attaching a note to a non-object which is a > patch. But this "add" is about "inserting the contents of the note > attached to the commit being formatted" and corresponds to "can be > shown by" in the previous paragraph. We may want to avoid the verb > "add" when talking about the use of data stored in an existing note > to somewhere else like this. Done. > add:: > Add notes for a given object (defaults to HEAD). Abort if the > > And this "Add notes for " should probably be reworded to "Attach > notes to" to match the first sentence in the description. After a bit of thinking, I don't believe we should do this. All subcommand docs start with the verb the subcommand is named after. In that sense making the "add" docs start with "Attach" would be inconsistent and probably raise the question why the subcommand is not called "attach" after all. Also, in the description it says "Adds, removes, or reads notes attached to objects", so it includes "[...] removes [...] notes attached to objects", and if you read it like this the word "attach" is not specific to the "add" subcommand. So I left this as-is in my patch. -- Sebastian Schuberth -- 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] doc: Clarify which objects notes can be attached to
Explicitly name the supported object types, and ensure patches cannot be misinterpreted as non-objects that can have notes attached. Signed-off-by: Sebastian Schuberth --- Documentation/git-notes.txt | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt index 8de3499..101e6ba 100644 --- a/Documentation/git-notes.txt +++ b/Documentation/git-notes.txt @@ -25,7 +25,8 @@ SYNOPSIS DESCRIPTION --- Adds, removes, or reads notes attached to objects, without touching -the objects themselves. +the objects themselves. Supported objects are commits, blobs, trees +and annotated tags. By default, notes are saved to and read from `refs/notes/commits`, but this default can be overridden. See the OPTIONS, CONFIGURATION, and @@ -39,9 +40,9 @@ message stored in the commit object, the notes are indented like the message, after an unindented line saying "Notes ():" (or "Notes:" for `refs/notes/commits`). -Notes can also be added to patches prepared with `git format-patch` by -using the `--notes` option. Such notes are added as a patch commentary -after a three dash separator line. +Notes contents can also be included in patches prepared with +`git format-patch` by using the `--notes` option. Such notes are added +as a patch commentary after a three dash separator line. To change which notes are shown by 'git log', see the "notes.displayRef" configuration in linkgit:git-log[1]. -- 2.8.0.windows.1 -- 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/2] http.c: implements the GIT_CURL_DEBUG environment variable
2016-04-01 17:03 GMT+02:00 Ramsay Jones : > > > On 01/04/16 11:44, Elia Pinto wrote: >> Implements the GIT_CURL_DEBUG environment variable to allow a greater >> degree of detail of GIT_CURL_VERBOSE, in particular the complete >> transport header and all the data payload exchanged. >> It might be useful if a particular situation could require a more >> thorough debugging analysis. >> >> Signed-off-by: Elia Pinto >> --- >> http.c | 97 >> +- >> 1 file changed, 96 insertions(+), 1 deletion(-) >> >> diff --git a/http.c b/http.c >> index dfc53c1..079779d 100644 >> --- a/http.c >> +++ b/http.c > [snip] > >> @@ -532,7 +623,11 @@ static CURL *get_curl_handle(void) >> "your curl version is too old (>= 7.19.4)"); >> #endif >> >> - if (getenv("GIT_CURL_VERBOSE")) >> + if (getenv("GIT_CURL_DEBUG")) { >> + curl_easy_setopt(result, CURLOPT_VERBOSE, 1); >> + curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace); >> + curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL); >> + } else if (getenv("GIT_CURL_VERBOSE")) >> curl_easy_setopt(result, CURLOPT_VERBOSE, 1); >> >> curl_easy_setopt(result, CURLOPT_USERAGENT, >> > > Again, maybe something like: > > if (getenv("GIT_CURL_VERBOSE")) { > curl_easy_setopt(result, CURLOPT_VERBOSE, 1); > if (getenv("GIT_CURL_DEBUG")) > curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace); > } > > Although that does make GIT_CURL_DEBUG subordinate to GIT_CURL_VERBOSE. > So, that may not be desired ... Thank you. But actually it is not a desirable change, for me almost, I prefer that the two definitions are independent. And it is true the opposite: if it is defined the curl DEBUG flag then it is implicitly defined the curl VERBOSE flag, because it is a prerequisite of the DEBUG functionality. Thanks in any case for the review. Best > > ATB, > Ramsay Jones > > -- 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/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable
2016-04-01 16:56 GMT+02:00 Ramsay Jones : > > > On 01/04/16 11:44, Elia Pinto wrote: >> Implements the GIT_CURL_DEBUG environment variable to allow a greater >> degree of detail of GIT_CURL_VERBOSE, in particular the complete >> transport header and all the data payload exchanged. >> It might be useful if a particular situation could require a more >> thorough debugging analysis. >> >> Signed-off-by: Elia Pinto >> --- >> imap-send.c | 99 >> +++-- >> 1 file changed, 97 insertions(+), 2 deletions(-) >> >> diff --git a/imap-send.c b/imap-send.c >> index 4d3b773..cf79e7f 100644 >> --- a/imap-send.c >> +++ b/imap-send.c > [snip] > >> @@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc) >> >> curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L); >> >> - if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) >> - curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); >> + if (0 < verbosity ) It was already so in the previous code, I have not changed it. If it is a desirable change it would take another patch > > previously it was sufficient to set GIT_CURL_VERBOSE, now I have to > set verbosity too? > > [Does it matter that you change "1L" to "1" in the curl_easy_setopt() > call? In http.c (line 567) it also uses "1", but ...] > >> + if (getenv("GIT_CURL_DEBUG")) { >> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1); >> + curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, >> curl_trace); >> + } else if (getenv("GIT_CURL_VERBOSE")) >> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1); >> + OK. >> >> return curl; >> } >> > > I would have expected something like: > > if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) { > curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L); > if (getenv("GIT_CURL_DEBUG")) > curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace); > } > No. Thank you. But actually it is not a desirable change > Hope That Helps. > > ATB, > Ramsay Jones > > > -- 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 v12 1/5] t0040-test-parse-options.sh: fix style issues
On Mon, Apr 4, 2016 at 2:30 AM, Eric Sunshine wrote: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> Signed-off-by: Pranit Bauva >> --- >> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh >> @@ -7,7 +7,7 @@ test_description='our own option parser' >> >> . ./test-lib.sh >> >> -cat > expect << EOF >> +cat >expect <> usage: test-parse-options >> --yes get a boolean > > It would be better to use <<\EOF for this one to make it clear that no > interpolation is desired in the heredoc. > >> @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' ' >> check magnitude: 3221225472 -m 3g >> ' >> >> -cat > expect << EOF >> +cat >expect < > Ditto: <<\EOF > > Same applies to all similar heredocs in subsequent tests where > interpolation is not desired. > >> boolean: 2 >> integer: 1729 >> magnitude: 16384 >> @@ -310,12 +310,12 @@ arg 00: --quux >> EOF >> >> test_expect_success 'keep some options as arguments' ' >> - test-parse-options --quux > output 2> output.err && >> + test-parse-options --quux >output 2>output.err && >> test_must_be_empty output.err && >> -test_cmp expect output >> + test_cmp expect output > > Okay, this is a whitespace change (spaces -> tab). > >> ' >> @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not >> ambiguous' ' >> -cat >>expect <<'EOF' >> +cat >>expect < > This is not a desirable change. This heredoc does not require > interpolation, so you don't want to turn it into a form which does > interpolate. For style consistency, therefore, you should change 'EOF' > to \EOF. > >> list: foo >> list: bar >> list: baz Okay I will do the change. I was previously unaware about the use of '\' before EOF. I googled it now. But I am still confused about its use in this scenario. Upto what I understood, it is used where you want to expand a variable, substitute a command, arithmethic expansion. The use of '\' in the tests I have changed in v12 wrt 11 is understood by me as you want to remove the use of escape sequences which is justified. But this seems a bit vague. Is it some convention in git? -- 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 v3 4/4] tag: use pgp_verify_function in tag -v call
On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote: > > As a side note, it might actually be an improvement for pgp_verify_tag > > to take a sha1 (so that git-tag is sure that it is verifying the same > > object that it is printing), but that refactoring should probably come > > separately, I think. > > Just to be sure, this refactoring is something we should still include > in this set of patches, right? I think that otherwise we'd lose the > desambigutaion that git tag -v does in this patch. I think it can be part of this series, but doesn't have to be. As I understand it, the current code is just handing the name to the `git verify-tag` process, so if we continue to do so, that would be OK. > I also think that most of the rippling is gone if we use and adaptor as > you suggested. Should I add a patch on top of this to support a sha1 as > part for gpg_verify_tag()? Yes, though I'd generally advise against a function taking either a name or a sha1, and ignoring the other option. That often leads to confusing interfaces for the callers. Instead, perhaps just take the sha1, and let the caller do the get_sha1() themselves. Or possibly provide two functions, one of which is a convenience to translate the name to sha1 and then call the other. -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 v3 2/4] t/t7030-verify-tag.sh: Adds validation for multiple tags
On Sun, Apr 03, 2016 at 09:38:34PM -0400, Eric Sunshine wrote: > I think Peff meant that a simple grep would suffice; no need for > test_i18ngrep. In other words (reproducing Peff's example), something > like this: > > tags="fourth-signed sixth-signed seventh-signed" && > for i in $tags; do > git verify-tag -v --raw $i || return 1 > done >expect.stdout 2>expect.stderr.1 && > grep GOODSIG expect.stderr && > git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && > grep GOODSIG actual.stderr && > test_cmp expect.stdout actual.stdout && > test_cmp expect.stderr actual.stderr Yep, though I think I would actually have done: grep '^.GNUPG:.' ... or something to just catch all of the gnupg output. -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 1/4] config --show-origin: report paths with forward slashes
Johannes Sixt writes: > Although I am convinced that the change is not necessary for > correctness, I can buy the justification that we should produce forward > slashes for consistency. There are a number of occasions where we > present paths to the user, and we do show forward-slashes in all cases > that I found. We should keep the commit. > > But then let's do this: Sounds like a plan; even though I am mildly against adding more platform specific #ifdef to files outside compat/, this patch does not. Dscho? > > 8< > Subject: [PATCH] Windows: shorten code by re-using convert_slashes() > > Make a few more spots more readable by using the recently introduced, > Windows-specific helper. > > Signed-off-by: Johannes Sixt > --- > abspath.c | 5 + > compat/mingw.c | 9 ++--- > 2 files changed, 3 insertions(+), 11 deletions(-) > > diff --git a/abspath.c b/abspath.c > index 5edb4e7..2825de8 100644 > --- a/abspath.c > +++ b/abspath.c > @@ -167,7 +167,6 @@ const char *prefix_filename(const char *pfx, int pfx_len, > const char *arg) > strbuf_add(&path, pfx, pfx_len); > strbuf_addstr(&path, arg); > #else > - char *p; > /* don't add prefix to absolute paths, but still replace '\' by '/' */ > strbuf_reset(&path); > if (is_absolute_path(arg)) > @@ -175,9 +174,7 @@ const char *prefix_filename(const char *pfx, int pfx_len, > const char *arg) > else if (pfx_len) > strbuf_add(&path, pfx, pfx_len); > strbuf_addstr(&path, arg); > - for (p = path.buf + pfx_len; *p; p++) > - if (*p == '\\') > - *p = '/'; > + convert_slashes(path.buf + pfx_len); > #endif > return path.buf; > } > diff --git a/compat/mingw.c b/compat/mingw.c > index 54c82ec..0413d5c 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -763,15 +763,12 @@ struct tm *localtime_r(const time_t *timep, struct tm > *result) > > char *mingw_getcwd(char *pointer, int len) > { > - int i; > wchar_t wpointer[MAX_PATH]; > if (!_wgetcwd(wpointer, ARRAY_SIZE(wpointer))) > return NULL; > if (xwcstoutf(pointer, wpointer, len) < 0) > return NULL; > - for (i = 0; pointer[i]; i++) > - if (pointer[i] == '\\') > - pointer[i] = '/'; > + convert_slashes(pointer); > return pointer; > } > > @@ -2112,9 +2109,7 @@ static void setup_windows_environment() >* executable (by not mistaking the dir separators >* for escape characters). >*/ > - for (; *tmp; tmp++) > - if (*tmp == '\\') > - *tmp = '/'; > + convert_slashes(tmp); > } > > /* simulate TERM to enable auto-color (see color.c) */ -- 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/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable
2016-04-01 17:35 GMT+02:00 Junio C Hamano : > Elia Pinto writes: > >> Implements the GIT_CURL_DEBUG environment variable to allow a greater >> degree of detail of GIT_CURL_VERBOSE, in particular the complete >> transport header and all the data payload exchanged. >> It might be useful if a particular situation could require a more >> thorough debugging analysis. > > My impression is that using GIT_TRACE_* is the more mainstream > trend, and it may be beneficial to work any new debugging aid like > this one to fit within that mechanism. I thought about it, and I agree with you. The idea could be - Call the variable GIT_TRACE_CURL_DEBUG instead - Add the new GIT_TRACE_CURL_VERBOSE variable, keeping GIT_CURL_VERBOSE for compatibility - Documenting these GIT_TRACE_CURL_XXX variables (GIT_CURL_VERBOSE it is not even documented i think) - perhaps use the git trace api in doing these new patches Look reasonable? It seems reasonable? I'd like your own opinion Thank you for your suggestion > > I am not saying new GIT_*_DEBUG is wrong. I just wanted to make > sure you have considered doing this as a new trace in GIT_TRACE_* > family and rejected that apporach with a very good reason, in > which case that rationale deserves to be in the log message. -- 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 v3 13/16] ref-filter: allow porcelain to translate messages in the output
Matthieu Moy writes: > I'm not sure how important it is in this case, but it was in the case of > setup_unpack_trees_porcelain which I took inspiration from when we > discussed this (actually, in setup_unpack_trees_porcelain, there's isn't > any translation even in porcelain). OK, so paraphrase: In the most general case, we might want to have one code to issue different messages between plumbing and Porcelain; further, for Porcelain messages may or may not want to be translated. But I suspect that all Porcelain messages should be translatable in general, so there probably is a room for simplification. The single macro P_() approach was done without knowing that this codepath wanted the distinction between the plumbing and Porcelain. > Note that this can be worked around later by adding another function like > > static const char *get_message(const char *porcelain, const char > *plumbing) > { > return use_porcelain_msg ? porcelain : plumbing; > } > > to be called with get_message(_("this ref was gone"), "gone") or so. Yes, I think that would be a way to do this properly. And we do not have a separate "here is the list of all translatable messages" table, which is a big plus. > In summary: both would work. No strong opinion from me, but I slightly > prefer the version in the patch (i.e. the one I suggested IIRC) to > Junio's version. Yup. -- 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] doc: clarify that notes can be attached to any type of stored object
Sebastian Schuberth writes: > Done, I'll send a patch shortly. But I wanted to list the supported > object types explicitly, in particular as many guide in the net are > unclear that only annotated tags are object, and lightweight ones are > not. I'd really hate to see an explicit list of what object types there are, as it is one more place we'd need to update if we ever add new object types (and we are unlikely to do so anytime soon, which makes it even more likely that we would forget there is this explicit list we'd need to update). - Adds, removes, or reads notes attached to objects, without touching - the objects themselves. + Adds, removes, or reads notes attached to any object (not limited + to commit objects), without touching the objects themselves. should be sufficient, no? -- 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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true
On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine wrote: > On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain wrote: >> In test_autostash() there's a line >> >> echo test_cmp_rev HEAD^ copy && >> >> Originally it should have been >> >> test_cmp_rev HEAD^ copy && >> >> but this raise following error while testing >> >> ./t5520-pull.sh: 684: eval: diff -u: not found > > This is caused by the custom IFS=',\t=' which is still in effect when > test_cmp_rev() is invoked. You need to restore IFS within the loop > itself. Thanks for pointing it out. I made a mistake by not considering the consequences of setting IFS=',\t='. I tried it out again and this time all tests passed perfectly. I should been more careful in the first place while playing with IFS, but instead of that, I kept on thinking that there is some other problem with the script which lead to me making foolish changes in the script like putting an echo before "test_cmp_rev ...". It was nice of you to take out some time and point it out :) Also now that I have sent v2[1] of this series, which goes in different direction as far as implementation of these tests are concerned. I think the script now is useless (but I learned a bit about shell while writing it). Thanks, Mehul -- 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/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable
Elia Pinto writes: >> My impression is that using GIT_TRACE_* is the more mainstream >> trend, and it may be beneficial to work any new debugging aid like >> this one to fit within that mechanism. > > I thought about it, and I agree with you. The idea could be > > - Call the variable GIT_TRACE_CURL_DEBUG instead I think GIT_TRACE_CURL_DEBUG is overly verbose; tracing by definition is a debugging aid. > - Add the new GIT_TRACE_CURL_VERBOSE variable, keeping > GIT_CURL_VERBOSE for compatibility I do not care too deeply either way. - GIT_CURL_VERBOSE can stay the same as-is and show its output to whatever output channel it spits things out. - Or it can be a synonym for GIT_TRACE_CURL=2 (as I understand that the VERBOSE output goes to the standard error stream) If you want tracing as debugging aid and existing CURL_VERBOSE orthogonal, it would probably make more sense to go the former route, not linking this new "DEBUG" thing with the existing "VERBOSE" thing. > - Documenting these GIT_TRACE_CURL_XXX variables (GIT_CURL_VERBOSE > it is not even documented i think) If we decide to leave them untangled, this is not necessary. > - perhaps use the git trace api in doing these new patches > > Look reasonable? It seems reasonable? I'd like your own opinion Not really sensible as long as you have that "perhaps" in the list. Something that does not use the trace API shouldn't pretend to by using GIT_TRACE_* names. GIT_TRACE_CURL could be your new thing and would decide where to show its output by using the GIT_TRACE_* api. -- 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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true
Mehul Jain writes: > On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine > wrote: >> On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain wrote: >>> In test_autostash() there's a line >>> >>> echo test_cmp_rev HEAD^ copy && >>> >>> Originally it should have been >>> >>> test_cmp_rev HEAD^ copy && >>> >>> but this raise following error while testing >>> >>> ./t5520-pull.sh: 684: eval: diff -u: not found >> >> This is caused by the custom IFS=',\t=' which is still in effect when >> test_cmp_rev() is invoked. You need to restore IFS within the loop >> itself. > > Thanks for pointing it out. I made a mistake by not considering > the consequences of setting IFS=',\t='. I tried it out again and > this time all tests passed perfectly. I think it would be much simpler to drop the loop, and write instead something like (untested): test_autostash () { expect="$1" cmd="$2" config_variable="$3" value="$4" test_expect_success "$cmd, $config_variable=$value" ' if [ "$value" = "" ]; then test_unconfig $config_variable else test_config $config_variable $value fi && git reset --hard before-rebase && echo dirty >new_file && git add new_file && if [ $expect = "ok" ]; then git pull '$cmd' . copy && test_cmp_rev HEAD^ copy && test "$(cat new_file)" = dirty && test "$(cat file)" = "modified again" else test_must_fail git pull '$cmd' . copy 2>err && test_i18ngrep "uncommitted changes." err fi ' } test_autostash ok --rebase rebase.autostash=true test_autostash ok '--rebase --autostash' rebase.autostash=true test_autostash ok '--rebase --autostash' rebase.autostash=false test_autostash ok '--rebase --autostash' rebase.autostash= test_autostash err '--rebase --no-autostash' rebase.autostash=true test_autostash err '--rebase --no-autostash' rebase.autostash=false test_autostash err '--rebase --no-autostash' rebase.autostash= test_autostash ok --autostash pull.rebase=true test_autostash err --no-autostash pull.rebase=true -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 0/7] t5520: tests for --[no-]autostash option
On Mon, Apr 4, 2016 at 1:01 PM, Matthieu Moy wrote: > Mehul Jain writes: > >> -test_rebase_autostash () { >> +test_pull_autostash () { >> git reset --hard before-rebase && >> echo dirty >new_file && >> git add new_file && >> - git pull --rebase --autostash . copy && >> + git pull $@ . copy && > > Not strictly needed here, but I'd write "$@" (with the double-quotes) > which is the robust way to say "transmit all my arguments without > whitespace interpretation". > > I don't mind for this patch since there's no whitespace to interpret, > but some people (sysadmins ;-) ) have the bad habit of writting $@, $* > or "$*" in wrapper scripts and it breaks when you call them with spaces > so it's better to take good habits IHMO. Thanks for the suggestion, I will remember it. I'm relatively new to shell and therefore didn't know much about the difference between "$@" and $@, $*, "$*". Now that I have read[1][2] about it, it won't be repeated. [1]: http://unix.stackexchange.com/questions/41571/what-is-the-difference-between-and/94200#94200 [2]: http://unix.stackexchange.com/questions/131766/why-does-my-shell-script-choke-on-whitespace-or-other-special-characters Thanks, Mehul -- 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 0/7] t5520: tests for --[no-]autostash option
Matthieu Moy writes: > Mehul Jain writes: > >> -test_rebase_autostash () { >> +test_pull_autostash () { >> git reset --hard before-rebase && >> echo dirty >new_file && >> git add new_file && >> -git pull --rebase --autostash . copy && >> +git pull $@ . copy && > > Not strictly needed here, but I'd write "$@" (with the double-quotes) > which is the robust way to say "transmit all my arguments without > whitespace interpretation". Yes, these should be "$@" (with the double-quotes). > I don't mind for this patch since there's no whitespace to interpret, > but some people (sysadmins ;-) ) have the bad habit of writting $@, $* > or "$*" in wrapper scripts and it breaks when you call them with spaces > so it's better to take good habits IHMO. -- 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 0/7] t5520: tests for --[no-]autostash option
On Mon, Apr 4, 2016 at 10:30 PM, Junio C Hamano wrote: > Matthieu Moy writes: > >> Mehul Jain writes: >> >>> -test_rebase_autostash () { >>> +test_pull_autostash () { >>> git reset --hard before-rebase && >>> echo dirty >new_file && >>> git add new_file && >>> -git pull --rebase --autostash . copy && >>> +git pull $@ . copy && >> >> Not strictly needed here, but I'd write "$@" (with the double-quotes) >> which is the robust way to say "transmit all my arguments without >> whitespace interpretation". > > Yes, these should be "$@" (with the double-quotes). I will do a re-roll then. Thanks, Mehul -- 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] comment for a long #ifdef
Signed-off-by: Ivan Pozdeev --- compat/poll/poll.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compat/poll/poll.c b/compat/poll/poll.c index db4e03e..5eb0280 100644 --- a/compat/poll/poll.c +++ b/compat/poll/poll.c @@ -441,7 +441,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout) } return rc; -#else +#else /* #ifndef WIN32_NATIVE */ static struct timeval tv0; static HANDLE hEvent; WSANETWORKEVENTS ev; @@ -622,5 +622,5 @@ restart: } return rc; -#endif +#endif /* #ifndef WIN32_NATIVE */ } -- 1.9.5.msysgit.1 -- Regards, Ivan -- 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 v12 1/5] t0040-test-parse-options.sh: fix style issues
On Mon, Apr 4, 2016 at 8:45 AM, Pranit Bauva wrote: > Okay I will do the change. I was previously unaware about the use of > '\' before EOF. I googled it now. But I am still confused about its > use in this scenario. Upto what I understood, it is used where you > want to expand a variable, substitute a command, arithmethic > expansion. The use of '\' in the tests I have changed in v12 wrt 11 is > understood by me as you want to remove the use of escape sequences > which is justified. But this seems a bit vague. Is it some convention > in git? Both 'EOF' and \EOF suppress interpolation and other transformations in the heredoc content which would otherwise occur with plain EOF. The 'EOF' form is well documented; \EOF not so much, but is used heavily in git test scripts. So: x=flormp echo
Re: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options
Ping? On Sun, Mar 27, 2016 at 5:26 PM, Eric Sunshine wrote: > git-format-patch recognizes -s as shorthand only for --signoff, however, > its documentation shows -s as shorthand for both --signoff and > --no-patch. Resolve this confusion by suppressing the bogus -s shorthand > for --no-patch. > > While here, also avoid showing the --no-patch option in git-format-patch > documentation since it doesn't make sense to ask to suppress the patch > while at the same time explicitly asking to format the patch (which, > after all, is the purpose of git-format-patch). > > Reported-by: Kevin Brodsky > Signed-off-by: Eric Sunshine > --- > > I haven't quite managed to trace the code yet, but git-format-patch > oddly does recognize --no-patch, and it appears to act as an alias of > --no-stat. At any rate, --no-patch seems rather senseless with > git-format-patch, hence this patch suppresses it in documentation > altogether. > > Documentation/diff-options.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..6eb591f 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -28,10 +28,12 @@ ifdef::git-diff[] > endif::git-diff[] > endif::git-format-patch[] > > +ifndef::git-format-patch[] > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] > > -U:: > --unified=:: > -- > 2.8.0.rc4.285.gc3ac548 -- 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 is an efficient way to get all blobs / trees that have notes attached?
On Mon, Apr 4, 2016 at 9:46 AM, Sebastian Schuberth wrote: > On Fri, Apr 1, 2016 at 2:16 PM, Johan Herland wrote: >>> 3) Recursively list all blobs / trees (git-ls-tree) and look whether an >>> object's hash is conatined in our table to get its notes. >>> >>> In particular 3) could be expensive for repos with a lot of files as we're >>> looking at all of them just to see whether they have notes attached. >> >> In (3), why would you need to search through _all_ blobs/trees? Would >> it not be cheaper to simply query the object type of each annotated >> object from (2)? I.e. something like: >> >> for notes_ref in $(git for-each-ref refs/notes | cut -c 49-) >> do >> echo "--- $notes_ref ---" >> for annotated_obj in $(git notes --ref=$notes_ref list | cut -c 41-) >> do >> type=$(git cat-file -t "$annotated_obj") >> if test "$type" != "commit" >> then >> echo "$annotated_obj: $type" >> fi >> done >> done > > Thanks for the idea. The problem is that I do want to list the notes > by path of the object they belong to. As a blob could potentially > belong to more than one path (copies of files in the repo), I do not > see another way of getting that information other than iterating over > all blobs and checking what path(s) they belong to. True; fundamentally what you want is a blob/tree ID -> path(s) mapping, which is an independent problem, unrelated to to the initial notes lookup. I don't know of a solution faster than the brute-force search you already sketched. If this lookup is important to your use case, you could consider building/caching the required mapping when the notes are added in the first place, but I don't know if that is possible in your scenario... ...Johan > -- > Sebastian Schuberth -- Johan Herland, www.herland.net -- 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: [sort-of-BUG] merge-resolve cannot resolve "content/mode" conflict
Jeff King writes: > Imagine a merge where one side changes the content of a path and the > other changes the mode. Here's a minimal reproduction: > > git init repo && cd repo && > > echo base >file && > git add file && > git commit -m base && > > echo changed >file && > git commit -am content && > > git checkout -b side HEAD^ > chmod +x file && > git commit -am mode > ... > This is a leftover from my experiments with merge-resolve versus > merge-recursive last fall, which resulted in a few actual bug-fixes. I > looked into fixing this case, too, at that time. It seemed possible, but > a little more involved than you might think (because the logic is driven > by a bunch of case statements, and this adds a multiplicative layer to > the cases; we might need to resolve the permissions, and _then_ see if > the content can be resolved). Perhaps I am missing some other codepath in the "multiplicative" layer, but is this not sufficient? The convoluted "update-index/chmod" dance is to help those on filesystems that lack proper executable bits. Otherwise the last "update-index --chmod" is not needed. git-merge-one-file.sh | 25 +++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh index 424b034..36bcdcc 100755 --- a/git-merge-one-file.sh +++ b/git-merge-one-file.sh @@ -142,8 +142,19 @@ case "${1:-.}${2:-.}${3:-.}" in git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1 rm -f -- "$orig" "$src1" "$src2" - if test "$6" != "$7" + # Three-way merge of the permissions + perm= ;# assume the result is the same from stage #2, i.e. $6 + if test "$6" = "$7" || test "$5" = "$7" + then + : nothing + elif test "$5" = "$6" then + case "$7" in + 100644) perm=-x ;; + 100755) perm=+x ;; + *) echo "ERROR: $7: funny filemode not handled." >&2 ;; + esac + else if test -n "$msg" then msg="$msg, " @@ -157,7 +168,17 @@ case "${1:-.}${2:-.}${3:-.}" in echo "ERROR: $msg in $4" >&2 exit 1 fi - exec git update-index -- "$4" + + if test -n "$perm" + then + chmod "$perm" -- "$4" + fi && + git update-index -- "$4" && + if test -n "$perm" + then + git update-index --chmod="$perm" -- "$4" + fi + exit ;; *) -- 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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true
On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy wrote: > I think it would be much simpler to drop the loop, and write instead > something like (untested): I tested it (with few minor changes), and worked fine. test_autostash () { OLDIFS=$IFS IFS='=' set -- $* IFS=$OLDIFS expect=$1 cmd=$2 config_variable=$3 value=$4 test_expect_success "$cmd, $config_variable=$value" ' if [ "$value" = "" ]; then test_unconfig $config_variable else test_config $config_variable $value fi && git reset --hard before-rebase && echo dirty >new_file && git add new_file && if [ $expect = "ok" ]; then git pull $cmd . copy && test_cmp_rev HEAD^ copy && test "$(cat new_file)" = dirty && test "$(cat file)" = "modified again" else test_must_fail git pull $cmd . copy 2>err && test_i18ngrep "uncommitted changes." err fi ' } test_autostash ok '--rebase' rebase.autostash=true test_autostash ok '--rebase --autostash' rebase.autostash=true test_autostash ok '--rebase --autostash' rebase.autostash=false test_autostash ok '--rebase --autostash' rebase.autostash= test_autostash err '--rebase --no-autostash' rebase.autostash=true test_autostash err '--rebase --no-autostash' rebase.autostash=false test_autostash err '--rebase --no-autostash' rebase.autostash= test_autostash ok '--autostash' pull.rebase=true test_autostash err '--no-autostash' pull.rebase=true Perhaps this looks better than the one with the loop. Even better than the implementation in v2[1]. I think it would be wise to go with the above script for v3 (as I will be doing a re-roll of the series[1]). [1]: http://thread.gmane.org/gmane.comp.version-control.git/290596 Thanks, Mehul -- 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] Fix spelling mistakes
Eric Engestrom writes: > I went by word-count: "through" was way more common than "thru", so > I kept the fix. That is not a "fix" (as it is not touching "mistakes") but "making them consistent". I do not personally feel strongly against such a change, but please leave them out of "Subject: [PATCH] typofix". > BTW, I used codespell to fix those, I didn't do it by hand :] > https://github.com/lucasdemarchi/codespell Heh, cute ;-) > I can remove the "thru" fixes if you'd prefer, and resend with only the > other fixes. 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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true
On Mon, Apr 4, 2016 at 1:36 PM, Mehul Jain wrote: > On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy > wrote: >> I think it would be much simpler to drop the loop, and write instead >> something like (untested): > > I tested it (with few minor changes), and worked fine. > > test_autostash () { > OLDIFS=$IFS > IFS='=' > set -- $* > IFS=$OLDIFS > expect=$1 > cmd=$2 > config_variable=$3 > value=$4 > test_expect_success "$cmd, $config_variable=$value" ' > if [ "$value" = "" ]; then > test_unconfig $config_variable > else > test_config $config_variable $value > fi && > > git reset --hard before-rebase && > echo dirty >new_file && > git add new_file && > > if [ $expect = "ok" ]; then > git pull $cmd . copy && > test_cmp_rev HEAD^ copy && > test "$(cat new_file)" = dirty && > test "$(cat file)" = "modified again" > else > test_must_fail git pull $cmd . copy 2>err && > test_i18ngrep "uncommitted changes." err > fi > ' > } > > test_autostash ok '--rebase' rebase.autostash=true > test_autostash ok '--rebase --autostash' rebase.autostash=true > test_autostash ok '--rebase --autostash' rebase.autostash=false > test_autostash ok '--rebase --autostash' rebase.autostash= > test_autostash err '--rebase --no-autostash' rebase.autostash=true > test_autostash err '--rebase --no-autostash' rebase.autostash=false > test_autostash err '--rebase --no-autostash' rebase.autostash= > test_autostash ok '--autostash' pull.rebase=true > test_autostash err '--no-autostash' pull.rebase=true > > Perhaps this looks better than the one with the loop. Even better than > the implementation in v2[1]. > > I think it would be wise to go with the above script for v3 (as I will > be doing a re-roll of the series[1]). This new function is sufficiently complex that it increases cognitive load enough for me to question if it is really a win for such a small number of tests. The individual tests, as implemented in the current round, are quite easy to understand, and don't place any significant cognitive burden on the reader. Although I'm the one who brought up the idea of "automating" these tests, I'm not convinced that it's an improvement in this case, but I don't feel so strongly that I'd forbid it. So, choose the approach which seems best to you while weighing comprehension load for people new to these tests, as well as maintainability costs. -- 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: Out Of Place Capitalization In Stash Feedback
Saxon Knight writes: > I recently did a git stash save "working on main", and in the > feedback, I noticed that "On" was capitalized ("...state On > master..."). > I don't think this is intentional, so I figured I'd try to let someone know. > > Here is the output (quotes included by me): > > "Saved working directory and index state On master: working on main" "git stash list" at this point would show stash@{0}: On master: working on main In other words, the title of the stash entry is "On master: working on main", and that is what is shown in the feedback. I guess the output could quote the title in quotes to avoid this puzzlement, i.e. Saved working directory and index state 'On master: working on main' git-stash.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-stash.sh b/git-stash.sh index c7c65e2..6d8eb22 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -266,7 +266,7 @@ save_stash () { create_stash "$stash_msg" $untracked store_stash -m "$stash_msg" -q $w_commit || die "$(gettext "Cannot save the current status")" - say Saved working directory and index state "$stash_msg" + say "Saved working directory and index state '$stash_msg'" if test -z "$patch_mode" then -- 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] comment for a long #ifdef
Ivan Pozdeev writes: > --- > compat/poll/poll.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) As Eric pointed out, please sign-off your patch. > diff --git a/compat/poll/poll.c b/compat/poll/poll.c > index db4e03e..5eb0280 100644 > --- a/compat/poll/poll.c > +++ b/compat/poll/poll.c > @@ -441,7 +441,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout) > } > > return rc; > -#else > +#else /* #ifndef WIN32_NATIVE */ > static struct timeval tv0; > static HANDLE hEvent; > WSANETWORKEVENTS ev; > @@ -622,5 +622,5 @@ restart: > } > > return rc; > -#endif > +#endif /* #ifndef WIN32_NATIVE */ > } > -- > 1.9.5.msysgit.1 There also is an #ifndef/#else/#endif in abspath.c that is larger than a typical patch context; could you include a fix for that, too? -- 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] doc: clarify that notes can be attached to any type of stored object
Sebastian Schuberth writes: >>> -It is also permitted for a notes ref to point directly to a tree >>> -object, in which case the history of the notes can be read with >>> +It is also permitted for a notes ref to point to any other object in >>> +the object store besides commit objects, that is annotated tags, blobs >>> +or trees. For the latter, the history of the notes can be read with >>> `git log -p -g `. >> >> I do not think this is correct place to patch. The original is not >> talking about what objects can have notes attached at all. What it >> explains is this. > > Thanks for the explanation, I was indeed misreading this. I'll try to > clarify this section then, too. In order to do so, I think we should > mention how to actually create a that directly points to a > tree instead of a commit for the history of notes. Would you have an > example how to do that? Interesting. This came from 9eb3f816 (Documentation/notes: document format of notes trees, 2010-05-08): Documentation/notes: document format of notes trees Separate the specification of the notes format exposed in git-config.1 from the description of the option; or in other words, move the explanation for what to expect to find at refs/notes/commits from git-config.1 to git-notes.1. Suggested-by: Thomas Rast Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano but I do not find a corresponding sentence that says a notes ref can point at a tree in the text before the patch. I highly suspect that "git notes add" and other Porcelain level commands that manipulate an existing notes tree would be unhappy if a notes ref is not a commit, as it is clear from the paragraph before the one under discussion, i.e. Every notes change creates a new commit at the specified notes ref. You can therefore inspect the history of the notes by invoking, e.g., `git log -p notes/commits`. Currently the commit message only records which operation triggered the update, and the commit authorship is determined according to the usual rules (see linkgit:git-commit[1]). These details may change in the future. that in order to create a "new" commit, setting the current one as its parent, would require that the current one to be a commit and not a bare tree. "git notes list" and others that merely read from the notes tree would probably work. -- 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: RFD: removing git.spec.in (Re: git 2.8.0 tarball rpmbuild error)
On za, 2016-04-02 at 20:41 -0700, Junio C Hamano wrote: > I think by now it is very clear that nobody active in the Git > development community tests RPM binaries built with git.spec.in we > have in our tree. I suspect RPM based distros are using their own > RPM build recipe without paying any attention to what we have in our > tree, and that is why no packager from RPM land gave any bug report > and correction before the release happened. Fedora, RHEL, CentOS, OpenSUSE and Mageia all use their own specfiles and local patches. I do test and distribute RPM packages based on the next branch, but use fedora's packaging to do so (which incidentally also broke and I had to work around this change, but I completely forgot about git.spec.in). > I'd propose that during the cycle for the next feature release, we'd > remove git.spec.in and stop pretending as if we support RPM > packaging. I would be in favor of that. In general, I find it much better to use a distro's packaging and simply transplanting a tarball into it. That keeps the difference with what the distro provides minimal. -- Dennis Kaarsemaker www.kaarsemaker.net -- 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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true
Mehul Jain writes: > On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy > wrote: >> I think it would be much simpler to drop the loop, and write instead >> something like (untested): > > I tested it (with few minor changes), and worked fine. > > test_autostash () { > OLDIFS=$IFS > IFS='=' > set -- $* > IFS=$OLDIFS This $IFS dance is not needed. If you need to split variable and value, then just pass two arguments on the caller side. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 v3 4/4] tag: use pgp_verify_function in tag -v call
On Mon, Apr 04, 2016 at 09:38:54AM -0400, Jeff King wrote: > On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote: > > > > As a side note, it might actually be an improvement for pgp_verify_tag > > > to take a sha1 (so that git-tag is sure that it is verifying the same > > > object that it is printing), but that refactoring should probably come > > > separately, I think. > > > > Just to be sure, this refactoring is something we should still include > > in this set of patches, right? I think that otherwise we'd lose the > > desambigutaion that git tag -v does in this patch. > > I think it can be part of this series, but doesn't have to be. As I > understand it, the current code is just handing the name to the `git > verify-tag` process, so if we continue to do so, that would be OK. IIRC, the current code for git tag -v hands the hex-representation[1] of the sha1 to git verify-tag --- I believe that's related to the desamgibuation issue I've seen people discuss. I think this behavior is lost unless we add this on top of the patch. > > > I also think that most of the rippling is gone if we use and adaptor as > > you suggested. Should I add a patch on top of this to support a sha1 as > > part for gpg_verify_tag()? > > Yes, though I'd generally advise against a function taking either a name or > a sha1, and ignoring the other option. That often leads to confusing > interfaces for the callers. Instead, perhaps just take the sha1, and let > the caller do the get_sha1() themselves. Or possibly provide two > functions, one of which is a convenience to translate the name to sha1 > and then call the other. I think the former sounds easier. I can replace the name argument and move the sha1-resolution code to in verify-tag. git tag -v already resolves the tagname to a sha1, so it is easier there. Does this sound reasonable? Thanks! -Santiago [1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c#n109 -- 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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true
Eric Sunshine writes: > Although I'm the one who brought up the idea of "automating" these > tests, I'm not convinced that it's an improvement in this case, but I > don't feel so strongly that I'd forbid it. Another option is to define helper functions to shorten the "manual" tests, e.g. define: setup_rebase_test () { git reset --hard before-rebase && echo dirty >new_file && git add new_file } rebase_test_ok () { git pull $1 . copy && test_cmp_rev HEAD^ copy && test "$(cat new_file)" = dirty && test "$(cat file)" = "modified again" } rebase_test_err () { test_must_fail git pull $1 . copy 2>err && test_i18ngrep "uncommitted changes." err } I'm also OK with keeping the "manual" tests. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] git-format-patch.txt: don't show -s as shorthand for multiple options
Eric Sunshine writes: > Documentation/diff-options.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..6eb591f 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -28,10 +28,12 @@ ifdef::git-diff[] > endif::git-diff[] > endif::git-format-patch[] > > +ifndef::git-format-patch[] > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] Given that the ifndef/endif block immediately before this part is also about excluding -p/-u/--patch when formatting the documentation for format-patch, perhaps the attached may be a smaller equivalent? Documentation/diff-options.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt index 306b7e3..42e6620 100644 --- a/Documentation/diff-options.txt +++ b/Documentation/diff-options.txt @@ -26,12 +26,12 @@ ifndef::git-format-patch[] ifdef::git-diff[] This is the default. endif::git-diff[] -endif::git-format-patch[] -s:: --no-patch:: Suppress diff output. Useful for commands like `git show` that show the patch by default, or to cancel the effect of `--patch`. +endif::git-format-patch[] -U:: --unified=:: -- 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] git-format-patch.txt: don't show -s as shorthand for multiple options
On Mon, Apr 4, 2016 at 3:32 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt >> @@ -28,10 +28,12 @@ ifdef::git-diff[] >> endif::git-diff[] >> endif::git-format-patch[] >> >> +ifndef::git-format-patch[] >> -s:: >> --no-patch:: >> Suppress diff output. Useful for commands like `git show` that >> show the patch by default, or to cancel the effect of `--patch`. >> +endif::git-format-patch[] > > Given that the ifndef/endif block immediately before this part is > also about excluding -p/-u/--patch when formatting the documentation > for format-patch, perhaps the attached may be a smaller equivalent? Perhaps. I kept self-contained to make it easier to add new options between the two if need be, but I don't feel strongly about it. > Documentation/diff-options.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt > index 306b7e3..42e6620 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -26,12 +26,12 @@ ifndef::git-format-patch[] > ifdef::git-diff[] > This is the default. > endif::git-diff[] > -endif::git-format-patch[] > > -s:: > --no-patch:: > Suppress diff output. Useful for commands like `git show` that > show the patch by default, or to cancel the effect of `--patch`. > +endif::git-format-patch[] > > -U:: > --unified=:: -- 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] git-format-patch.txt: don't show -s as shorthand for multiple options
Eric Sunshine writes: >> Given that the ifndef/endif block immediately before this part is >> also about excluding -p/-u/--patch when formatting the documentation >> for format-patch, perhaps the attached may be a smaller equivalent? > > Perhaps. I kept self-contained to make it easier to add new options > between the two if need be, but I don't feel strongly about it. I don't either, but the reason why I thought it would make sense to have them in the same block is because hiding --no-patch and --patch are about the same theme: format-patch is about presenting the diff, and neither disabling diff output nor explicitly asking for diff output makes sense. So... -- 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 v3 4/4] tag: use pgp_verify_function in tag -v call
On Mon, Apr 04, 2016 at 02:24:48PM -0400, Santiago Torres wrote: > > I think it can be part of this series, but doesn't have to be. As I > > understand it, the current code is just handing the name to the `git > > verify-tag` process, so if we continue to do so, that would be OK. > > IIRC, the current code for git tag -v hands the hex-representation[1] of > the sha1 to git verify-tag --- I believe that's related to the > desamgibuation issue I've seen people discuss. I think this behavior is > lost unless we add this on top of the patch. Oh, you're right. I didn't notice that. So yeah, we should make sure in this series to hand the sha1 over to gpg_verify_tag(). > > Yes, though I'd generally advise against a function taking either a name or > > a sha1, and ignoring the other option. That often leads to confusing > > interfaces for the callers. Instead, perhaps just take the sha1, and let > > the caller do the get_sha1() themselves. Or possibly provide two > > functions, one of which is a convenience to translate the name to sha1 > > and then call the other. > > I think the former sounds easier. I can replace the name argument and > move the sha1-resolution code to in verify-tag. git tag -v already > resolves the tagname to a sha1, so it is easier there. > > Does this sound reasonable? Yes, I think that is a good solution. 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: [Outreachy] Git remote whitelist/blacklist
Hi Lars, thank you so much for reaching out to me! and thank you for your suggestions too, I'm changing my application to reflect those. It does really make sense to start the discussion earlier then I scheduled, and add some extra ideas that could be completed after the whitelist/blacklist. Probably if those ideas are the extensions to the whitelist/blacklist, they could be also agreed on when discussing the overall project scope. Cheers, Elena On Wed, Mar 30, 2016 at 11:21 AM, Lars Schneider wrote: > Hi Elena, > > I thought a bit more about your proposal. The Outreachy internship is > scheduled for 3 months and I think you would be able to come up with a "Git > remote whitelist/blacklist" implementation that the Git list considers to > accept within a month. In that case it would be good if you would have a few > extra ideas in your Outreachy proposal that you could tackle afterwards. > These extras could be extensions to the "whitelist/blacklist" project or a > contribution to a completely different part of Git. According to the > Outreachy page [1] you can still change your application until April 22. > > In addition I wonder if you would be willing to start slowly with the > "drafting the implementation" task even before April 22. I, and AFAIK the > majority of the other people on the list, do not work full time on Git. That > means some email responses might be delayed for reasons unrelated to Git. > Therefore I think you will have a better experience if you work with a steady > slow pace instead of high burst of list activity :-) > > Cheers, > Lars > > > [1] https://wiki.gnome.org/Outreachy#Submit_an_Application > > >> On 29 Mar 2016, at 21:24, Lars Schneider wrote: >> >> Hi Elena, >> >> sorry for the late reply. I think it is great that you are interested in the >> Git remote whitelist/blacklist idea. Unfortunately I don't have any >> experience with Outreachy and I wonder what kind of feedback you are looking >> for. >> >> Thanks, >> Lars >> >> >>> On 26 Mar 2016, at 13:15, elena petrashen wrote: >>> >>> Hi everyone, >>> >>> I think I will submit the application as it is now, but still >>> it would be great to get feedback on it, as I don't think >>> there was no reply because everything was perfect :( >>> >>> Thank you! And have an awesome weekend. >>> >>> On Thu, Mar 24, 2016 at 5:50 PM, elena petrashen >>> wrote: Hi, I'm thinking of applying to Outreachy program this round with Git and the project I'm really interested in is "Git remote whitelist/blacklist" project (http://git.github.io/SoC-2016-Ideas/). I have drafted the description/timeline for this project and it would be great to get feedback/suggestions. (I'm actually a bit confused about the scale of this. The Outreachy application doesn't ask for "proposal" in the way GSoC seems to, but merely requests "details and the timeline", so I'm not sure whether the shorter description of what's planned is expected or should I go deeper in detail. I apologize if I chose a wrong approach.) Thank you! >> What project(s) are you interested in (these can be in the same or different organizations)? My preferred project to work on is Git remote whitelist/blacklist project listed on http://git.github.io/SoC-2016-Ideas/. I'm really interested in doing this project as I think this kind of effort is really important: I recently started using git myself, and sometimes I was really scared to push something to the location I was not supposed to push to. I would really appreciate the opportunity in participating in making git a bit more newbie-friendly. >> Who is a possible mentor for the project you are most interested in? Lars Schneider >> Please describe the details and the timeline of the work you plan to accomplish on the project you are most interested in (discuss these first with the mentor of the project): The goal is to provide a safer environment for newcomers to Git to enabling the possibility to modify git config, adding there "allowed" and "denied" remotes for pushing. Code, tests, and documentation are to be created. Timeline: 0. Analysis Apr 22 - May 22 - studying the current code and drafting the implementation proposal 1. Design a. May 22-June 1 - discussion with the mentor regarding the task, presenting the approach and amending it per mentor's feedback b. June 1st-June 15th - communicating with the community regarding the suggested changes and agreeing on logic, scope and format of changes. 2. Development c. June 15th-July 1st - submitting code for the first basic version, amending it according to the feedback d. July 1st - July 15th - extending the code to cover all of the agreed scope e. July 15th - Aug 1st - finalizing full coverage with tests and
Re: [PATCH v3][Outreachy] branch -D: allow - as abbreviation of @{-1}
On Thu, Mar 31, 2016 at 6:31 PM, Remi Galan Alfonso wrote: > Elena Petrashen wrote: >> +void delete_branch_advice(const char *name, const char *ref) >> +{ >> +const char fmt[] = >> +"\nNote: to restore the deleted branch:\n\ngit branch %s %s\n"; > > Shouldn't that be marked for translation, like is done with the other > strings? > > Thanks, > Rémi Thank you for letting me know about that! Could you please help me out and explain how do I mark it for translation? I tried to do it the same way as with the other strings but evidently didn't quite succeed. Thanks! Elena -- 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: run arguments through precompose_argv
I found an issue where file paths containing decomposed unicode chars are not converted to precomposed unicode form when passed to the diff command family. As a result, no diff is displayed. With the help of Torsten Bögershausen and Junio C Hamano I came up with the following patch to fix the issue. Alexander Rinass (1): diff: run arguments through precompose_argv builtin/diff-files.c | 1 + builtin/diff-index.c | 1 + builtin/diff-tree.c | 2 ++ builtin/diff.c | 1 + t/t3910-mac-os-precompose.sh | 42 ++ 5 files changed, 47 insertions(+) -- 2.7.2 -- 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: run arguments through precompose_argv
File paths containing decomposed unicode chars passed to diff command are not converted to precomposed unicode form. As a result, no diff is displayed when feeding such a file path to the diff command. Opposite to most builtin commands, the diff builtin is missing the parse_options call, which internally runs arguments through the precompose_argv call, which ensures all arguments are in precomposed unicode form. Fix the problem by adding a precompose_argv call directly, as a call to parse_options would require additional work to call it. Also applies to diff-index, diff-files and diff-tree. Signed-off-by: Alexander Rinass Thanks-to: Torsten Bögershausen Thanks-to: Junio C Hamano --- builtin/diff-files.c | 1 + builtin/diff-index.c | 1 + builtin/diff-tree.c | 2 ++ builtin/diff.c | 1 + t/t3910-mac-os-precompose.sh | 42 ++ 5 files changed, 47 insertions(+) diff --git a/builtin/diff-files.c b/builtin/diff-files.c index 8ed2eb8..15c61fd 100644 --- a/builtin/diff-files.c +++ b/builtin/diff-files.c @@ -24,6 +24,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) gitmodules_config(); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; + precompose_argv(argc, argv); argc = setup_revisions(argc, argv, &rev, NULL); while (1 < argc && argv[1][0] == '-') { diff --git a/builtin/diff-index.c b/builtin/diff-index.c index d979824..1af373d 100644 --- a/builtin/diff-index.c +++ b/builtin/diff-index.c @@ -21,6 +21,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) gitmodules_config(); git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; + precompose_argv(argc, argv); argc = setup_revisions(argc, argv, &rev, NULL); for (i = 1; i < argc; i++) { diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c index 2a12b81..806dd7a 100644 --- a/builtin/diff-tree.c +++ b/builtin/diff-tree.c @@ -114,6 +114,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) opt->disable_stdin = 1; memset(&s_r_opt, 0, sizeof(s_r_opt)); s_r_opt.tweak = diff_tree_tweak_rev; + + precompose_argv(argc, argv); argc = setup_revisions(argc, argv, opt, &s_r_opt); while (--argc > 0) { diff --git a/builtin/diff.c b/builtin/diff.c index 52c98a9..d6b8f98 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -319,6 +319,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (!no_index) gitmodules_config(); git_config(git_diff_ui_config, NULL); + precompose_argv(argc, argv); init_revisions(&rev, prefix); diff --git a/t/t3910-mac-os-precompose.sh b/t/t3910-mac-os-precompose.sh index 8319356..26dd5b7 100755 --- a/t/t3910-mac-os-precompose.sh +++ b/t/t3910-mac-os-precompose.sh @@ -49,12 +49,54 @@ test_expect_success "setup" ' test_expect_success "setup case mac" ' git checkout -b mac_os ' +# This will test nfd2nfc in git diff +test_expect_success "git diff f.Adiar" ' + touch f.$Adiarnfc && + git add f.$Adiarnfc && + echo f.Adiarnfc >f.$Adiarnfc && + git diff f.$Adiarnfd >expect && + git diff f.$Adiarnfc >actual && + test_cmp expect actual && + git reset HEAD f.Adiarnfc && + rm f.$Adiarnfc expect actual +' +# This will test nfd2nfc in git diff-files +test_expect_success "git diff-files f.Adiar" ' + touch f.$Adiarnfc && + git add f.$Adiarnfc && + echo f.Adiarnfc >f.$Adiarnfc && + git diff-files f.$Adiarnfd >expect && + git diff-files f.$Adiarnfc >actual && + test_cmp expect actual && + git reset HEAD f.Adiarnfc && + rm f.$Adiarnfc expect actual +' +# This will test nfd2nfc in git diff-index +test_expect_success "git diff-index f.Adiar" ' + touch f.$Adiarnfc && + git add f.$Adiarnfc && + echo f.Adiarnfc >f.$Adiarnfc && + git diff-index HEAD f.$Adiarnfd >expect && + git diff-index HEAD f.$Adiarnfc >actual && + test_cmp expect actual && + git reset HEAD f.Adiarnfc && + rm f.$Adiarnfc expect actual +' # This will test nfd2nfc in readdir() test_expect_success "add file Adiarnfc" ' echo f.Adiarnfc >f.$Adiarnfc && git add f.$Adiarnfc && git commit -m "add f.$Adiarnfc" ' +# This will test nfd2nfc in git diff-tree +test_expect_success "git diff-tree f.Adiar" ' + echo f.Adiarnfc >>f.$Adiarnfc && + git diff-tree HEAD f.$Adiarnfd >expect && + git diff-tree HEAD f.$Adiarnfc >actual && + test_cmp expect actual && + git checkout f.$Adiarnfc && + rm expect actual +' # This will test nfd2nfc in git stage() test_expect_success "stage file d.Adiarnfd/f.Adiarnfd" ' mkdir d.$Adiarnfd && -- 2.7.2 -- To unsubscribe from this list: send the line "uns
Re: git diff does not precompose unicode file paths (OS X)
> On 15 Mar 2016, at 06:45, Torsten Bögershausen wrote: > > >I created a test case but git diff exits with 0 if it does not recognize the > >file >path so the test case always succeeds. Can you give me a hint or one > >>example test case? > > The most clean (?) is to compare "git diff" NFC and git diff NFD, they should > give the same result: > for "git diff" something like this would do: > + > +# This will test git diff > +test_expect_success "git diff f.Adiar" ' > + echo "Modified" >f.$Adiarnfd && > + git diff f.$Adiarnfd >expect && > + git diff f.$Adiarnfc >actual && > + git checkout f.$Adiarnfd && > + test_cmp expect actual > +’ Thank you! I had to tweak it a little but it now reproduces the issue and confirms the fix for diff, diff-index, diff-files and diff-tree. I have just sent in the full patch. -- 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] config --show-origin: report paths with forward slashes
Hi Junio, On Mon, 4 Apr 2016, Junio C Hamano wrote: > Johannes Sixt writes: > > > Although I am convinced that the change is not necessary for > > correctness, I can buy the justification that we should produce forward > > slashes for consistency. There are a number of occasions where we > > present paths to the user, and we do show forward-slashes in all cases > > that I found. We should keep the commit. > > > > But then let's do this: > > Sounds like a plan; even though I am mildly against adding more > platform specific #ifdef to files outside compat/, this patch does > not. > > Dscho? Fine by me! Dscho -- 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 (Apr 2016, #02; Mon, 4)
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. The tip of 'next' has been rewound, the first maintenance release v2.8.1 that most everybody can safely ignore has been tagged, and the first batch of topics that have been cooking in 'next' during the pre-2.8 freeze period are now in 'master'. There are a handful of topics that are stuck; they are marked as "Needs review", "Needs an Ack", "Waiting for reroll" etc. in the following list. 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] * es/format-patch-doc-hide-no-patch (2016-04-04) 1 commit - git-format-patch.txt: don't show -s as shorthand for multiple options "git format-patch --help" showed `-s` and `--no-patch` as if these are valid options to the command. We already hide `--patch` option from the documentation, because format-patch is about showing the diff, and the documentation now hides these options as well. Will merge to next. * jk/branch-shortening-funny-symrefs (2016-04-04) 1 commit - branch: fix shortening of non-remote symrefs A change back in version 2.7 to "git branch" broke display of a symbolic refs in a non-standard place in the refs/ hierarchy (we expect symbolic refs to appear in refs/remotes/*/HEAD to point at the primary branch the remote has, and as .git/HEAD to point at the branch we locally checked out). Will merge to next. and later down to maint-2.7. -- [Graduated to "master"] * es/test-gpg-tags (2016-03-06) 4 commits (merged to 'next' on 2016-03-15 at 617119f) + t6302: skip only signed tags rather than all tests when GPG is missing + t6302: also test annotated in addition to signed tags + t6302: normalize names and descriptions of signed tags + lib-gpg: drop unnecessary "missing GPG" warning A test for tags has been restructured so that more parts of it can easily be run on a platform without a working GnuPG. * gf/fetch-pack-direct-object-fetch (2016-03-05) 2 commits (merged to 'next' on 2016-03-06 at 5628860) + fetch-pack: update the documentation for "..." arguments (merged to 'next' on 2016-03-04 at 49199e0) + fetch-pack: fix object_id of exact sha1 Fetching of history by naming a commit object name directly didn't work across remote-curl transport. * jc/index-pack (2016-03-03) 2 commits (merged to 'next' on 2016-03-15 at 42efaa7) + index-pack: add a helper function to derive .idx/.keep filename + Merge branch 'jc/maint-index-pack-keep' into jc/index-pack (this branch is used by jc/bundle; uses jc/maint-index-pack-keep; is tangled with jc/index-pack-clone-bundle.) Code clean-up. * jc/maint-index-pack-keep (2016-03-03) 1 commit (merged to 'next' on 2016-03-04 at bc1d37a) + index-pack: correct --keep[=] (this branch is used by jc/bundle, jc/index-pack and jc/index-pack-clone-bundle.) "git index-pack --keep[=] pack-$name.pack" simply did not work. * jk/add-i-highlight (2016-02-28) 1 commit (merged to 'next' on 2016-03-04 at 4ac3aa1) + add--interactive: allow custom diff highlighting programs A new "interactive.diffFilter" configuration can be used to customize the diff shown in "git add -i" session. * jk/config-get-urlmatch (2016-02-28) 3 commits (merged to 'next' on 2016-03-04 at feeb192) + Documentation/git-config: fix --get-all description + Documentation/git-config: use bulleted list for exit codes + config: fail if --get-urlmatch finds no value "git config --get-urlmatch", unlike other variants of the "git config --get" family, did not signal error with its exit status when there was no matching configuration. * jk/credential-clear-config (2016-02-26) 1 commit (merged to 'next' on 2016-03-04 at f7b26b7) + credential: let empty credential specs reset helper list The credential.helper configuration variable is cumulative and there is no good way to override it from the command line. As a special case, giving an empty string as its value now serves as the signal to clear the values specified in various files. * jk/getwholeline-getdelim-empty (2016-03-05) 1 commit (merged to 'next' on 2016-03-15 at e70d4bd) + strbuf_getwholeline: NUL-terminate getdelim buffer on error strbuf_getwholeline() did not NUL-terminate the buffer on certain corner cases in its error codepath. * jk/rev-parse-local-env-vars (2016-02-29) 2 commits (merged to 'next' on 2016-03-04 at a0300d5) + rev-parse: let some options run outside repository + t1515: add tests for rev-parse out-of-repo helpers The "--local-env-vars" and "--resolve-git-dir" options of "git rev-parse" failed to work outside a repository when t
Re: [PATCH v3][Outreachy] branch -D: allow - as abbreviation of @{-1}
elena petrashen wrote: > On Thu, Mar 31, 2016 at 6:31 PM, Remi Galan Alfonso > wrote: > > Elena Petrashen wrote: > >> +void delete_branch_advice(const char *name, const char *ref) > >> +{ > >> +const char fmt[] = > >> +"\nNote: to restore the deleted branch:\n\ngit branch %s %s\n"; > > > > Shouldn't that be marked for translation, like is done with the other > > strings? > > > > Thanks, > > Rémi > > Thank you for letting me know about that! Could you please > help me out and explain how do I mark it for translation? I tried > to do it the same way as with the other strings but evidently > didn't quite succeed. I am not sure. I tried to grep similar cases, it seems that you can do the following: const char fmt[] = N_("\nNote: to restore [...] \ngit branch %s %s\n"); fprintf(stderr, _(fmt), name, ref); Some similar example in builtin/add.c: static const char ignore_error[] = N_("The following paths are ignored by one of your .gitignore files:\n"); [...] fprintf(stderr, _(ignore_error)); Or you can define fmt as a 'const char *' and in that case do the following: const char *fmt = _("\nNote: to restore [...] \n git branch %s %s\n"); fprintf(stderr, fmt, name, ref); In builtin/am.c: const char *invalid_line = _("Malformed input line: '%s'."); [...] ret = error(invalid_line, sb.buf); I don't know which one is the best way to go though. Thanks, Rémi -- 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 v4 3/6] builtin/verify-tag: move verification code to tag.c
From: Santiago Torres The PGP verification routine for tags could be accessed by other commands that require it. We do this by moving it to the common tag.c code. We rename the verify_tag() function to gpg_verify_tag() to avoid conflicts with the mktag.c function. Signed-off-by: Santiago Torres --- Note: In this version, I just blndly moved code around and renamed the function's name for ease of review. The following patch only renames variables. builtin/verify-tag.c | 51 +-- tag.c| 49 + tag.h| 1 + 3 files changed, 51 insertions(+), 50 deletions(-) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 77f070a..7e36d53 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -18,55 +18,6 @@ static const char * const verify_tag_usage[] = { NULL }; -static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) -{ - struct signature_check sigc; - int len; - int ret; - - memset(&sigc, 0, sizeof(sigc)); - - len = parse_signature(buf, size); - - if (size == len) { - if (flags & GPG_VERIFY_VERBOSE) - write_in_full(1, buf, len); - return error("no signature found"); - } - - ret = check_signature(buf, len, buf + len, size - len, &sigc); - print_signature_buffer(&sigc, flags); - - signature_check_clear(&sigc); - return ret; -} - -static int verify_tag(const char *name, unsigned flags) -{ - enum object_type type; - unsigned char sha1[20]; - char *buf; - unsigned long size; - int ret; - - if (get_sha1(name, sha1)) - return error("tag '%s' not found.", name); - - type = sha1_object_info(sha1, NULL); - if (type != OBJ_TAG) - return error("%s: cannot verify a non-tag object of type %s.", - name, typename(type)); - - buf = read_sha1_file(sha1, &type, &size); - if (!buf) - return error("%s: unable to read file.", name); - - ret = run_gpg_verify(buf, size, flags); - - free(buf); - return ret; -} - static int git_verify_tag_config(const char *var, const char *value, void *cb) { int status = git_gpg_config(var, value, cb); @@ -96,7 +47,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) flags |= GPG_VERIFY_VERBOSE; while (i < argc) - if (verify_tag(argv[i++], flags)) + if (gpg_verify_tag(argv[i++], flags)) had_error = 1; return had_error; } diff --git a/tag.c b/tag.c index d72f742..81e86e6 100644 --- a/tag.c +++ b/tag.c @@ -6,6 +6,55 @@ const char *tag_type = "tag"; +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) +{ + struct signature_check sigc; + int len; + int ret; + + memset(&sigc, 0, sizeof(sigc)); + + len = parse_signature(buf, size); + + if (size == len) { + if (flags & GPG_VERIFY_VERBOSE) + write_in_full(1, buf, len); + return error("no signature found"); + } + + ret = check_signature(buf, len, buf + len, size - len, &sigc); + print_signature_buffer(&sigc, flags); + + signature_check_clear(&sigc); + return ret; +} + +int gpg_verify_tag(const char *name, unsigned flags) +{ + enum object_type type; + unsigned char sha1[20]; + char *buf; + unsigned long size; + int ret; + + if (get_sha1(name, sha1)) + return error("tag '%s' not found.", name); + + type = sha1_object_info(sha1, NULL); + if (type != OBJ_TAG) + return error("%s: cannot verify a non-tag object of type %s.", + name, typename(type)); + + buf = read_sha1_file(sha1, &type, &size); + if (!buf) + return error("%s: unable to read file.", name); + + ret = run_gpg_verify(buf, size, flags); + + free(buf); + return ret; +} + struct object *deref_tag(struct object *o, const char *warn, int warnlen) { while (o && o->type == OBJ_TAG) diff --git a/tag.h b/tag.h index f4580ae..877f180 100644 --- a/tag.h +++ b/tag.h @@ -17,5 +17,6 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si extern int parse_tag(struct tag *item); extern struct object *deref_tag(struct object *, const char *, int); extern struct object *deref_tag_noverify(struct object *); +extern int gpg_verify_tag(const char *name, unsigned flags); #endif /* TAG_H */ -- 2.8.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
[PATCH v4 4/6] tag.c: Replace varialbe name for readability
From: Santiago Torres The run_gpg_verify function has two variables size, and len. This may come off as confusing when reading the code. We clarify which one pertains to the length of the tag headers by renaming len to payload_length. Signed-off-by: Santiago Torres --- Note: this used to be part of the previous patch on v3. I moved the renaming part to a single patch to simplify the review. tag.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tag.c b/tag.c index 81e86e6..f6443db 100644 --- a/tag.c +++ b/tag.c @@ -9,20 +9,21 @@ const char *tag_type = "tag"; static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) { struct signature_check sigc; - int len; + int payload_length; int ret; memset(&sigc, 0, sizeof(sigc)); - len = parse_signature(buf, size); + payload_length = parse_signature(buf, size); - if (size == len) { + if (size == payload_length) { if (flags & GPG_VERIFY_VERBOSE) - write_in_full(1, buf, len); + write_in_full(1, buf, payload_length); return error("no signature found"); } - ret = check_signature(buf, len, buf + len, size - len, &sigc); + ret = check_signature(buf, payload_length, buf + payload_length, + size - payload_length, &sigc); print_signature_buffer(&sigc, flags); signature_check_clear(&sigc); -- 2.8.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
[PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1
From: Santiago Torres The gpg_verify_tag function resolves the ref for any existing object. However, git tag -v resolves to only tag-refs. We can provide support for sha1 by moving the refname resolution code out of gpg_verify_tag and allow for the object's sha1 as an argument. Signed-off-by: Santiago Torres --- builtin/tag.c| 2 +- builtin/verify-tag.c | 9 +++-- tag.c| 12 tag.h| 2 +- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index f4450f8..398c892 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref, static int verify_tag(const char *name, const char *ref, const unsigned char *sha1) { - return gpg_verify_tag(name, GPG_VERIFY_VERBOSE); + return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE); } static int do_sign(struct strbuf *buffer) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 7e36d53..2ff01d8 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -30,6 +30,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) { int i = 1, verbose = 0, had_error = 0; unsigned flags = 0; + unsigned char sha1[20]; const struct option verify_tag_options[] = { OPT__VERBOSE(&verbose, N_("print tag contents")), OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW), @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) if (verbose) flags |= GPG_VERIFY_VERBOSE; - while (i < argc) - if (gpg_verify_tag(argv[i++], flags)) + while (i < argc) { + if (get_sha1(argv[i++], sha1)) + return error("tag '%s' not found.", argv[i]); + + if (gpg_verify_tag(sha1, flags)) had_error = 1; + } return had_error; } diff --git a/tag.c b/tag.c index f6443db..8ac9de5 100644 --- a/tag.c +++ b/tag.c @@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags) return ret; } -int gpg_verify_tag(const char *name, unsigned flags) +int gpg_verify_tag(const unsigned char *sha1, unsigned flags) { enum object_type type; - unsigned char sha1[20]; char *buf; unsigned long size; int ret; - if (get_sha1(name, sha1)) - return error("tag '%s' not found.", name); - type = sha1_object_info(sha1, NULL); if (type != OBJ_TAG) - return error("%s: cannot verify a non-tag object of type %s.", - name, typename(type)); + return error("cannot verify a non-tag object of type %s.", + typename(type)); buf = read_sha1_file(sha1, &type, &size); if (!buf) - return error("%s: unable to read file.", name); + return error("unable to read file."); ret = run_gpg_verify(buf, size, flags); diff --git a/tag.h b/tag.h index 877f180..cb643b9 100644 --- a/tag.h +++ b/tag.h @@ -17,6 +17,6 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si extern int parse_tag(struct tag *item); extern struct object *deref_tag(struct object *, const char *, int); extern struct object *deref_tag_noverify(struct object *); -extern int gpg_verify_tag(const char *name, unsigned flags); +extern int gpg_verify_tag(const unsigned char *sha1, unsigned flags); #endif /* TAG_H */ -- 2.8.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
[PATCH v4 0/6] tag: move PGP verification code to tag.c
This is a follow up of [1], [2], and [3]: v4 (this): Thanks Eric, Peff, and Hannes for the feedback. * I relocated the sigchain_push call so it comes after the error on gpg-interface (thanks Hannnes for catching this). * I updated the unit test to match the discussion on [3]. Now it generates the expected output of the tag on the fly for comparison. (This is just copy and paste from [3], but I verified that it works by breaking the while) * I split moving the code and renaming the variables into two patches so these are easier to review. * I used an adapter on builtin/tag.c instead of redefining all the fn* declarations everywhere. This introduces an issue with the way git tag -v resolves refnames though. I added a new commit to restore the previous behavior of git-tag. I'm not sure if I should've split this into two commits though. v3: Thanks Eric, Jeff, for the feedback. * I separated the patch in multiple sub-patches. * I compared the behavior of previous git tag -v and git verify-tag invocations to make sure the behavior is the same * I dropped the multi-line comment, as suggested. * I fixed the issue with the missing brackets in the while (this is now detected by the test). v2: * I moved the pgp-verification code to tag.c * I added extra arguments so git tag -v and git verify-tag both work with the same function * Relocated the SIGPIPE handling code in verify-tag to gpg-interface v1: The verify tag function is just a thin wrapper around the verify-tag command. We can avoid one fork call by doing the verification inside the tag builtin instead. This applies on v2.8.0. Thanks! -Santiago [1] http://git.661346.n2.nabble.com/PATCH-RFC-builtin-tag-c-move-PGP-verification-inside-builtin-td7651529.html#a7651547 [2] http://git.661346.n2.nabble.com/PATCH-tag-c-move-PGP-verification-code-from-plumbing-td7651562.html [3] http://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-td7652334.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
[PATCH v4 5/6] tag: use pgp_verify_function in tag -v call
From: Santiago Torres Instead of running the verify-tag plumbing command, we use the pgp_verify_tag(). This avoids the usage of an extra fork call. To do this, we extend the number of parameters that tag.c takes, and verify-tag passes. Redundant calls done in the pgp_verify_tag function are removed. Signed-off-by: Santiago Torres --- Note: This follows Peff's suggestion to use an adapter, instead of changing the the fn* pointer structure to match gpg_verify_tag. builtin/tag.c | 8 +--- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 1705c94..f4450f8 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref, static int verify_tag(const char *name, const char *ref, const unsigned char *sha1) { - const char *argv_verify_tag[] = {"verify-tag", - "-v", "SHA1_HEX", NULL}; - argv_verify_tag[2] = sha1_to_hex(sha1); - - if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD)) - return error(_("could not verify the tag '%s'"), name); - return 0; + return gpg_verify_tag(name, GPG_VERIFY_VERBOSE); } static int do_sign(struct strbuf *buffer) -- 2.8.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
[PATCH v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags
From: Santiago Torres The verify-tag command supports mutliple tag names as an argument. However, no previous tests try to verify multiple tags at once. This test runs the verify-tag command against three tags separately and then compares the result against the invocation with the same three tags as an argument. The results shouldn't differ. Signed-off-by: Santiago Torres --- Note: In this version we don't check or uniq for the verify output, but instead compare against an invocation for each of the three tags indivdually. t/t7030-verify-tag.sh | 12 1 file changed, 12 insertions(+) diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 4608e71..f9161332 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' ' ) ' +test_expect_success GPG 'verify multiple tags' ' + tags="fourth-signed sixth-signed seventh-signed" && + for i in $tags; do + git verify-tag -v --raw $i || return 1 + done >expect.stdout 2>expect.stderr.1 && + grep GOODSIG expect.stderr && + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && + grep GOODSIG actual.stderr && + test_cmp expect.stdout actual.stdout && + test_cmp expect.stderr actual.stderr +' + test_done -- 2.8.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
[PATCH v4 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface
From: Santiago Torres The verify_signed_buffer comand might cause a SIGPIPE signal when the gpg child process terminates early (due to a bad keyid, for example) and git tries to write to it afterwards. Previously, ignoring SIGPIPE was done on the builtin/verify-tag.c command to avoid this issue. However, any other caller who wanted to use the verify_signed_buffer command would have to include this signal call. Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the verify_signed_buffer call (pretty much like in sign_buffer()) so that any caller is not required to perform this task. This will avoid possible mistakes by further developers using verify_signed_buffer. Signed-off-by: Santiago Torres --- Note: On this version i moved the sigchain_push below the return error in the same way that sign_buffer does(). Thanks to Johanes Sixt for catching this. builtin/verify-tag.c | 3 --- gpg-interface.c | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index 00663f6..77f070a 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -95,9 +95,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) if (verbose) flags |= GPG_VERIFY_VERBOSE; - /* sometimes the program was terminated because this signal -* was received in the process of writing the gpg input: */ - signal(SIGPIPE, SIG_IGN); while (i < argc) if (verify_tag(argv[i++], flags)) had_error = 1; diff --git a/gpg-interface.c b/gpg-interface.c index 3dc2fe3..2259938 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -237,6 +237,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, return error(_("could not run gpg.")); } + sigchain_push(SIGPIPE, SIG_IGN); write_in_full(gpg.in, payload, payload_size); close(gpg.in); @@ -250,6 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size, close(gpg.out); ret = finish_command(&gpg); + sigchain_pop(SIGPIPE); unlink_or_warn(path); -- 2.8.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: [PATCH] git-format-patch.txt: don't show -s as shorthand for multiple options
On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano wrote: > Eric Sunshine writes: >>> Given that the ifndef/endif block immediately before this part is >>> also about excluding -p/-u/--patch when formatting the documentation >>> for format-patch, perhaps the attached may be a smaller equivalent? >> >> Perhaps. I kept self-contained to make it easier to add new options >> between the two if need be, but I don't feel strongly about it. > > I don't either, but the reason why I thought it would make sense to > have them in the same block is because hiding --no-patch and --patch > are about the same theme: format-patch is about presenting the diff, > and neither disabling diff output nor explicitly asking for diff > output makes sense. That's reasonable. Should I re-roll, or would you like to amend it locally? -- 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] git-format-patch.txt: don't show -s as shorthand for multiple options
On Mon, Apr 4, 2016 at 6:38 PM, Eric Sunshine wrote: > On Mon, Apr 4, 2016 at 4:07 PM, Junio C Hamano wrote: >> Eric Sunshine writes: Given that the ifndef/endif block immediately before this part is also about excluding -p/-u/--patch when formatting the documentation for format-patch, perhaps the attached may be a smaller equivalent? >>> >>> Perhaps. I kept self-contained to make it easier to add new options >>> between the two if need be, but I don't feel strongly about it. >> >> I don't either, but the reason why I thought it would make sense to >> have them in the same block is because hiding --no-patch and --patch >> are about the same theme: format-patch is about presenting the diff, >> and neither disabling diff output nor explicitly asking for diff >> output makes sense. > > That's reasonable. Should I re-roll, or would you like to amend it locally? Okay, I just pulled upon seeing "What's Cooking" and see that you amended it locally. 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 v12 5/5] commit: add a commit.verbose config variable
On Sun, Apr 3, 2016 at 8:58 PM, Eric Sunshine wrote: > The fact that the 32 new tests are nearly identical suggests strongly > that the testing should instead either be table-driven or be done via > for-loops to systematically cover all cases. Not only would either of > these approaches be easier to digest, but they would make it easy to > tell at a glance if any cases were missing. See [2] for an example of > how the tests can be table-driven, and see the bottom of [3] for an > example of using for-loops to test systematically (though you'd need > to use nested for-loops rather than just the one in the example). > > I'm leaning toward systematic testing via nested for-loops as the more > suitable of the two approach for this particular application. By the way, while this would be a nice change, it doesn't necessarily have to be part of this series, and could be done as a follow-up by you or someone else. (The other changes suggested in the same review, however, ought to be fixed in this series; in particular, simplifying the "setup" test and making the first test after "setup" consistent with the remaining tests.) -- 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: [GSoC] A late proposal: a modern send-email
æƒ è½¶ç¾¤ wrote: > 2016-03-28 6:00 GMT+08:00 Eric Wong : > > While Gmail provides SMTP access, it was (last I was told) > > incompatible with two-factor auth; so I've encountered users > > unable to send patches with their normal 2FA-enabled accounts. > > That's the origin of this idea of `mailto`. > > In fact, you could send mail via 2FA-enabled accounts via > "app password" metioned by Javier. But it's annoying to create > "app password" for every client. Since it seems possible to use 2FA with gmail, can either you or Javier submit a patch to Documentation/git-send-email.txt to update the Gmail-related section where "Use gmail as the smtp server" is to describe how to use this "app password"? It's much easier to do than your entire GSoC proposal and would be immediately useful to 2FA gmail users out there who don't know this, yet or aren't running the latest git. Thanks. > > If there is a `mailto` method to send patch, you could type something > like > > git send-patch --mailto origin/master..HEAD > > Then, gmail is launched with the content of patch in it. You could edit > the list of `to` and `cc`(You could even take use of gmail contact). Then > just send. That's all. No need to SMTP config or "app password" any > more. > > > Maybe git hackers at Google have enough pull to lobby Gmail's > > web interface to make it easier to send plain-text patches; > > but I would love more to see users running local mail clients > > and even running their own SMTP servers. > > Yes, this should be free with user to pick their favorite mail client. > > >> That may not be a "Git" project, but GSoC is not limited to Git ;-) > > > > Completely agreed; email is critical to decentralized development; > > but I don't believe decentralization is in the best interests of > > any large and powerful corporation. > > > > IMHO, what we need is a SoIS: Summer of Independent Sysadmins :> > -- > 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
[PATCH v5 1/4] pretty: expand tabs in indented logs to make things line up properly
From: Linus Torvalds A commit log message sometimes tries to line things up using tabs, assuming fixed-width font with the standard 8-place tab settings. Viewing such a commit however does not work well in "git log", as we indent the lines by prefixing 4 spaces in front of them. This should all line up: Column 1 Column 2 A B ABCD EFGH SPACESInstead of Tabs Even with multi-byte UTF8 characters: Column 1 Column 2 Ä B åäö 100 A Møøse once bit my sister.. Tab-expand the lines in "git log --expand-tabs" output before prefixing 4 spaces. This is based on the patch by Linus Torvalds, but at this step, we require an explicit command line option to enable the behaviour. Signed-off-by: Junio C Hamano --- Documentation/pretty-options.txt | 5 +++ commit.h | 1 + log-tree.c | 1 + pretty.c | 71 ++-- revision.c | 2 ++ revision.h | 1 + 6 files changed, 79 insertions(+), 2 deletions(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index 4b659ac..d820653 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -42,6 +42,11 @@ people using 80-column terminals. verbatim; this means that invalid sequences in the original commit may be copied to the output. +--expand-tabs:: + Perform a tab expansion (replace each tab with enough spaces + to fill to the next display column that is multiple of 8) + in the log message before showing it in the output. + ifndef::git-rev-list[] --notes[=]:: Show the notes (see linkgit:git-notes[1]) that annotate the diff --git a/commit.h b/commit.h index 5d58be0..a7ef682 100644 --- a/commit.h +++ b/commit.h @@ -147,6 +147,7 @@ struct pretty_print_context { int preserve_subject; struct date_mode date_mode; unsigned date_mode_explicit:1; + unsigned expand_tabs_in_log:1; int need_8bit_cte; char *notes_message; struct reflog_walk_info *reflog_info; diff --git a/log-tree.c b/log-tree.c index 60f9839..78a5381 100644 --- a/log-tree.c +++ b/log-tree.c @@ -683,6 +683,7 @@ void show_log(struct rev_info *opt) ctx.fmt = opt->commit_format; ctx.mailmap = opt->mailmap; ctx.color = opt->diffopt.use_color; + ctx.expand_tabs_in_log = opt->expand_tabs_in_log; ctx.output_encoding = get_log_output_encoding(); if (opt->from_ident.mail_begin && opt->from_ident.name_begin) ctx.from_ident = &opt->from_ident; diff --git a/pretty.c b/pretty.c index 92b2870..c8b075d 100644 --- a/pretty.c +++ b/pretty.c @@ -1629,6 +1629,72 @@ void pp_title_line(struct pretty_print_context *pp, strbuf_release(&title); } +static int pp_utf8_width(const char *start, const char *end) +{ + int width = 0; + size_t remain = end - start; + + while (remain) { + int n = utf8_width(&start, &remain); + if (n < 0 || !start) + return -1; + width += n; + } + return width; +} + +static void strbuf_add_tabexpand(struct strbuf *sb, +const char *line, int linelen) +{ + const char *tab; + + while ((tab = memchr(line, '\t', linelen)) != NULL) { + int width = pp_utf8_width(line, tab); + + /* +* If it wasn't well-formed utf8, or it +* had characters with badly defined +* width (control characters etc), just +* give up on trying to align things. +*/ + if (width < 0) + break; + + /* Output the data .. */ + strbuf_add(sb, line, tab - line); + + /* .. and the de-tabified tab */ + strbuf_addchars(sb, ' ', 8 - (width % 8)); + + /* Skip over the printed part .. */ + linelen -= tab + 1 - line; + line = tab + 1; + } + + /* +* Print out everything after the last tab without +* worrying about width - there's nothing more to +* align. +*/ + strbuf_add(sb, line, linelen); +} + +/* + * pp_handle_indent() prints out the intendation, and + * the whole line (without the final newline), after + * de-tabifying. + */ +static void pp_handle_indent(struct pretty_print_context *pp, +struct strbuf *sb, int indent, +const char *line, int linelen) +{ + strbuf_addchars(sb, ' ', indent); + if (pp->expand_tabs_in_log) + strbuf_add_tabexpand(sb, line, linelen); + else + strbuf_add(sb, line, linelen); +} + void pp_remainder(struct pretty_pri
[PATCH v5 2/4] pretty: enable --expand-tabs by default for selected pretty formats
"git log --pretty={medium,full,fuller}" and "git log" by default prepend 4 spaces to the log message, so it makes sense to enable the new "expand-tabs" facility by default for these formats. Add --no-expand-tabs option to override the new default. The change alone breaks a test in t4201 that runs "git shortlog" on the output from "git log", and expects that the output from "git log" does not do such a tab expansion. Adjust the test to explicitly disable expand-tabs with --no-expand-tabs. Signed-off-by: Junio C Hamano --- Documentation/pretty-options.txt | 6 ++ builtin/log.c| 1 + pretty.c | 18 +++--- revision.c | 7 +++ revision.h | 3 ++- t/t4201-shortlog.sh | 2 +- 6 files changed, 28 insertions(+), 9 deletions(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index d820653..edbb02f 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -43,9 +43,15 @@ people using 80-column terminals. commit may be copied to the output. --expand-tabs:: +--no-expand-tabs:: Perform a tab expansion (replace each tab with enough spaces to fill to the next display column that is multiple of 8) in the log message before showing it in the output. ++ +By default, tabs are expanded in pretty formats that indent the log +message by 4 spaces (i.e. 'medium', which is the default, 'full', +and 'fuller'). `--no-expand-tabs` option can be used to disable +this. ifndef::git-rev-list[] --notes[=]:: diff --git a/builtin/log.c b/builtin/log.c index e00cea7..e5775ae 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1281,6 +1281,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) git_config(git_format_config, NULL); init_revisions(&rev, prefix); rev.commit_format = CMIT_FMT_EMAIL; + rev.expand_tabs_in_log_default = 0; rev.verbose_header = 1; rev.diff = 1; rev.max_parents = 1; diff --git a/pretty.c b/pretty.c index c8b075d..b7938e0 100644 --- a/pretty.c +++ b/pretty.c @@ -16,6 +16,7 @@ static struct cmt_fmt_map { const char *name; enum cmit_fmt format; int is_tformat; + int expand_tabs_in_log; int is_alias; const char *user_format; } *commit_formats; @@ -87,13 +88,13 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c static void setup_commit_formats(void) { struct cmt_fmt_map builtin_formats[] = { - { "raw",CMIT_FMT_RAW, 0 }, - { "medium", CMIT_FMT_MEDIUM,0 }, - { "short", CMIT_FMT_SHORT, 0 }, - { "email", CMIT_FMT_EMAIL, 0 }, - { "fuller", CMIT_FMT_FULLER,0 }, - { "full", CMIT_FMT_FULL, 0 }, - { "oneline",CMIT_FMT_ONELINE, 1 } + { "raw",CMIT_FMT_RAW, 0, 0 }, + { "medium", CMIT_FMT_MEDIUM,0, 1 }, + { "short", CMIT_FMT_SHORT, 0, 0 }, + { "email", CMIT_FMT_EMAIL, 0, 0 }, + { "fuller", CMIT_FMT_FULLER,0, 1 }, + { "full", CMIT_FMT_FULL, 0, 1 }, + { "oneline",CMIT_FMT_ONELINE, 1, 0 } }; commit_formats_len = ARRAY_SIZE(builtin_formats); builtin_formats_len = commit_formats_len; @@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev) rev->commit_format = commit_format->format; rev->use_terminator = commit_format->is_tformat; + rev->expand_tabs_in_log_default = commit_format->expand_tabs_in_log; if (commit_format->format == CMIT_FMT_USERFORMAT) { save_user_format(rev, commit_format->user_format, commit_format->is_tformat); @@ -1720,6 +1722,8 @@ void pp_remainder(struct pretty_print_context *pp, strbuf_grow(sb, linelen + indent + 20); if (indent) pp_handle_indent(pp, sb, indent, line, linelen); + else if (pp->expand_tabs_in_log) + strbuf_add_tabexpand(sb, line, linelen); else strbuf_add(sb, line, linelen); strbuf_addch(sb, '\n'); diff --git a/revision.c b/revision.c index e662230..da53b6c 100644 --- a/revision.c +++ b/revision.c @@ -1412,8 +1412,10 @@ void init_revisions(struct rev_info *revs, const char *prefix) revs->skip_count = -1; revs->max_count = -1; revs->max_parents = -1; + revs->expand_tabs_in_log = -1; revs->commit_format = CMIT_FMT_DEFAULT; + revs->expand_tabs_in_log_default = 1; init_grep_defau
[PATCH v5 3/4] pretty: allow tweaking tabwidth in --expand-tabs
When the local convention of the project is to use tab width that is not 8, it may make sense to allow "git log --expand-tabs=" to tweak the output to match it. Signed-off-by: Junio C Hamano --- Documentation/pretty-options.txt | 9 ++--- commit.h | 2 +- pretty.c | 15 --- revision.c | 9 +++-- 4 files changed, 22 insertions(+), 13 deletions(-) diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index edbb02f..93ad1cd 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -42,16 +42,19 @@ people using 80-column terminals. verbatim; this means that invalid sequences in the original commit may be copied to the output. +--expand-tabs=:: --expand-tabs:: --no-expand-tabs:: Perform a tab expansion (replace each tab with enough spaces - to fill to the next display column that is multiple of 8) + to fill to the next display column that is multiple of '') in the log message before showing it in the output. + `--expand-tabs` is a short-hand for `--expand-tabs=8`, and + `--no-expand-tabs` is a short-hand for `--expand-tabs=0`, + which disables tab expansion. + By default, tabs are expanded in pretty formats that indent the log message by 4 spaces (i.e. 'medium', which is the default, 'full', -and 'fuller'). `--no-expand-tabs` option can be used to disable -this. +and 'fuller'). ifndef::git-rev-list[] --notes[=]:: diff --git a/commit.h b/commit.h index a7ef682..b06db4d 100644 --- a/commit.h +++ b/commit.h @@ -147,7 +147,7 @@ struct pretty_print_context { int preserve_subject; struct date_mode date_mode; unsigned date_mode_explicit:1; - unsigned expand_tabs_in_log:1; + int expand_tabs_in_log; int need_8bit_cte; char *notes_message; struct reflog_walk_info *reflog_info; diff --git a/pretty.c b/pretty.c index b7938e0..87c4497 100644 --- a/pretty.c +++ b/pretty.c @@ -89,11 +89,11 @@ static void setup_commit_formats(void) { struct cmt_fmt_map builtin_formats[] = { { "raw",CMIT_FMT_RAW, 0, 0 }, - { "medium", CMIT_FMT_MEDIUM,0, 1 }, + { "medium", CMIT_FMT_MEDIUM,0, 8 }, { "short", CMIT_FMT_SHORT, 0, 0 }, { "email", CMIT_FMT_EMAIL, 0, 0 }, - { "fuller", CMIT_FMT_FULLER,0, 1 }, - { "full", CMIT_FMT_FULL, 0, 1 }, + { "fuller", CMIT_FMT_FULLER,0, 8 }, + { "full", CMIT_FMT_FULL, 0, 8 }, { "oneline",CMIT_FMT_ONELINE, 1, 0 } }; commit_formats_len = ARRAY_SIZE(builtin_formats); @@ -1645,7 +1645,7 @@ static int pp_utf8_width(const char *start, const char *end) return width; } -static void strbuf_add_tabexpand(struct strbuf *sb, +static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth, const char *line, int linelen) { const char *tab; @@ -1666,7 +1666,7 @@ static void strbuf_add_tabexpand(struct strbuf *sb, strbuf_add(sb, line, tab - line); /* .. and the de-tabified tab */ - strbuf_addchars(sb, ' ', 8 - (width % 8)); + strbuf_addchars(sb, ' ', tabwidth - (width % tabwidth)); /* Skip over the printed part .. */ linelen -= tab + 1 - line; @@ -1692,7 +1692,7 @@ static void pp_handle_indent(struct pretty_print_context *pp, { strbuf_addchars(sb, ' ', indent); if (pp->expand_tabs_in_log) - strbuf_add_tabexpand(sb, line, linelen); + strbuf_add_tabexpand(sb, pp->expand_tabs_in_log, line, linelen); else strbuf_add(sb, line, linelen); } @@ -1723,7 +1723,8 @@ void pp_remainder(struct pretty_print_context *pp, if (indent) pp_handle_indent(pp, sb, indent, line, linelen); else if (pp->expand_tabs_in_log) - strbuf_add_tabexpand(sb, line, linelen); + strbuf_add_tabexpand(sb, pp->expand_tabs_in_log, +line, linelen); else strbuf_add(sb, line, linelen); strbuf_addch(sb, '\n'); diff --git a/revision.c b/revision.c index da53b6c..47e9ee7 100644 --- a/revision.c +++ b/revision.c @@ -1415,7 +1415,7 @@ void init_revisions(struct rev_info *revs, const char *prefix) revs->expand_tabs_in_log = -1; revs->commit_format = CMIT_FMT_DEFAULT; - revs->expand_tabs_in_log_default = 1; + revs->expand_tabs_in_log_default = 8; init_grep_defaults(); grep_init(&revs->
[PATCH v5 0/4] Expanding tabs in "git log" output
So here is the fifth and hopefully the final try. Previous round are at $gmane/289694, $gmane/289166, $gmane/288987 and $gmane/290222. This round is different from v4 in the following ways. * wording changes, grammo- and typo-fixes in the documentation (thanks to Eric Sunshine and Jeff King). * v4 made --pretty=$fmt to override an earlier --expand-tabs=; this round only allows --pretty=$fmt to set the default behaviour when an explicit --expand-tabs= is given on the command line (thanks to Jeff King). * comes with a test. See the end of this cover letter for an interdiff. Junio C Hamano (3): pretty: enable --expand-tabs by default for selected pretty formats pretty: allow tweaking tabwidth in --expand-tabs pretty: test --expand-tabs Linus Torvalds (1): pretty: expand tabs in indented logs to make things line up properly Documentation/pretty-options.txt | 14 ++ builtin/log.c| 1 + commit.h | 1 + log-tree.c | 1 + pretty.c | 90 revision.c | 14 ++ revision.h | 2 + t/t4201-shortlog.sh | 2 +- t/t4213-log-tabexpand.sh | 98 9 files changed, 213 insertions(+), 10 deletions(-) create mode 100755 t/t4213-log-tabexpand.sh -- 2.8.1-251-g9997610 diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt index 8a944b1..93ad1cd 100644 --- a/Documentation/pretty-options.txt +++ b/Documentation/pretty-options.txt @@ -45,16 +45,16 @@ people using 80-column terminals. --expand-tabs=:: --expand-tabs:: --no-expand-tabs:: - Perform a tab expansion (replace each tab with enough number - of spaces to fill to the next display column that is - multiple of '') in the log message before using the message - to show in the output. `--expand-tabs` is a short-hand for - `--expand-tabs=8`, and `--no-expand-tabs` is a short-hand for - `--expand-tabs=0`, which disables tab expansion. + Perform a tab expansion (replace each tab with enough spaces + to fill to the next display column that is multiple of '') + in the log message before showing it in the output. + `--expand-tabs` is a short-hand for `--expand-tabs=8`, and + `--no-expand-tabs` is a short-hand for `--expand-tabs=0`, + which disables tab expansion. + By default, tabs are expanded in pretty formats that indent the log message by 4 spaces (i.e. 'medium', which is the default, 'full', -and "fuller'). +and 'fuller'). ifndef::git-rev-list[] --notes[=]:: diff --git a/builtin/log.c b/builtin/log.c index e00cea7..e5775ae 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1281,6 +1281,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) git_config(git_format_config, NULL); init_revisions(&rev, prefix); rev.commit_format = CMIT_FMT_EMAIL; + rev.expand_tabs_in_log_default = 0; rev.verbose_header = 1; rev.diff = 1; rev.max_parents = 1; diff --git a/commit.h b/commit.h index 2185c8d..b06db4d 100644 --- a/commit.h +++ b/commit.h @@ -147,7 +147,7 @@ struct pretty_print_context { int preserve_subject; struct date_mode date_mode; unsigned date_mode_explicit:1; - unsigned expand_tabs_in_log; + int expand_tabs_in_log; int need_8bit_cte; char *notes_message; struct reflog_walk_info *reflog_info; diff --git a/pretty.c b/pretty.c index b340ecd..87c4497 100644 --- a/pretty.c +++ b/pretty.c @@ -173,7 +173,7 @@ void get_commit_format(const char *arg, struct rev_info *rev) rev->commit_format = commit_format->format; rev->use_terminator = commit_format->is_tformat; - rev->expand_tabs_in_log = commit_format->expand_tabs_in_log; + rev->expand_tabs_in_log_default = commit_format->expand_tabs_in_log; if (commit_format->format == CMIT_FMT_USERFORMAT) { save_user_format(rev, commit_format->user_format, commit_format->is_tformat); @@ -1722,6 +1722,9 @@ void pp_remainder(struct pretty_print_context *pp, strbuf_grow(sb, linelen + indent + 20); if (indent) pp_handle_indent(pp, sb, indent, line, linelen); + else if (pp->expand_tabs_in_log) + strbuf_add_tabexpand(sb, pp->expand_tabs_in_log, +line, linelen); else strbuf_add(sb, line, linelen); strbuf_addch(sb, '\n'); diff --git a/revision.c b/revision.c index 4f9ecbe..47e9ee7 100644 --- a/revision.c +++ b/revision.c @@ -1412,9 +1412,10 @@ void init_revisions(struct rev_info *revs, const char *prefix) revs->skip_count = -1; revs->max_count = -1;
[PATCH v5 4/4] pretty: test --expand-tabs
The test prepares a simple commit with HT on its log message lines, and makes sure that - formats that should or should not expand tabs by default do or do not expand tabs respectively, - with explicit --expand-tabs= and short-hands --expand-tabs (equivalent to --expand-tabs=8) and --no-expand-tabs (equivalent to --expand-tabs=0) before or after the explicit --pretty=$fmt, the tabs are expanded (or not expanded) accordingly. The tests use the second line of the log message for formats other than --pretty=short, primarily because the first line of the email format is handled specially to add the [PATCH] prefix, etc. in a separate codepath (--pretty=short uses the first line because there is no other line to test). Signed-off-by: Junio C Hamano --- t/t4213-log-tabexpand.sh | 98 1 file changed, 98 insertions(+) create mode 100755 t/t4213-log-tabexpand.sh diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh new file mode 100755 index 000..74ca03a --- /dev/null +++ b/t/t4213-log-tabexpand.sh @@ -0,0 +1,98 @@ +#!/bin/sh + +test_description='log/show --expand-tabs' + +. ./test-lib.sh + +HT=" " +title='tab indent at the beginning of the title line' +body='tab indent on a line in the body' + +count_expand () +{ + case " $* " in + *' --pretty=short '*) + line=$title ;; + *) + line=$body ;; + esac + expect= + count=$(( $1 + $2 )) ;# expected spaces + while test $count -gt 0 + do + expect="$expect " + count=$(( $count - 1 )) + done + shift 2 + count=$1 ;# expected tabs + while test $count -gt 0 + do + expect="$expect$HT" + count=$(( $count - 1 )) + done + shift + { + echo "git show -s $*" + echo "$expect$line" + } | sed -e 's/ /./g' >expect + + { + echo "git show -s $*" + git show -s "$@" | + sed -n -e "/$line\$/p" + } | sed -e 's/ /./g' >actual + + test_cmp expect actual +} + +test_expand () +{ + fmt=$1 + case "$fmt" in + *=raw | *=short | *=email) + default="0 1" ;; + *) + default="8 0" ;; + esac + case "$fmt" in + *=email) + in=0 ;; + *) + in=4 ;; + esac + test_expect_success "expand/no-expand${fmt:+ for $fmt}" ' + count_expand $in $default $fmt && + count_expand $in 8 0 $fmt --expand-tabs && + count_expand $in 8 0 --expand-tabs $fmt && + count_expand $in 8 0 $fmt --expand-tabs=8 && + count_expand $in 8 0 --expand-tabs=8 $fmt && + count_expand $in 0 1 $fmt --no-expand-tabs && + count_expand $in 0 1 --no-expand-tabs $fmt && + count_expand $in 0 1 $fmt --expand-tabs=0 && + count_expand $in 0 1 --expand-tabs=0 $fmt && + count_expand $in 4 0 $fmt --expand-tabs=4 && + count_expand $in 4 0 --expand-tabs=4 $fmt + ' +} + +test_expect_success 'setup' ' + test_tick && + sed -e "s/Q/$HT/g" <<-EOF >msg && + Q$title + + Q$body + EOF + git commit --allow-empty -F msg +' + +test_expand "" +test_expand --pretty +test_expand --pretty=short +test_expand --pretty=medium +test_expand --pretty=full +test_expand --pretty=fuller +test_expand --pretty=fuller +test_expand --pretty=raw +test_expand --pretty=email + +test_done -- 2.8.1-251-g9997610 -- 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 v5 4/4] pretty: test --expand-tabs
On Mon, Apr 4, 2016 at 8:58 PM, Junio C Hamano wrote: > The test prepares a simple commit with HT on its log message lines, > and makes sure that > > - formats that should or should not expand tabs by default do or do >not expand tabs respectively, > > - with explicit --expand-tabs= and short-hands --expand-tabs >(equivalent to --expand-tabs=8) and --no-expand-tabs (equivalent >to --expand-tabs=0) before or after the explicit --pretty=$fmt, >the tabs are expanded (or not expanded) accordingly. > > The tests use the second line of the log message for formats other > than --pretty=short, primarily because the first line of the email > format is handled specially to add the [PATCH] prefix, etc. in a > separate codepath (--pretty=short uses the first line because there > is no other line to test). > > Signed-off-by: Junio C Hamano > --- > diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh > @@ -0,0 +1,98 @@ > +count_expand () > +{ > + case " $* " in > + *' --pretty=short '*) > + line=$title ;; > + *) > + line=$body ;; > + esac > + expect= > + count=$(( $1 + $2 )) ;# expected spaces > + while test $count -gt 0 > + do > + expect="$expect " > + count=$(( $count - 1 )) > + done > + shift 2 > + count=$1 ;# expected tabs Why semicolon before the hash here and above? -- 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 v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags
On Mon, Apr 4, 2016 at 6:22 PM, wrote: > t/t7030-verify-tag.sh: Adds validation for multiple tags Rewrite: t7030: test verify-tag with multiple tags The leading "t/" and the trailing "-*.sh" were dropped since they add no value. > The verify-tag command supports mutliple tag names as an argument. s/mutliple/multiple/ > However, no previous tests try to verify multiple tags at once. This > test runs the verify-tag command against three tags separately and then > compares the result against the invocation with the same three tags as > an argument. The results shouldn't differ. The bulk of this description is merely repeating what the patch itself already says, thus is redundant. The entire commit message could probably be collapsed to: t7030: test verify-tag with multiple tags git-verify-tag accepts multiple tags as arguments, however, existing tests only ever invoke it with a single tag, so add a test invoking it with multiple tags. > Signed-off-by: Santiago Torres > --- > diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh > index 4608e71..f9161332 100755 > --- a/t/t7030-verify-tag.sh > +++ b/t/t7030-verify-tag.sh > @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' ' > +test_expect_success GPG 'verify multiple tags' ' > + tags="fourth-signed sixth-signed seventh-signed" && > + for i in $tags; do > + git verify-tag -v --raw $i || return 1 > + done >expect.stdout 2>expect.stderr.1 && > + grep GOODSIG expect.stderr && > + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && > + grep GOODSIG actual.stderr && Hmm, I had expected you to adopt Peff's suggestion[1] for the greps: grep '^.GNUPG:.' ... [1]: http://article.gmane.org/gmane.comp.version-control.git/290691 > + test_cmp expect.stdout actual.stdout && > + test_cmp expect.stderr actual.stderr > +' > + > test_done > -- > 2.8.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: [PATCH v4 4/6] tag.c: Replace varialbe name for readability
On Mon, Apr 4, 2016 at 6:22 PM, wrote: > tag.c: Replace varialbe name for readability s/Replace/replace/ s/varialbe/variable/ > The run_gpg_verify function has two variables size, and len. This may > come off as confusing when reading the code. We clarify which one > pertains to the length of the tag headers by renaming len to > payload_length. > > Signed-off-by: Santiago Torres > --- -- 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 v4 5/6] tag: use pgp_verify_function in tag -v call
On Mon, Apr 4, 2016 at 6:22 PM, wrote: > Instead of running the verify-tag plumbing command, we use the > pgp_verify_tag(). This avoids the usage of an extra fork call. To do > this, we extend the number of parameters that tag.c takes, and > verify-tag passes. Redundant calls done in the pgp_verify_tag function > are removed. I'm confused about everything following "an extra fork call" since those subsequent sentences don't seem to pertain to this patch. Is that leftover gunk from the previous version of this series? > Signed-off-by: Santiago Torres > --- > diff --git a/builtin/tag.c b/builtin/tag.c > index 1705c94..f4450f8 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref, > static int verify_tag(const char *name, const char *ref, > const unsigned char *sha1) > { > - const char *argv_verify_tag[] = {"verify-tag", > - "-v", "SHA1_HEX", NULL}; > - argv_verify_tag[2] = sha1_to_hex(sha1); > - > - if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD)) > - return error(_("could not verify the tag '%s'"), name); > - return 0; > + return gpg_verify_tag(name, GPG_VERIFY_VERBOSE); > } > > static int do_sign(struct strbuf *buffer) > -- -- 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 v4 5/6] tag: use pgp_verify_function in tag -v call
On Mon, Apr 04, 2016 at 09:39:29PM -0400, Eric Sunshine wrote: > On Mon, Apr 4, 2016 at 6:22 PM, wrote: > > Instead of running the verify-tag plumbing command, we use the > > pgp_verify_tag(). This avoids the usage of an extra fork call. To do > > this, we extend the number of parameters that tag.c takes, and > > verify-tag passes. Redundant calls done in the pgp_verify_tag function > > are removed. > > I'm confused about everything following "an extra fork call" since > those subsequent sentences don't seem to pertain to this patch. Is > that leftover gunk from the previous version of this series? Yes, I thought I had taken this part of the commit message away. I'll take it out right away. Apologies for this. -Santiago. -- 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 v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags
> > The bulk of this description is merely repeating what the patch itself > already says, thus is redundant. The entire commit message could > probably be collapsed to: > > t7030: test verify-tag with multiple tags > > git-verify-tag accepts multiple tags as arguments, however, > existing tests only ever invoke it with a single tag, so add a > test invoking it with multiple tags. Yeah, this is actually clearer. Sorry for the academ-ose commit message. > > +test_expect_success GPG 'verify multiple tags' ' > > + tags="fourth-signed sixth-signed seventh-signed" && > > + for i in $tags; do > > + git verify-tag -v --raw $i || return 1 > > + done >expect.stdout 2>expect.stderr.1 && > > + grep GOODSIG expect.stderr && > > + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && > > + grep GOODSIG actual.stderr && > > Hmm, I had expected you to adopt Peff's suggestion[1] for the greps: > > grep '^.GNUPG:.' ... > > [1]: http://article.gmane.org/gmane.comp.version-control.git/290691 I thought this was an stylistic thing. I can of course adopt this suggestion. Thanks, -Santiago -- 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 v5 4/4] pretty: test --expand-tabs
On Mon, Apr 04, 2016 at 09:10:46PM -0400, Eric Sunshine wrote: > > + count=$1 ;# expected tabs > > Why semicolon before the hash here and above? I am in the habit of doing this, too. I have a vague recollection of getting bitten by a shell that treated: echo foo # bar or something similar as not-a-comment. But neither bash, dash, nor ksh seem to. So I'm not sure if it was some other shell in my past, or if I simply have an irrational fear. -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 v4 6/6] tag.c: Change gpg_verify_tag argument to sha1
On Mon, Apr 4, 2016 at 6:22 PM, wrote: > tag.c: Change gpg_verify_tag argument to sha1 s/Change/change/ > The gpg_verify_tag function resolves the ref for any existing object. > However, git tag -v resolves to only tag-refs. We can provide support > for sha1 by moving the refname resolution code out of gpg_verify_tag and > allow for the object's sha1 as an argument. This description leaves me fairly clueless about why this change is being made since justification seems to be lacking. More about this below... > Signed-off-by: Santiago Torres > --- > diff --git a/builtin/tag.c b/builtin/tag.c > @@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref, > static int verify_tag(const char *name, const char *ref, > const unsigned char *sha1) > { > - return gpg_verify_tag(name, GPG_VERIFY_VERBOSE); > + return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE); > } So, by this point, 'name' has already been resolved to 'sha1', thus this change avoids a second resolution of 'name' inside gpg_verify_tag(). Therefore, this is really an optimization, right? Perhaps the intent of the patch would be clearer if the commit message sold it as such. For instance, the commit message might start off: tag: avoid resolving tag name twice and then go on to say that by hefting tag name resolution out of gpg_verify_tag(), the extra resolution can be avoided. > static int do_sign(struct strbuf *buffer) > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c > @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const char > *prefix) > if (verbose) > flags |= GPG_VERIFY_VERBOSE; > > - while (i < argc) > - if (gpg_verify_tag(argv[i++], flags)) > + while (i < argc) { > + if (get_sha1(argv[i++], sha1)) > + return error("tag '%s' not found.", argv[i]); Why does this 'return' after the first error, but the gpg_verify_tag() call below merely sets a 'had_error' flag and continues? I would expect this one to set the flag and continue, as well. > + Style: unnecessary blank line > + if (gpg_verify_tag(sha1, flags)) > had_error = 1; > + } > return had_error; > } > diff --git a/tag.c b/tag.c > @@ -30,25 +30,21 @@ static int run_gpg_verify(const char *buf, unsigned long > size, unsigned flags) > -int gpg_verify_tag(const char *name, unsigned flags) > +int gpg_verify_tag(const unsigned char *sha1, unsigned flags) > { > enum object_type type; > - unsigned char sha1[20]; > char *buf; > unsigned long size; > int ret; > > - if (get_sha1(name, sha1)) > - return error("tag '%s' not found.", name); > - > type = sha1_object_info(sha1, NULL); > if (type != OBJ_TAG) > - return error("%s: cannot verify a non-tag object of type %s.", > - name, typename(type)); > + return error("cannot verify a non-tag object of type %s.", > + typename(type)); This error message becomes much less useful since it now only says that there is a problem with *some* tag but doesn't give any identifying information. How about including the sha1 in the error message? > > buf = read_sha1_file(sha1, &type, &size); > if (!buf) > - return error("%s: unable to read file.", name); > + return error("unable to read file."); Ditto regarding making this more useful by including the sha1. > ret = run_gpg_verify(buf, size, flags); -- 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 v4 6/6] tag.c: Change gpg_verify_tag argument to sha1
On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote: > On Mon, Apr 4, 2016 at 6:22 PM, wrote: > > tag.c: Change gpg_verify_tag argument to sha1 > > s/Change/change/ Sorry I've been consistently missing these... > > > The gpg_verify_tag function resolves the ref for any existing object. > > However, git tag -v resolves to only tag-refs. We can provide support > > for sha1 by moving the refname resolution code out of gpg_verify_tag and > > allow for the object's sha1 as an argument. > > This description leaves me fairly clueless about why this change is > being made since justification seems to be lacking. More about this > below... > > > Signed-off-by: Santiago Torres > > --- > > diff --git a/builtin/tag.c b/builtin/tag.c > > @@ -104,7 +104,7 @@ static int delete_tag(const char *name, const char *ref, > > static int verify_tag(const char *name, const char *ref, > > const unsigned char *sha1) > > { > > - return gpg_verify_tag(name, GPG_VERIFY_VERBOSE); > > + return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE); > > } > > So, by this point, 'name' has already been resolved to 'sha1', thus > this change avoids a second resolution of 'name' inside > gpg_verify_tag(). Therefore, this is really an optimization, right? > Perhaps the intent of the patch would be clearer if the commit message > sold it as such. For instance, the commit message might start off: > > tag: avoid resolving tag name twice > > and then go on to say that by hefting tag name resolution out of > gpg_verify_tag(), the extra resolution can be avoided. Yep, this is actually true, but something I didn't consider. I think that, from what I could draw on [1] and [2], git tag -v is reserved to tags only (refs/tags iirc). This patch makes it so that this behavior is not lost. I'm not sure if it should be separate from 5/6 though. > > > static int do_sign(struct strbuf *buffer) > > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c > > @@ -46,8 +47,12 @@ int cmd_verify_tag(int argc, const char **argv, const > > char *prefix) > > if (verbose) > > flags |= GPG_VERIFY_VERBOSE; > > > > - while (i < argc) > > - if (gpg_verify_tag(argv[i++], flags)) > > + while (i < argc) { > > + if (get_sha1(argv[i++], sha1)) > > + return error("tag '%s' not found.", argv[i]); > > Why does this 'return' after the first error, but the gpg_verify_tag() > call below merely sets a 'had_error' flag and continues? I would > expect this one to set the flag and continue, as well. This sounds better than what I had thought. I'll set had error and do continue instead. > > { > > enum object_type type; > > - unsigned char sha1[20]; > > char *buf; > > unsigned long size; > > int ret; > > > > - if (get_sha1(name, sha1)) > > - return error("tag '%s' not found.", name); > > - > > type = sha1_object_info(sha1, NULL); > > if (type != OBJ_TAG) > > - return error("%s: cannot verify a non-tag object of type > > %s.", > > - name, typename(type)); > > + return error("cannot verify a non-tag object of type %s.", > > + typename(type)); > > This error message becomes much less useful since it now only says > that there is a problem with *some* tag but doesn't give any > identifying information. How about including the sha1 in the error > message? > > > > > buf = read_sha1_file(sha1, &type, &size); > > if (!buf) > > - return error("%s: unable to read file.", name); > > + return error("unable to read file."); > > Ditto regarding making this more useful by including the sha1. yes, I wasn't sure about how to move forward here. I'll replace the name with the sha1 instead of just removing it. Thanks! -Santiago. [1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c [2] http://git.661346.n2.nabble.com/PATCH-v3-0-4-tag-move-PGP-verification-code-to-tag-c-tp7652334p7652437.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: [PATCH v5 0/4] Expanding tabs in "git log" output
On Mon, Apr 04, 2016 at 05:58:33PM -0700, Junio C Hamano wrote: > So here is the fifth and hopefully the final try. Previous round > are at $gmane/289694, $gmane/289166, $gmane/288987 and > $gmane/290222. With the exception of two minor nits on the final patch, this looks good to me. Thanks for cleaning up the option-parsing ordering thing; I think it turned out rather nice. -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 v5 4/4] pretty: test --expand-tabs
On Mon, Apr 04, 2016 at 05:58:37PM -0700, Junio C Hamano wrote: > +count_expand () > +{ This function takes a lot of unnamed arguments that we process with "shift". It might be nice to give a brief comment describing them. > +test_expand "" > +test_expand --pretty > +test_expand --pretty=short > +test_expand --pretty=medium > +test_expand --pretty=full > +test_expand --pretty=fuller > +test_expand --pretty=fuller > +test_expand --pretty=raw > +test_expand --pretty=email Duplicated fuller? -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
support block comments in gitconfig
Could we have block comments in gitconfig? It's a nice-to-have supported in most languages. eg: #{ commented out block #} or other intuitive syntax -- 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 v4 6/6] tag.c: Change gpg_verify_tag argument to sha1
On Mon, Apr 4, 2016 at 10:10 PM, Santiago Torres wrote: > On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote: >> On Mon, Apr 4, 2016 at 6:22 PM, wrote: >> > - return gpg_verify_tag(name, GPG_VERIFY_VERBOSE); >> > + return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE); >> >> So, by this point, 'name' has already been resolved to 'sha1', thus >> this change avoids a second resolution of 'name' inside >> gpg_verify_tag(). Therefore, this is really an optimization, right? >> Perhaps the intent of the patch would be clearer if the commit message >> sold it as such. For instance, the commit message might start off: >> >> tag: avoid resolving tag name twice >> >> and then go on to say that by hefting tag name resolution out of >> gpg_verify_tag(), the extra resolution can be avoided. > > Yep, this is actually true, but something I didn't consider. I think > that, from what I could draw on [1] and [2], git tag -v is reserved to > tags only (refs/tags iirc). This patch makes it so that this behavior is > not lost. I'm not sure if it should be separate from 5/6 though. Okay, so this is a fix for a regression introduced by the previous patch. I agree that this is suboptimal. You can avoid this regression issue altogether by merely re-ordering the patch series. For instance, the series could be ordered like this: 1. SIGPIPE dance 2. add new t7030 test 3. improve variable name in verify_tag() 4. heft get_sha1() lookup out of verify_tag() 5. move code and publish gpg_verify_tag() 6. re-implement "git-tag -v" in terms of gpg_verify_tag() Sell patch #4 as a libification preparation step, explaining as justification that some future clients may already have the sha1 in hand and would want to avoid duplicate resolution of the tag name. -- 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 v4 2/6] t/t7030-verify-tag.sh: Adds validation for multiple tags
On Mon, Apr 4, 2016 at 9:46 PM, Santiago Torres wrote: > Eric Sunshine wrote: >> > +test_expect_success GPG 'verify multiple tags' ' >> > + tags="fourth-signed sixth-signed seventh-signed" && >> > + for i in $tags; do >> > + git verify-tag -v --raw $i || return 1 >> > + done >expect.stdout 2>expect.stderr.1 && >> > + grep GOODSIG expect.stderr && >> > + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && >> > + grep GOODSIG actual.stderr && >> >> Hmm, I had expected you to adopt Peff's suggestion[1] for the greps: >> >> grep '^.GNUPG:.' ... >> >> [1]: http://article.gmane.org/gmane.comp.version-control.git/290691 > > I thought this was an stylistic thing. I can of course adopt this > suggestion. It's probably not a big deal, but Peff's suggestion (at least feels like it) makes the test a bit more comprehensive. By the way, as the test is heavily inspired by Peff's example, it might be worth giving him a nod via a Helped-by: just above your Signed-off-by:. -- 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 v12 1/5] t0040-test-parse-options.sh: fix style issues
On Mon, Apr 4, 2016 at 11:00 PM, Eric Sunshine wrote: > On Mon, Apr 4, 2016 at 8:45 AM, Pranit Bauva wrote: >> Okay I will do the change. I was previously unaware about the use of >> '\' before EOF. I googled it now. But I am still confused about its >> use in this scenario. Upto what I understood, it is used where you >> want to expand a variable, substitute a command, arithmethic >> expansion. The use of '\' in the tests I have changed in v12 wrt 11 is >> understood by me as you want to remove the use of escape sequences >> which is justified. But this seems a bit vague. Is it some convention >> in git? > > Both 'EOF' and \EOF suppress interpolation and other transformations > in the heredoc content which would otherwise occur with plain EOF. The > 'EOF' form is well documented; \EOF not so much, but is used heavily > in git test scripts. So: > > x=flormp > echo < Hello, $x > EOF > > prints "Hello, flormp", whereas: > > echo <<\EOF > Hello, $x > EOF > > prints "Hello, $x". > > While test scripts sometimes use \EOF to explicitly suppress variable > expansion, it's also quite common to use it even when there is nothing > which could be expanded in the heredoc content, in which case it > signals to the reader that the author doesn't expect the content to > undergo expansion or interpolation. It's also a bit of future-proofing > in case some later change to the heredoc content inserts something > which might otherwise be expanded. Thanks for taking out your time to explain this clearly. I will do the changes in the tests as suggested by your review. :) -- 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 v3 2/4] format-patch: add '--base' option to record base tree info
On Fri, Apr 01, 2016 at 09:00:20AM -0700, Junio C Hamano wrote: >Ye Xiaolong writes: > >> On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote: >> >>>The contents of this look OK, but does it format correctly via >>>AsciiDoc? I suspect that only the first paragraph up to "of this >>>shape:" would appear correctly and all the rest would become funny. >> >> Sorry, just heard of AsciiDoc, I will try to use it to do the right format >> work. > >Please make sure "make -C Documentation" produces sensible output >for *.1 (manpage) and *.html. OK. > + init_revisions(&revs, NULL); + revs.max_parents = 1; + base->object.flags |= UNINTERESTING; + add_pending_object(&revs, &base->object, "base"); + for (i = 0; i < total; i++) { + list[i]->object.flags |= 0; >>> >>>What does this statement do, exactly? Are you clearing some bits >>>but not others, and if so which ones? >> >> My mistake, it's useless and should be removed. > >It probably make sense to do "&= ~UNINTERESTING" there, though. You >are adding one UNINTERESTING object (i.e. the base) and adding >objects that are on the list[] as interesting. Yeah, it does make sense, I'll do the change. > >>>This shows the patches in the order discovered by the revision >>>traversal, which typically is newer to older. Is that intended? >>>Is it assumed that the order of the patches does not matter? >> >> The prerequisite patches should show in topological order, thus robot >> could parse them one by one and apply the patches in reverse order. > >If you have history where base is B, with three prerequisites 1-2-3, >before the patch series A-B-C, i.e. > > B---1---2---3---A---B---C > >if you are showing "base-commit: B" as the first line in the base >tree information block, it would be natural to expect that the >prerequisite patch ids are listed for 1 and then 2 and then finally >3, i.e. > > base-commit: B >prerequisite-patch-id: 1 >prerequisite-patch-id: 2 >prerequisite-patch-id: 3 > >no? I think this sounds more sensible than what I had thought, I'll adjust the showing sequence accordingly. > >Also I know _you_ intend to consume this by robot, but it makes me >wonder if with a minimum update you can make the output also more >useful for bystander humans. A mailing list participant may > > - see an early round of a series that interests her, > - try to apply them to her tree, > - find that the series does not apply, but > - sees that a block to help identify to what tree the series is > meant to apply. > >With a list of 40-hex alone, she may not be able to figure out the >prerequisites, but if there is some other clue that helps her to >identify the base commit and these patches, she may be able to >construct a tree that is close enough. Maybe you can help her by >appending the title of the commit and patches at the end of these >lines? > >This is not a strong suggestion (yet); I am thinking aloud at this >point, without knowing how much it would help in practice to do so. Thanks for the suggestions, actually it is what we have planned for next step to make the output info more friendly for human being, I'll follow up on it. Thanks, Xiaolong. >-- >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: [PATCH] branch: fix shortening of non-remote symrefs
Hello, On Sun, Apr 3, 2016 at 9:44 AM, Jeff King wrote: > On Sun, Apr 03, 2016 at 02:54:22PM +1200, Phil Sainty wrote: > >> Given the following symbolic reference: >> >> $ git symbolic-ref refs/heads/m refs/heads/master >> >> >> Correct in 2.6.6: >> >> $ PATH=~/git/git-2.6.6:$PATH git branch >> m -> master >> * master >> >> >> Wrong in 2.7.0: >> >> $ PATH=~/git/git-2.7.0:$PATH git branch >> m -> m >> * master Thanks for reporting this. > > Thanks for an easy test case. Though we don't officially support > arbitrary symrefs in the ref namespace, they do mostly work. And > certainly the current output is nonsense, and it worked before. This > bisects to aedcb7d (branch.c: use 'ref-filter' APIs, 2015-09-23). > > The fix is below. Karthik, I didn't look at all how this interacts with > your work to convert branch to ref-filter for printing. I imagine it > drops this code completely, but we should make sure that ref-filter gets > this case right. I almost didn't prepare this patch at all, but I > suspect we may want it for "maint", while the full conversion would wait > for "master". > It's dropped in my latest series. I should be able to replicate what you've done here onto ref-filter.c. Since I'm re-rolling my patches, I'll add this one along too. > -- >8 -- > Subject: branch: fix shortening of non-remote symrefs > > Commit aedcb7d (branch.c: use 'ref-filter' APIs, 2015-09-23) > adjusted the symref-printing code to look like this: > > if (item->symref) { > skip_prefix(item->symref, "refs/remotes/", &desc); > strbuf_addf(&out, " -> %s", desc); > } > > This has three bugs in it: > > 1. It always skips past "refs/remotes/", instead of > skipping past the prefix associated with the branch we > are showing (so commonly we see "refs/remotes/" for the > refs/remotes/origin/HEAD symref, but the previous code > would skip "refs/heads/" when showing a symref it found > in refs/heads/. > > 2. If skip_prefix() does not match, it leaves "desc" > untouched, and we show whatever happened to be in it > (which is the refname from a call to skip_prefix() > earlier in the function). > > 3. If we do match with skip_prefix(), we stomp on the > "desc" variable, which is later passed to > add_verbose_info(). We probably want to retain the > original refname there (though it likely doesn't matter > in practice, since after all, one points to the other). > > The fix to match the original code is fairly easy: record > the prefix to strip based on item->kind, and use it here. > However, since we already have a local variable named "prefix", > let's give the two prefixes verbose names so we don't > confuse them. > > Signed-off-by: Jeff King > --- > The test makes sure we restored the v2.6.x behavior, namely that > cross-prefix symrefs will not be shortened at all. It might be nice to > show: > > ref-to-remote -> remotes/origin/branch-one > > or something, but that should be separate from the fix (and I don't > overly care either way, so I probably won't work on it). > > builtin/branch.c | 19 --- > t/t3203-branch-output.sh | 12 > 2 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 7b45b6b..f6c23bf 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -393,22 +393,25 @@ static void format_and_print_ref_item(struct > ref_array_item *item, int maxwidth, > int current = 0; > int color; > struct strbuf out = STRBUF_INIT, name = STRBUF_INIT; > - const char *prefix = ""; > + const char *prefix_to_show = ""; > + const char *prefix_to_skip = NULL; > const char *desc = item->refname; > char *to_free = NULL; > > switch (item->kind) { > case FILTER_REFS_BRANCHES: > - skip_prefix(desc, "refs/heads/", &desc); > + prefix_to_skip = "refs/heads/"; > + skip_prefix(desc, prefix_to_skip, &desc); > if (!filter->detached && !strcmp(desc, head)) > current = 1; > else > color = BRANCH_COLOR_LOCAL; > break; > case FILTER_REFS_REMOTES: > - skip_prefix(desc, "refs/remotes/", &desc); > + prefix_to_skip = "refs/remotes/"; > + skip_prefix(desc, prefix_to_skip, &desc); > color = BRANCH_COLOR_REMOTE; > - prefix = remote_prefix; > + prefix_to_show = remote_prefix; > break; > case FILTER_REFS_DETACHED_HEAD: > desc = to_free = get_head_description(); > @@ -425,7 +428,7 @@ static void format_and_print_ref_item(struct > ref_array_item *item, int maxwidth, > color = BRANCH_COLOR_CURRENT; > } > > - strbuf_addf(&name, "%s%s", prefix, desc); > + strbuf_addf(&name, "%s%s", prefix_to_show,
Re: [PATCH v5 4/4] pretty: test --expand-tabs
Jeff King writes: > On Mon, Apr 04, 2016 at 09:10:46PM -0400, Eric Sunshine wrote: > >> > + count=$1 ;# expected tabs >> >> Why semicolon before the hash here and above? > > I am in the habit of doing this, too. I have a vague recollection of > getting bitten by a shell that treated: > > echo foo # bar > > or something similar as not-a-comment. But neither bash, dash, nor ksh > seem to. I think the reason why I started doing the same is because some shells can be configured to lose the comment-introducer-ness of "#" in interactive mode, and I wanted to make sure that many things I write can be tried out by others more easily by copy-and-paste to their interactive session. -- 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 v5 4/4] pretty: test --expand-tabs
Jeff King writes: > On Mon, Apr 04, 2016 at 05:58:37PM -0700, Junio C Hamano wrote: > >> +count_expand () >> +{ > > This function takes a lot of unnamed arguments that we process with > "shift". It might be nice to give a brief comment describing them. > ... >> +test_expand --pretty=fuller >> +test_expand --pretty=fuller > ... > Duplicated fuller? Thanks. Here is a replacement. -- >8 -- The test prepares a simple commit with HT on its log message lines, and makes sure that - formats that should or should not expand tabs by default do or do not expand tabs respectively, - with explicit --expand-tabs= and short-hands --expand-tabs (equivalent to --expand-tabs=8) and --no-expand-tabs (equivalent to --expand-tabs=0) before or after the explicit --pretty=$fmt, the tabs are expanded (or not expanded) accordingly. The tests use the second line of the log message for formats other than --pretty=short, primarily because the first line of the email format is handled specially to add the [PATCH] prefix, etc. in a separate codepath (--pretty=short uses the first line because there is no other line to test). Signed-off-by: Junio C Hamano --- t/t4213-log-tabexpand.sh | 105 +++ 1 file changed, 105 insertions(+) create mode 100755 t/t4213-log-tabexpand.sh diff --git a/t/t4213-log-tabexpand.sh b/t/t4213-log-tabexpand.sh new file mode 100755 index 000..e01a8f6 --- /dev/null +++ b/t/t4213-log-tabexpand.sh @@ -0,0 +1,105 @@ +#!/bin/sh + +test_description='log/show --expand-tabs' + +. ./test-lib.sh + +HT=" " +title='tab indent at the beginning of the title line' +body='tab indent on a line in the body' + +# usage: count_expand $indent $numSP $numHT @format_args +count_expand () +{ + expect= + count=$(( $1 + $2 )) ;# expected spaces + while test $count -gt 0 + do + expect="$expect " + count=$(( $count - 1 )) + done + shift 2 + count=$1 ;# expected tabs + while test $count -gt 0 + do + expect="$expect$HT" + count=$(( $count - 1 )) + done + shift + + # The remainder of the command line is "git show -s" options + case " $* " in + *' --pretty=short '*) + line=$title ;; + *) + line=$body ;; + esac + + # Prefix the output with the command line arguments, and + # replace SP with a dot both in the expecte and actual output + # so that test_cmp would show the differene together with the + # breakage in a way easier to consume by the debugging user. + { + echo "git show -s $*" + echo "$expect$line" + } | sed -e 's/ /./g' >expect + + { + echo "git show -s $*" + git show -s "$@" | + sed -n -e "/$line\$/p" + } | sed -e 's/ /./g' >actual + + test_cmp expect actual +} + +test_expand () +{ + fmt=$1 + case "$fmt" in + *=raw | *=short | *=email) + default="0 1" ;; + *) + default="8 0" ;; + esac + case "$fmt" in + *=email) + in=0 ;; + *) + in=4 ;; + esac + test_expect_success "expand/no-expand${fmt:+ for $fmt}" ' + count_expand $in $default $fmt && + count_expand $in 8 0 $fmt --expand-tabs && + count_expand $in 8 0 --expand-tabs $fmt && + count_expand $in 8 0 $fmt --expand-tabs=8 && + count_expand $in 8 0 --expand-tabs=8 $fmt && + count_expand $in 0 1 $fmt --no-expand-tabs && + count_expand $in 0 1 --no-expand-tabs $fmt && + count_expand $in 0 1 $fmt --expand-tabs=0 && + count_expand $in 0 1 --expand-tabs=0 $fmt && + count_expand $in 4 0 $fmt --expand-tabs=4 && + count_expand $in 4 0 --expand-tabs=4 $fmt + ' +} + +test_expect_success 'setup' ' + test_tick && + sed -e "s/Q/$HT/g" <<-EOF >msg && + Q$title + + Q$body + EOF + git commit --allow-empty -F msg +' + +test_expand "" +test_expand --pretty +test_expand --pretty=short +test_expand --pretty=medium +test_expand --pretty=full +test_expand --pretty=fuller +test_expand --pretty=raw +test_expand --pretty=email + +test_done -- 2.8.1-253-gd0f4798 -- 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 v3 3/4] format-patch: introduce --base=auto option
On Fri, Apr 01, 2016 at 09:06:20AM -0700, Junio C Hamano wrote: >Ye Xiaolong writes: > >> On Thu, Mar 31, 2016 at 10:43:48AM -0700, Junio C Hamano wrote: >>>Xiaolong Ye writes: >>> Introduce --base=auto to record the base commit info automatically, the base_commit will be the merge base of tip commit of the upstream branch and revision-range specified in cmdline. >>> >>>This line is probably a bit too long. >> >> How about simplifying it to "the base_commit is the merge base of upstream >> and >> specified revision-range."? > >What I meant was not that profound. I just wanted you to wrap your >lines a bit shorter so that quoting in the discussion thread like >this would not make the result overlong to fit on a 80-column >terminal ;-) Emm, get your point now, I'll shorten the lines to fit in 80-column. > + base = base_list->item; + free_commit_list(base_list); >>> >>>What should happen when there are multiple merge bases? The code >>>picks one at random and ignores the remainder, if I am reading this >>>correctly. >> >> If there is more than one merge base, commits in base_list should >> be sorted by date, if I am understanding it correctly, so >> base_list->item should be the lastest merge base commit, it should >> be enough for us to used as base commit. > >By definition, when there are multiple merge bases, there is no >latest one among them. > >When the history involves criss-cross merges, there can be more than >one 'best' common ancestor for two commits. For example, with this >topology (note that X is not a commit; it merely denotes crossing of >two lines): > > ---1---o---A > /\ / > ---O X > \/ \ > ---2---o---o---B > >both '1' and '2' are merge-bases of 'A' and 'B'. And the timestamps >on one (be it committer or author timestamp) being later than those >of the other do not make it any more suitable than the other one. > For this criss-cross merges, as neither merge base(like 1) is better than the other(both 1 and 2 are 'best' merge bases), I think it should be fine to pick a random one as base commit(Or you prefer to show all of them?) and I'll add this part of discusstion into documentation. Thanks, Xiaolong. -- 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] Makefile: stop pretending to support rpmbuild
Dennis Kaarsemaker writes: > On za, 2016-04-02 at 20:41 -0700, Junio C Hamano wrote: >> I think by now it is very clear that nobody active in the Git >> development community tests RPM binaries built with git.spec.in we >> have in our tree. I suspect RPM based distros are using their own >> RPM build recipe without paying any attention to what we have in our >> tree, and that is why no packager from RPM land gave any bug report >> and correction before the release happened. > > Fedora, RHEL, CentOS, OpenSUSE and Mageia all use their own specfiles > and local patches. I do test and distribute RPM packages based on the > next branch, but use fedora's packaging to do so (which incidentally > also broke and I had to work around this change, but I completely > forgot about git.spec.in). > >> I'd propose that during the cycle for the next feature release, we'd >> remove git.spec.in and stop pretending as if we support RPM >> packaging. > > I would be in favor of that. In general, I find it much better to use a > distro's packaging and simply transplanting a tarball into it. That > keeps the difference with what the distro provides minimal. OK, while we wait for other people to raise their opinions, here is to prepare for the removal, if we decide to follow through. -- >8 -- Nobody in the active development community seems to watch breakages in the rpmbuild target. As most major RPM based distros use their own specfile when packaging us, they aren't looking after us as their pristine upstream tree, either. At this point, it is turning to be a disservice to the users to pretend that our tree natively supports "make rpmbuild" target when we do not properly maintain it. Signed-off-by: Junio C Hamano --- Makefile| 17 +--- git.spec.in | 330 2 files changed, 5 insertions(+), 342 deletions(-) diff --git a/Makefile b/Makefile index 2742a69..23182bc 100644 --- a/Makefile +++ b/Makefile @@ -443,7 +443,6 @@ DIFF = diff TAR = tar FIND = find INSTALL = install -RPMBUILD = rpmbuild TCL_PATH = tclsh TCLTK_PATH = wish XGETTEXT = xgettext @@ -2396,31 +2395,25 @@ quick-install-html: ### Maintainer's dist rules -git.spec: git.spec.in GIT-VERSION-FILE - sed -e 's/@@VERSION@@/$(GIT_VERSION)/g' < $< > $@+ - mv $@+ $@ - GIT_TARNAME = git-$(GIT_VERSION) dist: git.spec git-archive$(X) configure ./git-archive --format=tar \ --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar @mkdir -p $(GIT_TARNAME) - @cp git.spec configure $(GIT_TARNAME) + @cp configure $(GIT_TARNAME) @echo $(GIT_VERSION) > $(GIT_TARNAME)/version @$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version $(TAR) rf $(GIT_TARNAME).tar \ - $(GIT_TARNAME)/git.spec \ $(GIT_TARNAME)/configure \ $(GIT_TARNAME)/version \ $(GIT_TARNAME)/git-gui/version @$(RM) -r $(GIT_TARNAME) gzip -f -9 $(GIT_TARNAME).tar -rpm: dist - $(RPMBUILD) \ - --define "_source_filedigest_algorithm md5" \ - --define "_binary_filedigest_algorithm md5" \ - -ta $(GIT_TARNAME).tar.gz +rpm:: + @echo >&2 "Use distro packaged sources to run rpmbuild" + @false +.PHONY: rpm htmldocs = git-htmldocs-$(GIT_VERSION) manpages = git-manpages-$(GIT_VERSION) diff --git a/git.spec.in b/git.spec.in deleted file mode 100644 index d61d537..000 --- a/git.spec.in +++ /dev/null @@ -1,330 +0,0 @@ -# Pass --without docs to rpmbuild if you don't want the documentation - -Name: git -Version: @@VERSION@@ -Release: 1%{?dist} -Summary: Core git tools -License: GPL -Group: Development/Tools -URL: http://kernel.org/pub/software/scm/git/ -Source:http://kernel.org/pub/software/scm/git/%{name}-%{version}.tar.gz -BuildRequires: zlib-devel >= 1.2, openssl-devel, curl-devel, expat-devel, gettext %{!?_without_docs:, xmlto, asciidoc > 6.0.3} -BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) - -Requires: perl-Git = %{version}-%{release} -Requires: zlib >= 1.2, rsync, less, openssh-clients, expat -Provides: git-core = %{version}-%{release} -Obsoletes: git-core <= 1.5.4.2 -Obsoletes: git-p4 - -%description -Git is a fast, scalable, distributed revision control system with an -unusually rich command set that provides both high-level operations -and full access to internals. - -The git rpm installs the core tools with minimal dependencies. To -install all git packages, including tools for integrating with other -SCMs, install the git-all meta-package. - -%package all -Summary: Meta-package to pull in all git tools -Group: Development/Tools -Requires: git = %{version}-%{release} -Requires: git-svn = %{version}-%{release} -Requires: git-cvs = %{version}-%{release} -Requires: