stripspace: allow -s/-c outside git repository

t/t0030-stripspace.sh +0 -0

index 5ce47e8af5..0c24a0f9a3 100755
@@ -430,9 +430,15 @@ test_expect_success '-c with changed comment char' '
 test_expect_success '-c with comment char defined in .git/config' '

builtin/stripspace.c int cmd_stripspace(int argc, const char **argv, const char *prefix) · line 52

Quoted 9 lines · Jonathan Nieder

That makes experimenting with the stripspace command unnecessarily fussy. Fix it by discovering the git directory gently, as intended all along.

        if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {

builtin/stripspace.c +1 -1

-               setup_git_directory_gently(NULL);
+               setup_git_directory_gently(&nongit);
                git_config(git_default_config, NULL);
        }

Makes me wonder if setup_git_directory_gently() should BUG if it receives NULL. That would require some reshuffling in setup.c, since setup_git_directory() calls out to it with NULL... Just thinking out loud. In any case, and more importantly, this is the last user providing NULL except for the one in setup_git_directory().

Quoted 5 lines

Makes me wonder if setup_git_directory_gently() should BUG if it receives NULL. That would require some reshuffling in setup.c, since setup_git_directory() calls out to it with NULL... Just thinking out loud. In any case, and more importantly, this is the last user providing NULL except for the one in setup_git_directory().

We could rename setup_git_directory_gently() to setup_git_directory_gently_2(), make that static and then call it from setup_git_directory_gently() and setup_git_directory().

v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21) improved stripspace --strip-comments / --comentlines by teaching them to read repository config, but it went a little too far: when running stripspace outside any repository, the result is

	$ git stripspace --strip-comments <test-input
	fatal: not a git repository (or any parent up to mount point /tmp)

That makes experimenting with the stripspace command unnecessarily fussy. Fix it by discovering the git directory gently, as intended all along.

Reported-by Han-Wen Nienhuys <[email protected]>

Signed-off-by Jonathan Nieder <[email protected]>

---

 builtin/stripspace.c  |  3 ++-
 t/t0030-stripspace.sh | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

builtin/stripspace.c +1 -0

index bdf0328869..be33eb83c1 100644
@@ -30,6 +30,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	enum stripspace_mode mode = STRIP_DEFAULT;
+	int nongit;
 
 	const struct option options[] = {
 		OPT_CMDMODE('s', "strip-comments", &mode,

builtin/stripspace.c +0 -0

 		git_config(git_default_config, NULL);
 	}
 

t/t0030-stripspace.sh +0 -0

index 5ce47e8af5..0c24a0f9a3 100755
@@ -430,9 +430,15 @@ test_expect_success '-c with changed comment char' '
 test_expect_success '-c with comment char defined in .git/config' '

t/t0030-stripspace.sh +4 -0

+
+test_expect_success '-c outside git repository' '
+	printf "# foo\n" >expect &&
+	printf "foo" | nongit git stripspace -c >actual &&
 	test_cmp expect actual
 '
 

Signature · 1 line

2.20.0.405.gbc1bbc6f85

v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)

Quoted 1 line · Jonathan Nieder

v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)

Minor nit not just on this patch, but your patches in general: I think you're the only one using this type of template instead of the %h ("%s", %ad) format documented in SubmittingPatches.

I've had at least a couple of cases where I've git log --grep=<abbr sha> and missed a commit of yours when you referred to another commit.

E.g. when composing
https://public-inbox.org/git/[email protected]/ I remembered PERLLIB_EXTRA went back & forth between working/breaking/working with your/my/your patch, so:

    git log --grep=0386dd37b1

Just found the chain up to my breaking change, but not your 7a7bfc7adc, which refers to that commit as v1.9-rc0~88^2.

Maybe this is really a feature request. I.e. maybe we should have some mode where --grep=<commitish> will be combined with some mode where we try to find various forms of <commitish> in commit messages, then normalize & match them....

On Wed, Dec 19, 2018 at 6:04 PM Ævar Arnfjörð Bjarmason

Quoted 9 lines

E.g. when composing
https://public-inbox.org/git/[email protected]/ I remembered PERLLIB_EXTRA went back & forth between working/breaking/working with your/my/your patch, so:

    git log --grep=0386dd37b1

Just found the chain up to my breaking change, but not your 7a7bfc7adc, which refers to that commit as v1.9-rc0~88^2.

To follow email model, this sounds like a good trailer for related commits, like In-Reply-To for email. We could even have special trailer "Fixes" to indicate what commit is the problem that this commit fixes.

Hi,

Quoted 11 lines · Duy Nguyen

On Wed, Dec 19, 2018 at 6:04 PM Ævar Arnfjörð Bjarmason

Quoted 9 lines

E.g. when composing
https://public-inbox.org/git/[email protected]/ I remembered PERLLIB_EXTRA went back & forth between working/breaking/working with your/my/your patch, so:

    git log --grep=0386dd37b1

Just found the chain up to my breaking change, but not your 7a7bfc7adc, which refers to that commit as v1.9-rc0~88^2.

[...]

Quoted 4 lines

To follow email model, this sounds like a good trailer for related commits, like In-Reply-To for email. We could even have special trailer "Fixes" to indicate what commit is the problem that this commit fixes.

In Linux kernel land, Documentation/process/submitting-patches.rst contains the following:

-- >8 --
If your patch fixes a bug in a specific commit, e.g. you found an issue using ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of the SHA-1 ID, and the one line summary. For example::

	Fixes: e21d2170f366 ("video: remove unnecessary platform_set_drvdata()")

The following ``git config`` settings can be used to add a pretty format for outputting the above style in the ``git log`` or ``git show`` commands::

	[core]
		abbrev = 12
	[pretty]
		fixes = Fixes: %h (\"%s\")

-- 8< --

I like it because (1) the semantics are clear, (2) it's very concrete (e.g. "first 12 characters", (3) it goes in a trailer, where other bits intended for machine consumption already go.

Should we adopt it? In other words, how about something like the following?

If it seems like a good idea, I can add a commit message.

Signed-off-by Jonathan Nieder <[email protected]>

Documentation/SubmittingPatches +7 -0

index ec8b205145..36ce1ac5d8 100644
@@ -367,6 +367,20 @@ If you like, you can put extra tags at the end:
 You can also create your own tag or use one that's in common usage
 such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
 
+If your patch fixes a bug in a specific commit, e.g. you found an issue using
+``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
+the SHA-1 ID, and the one line summary.  For example::
+
+	Fixes: 539047c19e ("revert: introduce --abort to cancel a failed cherry-pick")
+
+The following ``git config`` settings can be used to add a pretty format for

In Linux kernel land, Documentation/process/submitting-patches.rst contains the following:

-- >8 --
If your patch fixes a bug in a specific commit, e.g. you found an issue using ``git bisect``, please use the 'Fixes:' tag with the first 12 characters of the SHA-1 ID, and the one line summary. For example::

        Fixes: e21d2170f366 ("video: remove unnecessary platform_set_drvdata()")

The following ``git config`` settings can be used to add a pretty format for outputting the above style in the ``git log`` or ``git show`` commands::

        [core]
                abbrev = 12
        [pretty]
                fixes = Fixes: %h (\"%s\")

-- 8< --

I like it because (1) the semantics are clear, (2) it's very concrete (e.g. "first 12 characters", (3) it goes in a trailer, where other bits intended for machine consumption already go.

Quoted 1 line · Jonathan Nieder

v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)

Minor nit not just on this patch, but your patches in general: I think you're the only one using this type of template instead of the %h ("%s", %ad) format documented in SubmittingPatches.

Quoted 1 line · Jonathan Nieder

v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)

Minor nit not just on this patch, but your patches in general: I think you're the only one using this type of template instead of the %h ("%s", %ad) format documented in SubmittingPatches.

I've had at least a couple of cases where I've git log --grep=<abbr sha> and missed a commit of yours when you referred to another commit.

E.g. when composing
https://public-inbox.org/git/[email protected]/ I remembered PERLLIB_EXTRA went back & forth between working/breaking/working with your/my/your patch, so:

    git log --grep=0386dd37b1

Just found the chain up to my breaking change, but not your 7a7bfc7adc, which refers to that commit as v1.9-rc0~88^2.

Maybe this is really a feature request. I.e. maybe we should have some mode where --grep=<commitish> will be combined with some mode where we try to find various forms of <commitish> in commit messages, then normalize & match them....

- web interfaces like GitHub won't linkify this type of reference (whereas they will for something that looks like a hex object id)

- my terminal makes it easy to select hex strings, but doesn't understand this git-describe output :)

These tools _could_ be taught a regex like /v(\d+.)(-rc\d+)?([~^]+d)*/. But matching hex strings is a lot simpler, and works across many projects.

In commit 1234abcd (the subject line, 2016-01-01, v2.11.0), we did blah blah blah

Quoted 6 lines · Jeff King

- web interfaces like GitHub won't linkify this type of reference (whereas they will for something that looks like a hex object id)

- my terminal makes it easy to select hex strings, but doesn't understand this git-describe output :)

Quoted 3 lines

These tools _could_ be taught a regex like /v(\d+.)(-rc\d+)?([~^]+d)*/. But matching hex strings is a lot simpler, and works across many projects.

Is there some rule about how long the hex string has to be for this to work?

Quoted 2 lines

In commit 1234abcd (the subject line, 2016-01-01, v2.11.0), we did blah blah blah

The issue with this is that it is ambiguous about what the tag name is referring to: does that mean that "git describe" and "git version" tell me that v2.11.0 is the nearest *previous* release to that commit or that "git name-rev" tells me that v2.11.0 is a nearby *subsequent* release that contains it?

Of course the latter is the only answer that's useful, but in practice the former is what people tend to do when they are trying to follow a convention like this. So we'd need some automatic linting to make it useful.

I think a more promising approach is the Fixes trailer Duy mentioned, which has been working well for the Linux kernel project. I'll follow up in a reply to his message.

Quoted 2 lines · Jonathan Nieder

Is there some rule about how long the hex string has to be for this to work?

In both cases, it has to be 7 characters.

Quoted 5 lines

The issue with this is that it is ambiguous about what the tag name is referring to: does that mean that "git describe" and "git version" tell me that v2.11.0 is the nearest *previous* release to that commit or that "git name-rev" tells me that v2.11.0 is a nearby *subsequent* release that contains it?

Sure, it's ambiguous if you've never seen it. But if it becomes a convention in the project, then I don't think that's an obstacle.

I'm also not sure it really matters all that much either way. If you buy my argument that this is just about placing the general era of the commit in the mind of the reader, then "just before v2.11" or "just after v2.11" are about the same.

Quoted 3 lines

I think a more promising approach is the Fixes trailer Duy mentioned, which has been working well for the Linux kernel project. I'll follow up in a reply to his message.

I think that's a good idea if something is in fact being fixed. But there are many other reasons to refer to another commit in prose (or even outside of a commit message entirely).

Quoted 4 lines

I'm also not sure it really matters all that much either way. If you buy my argument that this is just about placing the general era of the commit in the mind of the reader, then "just before v2.11" or "just after v2.11" are about the same.

If it's that unreliable, I'd rather just have the hash, to be honest.

Quoted 3 lines

I think that's a good idea if something is in fact being fixed. But there are many other reasons to refer to another commit in prose (or even outside of a commit message entirely).

Sure, but in those cases do we need the ability to query on them?

To me it seems similar to having a policy on how to reference people in commit messages (e.g. "always include their email address"), so that I can grep for a contributor to see how they were involved in a patch. If it's not structured data, then at some point I stop worrying so much about machine parsability.

Quoted 18 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Jonathan Nieder

v2.11.0-rc3~3^2~1 (stripspace: respect repository config, 2016-11-21)

Minor nit not just on this patch, but your patches in general: I think you're the only one using this type of template instead of the %h ("%s", %ad) format documented in SubmittingPatches.

I've had at least a couple of cases where I've git log --grep=<abbr sha> and missed a commit of yours when you referred to another commit.

E.g. when composing
https://public-inbox.org/git/[email protected]/ I remembered PERLLIB_EXTRA went back & forth between working/breaking/working with your/my/your patch, so:

    git log --grep=0386dd37b1

Quoted 7 lines

Just found the chain up to my breaking change, but not your 7a7bfc7adc, which refers to that commit as v1.9-rc0~88^2.

Maybe this is really a feature request. I.e. maybe we should have some mode where --grep=<commitish> will be combined with some mode where we try to find various forms of <commitish> in commit messages, then normalize & match them....

That would help when you're using --grep, but not other things that are trying to parse the commit message. Two instances I've noticed:

- web interfaces like GitHub won't linkify this type of reference (whereas they will for something that looks like a hex object id)

- my terminal makes it easy to select hex strings, but doesn't understand this git-describe output :)

These tools _could_ be taught a regex like /v(\d+.)(-rc\d+)?([~^]+d)*/. But matching hex strings is a lot simpler, and works across many projects.

So I agree with you that this git-describe format is less convenient for readers, but my preferred solution is to use a different format, rather than try to teach every reading tool to be more clever.

As far as I can tell, the main advantage of using "v2.11.0-rc3~3^2~1" over its hex id is that it gives a better sense in time of which Git version we're talking about. The date in the parentheses does something similar for wall-clock time, but it's left to the reader to map that to a Git version if they choose.

Personally, I find the wall-clock time to be enough, since usually what I want to know is "how ancient is this". And in the rare instance that I care about the containing version, it's not a big deal to use "git tag --contains". If we really want to convey that information in the text, I think it would be reasonable to say something like:

In commit 1234abcd (the subject line, 2016-01-01, v2.11.0), we did blah blah blah

with a few simple rules:

- only mention a single version, the oldest one that contains the

    commit[1]. If it's in v2.11.1, we can infer that it's in v2.12.0,
    etc.

- only mention released commits; for the granularity we're talking about here, the distinction between v2.11.0 and v2.11.0-rc3 is not important

- if it hasn't been in a released version, don't include a version at all.

That's probably over-engineering, and I'm perfectly fine with the oid/subject/date format most people use. Just trying to give an option if people really think the tag name is useful.

-Peff

[1] I usually compute the containing version with:

      $ git help has
      'has' is aliased to '!f() { git tag --contains "$@" | grep ^v | grep -v -- -rc | sort -V | head -1; }; f'

though I suspect it could be done these days with fewer processes using "tag --sort".