Re: [Quilt-dev] CVS / source control support

[Dean Roehrich]

On Wed, 2005-08-24 at 20:46, Joe Green wrote:


> Index: quilt-0.42/quilt/import.in
> ===================================================================
> --- quilt-0.42.orig/quilt/import.in
> +++ quilt-0.42/quilt/import.in
> @@ -104,10 +104,13 @@ do
>               fi
>               printf $"Replacing patch %s with new version\n" \
>                      "$(print_patch $patch)" >&2
> +             scm_modify_patch "edit" "$QUILT_PATCHES/$patch" || exit 1
> +             newpatch=no


scm_modify_patch $QUILT_PATCHES/$patch

That should imply the "edit".   Then...



> @@ -117,6 +120,11 @@ do
>               status=1
>       fi
>  
> +     if [ "$newpatch" = "yes" ]
> +     then
> +             scm_modify_patch "add" "$QUILT_PATCHES/$patch" || exit 1
> +     fi

scm_add_patch $QUILT_PATCHES/$patch

That should imply..."add" :)

I know my own version of this stuff also had this multiplexer but I think
we should move away from that.

I'd also like to distinguish between pre-modify and post-modify steps, to 
accomodate
SCMs such as BK.  (I know, we've all migrated away from that by now...and I 
haven't
learned the git semantics yet, but I think that's post-modify, right?)  We 
could keep the scm_modify_patch() and then also have scm_pre_modify_patch().  
The name "scm_modify_patch" has a finality to it so it doesn't need to be 
renamed scm_post_modify_patch.

I'm also concerned about your use of "|| exit 1".  I think you need to unroll, 
in some
circumstances at least.  In the import newpatch=yes case, you've already 
successfully
executed a cp to replace the patch and it seems wrong to simply "exit 1" after 
the
scm_add_patch fails.  I think this will fail quite often, with people hitting 
^C now and
again (you have given them more time to reconsider, and they will)...and they 
will expect the import itself to fail in this case.

Also, you wouldn't call scm_modify_patch if status=1.


> Index: quilt-0.42/scripts/patchfns.in
> ===================================================================
> --- quilt-0.42.orig/scripts/patchfns.in
> +++ quilt-0.42/scripts/patchfns.in

> @@ -192,6 +226,7 @@ insert_in_series()
>  {
>       local patch=$1 patch_args=$2
>       local top=$(top_patch) tmpfile
> +     local new_series=no
>  
>       if [ -n "$patch_args" ]
>       then
> @@ -227,8 +262,24 @@ insert_in_series()
>       else
>               echo "$patch$patch_args" > $tmpfile
>       fi

Hmm, does your workarea match what's currently at the top of the CVS tree?


> Index: quilt-0.42/quilt/delete.in
> ===================================================================
> --- quilt-0.42.orig/quilt/delete.in
> +++ quilt-0.42/quilt/delete.in
> @@ -146,6 +146,7 @@ then
>                       exit 1
>               fi
>       fi
> +     scm_modify_patch "delete" "$patch_file" || exit 1
>  fi
>  ### Local Variables:
>  ### mode: shell-script

Again, you're not synchronized with the CVS tree.  

Unroll steps will be required, to satisfy the principle of least surprise.



> Index: quilt-0.42/quilt/fork.in
> ===================================================================
> --- quilt-0.42.orig/quilt/fork.in
> +++ quilt-0.42/quilt/fork.in
> @@ -118,6 +118,13 @@ printf $"Fork of patch %s created as %s\
>         "$(print_patch $top_patch)" \
>         "$(print_patch $new_patch)"
>  
> +patch_file=$(patch_file_name "$new_patch")
> +if [ -e "$patch_file" ]
> +then
> +     scm_modify_patch "add" "$patch_file" || exit 1
> +fi
> +
> +exit 0
>  ### Local Variables:
>  ### mode: shell-script
>  ### End:

I'll bet you'll want to look at some unroll steps.




> Index: quilt-0.42/quilt/refresh.in
> ===================================================================

Ah, refresh.  Now here is where I become concerned that you'll be changing 
people's
habits--they're methods of work.  We've all learned over the years to save 
often, and
I've no doubt that quilt users also refresh often.  If this requires answering 
SCM
prompts then this will become unpopular mighty quick.  I think we need 
something other
than scm_edit_patch and scm_add_patch in this case.


> --- quilt-0.42.orig/quilt/refresh.in
> +++ quilt-0.42/quilt/refresh.in
> @@ -290,6 +290,14 @@ fi
>  
>  cat $tmp_patch >> $tmp_result
>  
> +if [ -e "$patch_file" ]
> +then
> +     scm_modify_patch "edit" "$patch_file" || die 1
> +     newpatch=no
> +else
> +     newpatch=yes
> +fi
> +
>  if [ -e $patch_file ] && \
>     @DIFF@ -q $patch_file $tmp_result > /dev/null
>  then
> @@ -305,6 +313,11 @@ fi
>  
>  touch $QUILT_PC/$patch/.timestamp
>  
> +if [ "$newpatch" = "yes" ]
> +then
> +     scm_modify_patch "add" "$patch_file" || die 1
> +fi
> +

No unroll steps are required for the scm_edit_patch failure case, but you do 
need unroll steps for the scm_add_patch failure case.

The unroll steps concern me the most.  If you don't do it, then people will 
eventually
start asking for it when they notice the violation of the principle of least 
surprise--they'll be unsure about how to put things back together again and 
they may
believe they've lost work.

If you do the unroll steps you will, in some cases, severely uglify the code 
(can I say
uglify?).

It's one of those rock-and-hard-place problems.

Dean