Re: [PATCH v5 11/15] remote-testgit: make clear the 'done' feature
On Wed, Nov 21, 2012 at 10:11 AM, Junio C Hamano wrote: > I'd state it like this, but I may have guessed what Felipe intended > incorrectly. > > remote-testgit: advertise "done" feature and write "done" ourselves > > Instead of letting "fast-export" advertise the feature and ending > its stream with "done", do it ourselves. This way, it would make it > more clear to people who want to write their own remote-helper to > produce fast-export streams without using "fast-export > --use-done-feature" that they are supposed to end their stream with > "done". With that commit message: Acked-by: Sverre Rabbelier -- Cheers, Sverre Rabbelier -- 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 11/15] remote-testgit: make clear the 'done' feature
Max Horn writes: > Aha, now I understand what this patch is about. So I would suggest this > alternate commit message: > > remote-testgit: make it explicit clear that we use the 'done' feature > > Previously we relied on passing '--use-done-feature ' to git fast-export, > which is > easy to miss when looking at this script. Since remote-testgit is also a > reference > implementation, we now explicitly output 'feature done' / 'done' to make it > crystal clear that we implement this feature. I'd state it like this, but I may have guessed what Felipe intended incorrectly. remote-testgit: advertise "done" feature and write "done" ourselves Instead of letting "fast-export" advertise the feature and ending its stream with "done", do it ourselves. This way, it would make it more clear to people who want to write their own remote-helper to produce fast-export streams without using "fast-export --use-done-feature" that they are supposed to end their stream with "done". -- 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 11/15] remote-testgit: make clear the 'done' feature
On Mon, Nov 12, 2012 at 4:45 PM, Jonathan Nieder wrote: > Max Horn wrote: > >> Aha, now I understand what this patch is about. So I would suggest >> this alternate commit message: >> >> remote-testgit: make it explicit clear that we use the 'done' feature >> >> Previously we relied on passing '--use-done-feature ' to git >> fast-export, which is easy to miss when looking at this script. > > I'm not immediately sure I agree this is even a problem. Is the point > that other fast-import frontends do not have a --use-done-feature > switch, You mean other fast-exports. And what other fast-exports? Most remote helpers don't use an external fast-export tool, and the only I know that used one is the one I wrote (and is now deprecated) that used bzr fast-export, and no, that one didn't support the done feature. Most remote helpers would probably be doing the equivalent of fast-export themselves. > so a typical remote helper has to do that work itself, and the > sample "testgit" remote helper would be a more helpful example by > doing that work itself? Yes. > The idea behind --use-done-feature is that if fast-export exits early > for some reason and its output is going to a pipe then at least the > stream will be malformed, making it easier to catch errors. So there > is something to be weighed here: is it more important to illustrate > how to make your fast-export tool's output prefix-free, What fast-export tool? This is a remote helper. > or is it more > important to illustrate how to work around a fast-export tool that > doesn't support that feature? Ditto. If you want to launch a campaign of adding the 'done' feature to whatever fast-export tools are out there (that I'm not aware of), go ahead, but this is about remote helpers, most (all?) of which would not use a fast-export tool to achieve the export, but do it themselves. Cheers. -- Felipe Contreras -- 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 11/15] remote-testgit: make clear the 'done' feature
Max Horn wrote: > Aha, now I understand what this patch is about. So I would suggest > this alternate commit message: > > remote-testgit: make it explicit clear that we use the 'done' feature > > Previously we relied on passing '--use-done-feature ' to git > fast-export, which is easy to miss when looking at this script. I'm not immediately sure I agree this is even a problem. Is the point that other fast-import frontends do not have a --use-done-feature switch, so a typical remote helper has to do that work itself, and the sample "testgit" remote helper would be a more helpful example by doing that work itself? The idea behind --use-done-feature is that if fast-export exits early for some reason and its output is going to a pipe then at least the stream will be malformed, making it easier to catch errors. So there is something to be weighed here: is it more important to illustrate how to make your fast-export tool's output prefix-free, or is it more important to illustrate how to work around a fast-export tool that doesn't support that feature? The answer is not immediately obvious to me. A good description could provide context to make it obvious. Hoping that clarifies, Jonathan -- 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 11/15] remote-testgit: make clear the 'done' feature
On 11.11.2012, at 22:22, Felipe Contreras wrote: > On Sun, Nov 11, 2012 at 9:49 PM, Max Horn wrote: >> >> On 11.11.2012, at 14:59, Felipe Contreras wrote: >> >>> People seeking for reference would find it useful. >> >> Hm, I don't understand this commit message. Probably means I am j git >> fast-export --use-done-featureust too dumb, but since I am one of those >> people who would likely be seeking for reference, I would really appreciate >> if it could clarified. Like, for example, I don't see how the patch below >> makes anything "clear", it just seems to change the "import" command of >> git-remote-testgit to make use of the 'done' feature? > > No, the done feature was there already, but not so visible: git > fast-export --use-done-feature <-there. Which is the problem, it's too > easy to miss, therefore the need to make it clear. Aha, now I understand what this patch is about. So I would suggest this alternate commit message: remote-testgit: make it explicit clear that we use the 'done' feature Previously we relied on passing '--use-done-feature ' to git fast-export, which is easy to miss when looking at this script. Since remote-testgit is also a reference implementation, we now explicitly output 'feature done' / 'done' to make it crystal clear that we implement this feature. Or perhaps a little bit less verbose. With a commit message like the above, I think I would have grokked the patch right away. With the original message, that was not the case (else I wouldn't have wrote my initial email). And even though I now understand (or at least believe to understand) the patch, I don't think the original message is that helpful... indeed, "make clear the 'done' feature" is ambiguous. You meant it as "make clear the 'done' feature is implemented / used", while I understood it as "make clear what the 'done' feature is about". Looking at the patch can help to resolve that, but (a) my wrong interpretation threw me off-track and (b) I thought that the point of commit messages was to give an overview of a patch without having to look at it... So at the very least, the message should explain what exactly is "made clear". Anyway, a small change to the commit message hopefully will not be a problem. :-) Cheers, Max-- 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 11/15] remote-testgit: make clear the 'done' feature
On Sun, Nov 11, 2012 at 9:49 PM, Max Horn wrote: > > On 11.11.2012, at 14:59, Felipe Contreras wrote: > >> People seeking for reference would find it useful. > > Hm, I don't understand this commit message. Probably means I am j git > fast-export --use-done-featureust too dumb, but since I am one of those > people who would likely be seeking for reference, I would really appreciate > if it could clarified. Like, for example, I don't see how the patch below > makes anything "clear", it just seems to change the "import" command of > git-remote-testgit to make use of the 'done' feature? No, the done feature was there already, but not so visible: git fast-export --use-done-feature <-there. Which is the problem, it's too easy to miss, therefore the need to make it clear. > Perhaps the idea of the patch is to make use of the "done" feature so that > remote-testgit acts as "reference implementation"? If that is the intention, > then perhaps this could be used as commit message: It's already there. > remote-testgit: make use of the 'done' feature > > This might be helpful for people who would like to see how to properly > implement the "done" feature. Everybody should implement the 'done' feature. Otherwise random error messages quite easily appear. -- Felipe Contreras -- 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 11/15] remote-testgit: make clear the 'done' feature
On 11.11.2012, at 14:59, Felipe Contreras wrote:
> People seeking for reference would find it useful.
Hm, I don't understand this commit message. Probably means I am just too dumb,
but since I am one of those people who would likely be seeking for reference, I
would really appreciate if it could clarified. Like, for example, I don't see
how the patch below makes anything "clear", it just seems to change the
"import" command of git-remote-testgit to make use of the 'done' feature?
Perhaps the idea of the patch is to make use of the "done" feature so that
remote-testgit acts as "reference implementation"? If that is the intention,
then perhaps this could be used as commit message:
remote-testgit: make use of the 'done' feature
This might be helpful for people who would like to see how to properly
implement the "done" feature.
But again, I am not sure if I understood the purpose of this patch correctly.
So please forgive me if this was totally off-base :-(.
Cheers,
Max
>
> Signed-off-by: Felipe Contreras
> ---
> git-remote-testgit | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index 698effc..812321e 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -55,8 +55,10 @@ while read line; do
>
> echo "feature import-marks=$gitmarks"
> echo "feature export-marks=$gitmarks"
> - git fast-export --use-done-feature
> --{import,export}-marks="$testgitmarks" $refs | \
> + echo "feature done"
> + git fast-export --{import,export}-marks="$testgitmarks" $refs |
> \
> sed -e "s#refs/heads/#${prefix}/heads/#g"
> + echo "done"
> ;;
> export)
> before=$(git for-each-ref --format='%(refname) %(objectname)')
> --
> 1.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
>
--
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 11/15] remote-testgit: make clear the 'done' feature
People seeking for reference would find it useful.
Signed-off-by: Felipe Contreras
---
git-remote-testgit | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/git-remote-testgit b/git-remote-testgit
index 698effc..812321e 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -55,8 +55,10 @@ while read line; do
echo "feature import-marks=$gitmarks"
echo "feature export-marks=$gitmarks"
- git fast-export --use-done-feature
--{import,export}-marks="$testgitmarks" $refs | \
+ echo "feature done"
+ git fast-export --{import,export}-marks="$testgitmarks" $refs |
\
sed -e "s#refs/heads/#${prefix}/heads/#g"
+ echo "done"
;;
export)
before=$(git for-each-ref --format='%(refname) %(objectname)')
--
1.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
